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

background.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 29 matching lines @@
   this.isThirdParty = isThirdParty;
   this.extractHostFromFrame = extractHostFromFrame;
 }
 with(require("icon"))
 {
   this.updateIcon = updateIcon;
   this.startIconAnimation = startIconAnimation;
   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 FilterNotifier = require("filterNotifier").FilterNotifier;
 var ElemHide = require("elemHide").ElemHide;
 var defaultMatcher = require("matcher").defaultMatcher;
 var Prefs = require("prefs").Prefs;
 var Synchronizer = require("synchronizer").Synchronizer;
 var Utils = require("utils").Utils;
 var NotificationStorage = require("notification").Notification;
 var initAntiAdblockNotification = require("antiadblockInit").initAntiAdblockNotification;
 var parseFilters = require("filterValidation").parseFilters;
 var composeFilters = require("filterComposer").composeFilters;
 
-// Chrome on Linux does not fully support chrome.notifications until version 35
-// https://code.google.com/p/chromium/issues/detail?id=291485
-var canUseChromeNotifications = require("info").platform == "chromium"
-  && "notifications" in chrome
-  && (navigator.platform.indexOf("Linux") == -1 || parseInt(require("info").applicationVersion, 10) > 34);
-
 var seenDataCorruption = false;
 var filterlistsReinitialized = false;
 
 function init()
 {
-  var previousVersion = null;
-
   var filtersLoaded = false;
   var prefsLoaded = false;
 
-  var onLoaded = function()
-  {
-    var addonVersion = require("info").addonVersion;
+  var checkLoaded = function()
+  {
+    if (!filtersLoaded || !prefsLoaded)
+      return;
+
+    var info = require("info");
+    var previousVersion = Prefs.currentVersion;
 
     // There are no filters stored so we need to reinitialize all filterlists
     if (!FilterStorage.firstRun && FilterStorage.subscriptions.length === 0)
     {
       filterlistsReinitialized = true;
       previousVersion = null;
     }
 
-    if (previousVersion != addonVersion || FilterStorage.firstRun)
+    if (previousVersion != info.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.

+      Prefs.currentVersion = info.addonVersion;
       addSubscription(previousVersion);
     }
 
     // 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)
+    // Chrome on Linux does not fully support chrome.notifications until version 35
+    // https://code.google.com/p/chromium/issues/detail?id=291485
+    if (info.platform == "chromium" && "notifications" in chrome &&
+        (navigator.platform.indexOf("Linux") == -1 ||
+         Services.vc.compare(info.applicationVersion, "35") >= 0))
       initChromeNotifications();
     initAntiAdblockNotification();
 
-    // 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?

+    // Update browser actions and context menus when whitelisting might have
+    // changed. That is now when initally loading the filters and later when
+    // importing backups or saving filter changes.
     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.

+      if (action == "load" || action == "save")
         refreshIconAndContextMenuForAllPages();
     });
     refreshIconAndContextMenuForAllPages();
   };
 
   var onFilterAction = function(action)
   {
     if (action == "load")
     {
+      FilterNotifier.removeListener(onFilterAction);
       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.

+      checkLoaded();
     }
   };
 
   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);
+    prefsLoaded = true;
+    checkLoaded();
   };
 
   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.
 var noStyleRulesHosts = ["mail.google.com", "mail.yahoo.com", "www.google.com"];
@@ skipping 466 matching lines @@
   page.sendMessage({type: "clickhide-deactivate"});
   refreshIconAndContextMenu(page);
 });
 
 setTimeout(function()
 {
   var notificationToShow = NotificationStorage.getNextToShow();
   if (notificationToShow)
     showNotification(notificationToShow);
 }, 3 * 60 * 1000);