Issue 1148 - ABP on chrome blocks ads but IE doesnt

src/plugin/PluginFilter.cpp

Index: src/plugin/PluginFilter.cpp
===================================================================
--- a/src/plugin/PluginFilter.cpp
+++ b/src/plugin/PluginFilter.cpp
@@ -11,12 +11,54 @@
 #include "mlang.h"
 
 #include "..\shared\CriticalSection.h"
+#include "..\shared\Utils.h"
 
 
 // The filters are described at http://adblockplus.org/en/filters
 
 static CriticalSection s_criticalSectionFilterMap;
 
+namespace
+{
+  std::wstring GetAttributeValueAsString(const ATL::CComVariant& vAttr)

[Oleksandr, 2014/11/19 03:24:23]

Is it really required for this to be a separate function? It's used once.

[sergei, 2014/11/19 11:53:24]

It's not required. Firstly I extracted it during developing, then it turned out
to be quite small and it's indeed used only once. I thought to remove it, but it
contains several exit points and the caller is already quite long, so I decided
to keep it separately.

Merged into the caller.

+  {
+    if (vAttr.vt == VT_BSTR && vAttr.bstrVal)
+    {
+      return vAttr.bstrVal;
+    }
+    else if (vAttr.vt == VT_I4)
+    {
+      return std::to_wstring(vAttr.iVal);
+    }
+    return std::wstring();
+  }
+
+  std::pair<std::wstring, bool> GetHtmlElementAttribute(IHTMLElement& htmlElement, const ATL::CComBSTR& attributeName)

[Oleksandr, 2014/11/19 03:24:23]

How about something like this instead
std::wstring GetHtmlElementAttribute(IHTMLElement* const htmlElemen
     t, const ATL::CComBSTR& attributeName, /*out*/ bool* const result)
Looks more consistent with all the rest of the code and Windows APIs, IMO. Note
the first element is a pointer and there's a third element for a return value.

[sergei, 2014/11/19 11:53:24]

- I don't think that making `htmlElement` to be pointer is a good idea because
in that case we ought to verify that it's not nullptr which only complicates
this method as well as the caller. In most cases the code would be much
cleaner/safer/better readable if we prefer references to pointers. If it's
inconsistent with the current approach then we should change it.
- I would not like to spread the return value among arguments and the return
value until we don't have to do it. For `/*out*/ bool* const result` I would put
the meaning whether the method is succeeded or not. So I would say there could
be two types of method signatures: `RetValue method(parameters..., bool* isOK =
nullptr)`, where `bool* isOK` can be `ErrorCode* errorCode`, and `bool
method(parameters..., RetVaelu& retValue)` or it can return some `ErrorCode`.
In the current case `retResult.second` means whether the header found or not,
similar how std::pair<iterator, bool> is used in stl. I personally would like to
have a custom structure `struct GetHtmlElementAttributeResult { std::wstring
header; bool headerFound };` for better readability. If it makes sense let me
know.

[Oleksandr, 2014/11/20 10:21:56]

- I think it is inconsistent. We use pointers for COM interfaces everywhere. For
example IsMatchFilterElementHide or IsElementHidden in this file. Also what
would be complicated in the caller if we use a pointer instead? It would only
make the function call more readable, IMHO.
 
- Sorry, I do not understand your point here. When would be the situation when
we will *have* to spread the return value among the arguments? We can always
return some custom structure, so by that logic we can never split those. Why not
use one of the two types of signatures you've mentioned. I would actually prefer
"`bool method(parameters..., RetVaelu& retValue)'", which is very COM-like,
which is used a lot in the codebase. Just for consistency sake.

[sergei, 2014/11/21 12:02:21]

In the function we need to check that it's not null and the caller should verify
the return value. I don't think that such additional logic is required here
actually. BTW, the caller,
CFilterElementHide::IsMatchFilterElementHide, here also can accept reference
instead of a pointer.
The return value here consists from the attribute value and from the flag
whether it was found or not. The return code indicates some error during the
procedure, they are from different domains. In general the return code is
negative when arguments are incorrect or some resources are not available or
something else went wrong.
BTW, the signature "`bool method(parameters..., RetValue& retValue)'" requires
to always care about the return type, so we cannot use `auto` for the return
type.

[Felix Dahlke, 2014/11/26 14:23:16]

I agree with Sergei here, IMO we should go with references wherever possible,
i.e. when the function doesn't need to deal with null references. Less room for
error and increased readability (no need to ponder whether the function can deal
with null or not).
Not a fan of returning the pair here, either. I'd like either of your
suggestions (output parameter and struct return value), leaning a bit towards
the output parameter because it's less code and clear enough IMO.

+  {
+    std::pair<std::wstring, bool> retResult;
+    retResult.second = false;
+    ATL::CComVariant vAttr;
+    ATL::CComQIPtr<IHTMLElement4> htmlElement4 = &htmlElement;

[Oleksandr, 2014/11/19 03:24:23]

I prefer manually calling QueryInterface. We are checking the result anyway, so
it's not like this saves a lot of typing. And with CComQIPtr we'll only
introduce another class, not managed by us.

[sergei, 2014/11/19 11:53:24]

We don't need HRESULT here, so I would say that
ATL::CComQIPtr<IHTMLElement4> htmlElement4 = &htmlElement;
if (!htmlElement4)
{
  return retResult;
}
is better than
ATL::CComPtr<IHTMLElement4> htmlElement4;
if (FAILED(htmlElement.QueryInterface(&htmlElement)) || !htmlElement4)
{
  return retResult;
}
It's not about amount of typing, I would say, less logic and operations in our
code is better in most cases.
I don't think ATL::CComQIPtr causes or could cause any troubles.

[Oleksandr, 2014/11/20 10:21:56]

We do QueryInterface in a lot of places in the code, but this would be the first
time we use CComQIPtr. I don't see a point of it, so I disagree here. 

+    if (!htmlElement4)
+    {
+      return retResult;
+    }
+    ATL::CComPtr<IHTMLDOMAttribute> attributeNode;
+    if (FAILED(htmlElement4->getAttributeNode(attributeName, &attributeNode)) || !attributeNode)
+    {
+      return retResult;
+    }
+    // we set that attribute found but it's not necessary that we can retrieve its value
+    retResult.second = true;
+    if (FAILED(attributeNode->get_nodeValue(&vAttr)))
+    {
+      return retResult;
+    }
+    retResult.first = GetAttributeValueAsString(vAttr);
+    return retResult;
+  }
+}
+
 // ============================================================================
 // CFilterElementHideAttrSelector
 // ============================================================================
