Issue #276 - introduce class BSTR_Argument

src/plugin/PluginFilter.cpp

 #include "PluginStdAfx.h"
 
 #include "PluginFilter.h"
 
+#include "COM_Value.h"
 #if (defined PRODUCT_ADBLOCKPLUS)
 #include "PluginSettings.h"
 #include "PluginClient.h"
 #include "PluginClientFactory.h"
 #endif
 
 #include "PluginMutex.h"
 #include "PluginSettings.h"
 #include "PluginSystem.h"
 #include "PluginClass.h"
 #include "mlang.h"
 
 #include "..\shared\CriticalSection.h"
 
-
 // The filters are described at http://adblockplus.org/en/filters
 
 static CriticalSection s_criticalSectionFilterMap;
 
 // ============================================================================
 // CFilterElementHideAttrSelector
 // ============================================================================
 
-CFilterElementHideAttrSelector::CFilterElementHideAttrSelector() : m_type(TYPE_NONE), m_pos(POS_NONE), m_bstrAttr(NULL)
+CFilterElementHideAttrSelector::CFilterElementHideAttrSelector() : m_type(TYPE_NONE), m_pos(POS_NONE), m_attr(L"")

[sergei, 2014/07/28 09:18:26]

CString should have a default constructor, just remove `, m_attr(L"")`

[Eric, 2014/07/29 16:59:27]

Done.

 {
 }
 
 CFilterElementHideAttrSelector::CFilterElementHideAttrSelector(const CFilterElementHideAttrSelector& filter)
 {
   m_type = filter.m_type;
   m_pos = filter.m_pos;
-  m_bstrAttr = filter.m_bstrAttr;
+  m_attr = filter.m_attr;
 
   m_value = filter.m_value;
 }
 
 CFilterElementHideAttrSelector::~CFilterElementHideAttrSelector()
 {
 }
 
 
 // ============================================================================
@@ skipping 96 matching lines @@
     if (delimiterPos > 0)
     {
       attrSelector.m_value = arg.Mid(delimiterPos + 1);
       if (attrSelector.m_value.GetLength() >= 2 && attrSelector.m_value.GetAt(0) == '\"' && attrSelector.m_value.GetAt(attrSelector.m_value.GetLength() - 1) == '\"')
       {
         attrSelector.m_value = attrSelector.m_value.Mid(1, attrSelector.m_value.GetLength() - 2);
       }
 
       if (arg.GetAt(delimiterPos - 1) == '^')
       {
-        attrSelector.m_bstrAttr = arg.Left(delimiterPos - 1);
+        attrSelector.m_attr = arg.Left(delimiterPos - 1);
         attrSelector.m_pos = CFilterElementHideAttrPos::STARTING;
       }
       else if (arg.GetAt(delimiterPos - 1) == '*')
       {
-        attrSelector.m_bstrAttr = arg.Left(delimiterPos - 1);
+        attrSelector.m_attr = arg.Left(delimiterPos - 1);
         attrSelector.m_pos = CFilterElementHideAttrPos::ANYWHERE;
       }
       else if (arg.GetAt(delimiterPos - 1) == '$')
       {
-        attrSelector.m_bstrAttr = arg.Left(delimiterPos - 1);
+        attrSelector.m_attr = arg.Left(delimiterPos - 1);
         attrSelector.m_pos = CFilterElementHideAttrPos::ENDING;
       }
       else
       {
-        attrSelector.m_bstrAttr = arg.Left(delimiterPos);
+        attrSelector.m_attr = arg.Left(delimiterPos);
         attrSelector.m_pos = CFilterElementHideAttrPos::EXACT;
       }
     }
-    CString tag = attrSelector.m_bstrAttr;
+    CString tag = attrSelector.m_attr;
     if (tag == "style")
     {
       attrSelector.m_type = CFilterElementHideAttrType::STYLE;
       attrSelector.m_value.MakeLower();
     }
     else if (tag == "id")
     {
       attrSelector.m_type = CFilterElementHideAttrType::ID;
     }
     else if (tag == "class")
