Issue 5653480979038208: Issue 2325 - Add a way to set settings in libadblockplus for FRP and automatic updates (Closed)
|
Created:
April 17, 2015, 3:54 a.m. by Oleksandr Modified:
June 30, 2015, 12:47 a.m. Visibility:
Public. |
DescriptionLooks 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 #
MessagesTotal messages: 30
http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/... File lib/updater.js (right): http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/... lib/updater.js:37: function getDownloadable(forceCheck) In downloader.js there is a function which is called by timer `_doCheck: function()` which evaluates the following code `for (let downloadable of this.dataSource()){...}`. I don't know how it looks after our transformations but semantically it seems that to disable we should return an empty array or null or undefined. http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/... lib/updater.js:50: yield getDownloadable(!!autoupdatesDisabled); I think we should not touch this line because it influences merely the url. http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/src/... File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/src/... src/FilterEngine.cpp:147: jsEngine->Evaluate(std::string("var firstRunDisabled = ") + (firstRunDisabled ? "true;" : "false;")); Would it be better to add them at least as members of `_appInfo`?
http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:196: explicit FilterEngine(JsEnginePtr jsEngine, bool firstRunDisabled = false, bool autoUpdatesDisabled = false); Even at this early stage, I think it would be better to pass in an initialization structure rather than an explicit list of arguments that we're using at the moment. A list of name-value pairs would suffice here. The name is a variable name; the value its initial value. We're already invoking the interpreter to do the initialization. With hardly more effort we could do initialization here more generally and more flexibly. If this were just for the IE version, I don't know that my opinion would be the same. But we can anticipate that there may be future pre-customization requirements for Android. http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/... File lib/init.js (right): http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/... lib/init.js:29: _triggerEvent("init", (!!firstRunDisabled ? false : require("filterStorage").FilterStorage.firstRun)); I don't think this works correctly, or if it does, it's not at all clear why. First-run does both initialization of filter lists into storage and display of a UI element (display of the first run page). As I understand our requirement, we only want to suppress the UI element, not the other initialization. The way this statement is written, it suppress both actions. http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/... File lib/updater.js (right): http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/... lib/updater.js:32: let downloader = new Downloader(getDownloadables, INITIAL_DELAY, CHECK_INTERVAL); The actual timer is in the 'Downloader' prototype. To avoid the timer entirely, avoid instantiating the object 'downloader' as a 'new Downloader'. Personally, I'd just not instantiate it at all. That would mean including the next three lines in the condition block (easy) and adding a condition in 'checkForUpdates' below, which is needed anyway. All the other references to this variable are within handlers below that won't be called without a 'Downloader' instance activating them. http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/... lib/updater.js:50: yield getDownloadable(!!autoupdatesDisabled); On 2015/04/21 12:35:24, sergei wrote: > I think we should not touch this line because it influences merely the url. Agreed. http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/... lib/updater.js:101: let checkForUpdates = exports.checkForUpdates = function checkForUpdates(_callback) We also need to intervene here, since this is where explicit, manual update checks ultimately arrive. The correct behavior is likely to immediately call the callback with a suitable status message. Aside: It would also be good to disable the menu item in the plugin UI, but I understand that's rather beyond the scope of the present change set. http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/src/... File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/src/... src/FilterEngine.cpp:147: jsEngine->Evaluate(std::string("var firstRunDisabled = ") + (firstRunDisabled ? "true;" : "false;")); On 2015/04/21 12:35:24, sergei wrote: > Would it be better to add them at least as members of `_appInfo`? Not particularly. Application info is a description of the browser host, not how ABP is configured. I do agree that it would be good to add these to _some_ global object, if only to mark them more clearly. I'd call such a global object (unsurprisingly I hope) 'configuration'. http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/src/... src/FilterEngine.cpp:148: jsEngine->Evaluate(std::string("var autoupdatesDisabled = ") + (autoUpdatesDisabled ? "true;" : "false;")); My suggestion to use name value pairs would change these evaluations to something like this: (lots of newlines to avoid wrapping) jsEngine->Evaluate( std::string("configuration.") + entry.first + "=" + entry.second + ";" ); This would be the body of an iterator, with name/value pairs declared as "std::vector<std::pair>".
http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:196: explicit FilterEngine(JsEnginePtr jsEngine, bool firstRunDisabled = false, bool autoUpdatesDisabled = false); On 2015/05/15 19:36:13, Eric wrote: > Even at this early stage, I think it would be better to pass in an > initialization structure rather than an explicit list of arguments that we're > using at the moment. > > A list of name-value pairs would suffice here. The name is a variable name; the > value its initial value. We're already invoking the interpreter to do the > initialization. With hardly more effort we could do initialization here more > generally and more flexibly. > > If this were just for the IE version, I don't know that my opinion would be the > same. But we can anticipate that there may be future pre-customization > requirements for Android. I agree - in fact, I had something more generic in mind: We could just pass in an `std::map<std::string, std::string> preconfiguredPrefs` here that holds default pref values, which can then overwrite the ones read from lib/prefs.js. That's in line with how preconfiguration works on the other platforms so far, and it's quite flexible, so I think we should do that. http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/... File lib/init.js (right): http://codereview.adblockplus.org/5653480979038208/diff/5629499534213120/lib/... lib/init.js:29: _triggerEvent("init", (!!firstRunDisabled ? false : require("filterStorage").FilterStorage.firstRun)); On 2015/05/15 19:36:13, Eric wrote: > I don't think this works correctly, or if it does, it's not at all clear why. > > First-run does both initialization of filter lists into storage and display of a > UI element (display of the first run page). As I understand our requirement, we > only want to suppress the UI element, not the other initialization. The way this > statement is written, it suppress both actions. Yes, I'm afraid we'll have to respect the suppress_first_run_page pref (that's how it's called on Firefox and Chrome) in the client code, i.e. in IE :/
Addressed comments. Sample usage of the override can be found here: http://codereview.adblockplus.org/4766634489151488/
On 2015/06/03 13:42:54, Oleksandr wrote: > Addressed comments. Sample usage of the override can be found here: > http://codereview.adblockplus.org/4766634489151488/ Something's funny with the new patch set, is it based on patch set 1 rather than the original state?
Yes, you're right. Fixed now.
libadblockplus has pretty good test coverage so we should definitely add some unit tests that cover both preconfiguring prefs and disabling the updater. http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/incl... include/AdblockPlus/FilterEngine.h:189: * Constructor. Allows overloading the prefs Nit: Kind of redundant with the parameter description, I'd just leave this part as it is. http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/incl... include/AdblockPlus/FilterEngine.h:192: * @param preconfiguredPrefs `std::map<std::string, std::string>` name-value list of preconfigured Prefs Nit: 80 columns? We usually manage to stick to that at least in comments :) Also, Prefs with a lower case "p", plus a period at the end. http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/incl... include/AdblockPlus/FilterEngine.h:194: explicit FilterEngine(JsEnginePtr jsEngine, std::map<std::string, std::string> preconfiguredPrefs = std::map<std::string, std::string>()); Nit: Put preconfiguredPrefs on its own line? Probably won't fit on 80 columns either but it's closer that way. We could probably make things short enough by using a typedef for the map, but that's up to you. http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/lib/... File lib/init.js (right): http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/lib/... lib/init.js:42: Prefs[pref] = Preconfig[pref]; Rather than always overwriting the prefs, this is about preconfiguring them, that is, just providing the defaults, but still allowing the client to change prefs. I suppose this logic should go into prefs.js:load() - before attempting to read from the file system, we overwrite values[key] for each key in preconfiguredPrefs. http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/lib/... File lib/updater.js (right): http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/lib/... lib/updater.js:39: if (!forceCheck && Preconfig.disable_auto_updates) I think we should be using Pref.disable_auto_updates here instead - the object with preconfigured prefs is only relevant for initialisation, prefs can be changed later. Applications could change this pref to make automatic updates configurable, e.g. http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/src/... File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/src/... src/FilterEngine.cpp:149: jsEngine->Evaluate(std::string("var Preconfig = {};")); 1. _preconfiguredPrefs would be a better name. We usually prefix internal globals provided by libadblockplus with _, and being an object and not a module or type, it should start with a lower case letter. 2. I think using JsEngine->Evaluate() to populate this is too hacky. We're currently setting all globals in JsEngine, which is why AppInfo is passed into it - that however is bad as well. So what I believe we should do at this stage is: Add an accessor for the global JsValue to JsEngine. With that, we can create the object using JsEngine->NewObject() and use the JsValue API to populate it, then add it as a property to the global JsValue. (We should actually use the same approach for AppInfo too, then, but that's something for a follow-up issue/patch.) http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/src/... src/FilterEngine.cpp:150: for (auto it = preconfiguredPrefs.begin(); it != preconfiguredPrefs.end(); it++) It's still C++03 here.
Two more things actually: 1. We should add default values for the newly introduced prefs (both the one for disabling updates and suppress_first_run_page) in prefs.js. 2. Not all prefs should be preconfigurable, there should be a whitelist as on other platforms. Basically just a "let preconfigurable = [...]" right below "let defaults = " in prefs.js.
> libadblockplus has pretty good test coverage so we should definitely add some > unit tests that cover both preconfiguring prefs and disabling the updater. I have added a test for preconfiguring. However automatic update is started 6 minutes after the engine start - I really don't think we should be having a test that sticks around that long. The INITIAL_DELAY doesn't seem to be overridable. http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/src/... File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/5718998062727168/src/... src/FilterEngine.cpp:149: jsEngine->Evaluate(std::string("var Preconfig = {};")); On 2015/06/09 19:40:51, Felix H. Dahlke wrote: > 1. _preconfiguredPrefs would be a better name. We usually prefix internal > globals provided by libadblockplus with _, and being an object and not a module > or type, it should start with a lower case letter. > > 2. I think using JsEngine->Evaluate() to populate this is too hacky. We're > currently setting all globals in JsEngine, which is why AppInfo is passed into > it - that however is bad as well. So what I believe we should do at this stage > is: Add an accessor for the global JsValue to JsEngine. With that, we can create > the object using JsEngine->NewObject() and use the JsValue API to populate it, > then add it as a property to the global JsValue. (We should actually use the > same approach for AppInfo too, then, but that's something for a follow-up > issue/patch.) I agree it was hacky, but it had it's advantages. There was no need to carry the type of the preconfig this way. Which is both good and bad, I suppose. Now the preconfig is JsValuePtr not just a string.
http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/incl... include/AdblockPlus/FilterEngine.h:197: std::map<std::string, AdblockPlus::JsValuePtr>() It would be useful to add a class-member typedef for this map type. It appears fairly frequently. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/incl... File include/AdblockPlus/JsEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/incl... include/AdblockPlus/JsEngine.h:217: * @param The `JsValuePtr` to use @param is for the following function, not this one. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/incl... include/AdblockPlus/JsEngine.h:224: void SetGlobalJsObject(JsValuePtr globalJsObject); SetGlobalJsObject() is never called. It probably should never be called, either, I'm guessing. If 'JsEngine' were being dispensed from a pool, there would be a need of reconfiguring it on the fly. But that's not how we're doing it. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... File lib/init.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... lib/init.js:36: Nit: no need for blank line. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... lib/prefs.js:90: values[key] = _preconfiguredPrefs[key]; Shouldn't this first check to see if there is a preconfiguration preference stated? As written, this will overwrite ones that are not so stated with 'null'. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... File lib/updater.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... lib/updater.js:39: if (!forceCheck && Pref.disable_auto_updates) This would read more clearly if we were using "enable_auto_updates". http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/src/... File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/src/... src/FilterEngine.cpp:158: globalJsObject->SetProperty("_preconfiguredPrefs", preconfiguredPrefsObject); Apart from initialization (where 'GetGlobalJsObject' isn't needed), this is the only place where 'GetGlobalJsObject' is used. It would be just as good to define 'JsEngine::SetGlobalProperty'. Properties are only read out in JavaScript code, so there's no need for a corresponding "Get..." version. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/src/... File src/JsEngine.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/src/... src/JsEngine.cpp:73: AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const AppInfo& appInfo) This is perhaps beyond the scope of this change set, but there doesn't seem to be anything here that couldn't be done in the 'JsEngine' constructor. There seems to be no need for a factory method. I mention this principally to point out that this is the _de facto_ constructor for 'JsEngine', and this is salient to the initialization of 'globalJsObject'. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/src/... src/JsEngine.cpp:231: if (!globalJsObject) There's really no need to use deferred evaluation for this. 'globalJsObject' is a member constant, as it's initialized in the (_de facto_) constructor and never modified thereafter. (Reason: 'SetGlobalJsObject' is never called.) 'globalJsOjbect' should be initialized in the constructor, or, barring that, in the factory method 'New'. This function should be a simple 'const' accessor. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/test... File test/Prefs.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/test... test/Prefs.cpp:163: ASSERT_FALSE(filterEngine->GetPref("unsupported_preconfig")->AsBool()); This assertion should really be in a separate test, since it's exercising a different piece of the code. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/test... test/Prefs.cpp:164: } With preconfigurable preferences, we now have three different sources of preferences: (a) defaults, (b) preconfigured values, (c) user-set values. We need tests for the different combinations of these. The existing test exercises (a) vs. (b). Since (a) is always present as a source, there are three more combinations that need testing. -- (a) only -- (a) and (c) -- all of (a), (b), and (c)
On 2015/06/10 08:40:30, Oleksandr wrote: > > libadblockplus has pretty good test coverage so we should definitely add some > > unit tests that cover both preconfiguring prefs and disabling the updater. > I have added a test for preconfiguring. However automatic update is started 6 > minutes after the engine start - I really don't think we should be having a test > that sticks around that long. The INITIAL_DELAY doesn't seem to be overridable. Thats true... In the notification tests we require INITIAL_DELAY to be changed in notification.js which is too big of a hack to repeat IMHO. Can you file a follow-up issue for testing this properly? Something for a rainy evening I suppose. We CAN however test that forced update checks still happen regardless of the pref - better than nothing :)
Agree with Eric's comments where I didn't say otherwise. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... lib/prefs.js:49: let isPreconfigurable = { I'd prefer this to work the same way as in Firefox, namely: let preconfigurable = ["suppress_first_run_page", "disable_auto_updates"]; http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... lib/prefs.js:90: values[key] = _preconfiguredPrefs[key]; On 2015/06/10 17:48:16, Eric wrote: > Shouldn't this first check to see if there is a preconfiguration preference > stated? As written, this will overwrite ones that are not so stated with 'null'. Yes, should be: `if (key in _preconfiguredPrefs && preconfigurable.indexOf(key) != -1)` http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... File lib/updater.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... lib/updater.js:39: if (!forceCheck && Pref.disable_auto_updates) On 2015/06/10 17:48:16, Eric wrote: > This would read more clearly if we were using "enable_auto_updates". I've wondered about that too, but IMHO disable makes more sense, since the default behaviour is to check for updates and this pref is about disabling that, same as with suppress_first_run_page. Wouldn't mind either way however. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/src/... File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/src/... src/FilterEngine.cpp:158: globalJsObject->SetProperty("_preconfiguredPrefs", preconfiguredPrefsObject); On 2015/06/10 17:48:16, Eric wrote: > Apart from initialization (where 'GetGlobalJsObject' isn't needed), this is the > only place where 'GetGlobalJsObject' is used. It would be just as good to define > 'JsEngine::SetGlobalProperty'. Properties are only read out in JavaScript code, > so there's no need for a corresponding "Get..." version. For the record: I asked for it this way. But Eric's got a point, that would simplify things a bit, and I suppose we don't have to be overly general purpose here. Either way, I think we want to have a test for this in test/JsEngine.cpp. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/src/... File src/JsEngine.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/src/... src/JsEngine.cpp:73: AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const AppInfo& appInfo) On 2015/06/10 17:48:16, Eric wrote: > This is perhaps beyond the scope of this change set, but there doesn't seem to > be anything here that couldn't be done in the 'JsEngine' constructor. There > seems to be no need for a factory method. > > I mention this principally to point out that this is the _de facto_ constructor > for 'JsEngine', and this is salient to the initialization of 'globalJsObject'. I frankly don't recall why we did it that way, possibly because the v8 initialisation has to happen before the members of JsEngine are initialised. But yeah, out of scope here.
http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... lib/prefs.js:90: values[key] = _preconfiguredPrefs[key]; > As written, this will overwrite ones that are not so stated with 'null'. Hm, am I missing something? I don't see why would it override with 'null'. This is in a loop that iterates only through preconfiguration preferences. It will override only those that were preconfigured... Worst case it can also iterate through inherited properties of _preconfiguredPrefs (if any), but those are not in the whitelist. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... File lib/updater.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... lib/updater.js:39: if (!forceCheck && Pref.disable_auto_updates) On 2015/06/11 20:21:13, Felix H. Dahlke wrote: > On 2015/06/10 17:48:16, Eric wrote: > > This would read more clearly if we were using "enable_auto_updates". > > I've wondered about that too, but IMHO disable makes more sense, since the > default behaviour is to check for updates and this pref is about disabling that, > same as with suppress_first_run_page. Wouldn't mind either way however. This is subjective, of course, but I think enable_auto_updates is better. I'd live as is, if you don't insist. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/test... File test/Prefs.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/test... test/Prefs.cpp:164: } On 2015/06/10 17:48:16, Eric wrote: > With preconfigurable preferences, we now have three different sources of > preferences: (a) defaults, (b) preconfigured values, (c) user-set values. We > need tests for the different combinations of these. > > The existing test exercises (a) vs. (b). Since (a) is always present as a > source, there are three more combinations that need testing. > -- (a) only > -- (a) and (c) > -- all of (a), (b), and (c) I think (a) and (a) and (c) are tested quite well in other tests in this file
http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... lib/prefs.js:90: values[key] = _preconfiguredPrefs[key]; On 2015/06/12 07:36:14, Oleksandr wrote: > > As written, this will overwrite ones that are not so stated with 'null'. > Hm, am I missing something? I don't see why would it override with 'null'. This > is in a loop that iterates only through preconfiguration preferences. It will > override only those that were preconfigured... Worst case it can also iterate > through inherited properties of _preconfiguredPrefs (if any), but those are not > in the whitelist. > Yes you're right, guess me and Eric both missed that above :P http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/test... File test/Prefs.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/test... test/Prefs.cpp:164: } On 2015/06/12 07:36:14, Oleksandr wrote: > On 2015/06/10 17:48:16, Eric wrote: > > With preconfigurable preferences, we now have three different sources of > > preferences: (a) defaults, (b) preconfigured values, (c) user-set values. We > > need tests for the different combinations of these. > > > > The existing test exercises (a) vs. (b). Since (a) is always present as a > > source, there are three more combinations that need testing. > > -- (a) only > > -- (a) and (c) > > -- all of (a), (b), and (c) > > I think (a) and (a) and (c) are tested quite well in other tests in this file Most of that is covered here, but I think we should have a test for the b/c combination, in particular when it comes to pref loading. Your earlier patch wouldn't have passed that test, so I think it's worth having :) Basically something like PrefsPersistWhenPreconfigured or something, similar to the PrefsPersist test above: 1. Preconfigure a pref 2. Ensure it's set properly 3. Set it to a different value 4. Ensure it's set properly 5. Reload prefs (how PrefsPersist does it) 6. Ensure it's still set to the user defined value
Didn't realise there was a new patch set, looks pretty good now. http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/incl... File include/AdblockPlus/JsEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/incl... include/AdblockPlus/JsEngine.h:215: * Adds a global object that can be accessed by all the scripts. Nit: Not necessarily an object, rather "Sets a global properly." (I'm nit picking so much around the doc comments because it's a library, and a bunch of people are actually using the documentation. So being a bit more anal now saves time later.) http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/incl... include/AdblockPlus/JsEngine.h:216: * @param The `std::sting` name of the object to add Nit: std::string I guess? Also "name of the property to set" I'd say. http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/incl... include/AdblockPlus/JsEngine.h:217: * @param The `JsValuePtr` value the object to add Would be "value of the property to set" then http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/src/... File src/JsEngine.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/src/... src/JsEngine.cpp:234: void AdblockPlus::JsEngine::SetGlobalProperty(std::string name, AdblockPlus::JsValuePtr value) Nit: 80 columns please. http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/test... File test/Prefs.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/test... test/Prefs.cpp:173: TEST_F(PrefsTest, PreconfiguredPrefsPreconfiguredOverride) Micro nit: Seems redundant, "PreconfiguredPrefsOverride" maybe?
Addressed all of the comments now, I think. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/incl... include/AdblockPlus/FilterEngine.h:197: std::map<std::string, AdblockPlus::JsValuePtr>() On 2015/06/10 17:48:16, Eric wrote: > It would be useful to add a class-member typedef for this map type. It appears > fairly frequently. Done. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... File lib/init.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... lib/init.js:36: On 2015/06/10 17:48:16, Eric wrote: > Nit: no need for blank line. Done. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/lib/... lib/prefs.js:49: let isPreconfigurable = { On 2015/06/11 20:21:13, Felix H. Dahlke wrote: > I'd prefer this to work the same way as in Firefox, namely: > > let preconfigurable = ["suppress_first_run_page", "disable_auto_updates"]; Done. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/src/... File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/src/... src/FilterEngine.cpp:158: globalJsObject->SetProperty("_preconfiguredPrefs", preconfiguredPrefsObject); On 2015/06/10 17:48:16, Eric wrote: > Apart from initialization (where 'GetGlobalJsObject' isn't needed), this is the > only place where 'GetGlobalJsObject' is used. It would be just as good to define > 'JsEngine::SetGlobalProperty'. Properties are only read out in JavaScript code, > so there's no need for a corresponding "Get..." version. Done. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/test... File test/Prefs.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/test... test/Prefs.cpp:163: ASSERT_FALSE(filterEngine->GetPref("unsupported_preconfig")->AsBool()); On 2015/06/10 17:48:16, Eric wrote: > This assertion should really be in a separate test, since it's exercising a > different piece of the code. Done. http://codereview.adblockplus.org/5653480979038208/diff/6204282122534912/test... test/Prefs.cpp:164: } On 2015/06/12 07:55:03, Felix H. Dahlke wrote: > On 2015/06/12 07:36:14, Oleksandr wrote: > > On 2015/06/10 17:48:16, Eric wrote: > > > With preconfigurable preferences, we now have three different sources of > > > preferences: (a) defaults, (b) preconfigured values, (c) user-set values. We > > > need tests for the different combinations of these. > > > > > > The existing test exercises (a) vs. (b). Since (a) is always present as a > > > source, there are three more combinations that need testing. > > > -- (a) only > > > -- (a) and (c) > > > -- all of (a), (b), and (c) > > > > I think (a) and (a) and (c) are tested quite well in other tests in this file > > Most of that is covered here, but I think we should have a test for the b/c > combination, in particular when it comes to pref loading. Your earlier patch > wouldn't have passed that test, so I think it's worth having :) > > Basically something like PrefsPersistWhenPreconfigured or something, similar to > the PrefsPersist test above: > > 1. Preconfigure a pref > 2. Ensure it's set properly > 3. Set it to a different value > 4. Ensure it's set properly > 5. Reload prefs (how PrefsPersist does it) > 6. Ensure it's still set to the user defined value Done. That's a great test, actually. Revealed one more bug, which is fixed in the latest patchset. http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/incl... File include/AdblockPlus/JsEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/incl... include/AdblockPlus/JsEngine.h:215: * Adds a global object that can be accessed by all the scripts. On 2015/06/12 08:04:20, Felix H. Dahlke wrote: > Nit: Not necessarily an object, rather "Sets a global properly." > > (I'm nit picking so much around the doc comments because it's a library, and a > bunch of people are actually using the documentation. So being a bit more anal > now saves time later.) Done. http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/incl... include/AdblockPlus/JsEngine.h:216: * @param The `std::sting` name of the object to add On 2015/06/12 08:04:20, Felix H. Dahlke wrote: > Nit: std::string I guess? Also "name of the property to set" I'd say. Done. http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/incl... include/AdblockPlus/JsEngine.h:217: * @param The `JsValuePtr` value the object to add On 2015/06/12 08:04:20, Felix H. Dahlke wrote: > Would be "value of the property to set" then Done. http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/src/... File src/JsEngine.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/src/... src/JsEngine.cpp:234: void AdblockPlus::JsEngine::SetGlobalProperty(std::string name, AdblockPlus::JsValuePtr value) On 2015/06/12 08:04:20, Felix H. Dahlke wrote: > Nit: 80 columns please. Done. http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/test... File test/Prefs.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/test... test/Prefs.cpp:173: TEST_F(PrefsTest, PreconfiguredPrefsPreconfiguredOverride) On 2015/06/12 08:04:20, Felix H. Dahlke wrote: > Micro nit: Seems redundant, "PreconfiguredPrefsOverride" maybe? Done.
Getting close :) http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/incl... File include/AdblockPlus/JsEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/incl... include/AdblockPlus/JsEngine.h:217: * @param The `JsValuePtr` value the property to set "value of"? :) http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/lib/... lib/prefs.js:87: if (typeof _preconfiguredPrefs[key] != typeof defaults[key]) Don't think we need to check for this - we don't do it on other platforms either. http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/lib/... lib/prefs.js:89: defaults[key] = _preconfiguredPrefs[key]; Was about to complain that this isn't necessary, but it actually could be. If we don't this, we'd be introducing a footgun: defaults = {x: 1} preconfiguredPrefs = {x: 2} Prefs.x = 1; restoreDefault("x"); Prefs.x == 2; // Nope, it's 1 Now, restoreDefault() doesn't actually exist IIRC, but in Firefox we have it, and if we add it in libdablockplus as well, our feet are in danger. But I think we can solve this by simply modifying defaults once, rather than messing with both defaults and values. In particular: 1. We move this out of load(), where it probably doesn't really belong, and put it right before the `for (let key in defaults)` loop below. And then we just do this: for (let key in _preconfiguredPrefs) if (preconfigurable.indexOf(key) != -1) defaults[key] = _preconfiguredPrefs[key]; 2. We will have to move the initialisation of "values" down there, before the load() call. IMHO it's simpler that way. We just modify defaults to begin with, the rest of the code works the same as before. http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/test... File test/Prefs.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/test... test/Prefs.cpp:196: Nit: Just one line of whitespace?
Fingers crossed. http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/incl... File include/AdblockPlus/JsEngine.h (right): http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/incl... include/AdblockPlus/JsEngine.h:217: * @param The `JsValuePtr` value the property to set On 2015/06/12 11:55:35, Felix H. Dahlke wrote: > "value of"? :) Done. http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/lib/... lib/prefs.js:87: if (typeof _preconfiguredPrefs[key] != typeof defaults[key]) On 2015/06/12 11:55:35, Felix H. Dahlke wrote: > Don't think we need to check for this - we don't do it on other platforms > either. Done. http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/lib/... lib/prefs.js:89: defaults[key] = _preconfiguredPrefs[key]; On 2015/06/12 11:55:35, Felix H. Dahlke wrote: > Was about to complain that this isn't necessary, but it actually could be. If we > don't this, we'd be introducing a footgun: > > defaults = {x: 1} > preconfiguredPrefs = {x: 2} > Prefs.x = 1; > restoreDefault("x"); > Prefs.x == 2; // Nope, it's 1 > > Now, restoreDefault() doesn't actually exist IIRC, but in Firefox we have it, > and if we add it in libdablockplus as well, our feet are in danger. > > But I think we can solve this by simply modifying defaults once, rather than > messing with both defaults and values. In particular: > > 1. We move this out of load(), where it probably doesn't really belong, and put > it right before the `for (let key in defaults)` loop below. And then we just do > this: > > for (let key in _preconfiguredPrefs) > if (preconfigurable.indexOf(key) != -1) > defaults[key] = _preconfiguredPrefs[key]; > > 2. We will have to move the initialisation of "values" down there, before the > load() call. > > IMHO it's simpler that way. We just modify defaults to begin with, the rest of > the code works the same as before. Done. http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/test... File test/Prefs.cpp (right): http://codereview.adblockplus.org/5653480979038208/diff/5293214332354560/test... test/Prefs.cpp:196: On 2015/06/12 11:55:35, Felix H. Dahlke wrote: > Nit: Just one line of whitespace? Done.
Sorry :P http://codereview.adblockplus.org/5653480979038208/diff/6025343047565312/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5653480979038208/diff/6025343047565312/lib/... lib/prefs.js:148: for (let key in _preconfiguredPrefs) Two things and we're done: 1. Turn the `let values = Object.create(defaults);` above into a simple declaration, i.e. `let values;` 2. Get rid of this loop and just do: `values = Object.create(defaults);` instead.
Ouch. Totally missed your message on Friday. Done now, but I think diffs between patch sets can't be displayed due to rietveld's migration :/ https://codereview.adblockplus.org/5653480979038208/diff/6025343047565312/lib... File lib/prefs.js (right): https://codereview.adblockplus.org/5653480979038208/diff/6025343047565312/lib... lib/prefs.js:148: for (let key in _preconfiguredPrefs) On 2015/06/12 12:37:08, Felix Dahlke wrote: > Two things and we're done: > > 1. Turn the `let values = Object.create(defaults);` above into a simple > declaration, i.e. `let values;` > 2. Get rid of this loop and just do: `values = Object.create(defaults);` > instead. Done. Miss-understood your intent before.
Managed to find some more small stuff :D But it's looking good now. https://codereview.adblockplus.org/5653480979038208/diff/29317033/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/5653480979038208/diff/29317033/include/Adb... include/AdblockPlus/JsEngine.h:215: * Sets a global property that can be accessed by all the scripts. Nit: Seems like the indentation is off here, should be one more space in front of the stars. https://codereview.adblockplus.org/5653480979038208/diff/29317033/include/Adb... include/AdblockPlus/JsEngine.h:216: * @param The `std::string` name of the property to set Nit: Don't think Doxygen will parse this as intended, should rather be: @param name Name of the property to set. @param value Value of the property to set. (Also left out the types since Doxygen adds those automatically.) https://codereview.adblockplus.org/5653480979038208/diff/29317033/include/Adb... include/AdblockPlus/JsEngine.h:219: void SetGlobalProperty(std::string name, AdblockPlus::JsValuePtr value); Nit: The std::string should be passed as a const reference. https://codereview.adblockplus.org/5653480979038208/diff/29317033/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/5653480979038208/diff/29317033/lib/prefs.j... lib/prefs.js:147: // Set values of prefs based on _preconfiguredPrefs Not really, we're setting them based on `defaults`.
https://codereview.adblockplus.org/5653480979038208/diff/29317033/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/5653480979038208/diff/29317033/include/Adb... include/AdblockPlus/JsEngine.h:215: * Sets a global property that can be accessed by all the scripts. On 2015/06/17 18:49:24, Felix Dahlke wrote: > Nit: Seems like the indentation is off here, should be one more space in front > of the stars. Done. https://codereview.adblockplus.org/5653480979038208/diff/29317033/include/Adb... include/AdblockPlus/JsEngine.h:216: * @param The `std::string` name of the property to set On 2015/06/17 18:49:24, Felix Dahlke wrote: > Nit: Don't think Doxygen will parse this as intended, should rather be: > > @param name Name of the property to set. > @param value Value of the property to set. > > (Also left out the types since Doxygen adds those automatically.) Done. It would still produce understandable output either way, however there was a warning, you're right. In fact Doxygen build throws a lot of warnings, but we've agreed before to keep it casual, IIRC. https://codereview.adblockplus.org/5653480979038208/diff/29317033/include/Adb... include/AdblockPlus/JsEngine.h:219: void SetGlobalProperty(std::string name, AdblockPlus::JsValuePtr value); On 2015/06/17 18:49:24, Felix Dahlke wrote: > Nit: The std::string should be passed as a const reference. Done. https://codereview.adblockplus.org/5653480979038208/diff/29317033/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/5653480979038208/diff/29317033/lib/prefs.j... lib/prefs.js:147: // Set values of prefs based on _preconfiguredPrefs On 2015/06/17 18:49:24, Felix Dahlke wrote: > Not really, we're setting them based on `defaults`. Done.
LGTM
In general LGTM. https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/inc... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/inc... include/AdblockPlus/FilterEngine.h:193: * name-value list of preconfigured prefs. It seems this comment should be for the typedef above. https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/tes... File test/Prefs.cpp (right): https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/tes... test/Prefs.cpp:81: void Reset(std::map<std::string, AdblockPlus::JsValuePtr> preconfiguredPrefs = Why not to use `const Prefs&` in this file and in another files? 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"]; I saw the comment that `preconfigurable` is an array because it's done the same way as in FF. However, I find it a little bit awkward because of code duplication. Why is it better than to have `preconfigurable` object with the default values (which are now in `defaults` object) instead of array of strings? https://codereview.adblockplus.org/5653480979038208/diff/29317062/test/Prefs.cpp File test/Prefs.cpp (right): https://codereview.adblockplus.org/5653480979038208/diff/29317062/test/Prefs.... test/Prefs.cpp:170: ASSERT_FALSE(filterEngine->GetPref("unsupported_preconfig")->AsBool()); Actually I would expect to check that it's 'undefined' without implicit conversion to Boolean.
https://codereview1.adblockplus.org/5653480979038208/diff/29317062/include/Ad... File include/AdblockPlus/FilterEngine.h (right): https://codereview1.adblockplus.org/5653480979038208/diff/29317062/include/Ad... include/AdblockPlus/FilterEngine.h:187: typedef std::map<std::string, AdblockPlus::JsValuePtr> Prefs; Since this file has documentation for typedef, there should be one here. Possible description text: "Container of name-value pairs representing a set of preferences." https://codereview1.adblockplus.org/5653480979038208/diff/29317062/include/Ad... File include/AdblockPlus/JsEngine.h (right): https://codereview1.adblockplus.org/5653480979038208/diff/29317062/include/Ad... include/AdblockPlus/JsEngine.h:220: Nit: whitespace so asterisks line up. https://codereview1.adblockplus.org/5653480979038208/diff/29317062/lib/update... File lib/updater.js (right): https://codereview1.adblockplus.org/5653480979038208/diff/29317062/lib/update... lib/updater.js:39: if (!forceCheck && Pref.disable_auto_updates) This comment is actually in response to a previous patch set. Consider this a continuation. I still don't care for "disable_" as a prefix for these preference names. The real problem I have, I've figured out, is that these identifiers are named with respect their default values, not what they actually do. I suggested "enable_auto_updates" before, but that's wrong as well. What would be better is "perform_auto_updates". It would update to "true", but no one would need to care what the default was when looking at the statement above: if (!(forceCheck || Pref.performAutoUpdate)) { return null; } https://codereview1.adblockplus.org/5653480979038208/diff/29317062/test/Prefs... File test/Prefs.cpp (right): https://codereview1.adblockplus.org/5653480979038208/diff/29317062/test/Prefs... test/Prefs.cpp:82: std::map<std::string, AdblockPlus::JsValuePtr>()) We should used the typedef in FilterEngine.h here: "Prefs" See the second argument in the constructor for 'FilterEngine', which uses the same pattern. https://codereview1.adblockplus.org/5653480979038208/diff/29317062/test/Prefs... test/Prefs.cpp:159: We should also check types. ASSERT_TRUE(filterEngine->GetPref("disable_auto_updates")->IsBool()) https://codereview1.adblockplus.org/5653480979038208/diff/29317062/test/Prefs... test/Prefs.cpp:170: ASSERT_FALSE(filterEngine->GetPref("unsupported_preconfig")->AsBool()); On 2015/06/19 10:07:16, sergei wrote: > Actually I would expect to check that it's 'undefined' without implicit > conversion to Boolean. I agree. We should check the types first, then do conversions. Not only here, but other places. ASSERT_TRUE(filterEngine->GetPref("unsupported_preconfig")->IsUndefined());
https://codereview1.adblockplus.org/5653480979038208/diff/29317062/lib/prefs.js File lib/prefs.js (right): https://codereview1.adblockplus.org/5653480979038208/diff/29317062/lib/prefs.... lib/prefs.js:49: let preconfigurable = ["suppress_first_run_page", "disable_auto_updates"]; On 2015/06/19 10:07:16, sergei wrote: > I saw the comment that `preconfigurable` is an array because it's done the same > way as in FF. However, I find it a little bit awkward because of code > duplication. Why is it better than to have `preconfigurable` object with the > default values (which are now in `defaults` object) instead of array of strings? You mean splitting up defaults into two objects, preconfigurable and non-preconfigurable ones? While that would be possible, I don't really see a downside of an explicit whitelist, we should stay consistent with the other platforms here. https://codereview1.adblockplus.org/5653480979038208/diff/29317062/lib/update... File lib/updater.js (right): https://codereview1.adblockplus.org/5653480979038208/diff/29317062/lib/update... lib/updater.js:39: if (!forceCheck && Pref.disable_auto_updates) On 2015/06/19 15:23:37, Eric wrote: > This comment is actually in response to a previous patch set. Consider this a > continuation. > > I still don't care for "disable_" as a prefix for these preference names. The > real problem I have, I've figured out, is that these identifiers are named with > respect their default values, not what they actually do. I suggested > "enable_auto_updates" before, but that's wrong as well. > > What would be better is "perform_auto_updates". It would update to "true", but > no one would need to care what the default was when looking at the statement > above: > > if (!(forceCheck || Pref.performAutoUpdate)) > { > return null; > } I suppose this was named in line with "suppress_first_run_page". Either "disable_auto_updates" or "perform_auto_updates" is fine by me, both obvious enough to me.
https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/inc... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/inc... include/AdblockPlus/FilterEngine.h:193: * name-value list of preconfigured prefs. On 2015/06/19 10:07:15, sergei wrote: > It seems this comment should be for the typedef above. Acknowledged. https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/tes... File test/Prefs.cpp (right): https://codereview.adblockplus.org/5653480979038208/diff/5167242941562880/tes... test/Prefs.cpp:81: void Reset(std::map<std::string, AdblockPlus::JsValuePtr> preconfiguredPrefs = On 2015/06/19 10:07:15, sergei wrote: > Why not to use `const Prefs&` in this file and in another files? Done. https://codereview.adblockplus.org/5653480979038208/diff/29317062/include/Adb... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/5653480979038208/diff/29317062/include/Adb... include/AdblockPlus/FilterEngine.h:187: typedef std::map<std::string, AdblockPlus::JsValuePtr> Prefs; On 2015/06/19 15:23:36, Eric wrote: > Since this file has documentation for typedef, there should be one here. > > Possible description text: "Container of name-value pairs representing a set of > preferences." Done. https://codereview.adblockplus.org/5653480979038208/diff/29317062/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/5653480979038208/diff/29317062/include/Adb... include/AdblockPlus/JsEngine.h:220: On 2015/06/19 15:23:36, Eric wrote: > Nit: whitespace so asterisks line up. Done. 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/19 10:07:16, sergei wrote: > I saw the comment that `preconfigurable` is an array because it's done the same > way as in FF. However, I find it a little bit awkward because of code > duplication. Why is it better than to have `preconfigurable` object with the > default values (which are now in `defaults` object) instead of array of strings? I don't think this is a code duplication, really. Having all defaults listed in one place is better for readability, IMHO. And in any case, I think we should use the same logic we use in Firefox, so I'll leave this as is. 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/19 15:23:37, Eric wrote: > This comment is actually in response to a previous patch set. Consider this a > continuation. > > I still don't care for "disable_" as a prefix for these preference names. The > real problem I have, I've figured out, is that these identifiers are named with > respect their default values, not what they actually do. I suggested > "enable_auto_updates" before, but that's wrong as well. > > What would be better is "perform_auto_updates". It would update to "true", but > no one would need to care what the default was when looking at the statement > above: > > if (!(forceCheck || Pref.performAutoUpdate)) > { > return null; > } By this logic, we'll also have to rename the suppress_first_run to something like display_first_run. But we are trying to be as consistent across browsers as possible, and since we already have suppress_first_run in Chrome - I'd leave this as is too. Also, if there was a performAutoUpdate setting, it would be more ambivalent - should set it to true JIC, or not? Right now it is pretty straightforward that I should set this only if I want to disable the auto updates. https://codereview.adblockplus.org/5653480979038208/diff/29317062/test/Prefs.cpp File test/Prefs.cpp (right): https://codereview.adblockplus.org/5653480979038208/diff/29317062/test/Prefs.... test/Prefs.cpp:82: std::map<std::string, AdblockPlus::JsValuePtr>()) On 2015/06/19 15:23:37, Eric wrote: > We should used the typedef in FilterEngine.h here: "Prefs" > > See the second argument in the constructor for 'FilterEngine', which uses the > same pattern. Done. https://codereview.adblockplus.org/5653480979038208/diff/29317062/test/Prefs.... test/Prefs.cpp:159: On 2015/06/19 15:23:38, Eric wrote: > We should also check types. > > ASSERT_TRUE(filterEngine->GetPref("disable_auto_updates")->IsBool()) Done. https://codereview.adblockplus.org/5653480979038208/diff/29317062/test/Prefs.... test/Prefs.cpp:170: ASSERT_FALSE(filterEngine->GetPref("unsupported_preconfig")->AsBool()); On 2015/06/19 15:23:37, Eric wrote: > On 2015/06/19 10:07:16, sergei wrote: > > Actually I would expect to check that it's 'undefined' without implicit > > conversion to Boolean. > > I agree. We should check the types first, then do conversions. Not only here, > but other places. > > ASSERT_TRUE(filterEngine->GetPref("unsupported_preconfig")->IsUndefined()); Done.
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"]; Let me break the original quotes to comment them and I'm answering my question myself. Felix Dahlke wrote: > we should stay consistent with the other platforms here. But we should not blindly borrow suspicious approaches used on other platforms, we should rather fix them there or explain the benefit from them in some comment. It's because I'm asking, why it is better. Felix Dahlke wrote: > You mean splitting up defaults into two objects, preconfigurable and > non-preconfigurable ones? While that would be possible, I don't really see a > downside of an explicit whitelist, Oleksandr wrote: > I don't think this is a code duplication, really. Having all defaults listed in > one place is better for readability, IMHO. Despite it's absolutely not critical here, it merely does not look simple. And now I see the benefit of it, using such approach we can potentially extend few properties with attributes without involving complicated syntax for "defaults". Additional attributes can be like "preconfigurable", "read-only", "in-memory" (they are not saved) an so on. So, I agree, let's leave it as is. Nevertheless, the downside is worse code quality. Here is unnecessary code duplication where property names are hardcoded as strings in the second place which complicates the maintainability as well as it complicates the understanding of the code by causing additional questions, for example, what happens if we specify pre-configurable setting without having any default value for it?
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. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
