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

Issue 29363607: Issue 4612 - enable AA on first run and make automatic adding of any subscription optional (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years ago by sergei
Modified:
3 years ago
Reviewers:
Oleksandr, Felix Dahlke
Visibility:
Public.

Patch Set 1 #

Total comments: 17

Patch Set 2 : address comments #

Patch Set 3 : address comments #

Total comments: 26

Patch Set 4 : address comments #

Patch Set 5 : reduce number of attempts to remove test files #

Unified diffs Side-by-side diffs Delta from patch set Stats (+115 lines, -12 lines) Patch
M lib/init.js View 1 2 3 4 1 chunk +19 lines, -11 lines 0 comments Download
M lib/prefs.js View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M test/FilterEngine.cpp View 1 2 3 4 3 chunks +93 lines, -0 lines 0 comments Download

Messages

Total messages: 14
sergei
3 years ago (2016-11-21 10:24:49 UTC) #1
Felix Dahlke
Didn't check the test code yet, but looks good all in all. Some comments. https://codereview.adblockplus.org/29363607/diff/29363608/lib/init.js ...
3 years ago (2016-11-21 11:41:11 UTC) #2
sergei
https://codereview.adblockplus.org/29363607/diff/29363608/lib/init.js File lib/init.js (right): https://codereview.adblockplus.org/29363607/diff/29363608/lib/init.js#newcode56 lib/init.js:56: // Choose default subscription and add it On 2016/11/21 ...
3 years ago (2016-11-21 14:03:21 UTC) #3
sergei
https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode48 lib/prefs.js:48: first_run_enable_current_locale_subscription: true, On 2016/11/21 14:03:20, sergei wrote: > On ...
3 years ago (2016-11-22 09:43:03 UTC) #4
Felix Dahlke
https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode35 lib/prefs.js:35: first_run_enable_acceptable_ads: true, On 2016/11/21 14:03:20, sergei wrote: > On ...
3 years ago (2016-11-22 11:45:08 UTC) #5
sergei
https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode48 lib/prefs.js:48: first_run_enable_current_locale_subscription: true, On 2016/11/22 11:45:08, Felix Dahlke wrote: > ...
3 years ago (2016-11-22 11:58:43 UTC) #6
sergei
updated
3 years ago (2016-11-22 12:23:28 UTC) #7
Felix Dahlke
Looks good, just a bunch of nits from looking at the test code. https://codereview.adblockplus.org/29363607/diff/29363751/lib/init.js File ...
3 years ago (2016-11-22 17:37:17 UTC) #8
sergei
https://codereview.adblockplus.org/29363607/diff/29363751/lib/init.js File lib/init.js (right): https://codereview.adblockplus.org/29363607/diff/29363751/lib/init.js#newcode61 lib/init.js:61: FilterStorage.addSubscription(subscription); On 2016/11/22 17:37:16, Felix Dahlke wrote: > Nit: ...
3 years ago (2016-11-23 14:20:51 UTC) #9
sergei
Could you please review this issue? Since it blocks #4604, what do you think about ...
3 years ago (2016-12-02 08:41:38 UTC) #10
Oleksandr
JS part LGTM. The tests look somewhat hacky indeed, and since we are still not ...
3 years ago (2016-12-02 11:40:29 UTC) #11
Felix Dahlke
Sorry, forgot to comment here, thanks for the reminder. LGTM! My gut feeling would be ...
3 years ago (2016-12-02 12:26:34 UTC) #12
sergei
I have rebased on master and changed the number of iterations in tests, because otherwise ...
3 years ago (2016-12-02 14:17:03 UTC) #13
Felix Dahlke
3 years ago (2016-12-02 14:20:24 UTC) #14
LGTM
Sign in to reply to this message.

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