Issue 5141 - Convert filter match to C++

compiled/bindings/main.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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
@@ skipping 115 matching lines @@
         .property("lastDownload", &DownloadableSubscription::GetLastDownload, &DownloadableSubscription::SetLastDownload)
         .property("downloadStatus", &DownloadableSubscription::GetDownloadStatus, &DownloadableSubscription::SetDownloadStatus)
         .property("lastSuccess", &DownloadableSubscription::GetLastSuccess, &DownloadableSubscription::SetLastSuccess)
         .property("errors", &DownloadableSubscription::GetErrorCount, &DownloadableSubscription::SetErrorCount)
         .property("version", &DownloadableSubscription::GetDataRevision, &DownloadableSubscription::SetDataRevision)
         .property("requiredVersion", &DownloadableSubscription::GetRequiredVersion, &DownloadableSubscription::SetRequiredVersion)
         .property("downloadCount", &DownloadableSubscription::GetDownloadCount, &DownloadableSubscription::SetDownloadCount)
         .function("serialize", &DownloadableSubscription::Serialize);
 
     class_<ReMatchResults>("ReMatchResults")
         .function("push", &ReMatchResults::push);

[Wladimir Palant, 2017/10/09 08:39:47]

Why do we need to export what is clearly an internal class with an internal
method?

[sergei, 2017/10/09 15:27:51]

This class is used as a callback to obtain the results from JS implementation of
regular expressions. I expect it to be removed after complete switching to C++.

[hub, 2017/10/10 13:46:15]

The sole purpose of this class is to be able to get the results from a regexp
match. See in library.js. So it needs to have push() exported into JS.

 
     class_<CombinedMatcher>("CombinedMatcher")
+        .class_function("create", &CombinedMatcher::Create)
         .function("add", &CombinedMatcher::Add)
         .function("remove", &CombinedMatcher::Remove)
         .function("matchesAny", &CombinedMatcher::MatchesAny)
         .function("clear", &CombinedMatcher::Clear)
         .function("hasFilter", &CombinedMatcher::HasFilter)
         .function("getKeywordForFilter", &CombinedMatcher::GetKeywordForFilter)
         .function("findKeyword", &CombinedMatcher::FindKeyword);

[Wladimir Palant, 2017/10/09 08:39:47]

The three methods above shouldn't be exported, these are implementation details
now that JS code doesn't need to call.

In fact, the distinction between CombinedMatcher and Matcher was an
implementation details that shouldn't be exposed any more (mentioned in the
issue description).

[sergei, 2017/10/09 15:27:52]

I think they are currently exported for the sake of tests
(https://codereview.adblockplus.org/29556737/diff/29559667/compiled/bindings/m...).
I have no objection regarding removing the tests using them and not exposing
these methods but for present it seems they can be very useful to identify a
regression.

[Wladimir Palant, 2017/10/10 07:39:05]

Yes, that's probably it. We should remove/rewrite tests relying on internals
however.

[hub, 2017/10/10 13:46:15]

Note that currently these are test that exist in master as well.

Also notification.js currently explicitely instanciate a Matcher in addition to
using the defaultMatcher.

 
     class_<Matcher>("Matcher")
+        .class_function("create", &Matcher::Create)
         .function("add", &Matcher::Add)
         .function("remove", &Matcher::Remove)
         .function("matchesAny", &Matcher::MatchesAny)
         .function("clear", &Matcher::Clear)
         .function("hasFilter", &Matcher::HasFilter)
         .function("getKeywordForFilter", &Matcher::GetKeywordForFilter)
         .function("findKeyword", &Matcher::FindKeyword);
 
     singleton<FilterStorage>("FilterStorage", &FilterStorage::GetInstance)
         .property("subscriptionCount", &FilterStorage::GetSubscriptionCount)
@@ skipping 12 matching lines @@
   }
   catch (const std::exception& e)
   {
     EM_ASM_ARGS(
       console.error("Error occurred generating JavaScript bindings: " +
           Module.AsciiToString($0)), e.what()
     );
     return 1;
   }
 }