Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(937)

Issue 10100009: FilterEngine API improvements (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 7 months ago by Wladimir Palant
Modified:
6 years, 7 months ago
Reviewers:
Felix Dahlke, Oleksandr
Visibility:
Public.

Description

Addressed FilterEngine API issues from http://codereview.adblockplus.org/10016005/

Patch Set 1 #

Total comments: 7

Patch Set 2 : Added filter access and addressed review comments #

Total comments: 5

Patch Set 3 : Replace GetElementHidingRules by a domain-specific GetElementHidingSelectors #

Total comments: 8

Patch Set 4 : Addressed review comments #

Total comments: 3

Patch Set 5 : Changed filter type enum #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -22 lines) Patch
M include/AdblockPlus/FilterEngine.h View 1 2 3 4 1 chunk +4 lines, -4 lines 0 comments Download
M shell/src/FiltersCommand.cpp View 1 2 3 4 1 chunk +6 lines, -6 lines 0 comments Download
M src/FilterEngine.cpp View 1 2 3 4 1 chunk +5 lines, -5 lines 0 comments Download
M test/FilterEngineStubs.cpp View 1 2 3 4 2 chunks +7 lines, -7 lines 0 comments Download

Messages

Total messages: 18
Wladimir Palant
6 years, 7 months ago (2013-04-04 17:05:07 UTC) #1
Oleksandr
That comment and adding whitelist managing functions would be nice. http://codereview.adblockplus.org/10100009/diff/1/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/10100009/diff/1/src/FilterEngine.cpp#newcode141 ...
6 years, 7 months ago (2013-04-05 07:31:51 UTC) #2
Felix Dahlke
Some small remarks, otherwise LGTM. http://codereview.adblockplus.org/10100009/diff/1/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/1/include/AdblockPlus/FilterEngine.h#newcode17 include/AdblockPlus/FilterEngine.h:17: const std::string GetProperty(const std::string& ...
6 years, 7 months ago (2013-04-05 08:50:20 UTC) #3
Wladimir Palant
I've added an API to access custom filters (unstructured access should do for now). The ...
6 years, 7 months ago (2013-04-05 12:15:16 UTC) #4
Wladimir Palant
I've also replaced FilterEngine::GetElementHidingRules() by FilterEngine::GetElementHidingSelectors(domain) now - the latter should call ElemHide.getSelectorsForDomain() that we ...
6 years, 7 months ago (2013-04-05 12:24:24 UTC) #5
Oleksandr
http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/FilterEngine.h#newcode95 include/AdblockPlus/FilterEngine.h:95: const std::string& documentUrl); Might be just me, but based ...
6 years, 7 months ago (2013-04-05 14:09:07 UTC) #6
Felix Dahlke
http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/FilterEngine.h#newcode95 include/AdblockPlus/FilterEngine.h:95: const std::string& documentUrl); On 2013/04/05 14:09:07, Oleksandr wrote: > ...
6 years, 7 months ago (2013-04-05 14:20:59 UTC) #7
Oleksandr
On 2013/04/05 14:20:59, Felix H. Dahlke wrote: > http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/FilterEngine.h > File include/AdblockPlus/FilterEngine.h (right): > > ...
6 years, 7 months ago (2013-04-05 14:29:45 UTC) #8
Wladimir Palant
http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/FilterEngine.h#newcode95 include/AdblockPlus/FilterEngine.h:95: const std::string& documentUrl); Actually, the idea was that filters ...
6 years, 7 months ago (2013-04-05 14:31:44 UTC) #9
Felix Dahlke
http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/FilterEngine.h#newcode95 include/AdblockPlus/FilterEngine.h:95: const std::string& documentUrl); On 2013/04/05 14:31:45, Wladimir Palant wrote: ...
6 years, 7 months ago (2013-04-05 14:43:55 UTC) #10
Oleksandr
http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp#newcode200 src/FilterEngine.cpp:200: knownFilters[trimmed] = result; So I take it as no ...
6 years, 7 months ago (2013-04-05 15:14:12 UTC) #11
Felix Dahlke
Looks pretty complete by now. http://codereview.adblockplus.org/10100009/diff/10001/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/10001/include/AdblockPlus/FilterEngine.h#newcode13 include/AdblockPlus/FilterEngine.h:13: class JSObject Should be ...
6 years, 7 months ago (2013-04-06 05:17:05 UTC) #12
Wladimir Palant
All review comments should be addressed now. http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/FilterEngine.h#newcode95 include/AdblockPlus/FilterEngine.h:95: const std::string& ...
6 years, 7 months ago (2013-04-08 13:51:47 UTC) #13
Oleksandr
Appart from Windows specific changes needed LGTM. I will commit Windows specific stuff a bit ...
6 years, 7 months ago (2013-04-08 14:07:20 UTC) #14
Felix Dahlke
http://codereview.adblockplus.org/10100009/diff/23001/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/23001/include/AdblockPlus/FilterEngine.h#newcode7 include/AdblockPlus/FilterEngine.h:7: #include <tr1/memory> As of GCC 4.6 (we can assume ...
6 years, 7 months ago (2013-04-08 15:03:59 UTC) #15
Wladimir Palant
I changed the filter type enum. As to the tr1/memory include, Oleksandr is already taking ...
6 years, 7 months ago (2013-04-09 05:56:34 UTC) #16
Felix Dahlke
On 2013/04/09 05:56:34, Wladimir Palant wrote: > I changed the filter type enum. As to ...
6 years, 7 months ago (2013-04-09 06:50:13 UTC) #17
Felix Dahlke
6 years, 7 months ago (2013-04-09 06:51:58 UTC) #18
Oh, and LGTM :)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5