Issue 1488 - Add pre-configurable preference to suppress first run page

background.js

Index: background.js
===================================================================
--- a/background.js
+++ b/background.js
@@ -47,6 +47,7 @@
   this.stopIconAnimation = stopIconAnimation;
 }
 var FilterStorage = require("filterStorage").FilterStorage;
+var filterNotifier = require("filterNotifier").FilterNotifier;

[kzar, 2015/03/19 15:20:37]

Shouldn't this be `FilterNotifier`?

[Sebastian Noack, 2015/03/20 13:26:12]

No, the module is still called "filterNotifier".

[kzar, 2015/03/20 14:47:44]

I meant the variable name.

[Sebastian Noack, 2015/03/20 15:40:55]

I see ,you are right. Surprisingly, it still worked. Probably some other file is
already importing FilterNotifier into a global. I fixed it though.

 var ElemHide = require("elemHide").ElemHide;
 var defaultMatcher = require("matcher").defaultMatcher;
 var Prefs = require("prefs").Prefs;
@@ -65,45 +66,83 @@
 
 var seenDataCorruption = false;
 var filterlistsReinitialized = false;
-require("filterNotifier").FilterNotifier.addListener(function(action)
+
+function init()
 {
-  if (action == "load")
+  var previousVersion = null;
+
+  var filtersLoaded = false;
+  var prefsLoaded = false;
+
+  var onLoaded = function()
   {
-    ext.storage.get(["currentVersion"], function(items)
+    var addonVersion = require("info").addonVersion;
+
+    // There are no filters stored so we need to reinitialize all filterlists
+    if (!FilterStorage.firstRun && FilterStorage.subscriptions.length === 0)
     {
-      var addonVersion = require("info").addonVersion;
-      var prevVersion = items.currentVersion;
+      filterlistsReinitialized = true;
+      previousVersion = null;
+    }
 
-      // There are no filters stored so we need to reinitialize all filterlists
-      if (!FilterStorage.firstRun && FilterStorage.subscriptions.length === 0)
-      {
-        filterlistsReinitialized = true;
-        prevVersion = null;
-      }
+    if (previousVersion != addonVersion || FilterStorage.firstRun)
+    {
+      seenDataCorruption = previousVersion && FilterStorage.firstRun;
+      ext.storage.set("currentVersion", addonVersion);

[Wladimir Palant, 2015/03/19 16:57:04]

Prefs.currentVersion = addonVersion?

[Sebastian Noack, 2015/03/20 13:26:12]

currentVersion isn't a preference, but I agree we should make it one.

+      addSubscription(previousVersion);
+    }
 
-      if (prevVersion != addonVersion || FilterStorage.firstRun)
-      {
-        seenDataCorruption = prevVersion && FilterStorage.firstRun;
-        ext.storage.set("currentVersion", addonVersion);
-        addSubscription(prevVersion);
-      }
-
-      // The "Hide placeholders" option has been removed from the UI in 1.8.8.1285
-      // So we reset the option for users updating from older versions.
-      if (prevVersion && Services.vc.compare(prevVersion, "1.8.8.1285") < 0)
-        Prefs.hidePlaceholders = true;
-    });
+    // The "Hide placeholders" option has been removed from the UI in 1.8.8.1285
+    // So we reset the option for users updating from older versions.
+    if (previousVersion && Services.vc.compare(previousVersion, "1.8.8.1285") < 0)
+      Prefs.hidePlaceholders = true;
 
     if (canUseChromeNotifications)
       initChromeNotifications();
     initAntiAdblockNotification();
-  }
 
-  // update browser actions when whitelisting might have changed,
-  // due to loading filters or saving filter changes
-  if (action == "load" || action == "save")
+    // Update browser actions and context menus when whitelisting might have changed,
+    // now when  initally loading the filters, or later when saving filter changes.

[kzar, 2015/03/19 15:20:37]

nit: The second line of this comment doesn't read so well. How about "(When
initially loading the filters and when saving filter changes.)"?

[Sebastian Noack, 2015/03/20 13:26:12]

I had to rephrase it anyway while addressing Wladimir's comment below. Does it
read better to you now?

+    FilterNotifier.addListener(function(action)
+    {
+      if (action == "save")

[Wladimir Palant, 2015/03/19 16:57:04]

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.

[Sebastian Noack, 2015/03/20 13:26:12]

Done.

+        refreshIconAndContextMenuForAllPages();
+    });
     refreshIconAndContextMenuForAllPages();
-});
+  };
+
+  var onFilterAction = function(action)
+  {
+    if (action == "load")
+    {
+      filtersLoaded = true;
+      if (prefsLoaded)

[Wladimir Palant, 2015/03/19 16:57:04]

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.

[Sebastian Noack, 2015/03/20 13:26:12]

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.

+        onLoaded();
+
+      FilterNotifier.removeListener(onFilterAction);

[Wladimir Palant, 2015/03/19 16:57:04]

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.

[Sebastian Noack, 2015/03/20 13:26:12]

Done.

+    }
+  };
+
+  var onPrefsLoaded = function()
+  {
+    // We have to wait for Prefs.onLoaded before retrieving "currentVersion".
+    // Otherwise it might not be migrated from localStorage yet.
+    ext.storage.get(["currentVersion"], function(items)

[Wladimir Palant, 2015/03/19 16:57:04]

This shouldn't be necessary, you can simply use Prefs.currentVersion
synchronously here.

[Sebastian Noack, 2015/03/20 13:26:12]

See above, currentVersion wasn't a pref yet. Now where it is one, this isn't
necessary anymore of course.

+    {
+      previousVersion = items.currentVersion;
+
+      prefsLoaded = true;
+      if (filtersLoaded)
+        onLoaded();
+    });
+
+    Prefs.onLoaded.removeListener(onPrefsLoaded);
+  };
+
+  FilterNotifier.addListener(onFilterAction);
+  Prefs.onLoaded.addListener(onPrefsLoaded);
+}
+init();
 
 // Special-case domains for which we cannot use style-based hiding rules.
 // See http://crbug.com/68705.
@@ -213,7 +252,8 @@
 
   function notifyUser()
   {
-    ext.pages.open(ext.getURL("firstRun.html"));
+    if (!Prefs.suppress_first_run_page)
+      ext.pages.open(ext.getURL("firstRun.html"));
   }
 
   if (addSubscription)
@@ -243,7 +283,7 @@
     notifyUser();
 }
 
-Prefs.addListener(function(name)
+Prefs.onChanged.addListener(function(name)
 {
   if (name == "shouldShowBlockElementMenu")
     refreshIconAndContextMenuForAllPages();