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

Issue 29945631: Issue 7097 - Make element hiding filter objects ephemeral

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 months ago by Manish Jethani
Modified:
10 months ago
Reviewers:
hub
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Based on the following patches: https://codereview.adblockplus.org/29934588/ https://codereview.adblockplus.org/29935568/ https://codereview.adblockplus.org/29935564/ This final patch in the series reduces the initial memory footprint with just EasyList by ~5 MB.

Patch Set 1 #

Total comments: 5

Patch Set 2 : Simplify #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -9 lines) Patch
M lib/elemHide.js View 1 1 chunk +2 lines, -1 line 0 comments Download
M lib/filterClasses.js View 1 4 chunks +26 lines, -3 lines 1 comment Download
M lib/iniParser.js View 1 1 chunk +1 line, -1 line 0 comments Download
M lib/subscriptionClasses.js View 1 3 chunks +12 lines, -3 lines 0 comments Download
M lib/synchronizer.js View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 3
Manish Jethani
10 months ago (2018-11-18 03:22:39 UTC) #1
Manish Jethani
Patch Set 1 https://codereview.adblockplus.org/29945631/diff/29945632/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29945631/diff/29945632/lib/filterClasses.js#newcode463 lib/filterClasses.js:463: if (value && this.ephemeral) This could ...
10 months ago (2018-11-18 03:29:12 UTC) #2
Manish Jethani
10 months ago (2018-11-18 06:19:15 UTC) #3
Patch Set 2: Simplify

https://codereview.adblockplus.org/29945631/diff/29945632/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29945631/diff/29945632/lib/filterClasses.j...
lib/filterClasses.js:463: if (value && this.ephemeral)
On 2018/11/18 03:29:12, Manish Jethani wrote:
> This could be optimized a little bit, but this property is never set in the
> current extension. We are keeping it because it might get used in a future
> version (see discussion in #7095).

Ignore, now removed.

https://codereview.adblockplus.org/29945631/diff/29945632/lib/filterClasses.j...
lib/filterClasses.js:716: shouldSerialize()
On 2018/11/18 03:29:12, Manish Jethani wrote:
> Note that this will always return false in the current extension.

Ignore, now removed.

An object created with Filter.fromObject is never ephemeral.

https://codereview.adblockplus.org/29945631/diff/29946555/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29945631/diff/29946555/lib/filterClasses.j...
lib/filterClasses.js:245: Filter.fromText = function(text, persistent = true)
The idea here is that internally we'll always pass false, but UI for example
(for user-defined filters) will not pass anything so it'll default to true.

Rules for the ephemeral property:

 -  If persistent is true, ephemeral is false (otherwise it's the default value)
 -  The value cannot change once the filter object has been returned from here
 -  To persist state like disabled, hit count, etc., create a non-ephemeral
object first and then set the state property, otherwise it'll be lost
 -  Do not hold on to references to ephemeral objects because their state will
sooner or later be out of sync with reality (e.g. removed subscriptions will
still appear)
Sign in to reply to this message.

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