@@ skipping 52 matching lines @@
 {
 }
 
 
 bool CFilterElementHide::IsMatchFilterElementHide(IHTMLElement* pEl) const
 {
   HRESULT hr;
 
   if (!m_tagId.IsEmpty())
   {
-    CComBSTR id;
+    AdblockPlus::COM::BSTR_Argument id;
     hr = pEl->get_id(&id);
-    if ((hr != S_OK) || (id != CComBSTR(m_tagId)))
+    if ((hr != S_OK) || (to_CString(id) != m_tagId))
     {
       return false;
     }
   }
   if (!m_tagClassName.IsEmpty())
   {
-    CComBSTR classNameBSTR;
-    hr = pEl->get_className(&classNameBSTR);
+    AdblockPlus::COM::BSTR_Argument result;
+    hr = pEl->get_className(&result);
     if (hr == S_OK)
     {
-      CString className = classNameBSTR;
+      CString className = to_CString(result);
       int start = 0;
       CString specificClass;
       bool foundMatch = false;
       while ((specificClass = className.Tokenize(L" ", start)) != L"")
       {
         // TODO: Consider case of multiple classes. (m_tagClassName can be something like "foo.bar")
         if (specificClass == m_tagClassName)
         {
           foundMatch = true;
         }
       }
       if (!foundMatch)
       {
         return false;
       }
     }
   }
   if (!m_tag.IsEmpty())
   {
-    CComBSTR tagName;
-    tagName.ToLower();

[Eric, 2014/07/25 21:42:16]

This was pretty obviously a defect.

-    hr = pEl->get_tagName(&tagName);
-    if ((hr != S_OK) || (tagName != CComBSTR(m_tag)))
+    AdblockPlus::COM::BSTR_Argument result;
+    hr = pEl->get_tagName(&result);
+    CString tagName = to_CString(result);
+    tagName.MakeLower();
+    if ((hr != S_OK) || tagName != m_tag)
     {
       return false;
     }
   }
 
   // Check attributes
   for (std::vector<CFilterElementHideAttrSelector>::const_iterator attrIt = m_attributeSelectors.begin(); 
         attrIt != m_attributeSelectors.end(); ++ attrIt)
   {
     CString value;
     bool attrFound = false;
     if (attrIt->m_type == CFilterElementHideAttrType::STYLE)
     {
       CComPtr<IHTMLStyle> pStyle;
       if (SUCCEEDED(pEl->get_style(&pStyle)) && pStyle)
       {
-        CComBSTR bstrStyle;
-
-        if (SUCCEEDED(pStyle->get_cssText(&bstrStyle)) && bstrStyle)
+        AdblockPlus::COM::BSTR_Argument cssText;
+        if (SUCCEEDED(pStyle->get_cssText(&cssText)))
         {
-          value = bstrStyle;
-          value.MakeLower();
-          attrFound = true;
+          value = to_CString(cssText);
+          if (!value.IsEmpty())
+          {
+            value.MakeLower();
+            attrFound = true;
+          }
         }
       }
     }
     else if (attrIt->m_type == CFilterElementHideAttrType::CLASS)
     {
-      CComBSTR bstrClassNames;
-      if (SUCCEEDED(pEl->get_className(&bstrClassNames)) && bstrClassNames)
+      AdblockPlus::COM::BSTR_Argument classNames;
+      if (SUCCEEDED(pEl->get_className(&classNames)))
       {
-        value = bstrClassNames;
-        attrFound = true;
+        value = to_CString(classNames);
+        if (!value.IsEmpty())
+        {
+          attrFound = true;
+        }
       }
     }
     else if (attrIt->m_type == CFilterElementHideAttrType::ID)
     {
-      CComBSTR bstrId;
-      if (SUCCEEDED(pEl->get_id(&bstrId)) && bstrId)
+      AdblockPlus::COM::BSTR_Argument id;
+      if (SUCCEEDED(pEl->get_id(&id)))
       {
-        value = bstrId;
-        attrFound = true;
+        value = to_CString(id);
+        if (!value.IsEmpty())
+        {
+          attrFound = true;
+        }
       }
     }
     else 
     {
       CComVariant vAttr;
-      if (SUCCEEDED(pEl->getAttribute(attrIt->m_bstrAttr, 0, &vAttr)))
+      AdblockPlus::COM::BSTR_Argument attribute_name(to_wstring(attrIt->m_attr));
+      if (SUCCEEDED(pEl->getAttribute(attribute_name, 0, &vAttr)))
       {
         attrFound = true;
         if (vAttr.vt == VT_BSTR)
         {
           value = vAttr.bstrVal;
         }
         else if (vAttr.vt == VT_I4)
         {
           value.Format(L"%u", vAttr.iVal);
         }
@@ skipping 160 matching lines @@
       }
     } while (separatorChar != '\0');
   }
 
   return true;
 }
 
 bool CPluginFilter::IsElementHidden(const CString& tag, IHTMLElement* pEl, const CString& domain, const CString& indent) const
 {
   CString id;
-  CComBSTR bstrId;
-  if (SUCCEEDED(pEl->get_id(&bstrId)) && bstrId)
+  AdblockPlus::COM::BSTR_Argument result;
+  if (SUCCEEDED(pEl->get_id(&result)))
   {
-    id = bstrId;
+    id = to_CString(result);
   }
 
   CString classNames;
-  CComBSTR bstrClassNames;
-  if (SUCCEEDED(pEl->get_className(&bstrClassNames)) && bstrClassNames)
+  // reuse of 'result' is not an error, since address-of operator permits it

[sergei, 2014/07/28 09:18:26]

It's too specific and requires additional comment each time.

[Eric, 2014/07/29 16:59:27]

It will also disappear by itself, since these are both calls that will convert
to wrapper calls that encapsulate the type conversion. What are now two type
conversion in the same context will be in different function bodies.

[sergei, 2014/07/30 10:10:54]

Sorry, not clear.

[Eric, 2014/07/30 14:14:42]

What is your question?

+  if (SUCCEEDED(pEl->get_className(&result)))
   {
-    classNames = bstrClassNames;
+    classNames = to_CString(result);
   }
 
   CriticalSection::Lock filterEngineLock(s_criticalSectionFilterMap);    
   {
     CString domainTest = domain;
 
     // Search tag/id filters
     if (!id.IsEmpty())
     {
       std::pair<TFilterElementHideTagsNamed::const_iterator, TFilterElementHideTagsNamed::const_iterator> idItEnum =
@@ skipping 179 matching lines @@
       DEBUG_FILTER("Filter::ShouldBlock " + type + " YES")
 
 #ifdef ENABLE_DEBUG_RESULT
         CPluginDebug::DebugResultBlocking(type, src, domain);
 #endif
     }
     return true;
   }
   return false;
 }