Issue #1234 - Remove 'CString' from PluginFilter.*

src/plugin/PluginFilter.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
- * Copyright (C) 2006-2015 Eyeo GmbH
+ * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include "PluginStdAfx.h"
 #include "AdblockPlusClient.h"
 #include "PluginFilter.h"
 #include "PluginSettings.h"
 #include "PluginMutex.h"
 #include "PluginSettings.h"
 #include "PluginSystem.h"
 #include "PluginClass.h"
+#include "PluginUtil.h"
 #include "mlang.h"
-#include "TokenSequence.h"
 #include "..\shared\CriticalSection.h"
 #include "..\shared\Utils.h"
 #include "..\shared\MsHTMLUtils.h"
 
 // The filters are described at http://adblockplus.org/en/filters
 
 static CriticalSection s_criticalSectionFilterMap;
 
-std::wstring ToLowerString(const std::wstring& s)

[sergei, 2015/11/30 12:37:53]

It should be in anonymous namespace.

[Eric, 2015/11/30 15:54:06]

Done.

-{
-  return ToWstring(ToCString(s).MakeLower());
-}
-
 // ============================================================================
 // CFilterElementHide
 // ============================================================================
-void ElementHideParseError(const std::wstring& filterText, const std::string& reason)

[sergei, 2015/11/30 12:37:52]

It seems to know the current position would be also very useful. Could you
include `filterSuffix` here as well?

[sergei, 2015/11/30 12:37:52]

It's really not clear from the name of the method that it throws an exception.
The usage of it even looks like creating of a temporary instance of a class
`ElementHideParseError` 
e.g.
ElementHideParseError(filterText, "invalid tag");

Something like Throw_ParseElementHideException would be better.

[sergei, 2015/11/30 12:37:53]

It should be in anonymous namespace.

[Eric, 2015/11/30 15:54:05]

I changed the function to a subclass of std::runtime_error, so that the throw
statement is inline. It's only a few extra lines of extra code, but make the
exception behavior obvious.

[Eric, 2015/11/30 15:54:05]

Done.

[Eric, 2015/11/30 15:54:06]

I thought about doing that. I decided not to since syntax errors in filter lists
are fairly uncommon. My judgement call was that we could do that later.

I'm not averse to generating a proper syntax error message, but doing so
properly should use a utility class to do so and have associated unit tests. We
can do that later.

