Issue #276 - introduce class BSTR_Argument

src/plugin/PluginFilter.cpp

Index: src/plugin/PluginFilter.cpp
===================================================================
--- a/src/plugin/PluginFilter.cpp
+++ b/src/plugin/PluginFilter.cpp
@@ -2,6 +2,7 @@
 
 #include "PluginFilter.h"
 
+#include "COM_Value.h"
 #if (defined PRODUCT_ADBLOCKPLUS)
 #include "PluginSettings.h"
 #include "PluginClient.h"
@@ -16,7 +17,6 @@
 
 #include "..\shared\CriticalSection.h"
 
-
 // The filters are described at http://adblockplus.org/en/filters
 
 static CriticalSection s_criticalSectionFilterMap;
@@ -25,7 +25,7 @@
 // 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.

 {
 }
 
@@ -33,7 +33,7 @@
 {
   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;
 }
@@ -150,26 +150,26 @@
 
       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;
@@ -242,20 +242,20 @@
 
   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;
@@ -275,10 +275,11 @@
   }
   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;
     }
@@ -295,38 +296,47 @@
       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)
@@ -507,17 +517,17 @@
 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);