Issue 1148 - ABP on chrome blocks ads but IE doesnt

src/plugin/PluginFilter.cpp

 #include "PluginStdAfx.h"
 
 #include "PluginFilter.h"
 #include "PluginSettings.h"
 #include "PluginClient.h"
 #include "PluginClientFactory.h"
 #include "PluginMutex.h"
 #include "PluginSettings.h"
 #include "PluginSystem.h"
 #include "PluginClass.h"
 #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;
+  struct GetHtmlElementAttributeResult

[Felix Dahlke, 2014/11/27 13:58:46]

How about "HtmlAttributeMatch" or something in that spirit?

[sergei, 2014/11/27 14:47:25]

I also don't feel it to be a good name, but I find "HtmlAttributeMatch" even
more confusing.

[Felix Dahlke, 2014/11/27 14:57:27]

Fair enough, yeah. Let's go with GetHtmlElementAttributeResult then.

+  {
+    GetHtmlElementAttributeResult() : isAttributeFound(false)
+    {
+    }
+    std::wstring attributeValue;
+    bool isAttributeFound;
+  };
+
+  GetHtmlElementAttributeResult GetHtmlElementAttribute(IHTMLElement& htmlElement,
+    const ATL::CComBSTR& attributeName)
+  {
+    GetHtmlElementAttributeResult retValue;
     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<IHTMLElement4> htmlElement4;
+    if (FAILED(htmlElement.QueryInterface(&htmlElement4)) || !htmlElement4)
+    {
+      return retValue;
     }
     ATL::CComPtr<IHTMLDOMAttribute> attributeNode;
     if (FAILED(htmlElement4->getAttributeNode(attributeName, &attributeNode)) || !attributeNode)
     {
-      return retResult;
+      return retValue;
     }
     // we set that attribute found but it's not necessary that we can retrieve its value
-    retResult.second = true;
+    retValue.isAttributeFound = true;
     if (FAILED(attributeNode->get_nodeValue(&vAttr)))
     {
-      return retResult;
-    }
-    retResult.first = GetAttributeValueAsString(vAttr);
-    return retResult;
+      return retValue;
+    }
+    if (vAttr.vt == VT_BSTR && vAttr.bstrVal)
+    {
+      retValue.attributeValue = vAttr.bstrVal;
+    }
+    else if (vAttr.vt == VT_I4)
+    {
+      retValue.attributeValue = std::to_wstring(vAttr.iVal);
+    }
+    return retValue;
   }
 }
 
 // ============================================================================
 // CFilterElementHideAttrSelector
 // ============================================================================
 
 CFilterElementHideAttrSelector::CFilterElementHideAttrSelector() : m_type(TYPE_NONE), m_pos(POS_NONE), m_bstrAttr(NULL)
 {
 }
@@ skipping 238 matching lines @@
       }
       if (!foundMatch)
       {
         return false;
       }
     }
   }
   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();
     if ((hr != S_OK) || (tagName != CComBSTR(m_tag)))
     {
       return false;
     }
   }
 
   // Check attributes
   for (std::vector<CFilterElementHideAttrSelector>::const_iterator attrIt = m_attributeSelectors.begin(); 
         attrIt != m_attributeSelectors.end(); ++ attrIt)
   {
     ATL::CString value;

[Felix Dahlke, 2014/11/27 13:58:46]

This change seems unrelated, hm? Plus we're still using CString (without ATL::)
elsewhere in this file.

[sergei, 2014/11/27 14:47:25]

Yes, it's not important here, just was irritating the eye. I would not say that
we don't use it in this file elsewhere is an argument. We should tend to use
ATL:: for the same reason that we don't have `using namespace std` in any
header.

[Felix Dahlke, 2014/11/27 14:57:27]

Yes I agree. It's just that we should avoid unrelated changes, particularly with
bug fixes. A commit should make one logical change. That way we can trace down
regressions and back commits out much easier.

     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)
         {
@@ skipping 16 matching lines @@
     {
       CComBSTR bstrId;
       if (SUCCEEDED(pEl->get_id(&bstrId)) && bstrId)
       {
         value = bstrId;
         attrFound = true;
       }
     }
     else
     {
-      auto attribute = GetHtmlElementAttribute(*pEl, attrIt->m_bstrAttr);
-      if (attrFound = attribute.second)
-      {
-        value = ToCString(attribute.first);
+      auto attributeValue = GetHtmlElementAttribute(*pEl, attrIt->m_bstrAttr);
+      if (attrFound = attributeValue.isAttributeFound)
+      {
+        value = ToCString(attributeValue.attributeValue);
       }
     }
 
     if (attrFound)
     {
       if (attrIt->m_pos == CFilterElementHideAttrPos::EXACT)
       {
         // TODO: IE rearranges the style attribute completely. Figure out if anything can be done about it.
         if (value != attrIt->m_value)
           return false;
@@ skipping 67 matching lines @@
 
 
 // ============================================================================
 // CPluginFilter
 // ============================================================================
 
 CPluginFilter::CPluginFilter(const CString& dataPath) : m_dataPath(dataPath)
 {
   m_contentMapText[CFilter::contentTypeDocument] = "DOCUMENT";
   m_contentMapText[CFilter::contentTypeObject] = "OBJECT";
-  m_contentMapText[CFilter::contentTypeObjectSubrequest] = "OBJECT-SUBREQUEST";
+  m_contentMapText[CFilter::contentTypeObjectSubrequest] = "OBJECT_SUBREQUEST";
   m_contentMapText[CFilter::contentTypeImage] = "IMAGE";
   m_contentMapText[CFilter::contentTypeScript] = "SCRIPT";
   m_contentMapText[CFilter::contentTypeOther] = "OTHER";
   m_contentMapText[CFilter::contentTypeUnknown] = "OTHER";
   m_contentMapText[CFilter::contentTypeSubdocument] = "SUBDOCUMENT";
   m_contentMapText[CFilter::contentTypeStyleSheet] = "STYLESHEET";
   m_contentMapText[CFilter::contentTypeXmlHttpRequest] = "XMLHTTPREQUEST";
 
   ClearFilters(); 
 }
@@ skipping 268 matching lines @@
         CPluginDebug::DebugResultBlocking(type, srcCString, domain);
 #endif
     }
     return true;
   }
 #ifdef ENABLE_DEBUG_RESULT
   CPluginDebug::DebugResultIgnoring(type, srcCString, domain);
 #endif
   return false;
 }