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

Issue 5653480979038208: Issue 2325 - Add a way to set settings in libadblockplus for FRP and automatic updates (Closed)

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

Description

Looks kind of hacky, but that's what we want for now.

Patch Set 1 #

Total comments: 12

Patch Set 2 : Make the preconfiguration generic. Move FRP suppression to ABP engine. #

Total comments: 8

Patch Set 3 : Added a unit test. Changed the global object injection routine. #

Total comments: 27

Patch Set 4 : Simplify the setting of global property. Add more tests. #

Total comments: 14

Patch Set 5 : Make sure a pref is stored if it was modified. Add test. Address nits. #

Total comments: 8

Patch Set 6 : Refactored the defaults/preconfiguration relations #

Total comments: 2

Patch Set 7 : Simplify loading of the defaults #

Total comments: 8

Patch Set 8 : Address the nits #

Patch Set 9 : Remove the superfluous white space #

Total comments: 20

Patch Set 10 : Address more comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+130 lines, -8 lines) Patch
M include/AdblockPlus/FilterEngine.h View 1 chunk +10 lines, -1 line 0 comments Download
M include/AdblockPlus/JsEngine.h View 2 chunks +8 lines, -0 lines 0 comments Download
M lib/prefs.js View 2 chunks +15 lines, -2 lines 0 comments Download
M lib/updater.js View 1 chunk +4 lines, -0 lines 0 comments Download
M src/FilterEngine.cpp View 2 chunks +12 lines, -1 line 0 comments Download
M src/JsEngine.cpp View 2 chunks +14 lines, -2 lines 0 comments Download
M test/JsEngine.cpp View 1 chunk +10 lines, -0 lines 0 comments Download
M test/Prefs.cpp View 2 chunks +57 lines, -2 lines 0 comments Download

Messages

