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

Issue 29805597: Issue 6699 - Support the "circumvention" filter list as default subscription

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 days, 5 hours ago by hub
Modified:
3 days, 13 hours ago
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Issue 6699 - Support the "circumvention" filter list as default subscription

Patch Set 1 #

Total comments: 34

Patch Set 2 : Updated comments/doc #

Total comments: 5

Patch Set 3 : Use "circumvention" instead of "cv" #

Total comments: 6

Patch Set 4 : More review fixes. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+293 lines, -33 lines) Patch
M lib/subscriptionInit.js View 1 2 3 3 chunks +58 lines, -33 lines 0 comments Download
A qunit/subscriptions.xml View 1 2 3 1 chunk +173 lines, -0 lines 0 comments Download
A qunit/tests/subscriptionInit.js View 1 2 3 1 chunk +62 lines, -0 lines 0 comments Download

Messages

Total messages: 13
hub
5 days, 5 hours ago (2018-06-13 03:33:53 UTC) #1
hub
This implement the logic changes to select a default "cv" subscription. The UI changes are ...
5 days, 5 hours ago (2018-06-13 03:36:24 UTC) #2
Manish Jethani
https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js#newcode90 lib/subscriptionInit.js:90: * Finds the element for the default ad blocking ...
5 days, 1 hour ago (2018-06-13 08:15:17 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js#newcode128 lib/subscriptionInit.js:128: // If multiple items have a matching prefix of ...
4 days, 23 hours ago (2018-06-13 09:42:29 UTC) #4
Manish Jethani
https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js#newcode115 lib/subscriptionInit.js:115: // The "ads" subscription is the one driving the ...
4 days, 19 hours ago (2018-06-13 13:34:02 UTC) #5
Manish Jethani
https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscriptionInit.js File qunit/tests/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscriptionInit.js#newcode41 qunit/tests/subscriptionInit.js:41: .then(response => response.text()) This appears not to be mentioned ...
4 days, 19 hours ago (2018-06-13 13:45:39 UTC) #6
hub
updated patch https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js#newcode90 lib/subscriptionInit.js:90: * Finds the element for the default ...
4 days, 16 hours ago (2018-06-13 16:33:37 UTC) #7
Manish Jethani
https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js#newcode110 lib/subscriptionInit.js:110: if (subscriptionType in ["ads", "cv"] && !selectedItem[subscriptionType]) On 2018/06/13 ...
4 days, 16 hours ago (2018-06-13 16:53:10 UTC) #8
Manish Jethani
https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionInit.js#newcode225 lib/subscriptionInit.js:225: for (let name in subs) By the way, if ...
4 days, 16 hours ago (2018-06-13 16:58:12 UTC) #9
hub
Updated patch https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js#newcode139 lib/subscriptionInit.js:139: else if (subscriptionType == "cv") On 2018/06/13 ...
4 days, 11 hours ago (2018-06-13 21:58:19 UTC) #10
Manish Jethani
https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionInit.js#newcode217 lib/subscriptionInit.js:217: let subs = chooseFilterSubscriptions(nodes); On 2018/06/13 21:58:18, hub wrote: ...
4 days, 4 hours ago (2018-06-14 05:07:34 UTC) #11
Manish Jethani
https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionInit.js#newcode225 lib/subscriptionInit.js:225: for (let name in subs) On 2018/06/14 05:07:34, Manish ...
4 days, 3 hours ago (2018-06-14 05:42:57 UTC) #12
hub
3 days, 13 hours ago (2018-06-14 19:59:57 UTC) #13
Updated patch

https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni...
lib/subscriptionInit.js:225: for (let name in subs)
On 2018/06/14 05:42:56, Manish Jethani wrote:
> On 2018/06/14 05:07:34, Manish Jethani wrote:
> > On 2018/06/13 21:58:19, hub wrote:
> > > On 2018/06/13 16:58:12, Manish Jethani wrote:
> > > > By the way, if you want to keep the return type a plain object rather
than
> a
> > > Map
> > > > object, I think Object.keys would be faster and more appropriate because
> it
> > > only
> > > > returns the _own_ enumerable properties.
> > > 
> > > There are at most two properties.
> > 
> > Acknowledged.
> > 
> > Since the calling code is now assuming the number of properties, maybe it
> could
> > assume the names as well? So `for (let name of ["ads", "circumvention"])`.
> 
> OK, forget this, it makes it worse.
> 
> Like you said it doesn't matter in this case, but when the names of the keys
are
> unknown (coming from an XML file for example), a Map object performs
> significantly better (regardless of the number of keys).
> 
> Here's a test:
> 
>   (function()
>   {
>     function choose()
>     {
>       //return new Map([
>       //  ["" + Math.random(), (Math.random() * 2) | 0],
>       //  ["" + Math.random(), (Math.random() * 3) | 0]
>       //]);
>       return {
>         ["" + Math.random()]: (Math.random() * 2) | 0,
>         ["" + Math.random()]: (Math.random() * 3) | 0
>       };
>     }
> 
>     let array = new Array(100000);
>     for (let i = 0; i < array.length; i++)
>       array[i] = choose();
> 
>     let s = Date.now();
>     let n = 0;
>     for (let i = 0; i < array.length; i++)
>     {
>       let item = array[i];
>       //for (let [, value] of item)
>       //  n += value;
>       for (let prop in item)
>         n += item[prop];
>     }
>     console.log(Date.now() - s, n);
>   })();

Acknowledged.

https://codereview.adblockplus.org/29805597/diff/29806576/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29805597/diff/29806576/lib/subscriptionIni...
lib/subscriptionInit.js:223: let subs = chooseFilterSubscriptions(nodes);
On 2018/06/14 05:07:34, Manish Jethani wrote:
> Just a thought: Maybe a good idea to rename `subs` to `defaultSubscriptions`?
> Not sure.

Done.

https://codereview.adblockplus.org/29805597/diff/29806576/qunit/subscriptions...
File qunit/subscriptions.xml (right):

https://codereview.adblockplus.org/29805597/diff/29806576/qunit/subscriptions...
qunit/subscriptions.xml:151: prefixes="en"
On 2018/06/14 05:07:34, Manish Jethani wrote:
> What do you think about making the prefixes here "fr,en,en-US"? It would test
a
> couple of other parts of the logic.

Good idea. Done.

https://codereview.adblockplus.org/29805597/diff/29806576/qunit/tests/subscri...
File qunit/tests/subscriptionInit.js (right):

https://codereview.adblockplus.org/29805597/diff/29806576/qunit/tests/subscri...
qunit/tests/subscriptionInit.js:33: // TODO restore appLocale
On 2018/06/14 05:07:34, Manish Jethani wrote:
> We don't normally leave "todo" comments in the code, from what I can tell. Is
> this still a work in progress?

I had forgotten to remove it, done.
Sign in to reply to this message.

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