Issue 5251132066627584: Issue 1488 - Add pre-configurable preference to suppress first run page (Closed)
|
Created:
March 19, 2015, 10:36 a.m. by Sebastian Noack Modified:
April 20, 2015, 3:22 p.m. Visibility:
Public. |
DescriptionIssue 1488 - Add pre-configurable preference to suppress first run page
Patch Set 1 : #
Total comments: 27
Patch Set 2 : Rebased and addressed comments #Patch Set 3 : Rebased #Patch Set 4 : Don't dispatch onChanged before loading is complete #
Total comments: 8
Patch Set 5 : Rebased and renamed onProgress() to checkLoaded() #
MessagesTotal messages: 13
http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... File background.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:50: var filterNotifier = require("filterNotifier").FilterNotifier; Shouldn't this be `FilterNotifier`? http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:105: // now when initally loading the filters, or later when saving filter changes. nit: The second line of this comment doesn't read so well. How about "(When initially loading the filters and when saving filter changes.)"? http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... lib/prefs.js:22: Maybe these defaults should be stored in their own section in the metadata files instead? It seems like we're muddling up data and code here. (I guess it's kind of offtopic for this review though.) http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... lib/prefs.js:136: if (pref && pref in defaults) Shouldn't we check if key is in defaults not pref?
http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... File background.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:91: ext.storage.set("currentVersion", addonVersion); Prefs.currentVersion = addonVersion? http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:108: if (action == "save") Please check for action == "load" as well here. While this action currently cannot be received after startup on Chrome, this will change as soon as the backup&restore functionality is implemented. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:119: if (prefsLoaded) This makes the condition you are checking for (and the connection between this check and the one below) non-obvious. Please either check for (prefsLoaded && filtersLoaded) here or below or add this check to onLoaded() and just call it unconditionally. Personally, I would prefer the latter. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:122: FilterNotifier.removeListener(onFilterAction); Please always clean up before you run complicated actions that might throw an exception - removeListener() should be called as the very first thing here, same below. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:130: ext.storage.get(["currentVersion"], function(items) This shouldn't be necessary, you can simply use Prefs.currentVersion synchronously here. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... lib/prefs.js:139: if ("newValue" in change) This should be: if ("newValue" in change && change.newValue != defaults[pref]) http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... lib/prefs.js:150: if (managedLoaded) As before, please don't make the logic behind this check non-obvious - checking for (localLoaded && managedLoaded) won't make this code noteworthy slower but significantly easier to read. Same below of course. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... lib/prefs.js:170: managedLoaded = true; This relies on the callbacks to read local prefs to execute asynchronously, otherwise localLoaded might get set to true before we get here. That assumption is currently only true on Safari because you made the callback use setTimeout(). Maybe just move reading managed prefs up, before we read local prefs? Alternatively, this branch could get the same conditional Prefs.onLoaded call and be consistent.
Note that most comments have been addressed in http://codereview.adblockplus.org/5693109165883392 http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... File background.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:50: var filterNotifier = require("filterNotifier").FilterNotifier; On 2015/03/19 15:20:37, kzar wrote: > Shouldn't this be `FilterNotifier`? No, the module is still called "filterNotifier". http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:91: ext.storage.set("currentVersion", addonVersion); On 2015/03/19 16:57:04, Wladimir Palant wrote: > Prefs.currentVersion = addonVersion? currentVersion isn't a preference, but I agree we should make it one. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:105: // now when initally loading the filters, or later when saving filter changes. On 2015/03/19 15:20:37, kzar wrote: > nit: The second line of this comment doesn't read so well. How about "(When > initially loading the filters and when saving filter changes.)"? I had to rephrase it anyway while addressing Wladimir's comment below. Does it read better to you now? http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:108: if (action == "save") On 2015/03/19 16:57:04, Wladimir Palant wrote: > Please check for action == "load" as well here. While this action currently > cannot be received after startup on Chrome, this will change as soon as the > backup&restore functionality is implemented. Done. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:119: if (prefsLoaded) On 2015/03/19 16:57:04, Wladimir Palant wrote: > This makes the condition you are checking for (and the connection between this > check and the one below) non-obvious. Please either check for (prefsLoaded && > filtersLoaded) here or below or add this check to onLoaded() and just call it > unconditionally. Personally, I would prefer the latter. The reason I didn't put it into onLoading in the first place was that the name of the function would be misleading then. Moved the checks there now and renamed the function to onProgress. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:122: FilterNotifier.removeListener(onFilterAction); On 2015/03/19 16:57:04, Wladimir Palant wrote: > Please always clean up before you run complicated actions that might throw an > exception - removeListener() should be called as the very first thing here, same > below. Done. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:130: ext.storage.get(["currentVersion"], function(items) On 2015/03/19 16:57:04, Wladimir Palant wrote: > This shouldn't be necessary, you can simply use Prefs.currentVersion > synchronously here. See above, currentVersion wasn't a pref yet. Now where it is one, this isn't necessary anymore of course. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... lib/prefs.js:22: On 2015/03/19 15:20:37, kzar wrote: > Maybe these defaults should be stored in their own section in the metadata files > instead? It seems like we're muddling up data and code here. (I guess it's kind > of offtopic for this review though.) I kinda like the idea. But it only makes sense when introducing a common base metadata file used for Firefox as well, in order to get rid of duplication. But yeah, it's pretty much unrelated. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... lib/prefs.js:136: if (pref && pref in defaults) On 2015/03/19 15:20:37, kzar wrote: > Shouldn't we check if key is in defaults not pref? The key is what's stored in ext.storage (e.g. "pref:enabled"). pref is the name of the pref (e.g. "enabled") as it is defined above. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... lib/prefs.js:139: if ("newValue" in change) On 2015/03/19 16:57:04, Wladimir Palant wrote: > This should be: > > if ("newValue" in change && change.newValue != defaults[pref]) Done. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... lib/prefs.js:150: if (managedLoaded) On 2015/03/19 16:57:04, Wladimir Palant wrote: > As before, please don't make the logic behind this check non-obvious - checking > for (localLoaded && managedLoaded) won't make this code noteworthy slower but > significantly easier to read. Same below of course. Done. http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... lib/prefs.js:170: managedLoaded = true; On 2015/03/19 16:57:04, Wladimir Palant wrote: > This relies on the callbacks to read local prefs to execute asynchronously, > otherwise localLoaded might get set to true before we get here. That assumption > is currently only true on Safari because you made the callback use setTimeout(). It's true on Chrome/Opera as well, since we use chrome.storage.get() there which is asynchronous by itself. The setTimeout() call on Safari merely exists to mimic Chrome's asynchronous behavior.
http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... File background.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:50: var filterNotifier = require("filterNotifier").FilterNotifier; On 2015/03/20 13:26:12, Sebastian Noack wrote: > On 2015/03/19 15:20:37, kzar wrote: > > Shouldn't this be `FilterNotifier`? > > No, the module is still called "filterNotifier". I meant the variable name.
http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... File background.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/back... background.js:50: var filterNotifier = require("filterNotifier").FilterNotifier; On 2015/03/20 14:47:44, kzar wrote: > On 2015/03/20 13:26:12, Sebastian Noack wrote: > > On 2015/03/19 15:20:37, kzar wrote: > > > Shouldn't this be `FilterNotifier`? > > > > No, the module is still called "filterNotifier". > > I meant the variable name. I see ,you are right. Surprisingly, it still worked. Probably some other file is already importing FilterNotifier into a global. I fixed it though.
http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5724160613416960/lib/... lib/prefs.js:170: managedLoaded = true; On 2015/03/20 13:26:12, Sebastian Noack wrote: > On 2015/03/19 16:57:04, Wladimir Palant wrote: > > This relies on the callbacks to read local prefs to execute asynchronously, > > otherwise localLoaded might get set to true before we get here. That > assumption > > is currently only true on Safari because you made the callback use > setTimeout(). > > It's true on Chrome/Opera as well, since we use chrome.storage.get() there which > is asynchronous by itself. The setTimeout() call on Safari merely exists to > mimic Chrome's asynchronous behavior. Never mind, I just moved that code into a function. And calling it here as well doesn't hurt, if it's only for consistency.
http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/back... File background.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/back... background.js:101: initChromeNotifications(); Nit: IMHO should be a newline between initChromeNotifications() and initAntiAdblockNotification(). http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/lib/... lib/prefs.js:100: let onProgress = function() I found this function name a little confusing when trying to grok the code. How about something like "watchIfLoaded"? (Or alternatively how about a comment explaining the idea of the function?)
http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/back... File background.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/back... background.js:101: initChromeNotifications(); On 2015/04/08 13:55:45, kzar wrote: > Nit: IMHO should be a newline between initChromeNotifications() and > initAntiAdblockNotification(). Well, this code belongs together. So I'd prefer to not have a newline here. But anyway, those lines aren't touched by the latest patch. http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/lib/... lib/prefs.js:100: let onProgress = function() On 2015/04/08 13:55:45, kzar wrote: > I found this function name a little confusing when trying to grok the code. How > about something like "watchIfLoaded"? (Or alternatively how about a comment > explaining the idea of the function?) Renamed it to checkLoaded, as well for the code in background.js. However, note that the change there is only visible here due to rebasing. I have performed the change in the respective patch which is under review as well.
Otherwise LGTM http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/back... File background.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/back... background.js:101: initChromeNotifications(); On 2015/04/08 15:11:20, Sebastian Noack wrote: > On 2015/04/08 13:55:45, kzar wrote: > > Nit: IMHO should be a newline between initChromeNotifications() and > > initAntiAdblockNotification(). > > Well, this code belongs together. So I'd prefer to not have a newline here. But > anyway, those lines aren't touched by the latest patch. No they don't, well not right together at least. There's a big comment above explaining about the conditional logic and on first glance it's not obvious that the "initAntiAdblockNotification()" call is made separately.
http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/back... File background.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/back... background.js:101: initChromeNotifications(); On 2015/04/09 10:12:10, kzar wrote: > On 2015/04/08 15:11:20, Sebastian Noack wrote: > > On 2015/04/08 13:55:45, kzar wrote: > > > Nit: IMHO should be a newline between initChromeNotifications() and > > > initAntiAdblockNotification(). > > > > Well, this code belongs together. So I'd prefer to not have a newline here. > But > > anyway, those lines aren't touched by the latest patch. > > No they don't, well not right together at least. There's a big comment above > explaining about the conditional logic and on first glance it's not obvious that > the "initAntiAdblockNotification()" call is made separately. Again, those lines aren't even remotely related to the change here.
LGTM http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/back... File background.js (right): http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/back... background.js:101: initChromeNotifications(); While I agree that a newline might improve code readability here, I also agree that this isn't the right place to do it. @kzar: Feel free to create a Noissue review for this change. http://codereview.adblockplus.org/5251132066627584/diff/5697982787747840/back... background.js:101: initChromeNotifications(); While I agree that a newline might improve code readability here, I also agree that this isn't the right place to do it. @kzar: Feel free to create a Noissue review for this change.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