-{
-  std::string message(
-    "CFilterElementHide::CFilterElementHide, error parsing selector \""
-    + ToUtf8String(filterText) + "\" (" + reason + ")"
-    );
-  DEBUG_FILTER(ToUtf16String(message));
-  throw std::runtime_error(message);
+namespace
+{
+  class ElementHideParseException
+    : public std::runtime_error
+  {
+    std::string message(const std::wstring& filterText, const std::string& reason)
+    {
+      std::string s(
+        "CFilterElementHide::CFilterElementHide, error parsing selector \""
+        + ToUtf8String(filterText) + "\" (" + reason + ")"
+        );
+      DEBUG_FILTER(ToUtf16String(s));
+      return s;
+    }
+  public:
+    ElementHideParseException(const std::wstring& filterText, const std::string& reason)
+      : std::runtime_error(message(filterText, reason))
+    {}
+  };
 }
 
 CFilterElementHide::CFilterElementHide(const std::wstring& filterText)
   : m_filterText(filterText), m_type(ETraverserComplexType::TRAVERSER_TYPE_ERROR)
 {
   std::wstring filterSuffix(filterText); // The unparsed remainder of the input filter text
 
   // Find tag name, class or any (*)
   wchar_t firstTag = filterText[0];
   if (firstTag == '*')
   {
     // Any tag
     filterSuffix = filterSuffix.substr(1);
   }
   else if (firstTag == '[' || firstTag == '.' || firstTag == '#')
   {
     // Any tag (implicitly)
   }
   else if (isalnum(firstTag))
   {
     // Real tag
-    //TODO: Add support for descendant selectors 
+    // TODO: Add support for descendant selectors
     auto pos = filterSuffix.find_first_of(L".#[(");
     if (pos == std::wstring::npos)
     {
       pos = filterSuffix.length();
     }
-    m_tag = ToLowerString(filterSuffix.substr(0,pos));

[sergei, 2015/11/30 12:37:52]

space after comma is missed

[Eric, 2015/11/30 15:54:06]

Done.

+    m_tag = ToLowerString(filterSuffix.substr(0, pos));
     filterSuffix = filterSuffix.substr(pos);
   }
   else
   {
     // Error
-    ElementHideParseError(filterText, "invalid tag");
+    throw ElementHideParseException(filterText, "invalid tag");
   }
 
   // Find Id and class name
   if (!filterSuffix.empty())
   {
     wchar_t firstId = filterSuffix[0];
 
     // Id
     if (firstId == '#')
     {
       auto pos = filterSuffix.find('[');
       if (pos == std::wstring::npos)
       {
         pos = filterSuffix.length();
       }
       m_tagId = filterSuffix.substr(1, pos - 1);
       filterSuffix = filterSuffix.substr(pos);
       pos = m_tagId.find(L".");
       if (pos != std::wstring::npos)
       {
         if (pos == 0)
         {
-          ElementHideParseError(filterText, "empty tag id");
+          throw ElementHideParseException(filterText, "empty tag id");
         }
         m_tagClassName = m_tagId.substr(pos + 1);
         m_tagId = m_tagId.substr(0, pos);
       }
     }
     // Class name
     else if (firstId == '.')
     {
       auto pos = filterSuffix.find('[');
       if (pos == std::wstring::npos)
       {
         pos = filterSuffix.length();
       }
       m_tagClassName = filterSuffix.substr(1, pos - 1);
       filterSuffix = filterSuffix.substr(pos);
     }
   }
 
   while (!filterSuffix.empty())
   {
     if (filterSuffix[0] != '[')
     {
-      ElementHideParseError(filterText, "expected '['");
+      throw ElementHideParseException(filterText, "expected '['");
     }
     auto endPos = filterSuffix.find(']') ;
     if (endPos == std::wstring::npos)
     {
-      ElementHideParseError(filterText, "expected ']'");
+      throw ElementHideParseException(filterText, "expected ']'");
     }
     std::wstring arg = filterSuffix.substr(1, endPos - 1);
     filterSuffix = filterSuffix.substr(endPos + 1);
 
     CFilterElementHideAttrSelector attrSelector;
     auto posEquals = arg.find('=');
     if (posEquals != std::wstring::npos)
     {
       if (posEquals == 0)
       {
-        ElementHideParseError(filterText, "empty attribute name before '='");
+        throw ElementHideParseException(filterText, "empty attribute name before '='");
       }
       attrSelector.m_value = arg.substr(posEquals + 1);
       if (attrSelector.m_value.length() >= 2 && attrSelector.m_value[0] == '\"' && attrSelector.m_value[attrSelector.m_value.length() - 1] == '\"')
       {
         attrSelector.m_value = attrSelector.m_value.substr(1, attrSelector.m_value.length() - 2);
       }
 
       if (arg[posEquals - 1] == '^')
       {
         attrSelector.m_pos = CFilterElementHideAttrPos::STARTING;
       }
       else if (arg[posEquals - 1] == '*')
       {
         attrSelector.m_pos = CFilterElementHideAttrPos::ANYWHERE;
       }
       else if (arg[posEquals - 1] == '$')
       {
         attrSelector.m_pos = CFilterElementHideAttrPos::ENDING;
       }
       if (attrSelector.m_pos != CFilterElementHideAttrPos::POS_NONE)
       {
         if (posEquals == 1)
         {
-          ElementHideParseError(filterText, "empty attribute name before " + std::string(1, static_cast<char>(arg[0])) + "'='");

[sergei, 2015/11/30 12:37:52]

std::string(1, static_cast<char>(arg[0]))
it seems better to call `ToUtf8String(arg[0])`

[Eric, 2015/11/30 15:54:06]

ToUtf8String only takes wstring arguments. The only basic_string constructor for
characters is the fill constructor that requires a length. The statement would
have to be thus.

    ToUtf8String(std::wstring(1,arg[0]))

I find the code in the patch set more readable, because the std::string
constructor is on the outside and thus the type is obvious.

I should point out that there are only exactly three characters that arg[0] can
be, one of '^', '*', or '$', so there's no question of locale and ToUtf8String
is overkill. If you'd like a code comment to that effect, I can add one.

[sergei, 2015/11/30 16:36:03]

Acknowledged.

+          throw ElementHideParseException(filterText, "empty attribute name before " + std::string(1, static_cast<char>(arg[0])) + "'='");
         }
         attrSelector.m_attr = arg.substr(0, posEquals - 1);
       }
       else
       {
         attrSelector.m_pos = CFilterElementHideAttrPos::EXACT;
         attrSelector.m_attr = arg.substr(0, posEquals);
       }
     }
 
     if (attrSelector.m_attr == L"style")
     {
       attrSelector.m_type = CFilterElementHideAttrType::STYLE;
       attrSelector.m_value = ToLowerString(attrSelector.m_value);
     }
     else if (attrSelector.m_attr == L"id")
     {
       attrSelector.m_type = CFilterElementHideAttrType::ID;
     }
     else if (attrSelector.m_attr == L"class")
     {
       attrSelector.m_type = CFilterElementHideAttrType::CLASS;
     }
     m_attributeSelectors.push_back(attrSelector);
   }
 
   // End check
   if (!filterSuffix.empty())
   {
-    ElementHideParseError(filterText, "extra characters at end");
+    throw ElementHideParseException(filterText, "extra characters at end");
   }
 }
 
 CFilterElementHide::CFilterElementHide(const CFilterElementHide& filter)
 {
   m_filterText = filter.m_filterText;
 
   m_tagId = filter.m_tagId;
   m_tagClassName = filter.m_tagClassName;
 
@@ skipping 33 matching lines @@
 
 bool CFilterElementHide::IsMatchFilterElementHide(IHTMLElement* pEl) const
 {
   HRESULT hr;
   /*
    * If a tag id is specified, it must match
    */
   if (!m_tagId.empty())
   {
     CComBSTR idBstr;
-    if (FAILED( pEl->get_id(&idBstr)) || !idBstr || m_tagId != ToWstring(idBstr))

[sergei, 2015/11/30 12:37:53]

additional space after '('

+    if (FAILED(pEl->get_id(&idBstr)) || !idBstr || m_tagId != ToWstring(idBstr))
     {
       return false;
     }
   }
   /*
    * If a class name is specified, it must match
    */
   if (!m_tagClassName.empty())
   {
     CComBSTR classNameListBstr;
     hr = pEl->get_className(&classNameListBstr);
     if (FAILED(hr) || !classNameListBstr)
     {
       return false; // We can't match a class name if there's no class name
     }
     std::wstring classNameList(ToWstring(classNameListBstr));
     if (classNameList.empty())
     {
       return false;
     }
     // TODO: Consider case of multiple classes. (m_tagClassName can be something like "foo.bar")
     /*
      * Match when 'm_tagClassName' appears as a token within classNameList
      */
     bool foundMatch = false;
-    TokenSequence<std::wstring> ts(classNameList, L" ");

[sergei, 2015/11/30 12:37:52]

Did we decide to use `wcstok_s` instead of custom tokenization?

[Eric, 2015/11/30 15:54:05]

We're using that function at present in exactly one loop in the plugin. We used
it in that case, as I recall, because the proposed code wasn't any better at the
time.

The class 'TokenSequence', though, is definitely better that using 'wcstok_s'.
-- TokenSequence uses C++ strings natively. 'wcstok_s' uses C-style strings.
Using TokenSequence requires no conversion back and forth to C-style strings.
-- TokenSequence uses iterators in the native C++ pattern.
-- TokenSequence only requires a single specification of delimiters. 'wcstok_s'
always requires two calls with the same delimiter string (one to initialize the
loop, one to increment it), violating DRY.
-- TokenSequence loops are shorter than the alternative.

[sergei, 2015/11/30 16:36:03]

Of course it's better than 'wcstok_s', it's difficult to make something equally
or more bad.
@Oleksandr, previous time you was the originator of using 'wcstok_s', what is
your opinion?

[Eric, 2015/11/30 17:05:46]

FWIW, the standard library function it replaced, the infamous 'strtok()', was in
fact worse. That thing wasn't even thread-safe.

[Oleksandr, 2015/12/04 01:03:15]

TokenSequence looks beautiful but unnecessary, IMHO. I don't see why we need it.
Even if just using wcstok_s is questionable (because of native-std string
conversions), what's wrong with something like: 
   std::wstringstream classNameListStream(classNameList);
   for (std::wstring iterClassName; std::getline(classNameListStream,
iterClassName, L' ');)
   {
     ...
   }

[Eric, 2015/12/06 17:39:56]

No one ever needs beautiful code. It does, however, reduce defect rates, improve
maintainability, made code easier to understand and audit. If you don't want
these benefits, please use ugly code. I prefer not to write such code.
The performance overhead of stream buffering is rather large, particularly for
something as simple as delimiter-based parsing. This function is called once per
per element (roughly) in the traverser, so performance is at least somewhat a
concern.

-    for (auto iterClassName = ts.cbegin(); iterClassName != ts.cend(); ++iterClassName)
-    {
-      if (*iterClassName == m_tagClassName)
+    wchar_t* nextToken = nullptr;
+    const wchar_t* token = wcstok_s(&classNameList[0], L" ", &nextToken);
+    while (token != nullptr)
+    {
+      if (std::wstring(token) == m_tagClassName)
       {
         foundMatch = true;
         break;
       }
+      token = wcstok_s(nullptr, L" ", &nextToken);
     }
     if (!foundMatch)
     {
       return false;
     }
   }
   /*
    * If a tag name is specified, it must match
    */
   if (!m_tag.empty())
   {
     CComBSTR tagNameBstr;
     if (FAILED(pEl->get_tagName(&tagNameBstr)) || !tagNameBstr)
     {
       return false;
     }
     if (m_tag != ToLowerString(ToWstring(tagNameBstr)))
     {
       return false;
     }
   }
   /*
    * Match each attribute
    */
-  for (auto attrIt = m_attributeSelectors.begin(); attrIt != m_attributeSelectors.end(); ++ attrIt)

[sergei, 2015/11/30 12:37:52]

additional space after `++`

[Eric, 2015/11/30 15:54:07]

Done.

+  for (auto attrIt = m_attributeSelectors.begin(); attrIt != m_attributeSelectors.end(); ++attrIt)
   {
     std::wstring value;
     bool attrFound = false;
     if (attrIt->m_type == CFilterElementHideAttrType::STYLE)
     {
       CComPtr<IHTMLStyle> pStyle;
       if (SUCCEEDED(pEl->get_style(&pStyle)) && pStyle)
       {
         CComBSTR styleBstr;
         if (SUCCEEDED(pStyle->get_cssText(&styleBstr)) && styleBstr)
@@ skipping 179 matching lines @@
 
   return true;
 }
 
 bool CPluginFilter::IsElementHidden(const std::wstring& tag, IHTMLElement* pEl, const std::wstring& domain, const std::wstring& indent) const
 {
   std::wstring id;
   CComBSTR idBstr;
   if (SUCCEEDED(pEl->get_id(&idBstr)) && idBstr)
   {
-    id = std::wstring(ToWstring(idBstr));

[sergei, 2015/11/30 12:37:52]

It should be just `id = ToWstring(idBstr);`

[Eric, 2015/11/30 15:54:05]

Done.

Good catch. That's leftover code from an earlier version before I put in
ToWstring. I removed some others like this, but I missed this one.

+    id = ToWstring(idBstr);
   }
   std::wstring classNames;
   CComBSTR classNamesBstr;
   if (SUCCEEDED(pEl->get_className(&classNamesBstr)) && classNamesBstr)
   {
     classNames = ToWstring(classNamesBstr);
   }
 
   CriticalSection::Lock filterEngineLock(s_criticalSectionFilterMap);
   {
     // Search tag/id filters
     if (!id.empty())
     {
       auto idItEnum = m_elementHideTagsId.equal_range(std::make_pair(tag, id));
-      for (auto idIt = idItEnum.first; idIt != idItEnum.second; idIt ++)

[sergei, 2015/11/30 12:37:53]

Good chance to change `idIt ++` to `++idIt`.

[Eric, 2015/11/30 15:54:06]

Done.

+      for (auto idIt = idItEnum.first; idIt != idItEnum.second; ++idIt)
       {
         if (idIt->second.IsMatchFilterElementHide(pEl))
         {
 #ifdef ENABLE_DEBUG_RESULT
           DEBUG_HIDE_EL(indent + L"HideEl::Found (tag/id) filter:" + idIt->second.m_filterText);
           CPluginDebug::DebugResultHiding(tag, L"id:" + id, idIt->second.m_filterText);
 #endif
           return true;
         }
       }
 
       // Search general id
       idItEnum = m_elementHideTagsId.equal_range(std::make_pair(L"", id));
-      for (auto idIt = idItEnum.first; idIt != idItEnum.second; idIt ++)

[sergei, 2015/11/30 12:37:51]

Good chance to change `idIt ++` to `++idIt`

[Eric, 2015/11/30 15:54:06]

Done.

+      for (auto idIt = idItEnum.first; idIt != idItEnum.second; ++idIt)
       {
         if (idIt->second.IsMatchFilterElementHide(pEl))
         {
 #ifdef ENABLE_DEBUG_RESULT
           DEBUG_HIDE_EL(indent + L"HideEl::Found (?/id) filter:" + idIt->second.m_filterText);
           CPluginDebug::DebugResultHiding(tag, L"id:" + id, idIt->second.m_filterText);
 #endif
           return true;
         }
       }
     }
 
     // Search tag/className filters
     if (!classNames.empty())
     {
-      TokenSequence<std::wstring> ts(classNames, L" \t\n\r");

[sergei, 2015/11/30 12:37:52]

Same here, let's use `wcstok_s` instead of custom tokenization.

[Eric, 2015/11/30 15:54:06]

See above comment.

-      for (auto iterClassName = ts.cbegin(); iterClassName != ts.cend(); ++iterClassName)
-      {
-        std::wstring className = *iterClassName;
+      wchar_t* nextToken = nullptr;
+      const wchar_t* token = wcstok_s(&classNames[0], L" \t\n\r", &nextToken);
+      while (token != nullptr)
+      {
+        std::wstring className(token);
         auto classItEnum = m_elementHideTagsClass.equal_range(std::make_pair(tag, className));
         for (auto classIt = classItEnum.first; classIt != classItEnum.second; ++classIt)
         {
           if (classIt->second.IsMatchFilterElementHide(pEl))
           {
 #ifdef ENABLE_DEBUG_RESULT
             DEBUG_HIDE_EL(indent + L"HideEl::Found (tag/class) filter:" + classIt->second.m_filterText);
             CPluginDebug::DebugResultHiding(tag, L"class:" + className, classIt->second.m_filterText);
 #endif
             return true;
           }
         }
 
         // Search general class name
         classItEnum = m_elementHideTagsClass.equal_range(std::make_pair(L"", className));
         for (auto classIt = classItEnum.first; classIt != classItEnum.second; ++ classIt)
         {
           if (classIt->second.IsMatchFilterElementHide(pEl))
           {
 #ifdef ENABLE_DEBUG_RESULT
             DEBUG_HIDE_EL(indent + L"HideEl::Found (?/class) filter:" + classIt->second.m_filterText);
             CPluginDebug::DebugResultHiding(tag, L"class:" + className, classIt->second.m_filterText);
 #endif
             return true;
           }
         }
+        token = wcstok_s(nullptr, L" \t\n\r", &nextToken);
       }
     }
 
     // Search tag filters
     auto tagItEnum = m_elementHideTags.equal_range(tag);
-    for (auto tagIt = tagItEnum.first; tagIt != tagItEnum.second; ++ tagIt)

[sergei, 2015/11/30 12:37:53]

additional space after `++`

[Eric, 2015/11/30 15:54:06]

Done.

+    for (auto tagIt = tagItEnum.first; tagIt != tagItEnum.second; ++tagIt)
     {
       if (tagIt->second.IsMatchFilterElementHide(pEl))
       {
 #ifdef ENABLE_DEBUG_RESULT
         DEBUG_HIDE_EL(indent + L"HideEl::Found (tag) filter:" + tagIt->second.m_filterText);
         CPluginDebug::DebugResultHiding(tag, L"-", tagIt->second.m_filterText);
 #endif
         return true;
       }
     }
@@ skipping 41 matching lines @@
 
 void CPluginFilter::ClearFilters()
 {
   // Clear filter maps
   CriticalSection::Lock filterEngineLock(s_criticalSectionFilterMap);
   m_elementHideTags.clear();
   m_elementHideTagsId.clear();
   m_elementHideTagsClass.clear();
 }
 
-bool CPluginFilter::ShouldBlock(const std::wstring& src, AdblockPlus::FilterEngine::ContentType contentType, const std::wstring& domain, bool addDebug) const
-{
-  std::wstring srcTrimmed = TrimString(src);
-
-  // We should not block the empty string, so all filtering does not make sense
-  // Therefore we just return
-  if (srcTrimmed.empty())
-  {
-    return false;
-  }
-
-  CPluginSettings* settings = CPluginSettings::GetInstance();
-
-  CPluginClient* client = CPluginClient::GetInstance();
-  bool result = client->Matches(srcTrimmed, contentType, domain);
-
-#ifdef ENABLE_DEBUG_RESULT
-  if (addDebug)
-  {
-    std::wstring type = ToUtf16String(AdblockPlus::FilterEngine::ContentTypeToString(contentType));
-    if (result)
-    {
-      CPluginDebug::DebugResultBlocking(type, srcTrimmed, domain);
-    }
-    else
-    {
-      CPluginDebug::DebugResultIgnoring(type, srcTrimmed, domain);
-    }
-  }
-#endif
-  return result;
-}