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

Issue 29935564: Issue 7452 - Do not cache element hiding filter objects by default

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 3 weeks ago by Manish Jethani
Modified:
1 month, 2 weeks ago
Reviewers:
hub
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Patch Set 1 #

Total comments: 5

Patch Set 2 : Rename filter* to filterText* #

Total comments: 3

Patch Set 3 : Rebase on GitLab MR #24 #

Total comments: 4

Patch Set 4 : Rebase on next #

Patch Set 5 : Rename filter* to filterText* #

Unified diffs Side-by-side diffs Delta from patch set Stats (+87 lines, -69 lines) Patch
M lib/elemHide.js View 1 2 3 4 7 chunks +51 lines, -48 lines 0 comments Download
M lib/filterClasses.js View 1 2 3 chunks +17 lines, -2 lines 0 comments Download
M lib/filterListener.js View 1 2 2 chunks +7 lines, -7 lines 0 comments Download
M test/elemHide.js View 1 2 3 4 2 chunks +9 lines, -9 lines 0 comments Download
M test/filterListener.js View 1 2 3 4 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 13
Manish Jethani
6 months, 3 weeks ago (2018-11-03 19:54:05 UTC) #1
Manish Jethani
Patch Set 1 The plan is to make Filter objects (1) stateless and (2) ephemeral ...
6 months, 3 weeks ago (2018-11-03 20:01:25 UTC) #2
hub
https://codereview.adblockplus.org/29935564/diff/29935565/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29935564/diff/29935565/lib/elemHide.js#newcode38 lib/elemHide.js:38: * Lookup table, active flag, by filter by domain. ...
6 months ago (2018-11-22 15:59:34 UTC) #3
Manish Jethani
Patch Set 2: Rename filter* to filterText* I've changed the named of the variables as ...
6 months ago (2018-11-23 02:03:34 UTC) #4
Manish Jethani
On 2018/11/23 02:03:34, Manish Jethani wrote: > I've changed the named of the variables *names
6 months ago (2018-11-23 02:04:07 UTC) #5
Manish Jethani
By the way, I won't land this until all the other related patches have also ...
6 months ago (2018-11-23 02:05:43 UTC) #6
hub
https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js#newcode244 lib/elemHide.js:244: addToFiltersByDomain(Filter.fromText(unconditionalFilterTextForSelector)); So here, we call Filter.fromText() while before we ...
5 months, 4 weeks ago (2018-11-28 23:21:31 UTC) #7
Manish Jethani
https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js#newcode244 lib/elemHide.js:244: addToFiltersByDomain(Filter.fromText(unconditionalFilterTextForSelector)); On 2018/11/28 23:21:31, hub wrote: > So here, ...
1 month, 2 weeks ago (2019-04-07 18:05:09 UTC) #8
Manish Jethani
Patch Set 3: Rebase on GitLab MR #24 I have rebased this on a patch ...
1 month, 2 weeks ago (2019-04-07 18:14:12 UTC) #9
Manish Jethani
https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29935564/diff/29950555/lib/elemHide.js#newcode244 lib/elemHide.js:244: addToFiltersByDomain(Filter.fromText(unconditionalFilterTextForSelector)); On 2019/04/07 18:05:09, Manish Jethani wrote: > On ...
1 month, 2 weeks ago (2019-04-07 18:55:11 UTC) #10
hub
LGTM https://codereview.adblockplus.org/29935564/diff/30040555/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29935564/diff/30040555/lib/filterClasses.js#newcode141 lib/filterClasses.js:141: return !(filter instanceof ElemHideFilter); On 2019/04/07 18:14:11, Manish ...
1 month, 2 weeks ago (2019-04-08 20:44:54 UTC) #11
Manish Jethani
Patch Set 4: Rebase on next Patch Set 5: Rename filter* to filterText*
1 month, 2 weeks ago (2019-04-08 22:01:38 UTC) #12
Manish Jethani
1 month, 2 weeks ago (2019-04-09 02:55:31 UTC) #13
On 2019/04/08 22:01:38, Manish Jethani wrote:
> Patch Set 4: Rebase on next
> Patch Set 5: Rename filter* to filterText*

I have filed a new issue for this since the original ticket is now closed:

https://issues.adblockplus.org/ticket/7452

We still cannot land this because if we do then it would make element hiding
filters unable to retain their state, like whether a filter is disabled, how
many hits it got, and so on. While we do not use these properties in the current
WebExt version, we still support them so they must keep working. I have
described this in the issue.
Sign in to reply to this message.

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