Total messages: 30
Oleksandr
4 years, 6 months ago (2015-04-17 03:59:29 UTC) #1
sergei
http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/updater.js File lib/updater.js (right): http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/updater.js#newcode37 lib/updater.js:37: function getDownloadable(forceCheck) In downloader.js there is a function which ...
4 years, 6 months ago (2015-04-21 12:35:24 UTC) #2
Eric
http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/include/AdblockPlus/FilterEngine.h#newcode196 include/AdblockPlus/FilterEngine.h:196: explicit FilterEngine(JsEnginePtr jsEngine, bool firstRunDisabled = false, bool autoUpdatesDisabled ...
4 years, 5 months ago (2015-05-15 19:36:13 UTC) #3
Felix Dahlke
http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/include/AdblockPlus/FilterEngine.h#newcode196 include/AdblockPlus/FilterEngine.h:196: explicit FilterEngine(JsEnginePtr jsEngine, bool firstRunDisabled = false, bool autoUpdatesDisabled ...
4 years, 4 months ago (2015-05-28 20:42:56 UTC) #4
Oleksandr
Addressed comments. Sample usage of the override can be found here: http://codereview.adblockplus.org/4766634489151488/
4 years, 4 months ago (2015-06-03 13:42:54 UTC) #5
Felix Dahlke
On 2015/06/03 13:42:54, Oleksandr wrote: > Addressed comments. Sample usage of the override can be ...
4 years, 4 months ago (2015-06-09 13:16:43 UTC) #6
Oleksandr
Yes, you're right. Fixed now.
4 years, 4 months ago (2015-06-09 13:37:56 UTC) #7
Felix Dahlke
libadblockplus has pretty good test coverage so we should definitely add some unit tests that ...
4 years, 4 months ago (2015-06-09 19:40:51 UTC) #8
Felix Dahlke
Two more things actually: 1. We should add default values for the newly introduced prefs ...
4 years, 4 months ago (2015-06-09 19:45:50 UTC) #9
Oleksandr
> libadblockplus has pretty good test coverage so we should definitely add some > unit ...
4 years, 4 months ago (2015-06-10 08:40:30 UTC) #10
Eric
http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/include/AdblockPlus/FilterEngine.h#newcode197 include/AdblockPlus/FilterEngine.h:197: std::map<std::string, AdblockPlus::JsValuePtr>() It would be useful to add a ...
4 years, 4 months ago (2015-06-10 17:48:16 UTC) #11
Felix Dahlke
On 2015/06/10 08:40:30, Oleksandr wrote: > > libadblockplus has pretty good test coverage so we ...
4 years, 4 months ago (2015-06-11 20:20:50 UTC) #12
Felix Dahlke
Agree with Eric's comments where I didn't say otherwise. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/prefs.js#newcode49 lib/prefs.js:49: ...
4 years, 4 months ago (2015-06-11 20:21:13 UTC) #13
Oleksandr
http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/prefs.js#newcode90 lib/prefs.js:90: values[key] = _preconfiguredPrefs[key]; > As written, this will overwrite ...
4 years, 4 months ago (2015-06-12 07:36:14 UTC) #14
Felix Dahlke
http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/prefs.js#newcode90 lib/prefs.js:90: values[key] = _preconfiguredPrefs[key]; On 2015/06/12 07:36:14, Oleksandr wrote: > ...
4 years, 4 months ago (2015-06-12 07:55:03 UTC) #15
Felix Dahlke
Didn't realise there was a new patch set, looks pretty good now. http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/include/AdblockPlus/JsEngine.h File include/AdblockPlus/JsEngine.h ...
4 years, 4 months ago (2015-06-12 08:04:20 UTC) #16
Oleksandr
Addressed all of the comments now, I think. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/include/AdblockPlus/FilterEngine.h#newcode197 include/AdblockPlus/FilterEngine.h:197: std::map<std::string, ...
4 years, 4 months ago (2015-06-12 10:47:57 UTC) #17
Felix Dahlke
Getting close :) http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/include/AdblockPlus/JsEngine.h File include/AdblockPlus/JsEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/include/AdblockPlus/JsEngine.h#newcode217 include/AdblockPlus/JsEngine.h:217: * @param The `JsValuePtr` value the ...
4 years, 4 months ago (2015-06-12 11:55:35 UTC) #18
Oleksandr
Fingers crossed. http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/include/AdblockPlus/JsEngine.h File include/AdblockPlus/JsEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/include/AdblockPlus/JsEngine.h#newcode217 include/AdblockPlus/JsEngine.h:217: * @param The `JsValuePtr` value the property ...
4 years, 4 months ago (2015-06-12 12:29:59 UTC) #19
Felix Dahlke
Sorry :P http://codereview.adblockplus.org/5653480979038208/diff/6025343047565312/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6025343047565312/lib/prefs.js#newcode148 lib/prefs.js:148: for (let key in _preconfiguredPrefs) Two things ...
4 years, 4 months ago (2015-06-12 12:37:08 UTC) #20
Oleksandr
Ouch. Totally missed your message on Friday. Done now, but I think diffs between patch ...
4 years, 4 months ago (2015-06-17 13:27:42 UTC) #21
Felix Dahlke
Managed to find some more small stuff :D But it's looking good now. https://codereview.adblockplus.org/5653480979038208/diff/29317033/include/AdblockPlus/JsEngine.h File ...
4 years, 4 months ago (2015-06-17 18:49:26 UTC) #22
Oleksandr
https://codereview.adblockplus.org/5653480979038208/diff/29317033/include/AdblockPlus/JsEngine.h File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/5653480979038208/diff/29317033/include/AdblockPlus/JsEngine.h#newcode215 include/AdblockPlus/JsEngine.h:215: * Sets a global property that can be accessed ...
4 years, 4 months ago (2015-06-17 23:53:29 UTC) #23
Felix Dahlke
LGTM
4 years, 4 months ago (2015-06-18 11:17:16 UTC) #24
sergei
In general LGTM. https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/include/AdblockPlus/FilterEngine.h#newcode193 include/AdblockPlus/FilterEngine.h:193: * name-value list of preconfigured prefs. ...
4 years, 4 months ago (2015-06-19 10:07:17 UTC) #25
Eric
https://codereview1.adblockplus.org/5653480979038208/diff/29317062/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview1.adblockplus.org/5653480979038208/diff/29317062/include/AdblockPlus/FilterEngine.h#newcode187 include/AdblockPlus/FilterEngine.h:187: typedef std::map<std::string, AdblockPlus::JsValuePtr> Prefs; Since this file has documentation ...
4 years, 4 months ago (2015-06-19 15:23:39 UTC) #26
Felix Dahlke
https://codereview1.adblockplus.org/5653480979038208/diff/29317062/lib/prefs.js File lib/prefs.js (right): https://codereview1.adblockplus.org/5653480979038208/diff/29317062/lib/prefs.js#newcode49 lib/prefs.js:49: let preconfigurable = ["suppress_first_run_page", "disable_auto_updates"]; On 2015/06/19 10:07:16, sergei ...
4 years, 3 months ago (2015-06-22 07:34:02 UTC) #27
Oleksandr
https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/include/AdblockPlus/FilterEngine.h#newcode193 include/AdblockPlus/FilterEngine.h:193: * name-value list of preconfigured prefs. On 2015/06/19 10:07:15, ...
4 years, 3 months ago (2015-06-22 07:47:15 UTC) #28
sergei
https://codereview.adblockplus.org/5653480979038208/diff/29317062/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/5653480979038208/diff/29317062/lib/prefs.js#newcode49 lib/prefs.js:49: let preconfigurable = ["suppress_first_run_page", "disable_auto_updates"]; Let me break the ...
4 years, 3 months ago (2015-06-22 09:10:50 UTC) #29
Eric
4 years, 3 months ago (2015-06-25 17:44:41 UTC) #30
LGTM

https://codereview.adblockplus.org/5653480979038208/diff/29317062/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/5653480979038208/diff/29317062/lib/prefs.j...
lib/prefs.js:49: let preconfigurable = ["suppress_first_run_page",
"disable_auto_updates"];
On 2015/06/22 09:10:49, sergei wrote:
> what
> happens if we specify pre-configurable setting without having any default
value
> for it?

It is bad practice not to specify default values for these settings. Don't do
that.

It would be useful to have unit tests to ensure that they all do have default
values, but I'm not going to insist on it now.

https://codereview.adblockplus.org/5653480979038208/diff/29317062/lib/updater.js
File lib/updater.js (right):

https://codereview.adblockplus.org/5653480979038208/diff/29317062/lib/updater...
lib/updater.js:39: if (!forceCheck && Pref.disable_auto_updates)
On 2015/06/22 07:47:11, Oleksandr wrote:
> By this logic, we'll also have to rename the suppress_first_run to something
> like display_first_run.

That's right.

> But we are trying to be as consistent across browsers as
> possible, and since we already have suppress_first_run in Chrome

The same argument I've made applies to Chrome, too. It's not a great idea to
name variables for behavior after their default values.

> I'd leave this as is too.

I didn't make this comment to insist upon renaming the variable so much as to
capture my insight about why I disliked the variable name. I'm fine with leaving
the names as is.

> Also, if there was a performAutoUpdate setting, it would be more
> ambivalent - should set it to true JIC, or not?

Yes, you set it to true. Explicit default values should be present for every
preference. To do otherwise is asking for trouble down the road.
Sign in to reply to this message.

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