Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code
Issues
All Issues

Issue 29383799: Issue 4988 - [emscripten] Adjust API for Element Hiding Emulation filters (Closed)

Created:
March 14, 2017, 2:42 p.m. by Wladimir Palant
Modified:
March 27, 2017, 7:58 a.m.
Reviewers:
sergei
CC:
Felix Dahlke
Base URL:
https://hg.adblockplus.org/adblockpluscore
Visibility:
Public.

Description

Issue 4988 - [emscripten] Adjust API for Element Hiding Emulation filters

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+99 lines, -196 lines) Patch
M compiled/ElemHideBase.h View 2 chunks +3 lines, -5 lines 0 comments Download
M compiled/ElemHideBase.cpp View 3 chunks +3 lines, -28 lines 3 comments Download
M compiled/ElemHideEmulationFilter.h View 1 chunk +4 lines, -42 lines 0 comments Download
M compiled/ElemHideEmulationFilter.cpp View 1 chunk +3 lines, -9 lines 0 comments Download
M compiled/ElemHideException.h View 1 chunk +1 line, -1 line 0 comments Download
M compiled/ElemHideException.cpp View 1 chunk +1 line, -1 line 0 comments Download
M compiled/ElemHideFilter.h View 1 chunk +1 line, -1 line 0 comments Download
M compiled/ElemHideFilter.cpp View 1 chunk +1 line, -1 line 0 comments Download
M compiled/Filter.h View 1 chunk +1 line, -1 line 0 comments Download
M compiled/Filter.cpp View 2 chunks +5 lines, -5 lines 0 comments Download
M compiled/RegExpFilter.h View 1 chunk +0 lines, -1 line 0 comments Download
M compiled/RegExpFilter.cpp View 2 chunks +59 lines, -59 lines 1 comment Download
M compiled/bindings.cpp View 2 chunks +4 lines, -7 lines 0 comments Download
M lib/filterClassesNew.js View 1 chunk +1 line, -1 line 0 comments Download
M test/filterClasses.js View 5 chunks +12 lines, -34 lines 0 comments Download

Messages

Total messages: 6
Wladimir Palant
March 14, 2017, 2:42 p.m. (2017-03-14 14:42:44 UTC) #1
Wladimir Palant
https://codereview.adblockplus.org/29383799/diff/29383800/compiled/RegExpFilter.cpp File compiled/RegExpFilter.cpp (right): https://codereview.adblockplus.org/29383799/diff/29383800/compiled/RegExpFilter.cpp#newcode57 compiled/RegExpFilter.cpp:57: OwnedString RegExpFromSource(const String& source) This function was moved into ...
March 14, 2017, 2:44 p.m. (2017-03-14 14:44:43 UTC) #2
sergei
LGTM
March 24, 2017, 11:59 a.m. (2017-03-24 11:59:19 UTC) #3
sergei
https://codereview.adblockplus.org/29383799/diff/29383800/compiled/ElemHideBase.cpp File compiled/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29383799/diff/29383800/compiled/ElemHideBase.cpp#newcode107 compiled/ElemHideBase.cpp:107: if (text.find(u"[-abp-properties="_str, data.mSelectorStart) != text.npos) Nit: issue says "[-abp-properties" ...
March 24, 2017, 11:59 a.m. (2017-03-24 11:59:26 UTC) #4
Wladimir Palant
https://codereview.adblockplus.org/29383799/diff/29383800/compiled/ElemHideBase.cpp File compiled/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29383799/diff/29383800/compiled/ElemHideBase.cpp#newcode107 compiled/ElemHideBase.cpp:107: if (text.find(u"[-abp-properties="_str, data.mSelectorStart) != text.npos) On 2017/03/24 11:59:26, sergei ...
March 25, 2017, 6:48 p.m. (2017-03-25 18:48:19 UTC) #5
Wladimir Palant
March 27, 2017, 7:58 a.m. (2017-03-27 07:58:00 UTC) #6 
https://codereview.adblockplus.org/29383799/diff/29383800/compiled/ElemHideBa...
File compiled/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29383799/diff/29383800/compiled/ElemHideBa...
compiled/ElemHideBase.cpp:107: if (text.find(u"[-abp-properties="_str,
data.mSelectorStart) != text.npos)
On 2017/03/25 18:48:19, Wladimir Palant wrote:
> Nice catch. Actually, what I wrote in the issue matches the current
> implementation, yet what I ended up implemented makes more sense. I've asked
> Felix who wrote the current implementation, IMHO it makes sense to search for
> "[-abp-properties=" there as well.

The conclusion is: I've modified the issue, and we filed
https://issues.adblockplus.org/ticket/5037 on adjusting the current JS-based
code as well.

Powered by Google App Engine
This is Rietveld