@@ -272,8 +314,8 @@
   if (!m_tag.IsEmpty())
   {
     CComBSTR tagName;
+    hr = pEl->get_tagName(&tagName);

[Oleksandr, 2014/11/19 03:24:23]

Well this was a long living bug :D. Nice!

     tagName.ToLower();
-    hr = pEl->get_tagName(&tagName);
     if ((hr != S_OK) || (tagName != CComBSTR(m_tag)))
     {
       return false;
@@ -284,7 +326,7 @@
   for (std::vector<CFilterElementHideAttrSelector>::const_iterator attrIt = m_attributeSelectors.begin(); 
         attrIt != m_attributeSelectors.end(); ++ attrIt)
   {
-    CString value;
+    ATL::CString value;
     bool attrFound = false;
     if (attrIt->m_type == CFilterElementHideAttrType::STYLE)
     {
@@ -319,20 +361,12 @@
         attrFound = true;
       }
     }
-    else 
+    else
     {
-      CComVariant vAttr;
-      if (SUCCEEDED(pEl->getAttribute(attrIt->m_bstrAttr, 0, &vAttr)))
+      auto attribute = GetHtmlElementAttribute(*pEl, attrIt->m_bstrAttr);
+      if (attrFound = attribute.second)
       {
-        attrFound = true;
-        if (vAttr.vt == VT_BSTR)
-        {
-          value = vAttr.bstrVal;
-        }
-        else if (vAttr.vt == VT_I4)
-        {
-          value.Format(L"%u", vAttr.iVal);
-        }
+        value = ToCString(attribute.first);
       }
     }
 
@@ -435,13 +469,9 @@
 
 bool CPluginFilter::AddFilterElementHide(CString filterText)
 {
-
-
   DEBUG_FILTER("Input: " + filterText + " filterFile" + filterFile);
-
-  CriticalSection::Lock filterEngineLock(s_criticalSectionFilterMap);    
+  CriticalSection::Lock filterEngineLock(s_criticalSectionFilterMap);
   {
-
     CString filterString  = filterText;
     // Create filter descriptor
     std::auto_ptr<CFilterElementHide> filter;
@@ -464,7 +494,7 @@
       CString filterChunk = filterText.Left(chunkEnd).TrimRight();
       std::auto_ptr<CFilterElementHide> filterParent(filter);
 
-      filter.reset(new CFilterElementHide(filterChunk));       
+      filter.reset(new CFilterElementHide(filterChunk));
 
       if (filterParent.get() != 0)
       {
@@ -519,7 +549,7 @@
     classNames = bstrClassNames;
   }
 
-  CriticalSection::Lock filterEngineLock(s_criticalSectionFilterMap);    
+  CriticalSection::Lock filterEngineLock(s_criticalSectionFilterMap);
   {
     // Search tag/id filters
     if (!id.IsEmpty())
@@ -621,7 +651,7 @@
 
   // Parse hide string
   int pos = 0;
-  CriticalSection::Lock filterEngineLock(s_criticalSectionFilterMap);    
+  CriticalSection::Lock filterEngineLock(s_criticalSectionFilterMap);
   {
     for (std::vector<std::wstring>::iterator it = filters.begin(); it < filters.end(); ++it)
     {