First-run page (revisited)

chrome/content/ui/firstRun.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2013 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 (function()
 {
   var shade;
   var scrollTimer;
 
-  // Determine platform
-  var userAgent = "";
-  if (navigator.userAgent.indexOf("Gecko/") > -1)
-    userAgent = "firefox";
-  else if (navigator.userAgent.indexOf("Chrome/") > -1)
-    userAgent = "chrome";
-
-  if (userAgent !== "")
-    document.documentElement.className = userAgent;

[Wladimir Palant, 2013/05/27 11:02:00]

Please never do user agent detection, use feature detection. Either way, you
don't need it. Simply include utils.js in the HTML code and add utils.js in
Chrome with the following code:

var backgroundPage = chrome.extension.getBackgroundPage();
var require = backgroundPage.require;

You can also add require() calls for whatever API you need here.

If you still need to check application then, check require("info").application.
Normally you shouldn't need that however.

[Thomas Greiner, 2013/05/27 13:03:16]

I wanted to but the problem is that Firefox' utils.js uses JavaScript version
1.7 which - when declared in the <script> tag - is not being executed in Chrome.

Not declaring type="application/x-javascript;version=1.7" means that Firefox
won't treat utils.js as intended. Haven't found a good solution for dealing with
that, unfortunately.

[Wladimir Palant, 2013/05/27 14:10:37]

Feel free to replace let by var everywhere in utils.js in Firefox - that should
make it usable without version=1.7.

[Thomas Greiner, 2013/05/27 16:39:15]

Done.

-
   // Load subscriptions for features
-  var featureSubscriptions = {};
-  (function()
-  {
-    var request = new XMLHttpRequest();
-    request.open("GET", "featureSubscriptions.xml", false);
-    request.send();
-    var subscriptions = request.responseXML.getElementsByTagName("subscription");
-    for (var i = 0, len = subscriptions.length; i < len; i++)
-    {
-      var subscription = subscriptions[i];
-      featureSubscriptions[subscription.getAttribute("feature")] = {
-        "url": subscription.getAttribute("url"),
-        "title": subscription.getAttribute("title")
-      };
-    }
-  })();
-

[Wladimir Palant, 2013/05/27 11:02:00]

How about hardcoding the value of featureSubscriptions here instead of doing a
synchronous XMLHttpRequest (very bad)? subscriptions.xml is auto-generated, your
featureSubscriptions.xml isn't.

var featureSubscriptions = [
  {
    feature: "malware",
    url: "...",
    title: "..."
  },
  {
    ...
  }
];

Note that this is a list above, you don't really need a lookup table.

[Thomas Greiner, 2013/05/27 13:03:16]

Done.

-  // Determine scripts to load
-  var scripts = [];
-  if (userAgent == "chrome")
-  {
-    var backgroundPage = chrome.extension.getBackgroundPage();
-    var require = backgroundPage.require;
-  }
-  else if (userAgent == "firefox")
-  {
-    scripts.push("utils.js");
-  }
-  scripts.push("i18n.js");

[Wladimir Palant, 2013/05/27 11:02:00]

This shouldn't be necessary. As I said above, just load utils.js and i18n.js
every time - the contents of these files will be different depending on the
application however.

[Thomas Greiner, 2013/05/27 16:39:15]

Done.

-
-  function loadScripts()
-  {
-    var scriptName = scripts.shift();
-    var script = document.createElement("script");
-    script.type = (userAgent == "firefox") ? "application/x-javascript;version=1.7" : "text/javascript";
-    script.addEventListener("load", (scripts.length == 0) ? onScriptsLoaded : loadScripts, false);
-    script.src = scriptName;
-    document.head.appendChild(script);
-  }
-
-  function onScriptsLoaded()
-  {
-    var isFirstRun = (userAgent == "chrome" && backgroundPage.isFirstRun)
-      || (userAgent == "firefox" && !UI.firstRunDone);

[Wladimir Palant, 2013/05/27 11:02:00]

I think we can safely drop the update scenario now - if we are showing the first
run page then it's a first install. Consequently, you don't need to set the page
title dynamically.

[Thomas Greiner, 2013/05/27 13:03:16]

Done.

-    
-    if (userAgent == "chrome")
-    {
-      window.Synchronizer = require("synchronizer").Synchronizer;
-      window.Utils = require("utils").Utils;
-      window.Prefs = require("prefs").Prefs;
-      window.FilterStorage = require("filterStorage").FilterStorage;
-      window.FilterNotifier = require("filterNotifier").FilterNotifier;
-
-      var subscriptionClasses = require("subscriptionClasses");
-      window.Subscription = subscriptionClasses.Subscription;
-      window.DownloadableSubscription = subscriptionClasses.DownloadableSubscription;
-      window.Filter = require("filterClasses").Filter;

[Wladimir Palant, 2013/05/27 11:02:00]

This should be in the Chrome variant of utils.js.

[Thomas Greiner, 2013/05/27 16:39:15]

Done.

-    }
-
-    // Set up page title
-    var titleId = isFirstRun ? "firstRun_title_install" : "firstRun_title_update";
-    var pageTitle = i18n.getMessage(titleId);
-    document.title = document.getElementById("title-main").textContent = pageTitle;
-
-    // Only show changelog link on the update page
-    if (isFirstRun)
-      document.getElementById("title-changelog").style.display = "none";

[Wladimir Palant, 2013/05/27 11:02:00]

See above, the update case is irrelevant by now and hence that link can go.

[Thomas Greiner, 2013/05/27 13:03:16]

Done.

-
+  var featureSubscriptions = [
+    {
+      feature: "malware",
+      homepage: "http://malwaredomains.com/",
+      title: "Malware Domains",
+      url: "https://easylist-downloads.adblockplus.org/malwaredomains_full.txt"
+    },
+    {
+      feature: "social",
+      homepage: "https://www.fanboy.co.nz/",
+      title: "Fanboy's Social Blocking List",
+      url: "https://easylist-downloads.adblockplus.org/fanboy-social.txt"
+    },
+    {
+      feature: "tracking",
+      homepage: "https://easylist.adblockplus.org/",
+      title: "EasyPrivacy",
+      url: "https://easylist-downloads.adblockplus.org/easyprivacy.txt"
+    }
+  ];
+
+  function onDOMLoaded()
+  {
     // Show warning if data corruption was detected
-    // TODO: check in Firefox

[Thomas Greiner, 2013/05/24 10:19:12]

ignore the TODO here :)

-    if (userAgent == "chrome" && backgroundPage.seenDataCorruption)

[Wladimir Palant, 2013/05/27 11:02:00]

Please change that into (typeof backgroundPage != "undefined" &&
backgroundPage.seenDataCorruption) - Chrome isn't the only application with a
background page.

[Thomas Greiner, 2013/05/27 13:03:16]

Done.

-    {
-      document.getElementById("dataCorruptionWarning").removeAttribute("hidden");
-      setLinks("dataCorruptionWarning", getDocLink("knownIssuesChrome_filterstorage"));
-    }
-
-    // Set up URLs
-    var versionId;
-    var platformId;
-    if (userAgent == "firefox")
-    {
-      versionId = Utils.addonVersion.match(/^[0-9\.]+/)[0].replace(/\./g, "");
-      platformId = "firefox";
-    }
-    else if (userAgent == "chrome")
-    {
-      versionId = chrome.app.getDetails().version.split(".").slice(0, 2).join("");
-      platformId = "google-chrome";
-    }
-    setLinks("title-changelog", "https://adblockplus.org/releases/adblock-plus-" + versionId + "-for-" + platformId + "-released");
-    setLinks("acceptableAdsExplanation", getDocLink("acceptable_ads_criteria"), openFilters);
-
-    shade = document.getElementById("shade");
+    if (typeof backgroundPage != "undefined" && backgroundPage.seenDataCorruption)
+    {
+      E("dataCorruptionWarning").removeAttribute("hidden");
+      setLinks("dataCorruptionWarning", Utils.getDocLink("knownIssuesChrome_filterstorage"));
+    }
+
+    // Set up URL
+    setLinks("acceptableAdsExplanation", Utils.getDocLink("acceptable_ads_criteria"), openFilters);
+
+    shade = E("shade");
     shade.addEventListener("mouseover", scrollPage, false);
     shade.addEventListener("mouseout", stopScroll, false);
 
     // Set up typo feature
-    if (userAgent == "firefox")

[Wladimir Palant, 2013/05/27 11:02:00]

Please don't assume that only Firefox has the typo correction feature - just
check whether it's there, |if (require("typoBootstrap"))| should do. Also, don't
"reuse" an unrelated script - add the two lines to enable/disable type
correction to this script.

[Thomas Greiner, 2013/05/27 13:03:16]

Done.

-    {
-      var typoSettings = document.createElement("script");
-      typoSettings.type = "application/x-javascript;version=1.7";
-      typoSettings.src = "typoSettings.js";
-      document.head.appendChild(typoSettings);
-
-      var toggleTypo = document.getElementById("toggle-typo");
+    if (require("typoBootstrap"))
+    {
+      var featureTypo = E("feature-typo");
+      featureTypo.removeAttribute("hidden");
+      
       updateToggleButton("typo", Prefs.correctTypos);
-      Prefs.addListener(function(name)
+
+      var listener = function(name)
       {
         if (name == "correctTypos")
           updateToggleButton("typo", Prefs.correctTypos);
-      });
-      toggleTypo.addEventListener("click", function(event)
-      {
-        TypoActions.setEnabled(!Prefs.correctTypos);
+      }
+      Prefs.addListener(listener);
+      window.addEventListener("unload", function(event)
+      {
+        Prefs.removeListener(listener);
       }, false);
+
+      E("toggle-typo").addEventListener("click", toggleTypoCorrectionEnabled, false);
     }
 
     // Set up feature buttons linked to subscriptions
-    setToggleSubscriptionButton("malware");
-    setToggleSubscriptionButton("social");
-    setToggleSubscriptionButton("tracking");

[Wladimir Palant, 2013/05/27 11:02:00]

How about:

featureSubscriptions.forEach(setToggleSubscriptionButton);

[Thomas Greiner, 2013/05/27 13:03:16]

Done.

+    featureSubscriptions.forEach(setToggleSubscriptionButton);
+    var filterListener = function(action)
+    {
+      if (/^subscription\.(added|removed|disabled)$/.test(action))
+      {
+        for (var i = 0; i < featureSubscriptions.length; i++)
+        {
+          var featureSubscription = featureSubscriptions[i];
+          updateToggleButton(featureSubscription.feature, isSubscriptionEnabled(featureSubscription));
+        }
+      }
+    }
+    FilterNotifier.addListener(filterListener);
+    window.addEventListener("unload", function(event)
+    {
+      FilterNotifier.removeListener(filterListener);
+    }, false);
 
     window.addEventListener("resize", onWindowResize, false);
     document.addEventListener("scroll", onScroll, false);
 
     onWindowResize();
 
     initSocialLinks(null);
   }
 
   function onScroll()
   {
     var currentHeight = document.documentElement.scrollTop + document.body.scrollTop + document.documentElement.clientHeight;
     shade.style.opacity = (document.documentElement.scrollHeight == currentHeight) ? "0.0" : "0.5";
   }
 
   function onWindowResize()
   {
     onScroll();
   }
 
-  function getSubscription(featureSubscription)
-  {
-    var subscriptions = FilterStorage.subscriptions;
-    for (var i = 0, len = subscriptions.length; i < len; i++)
-    {
-      var subscription = subscriptions[i];
-      if (subscription.url == featureSubscription.url && subscription instanceof DownloadableSubscription)
-        return subscription;
-    }
-    
-    return null;
+  function toggleTypoCorrectionEnabled()
+  {
+    Prefs.correctTypos = !Prefs.correctTypos;
   }
 
   function isSubscriptionEnabled(featureSubscription)
   {
-    var subscription = getSubscription(featureSubscription);
-    return subscription != null && !subscription.disabled;

[Wladimir Palant, 2013/05/27 11:02:00]

FilterStorage keeps a lookup table for listed subscriptions, please use it:

return
  featureSubscription.url in FilterStorage.knownSubscriptions &&
  !Subscription.fromURL(featureSubscription.url).disabled;

getSubscription() shouldn't be necessary.

[Thomas Greiner, 2013/05/27 13:03:16]

Done.

-  }
-
-  function setToggleSubscriptionButton(feature)
-  {
-    var featureSubscription = featureSubscriptions[feature];
-
-    var element = document.getElementById("toggle-" + feature);
+    return featureSubscription.url in FilterStorage.knownSubscriptions
+      && !Subscription.fromURL(featureSubscription.url).disabled;
+  }
+
+  function setToggleSubscriptionButton(featureSubscription)
+  {
+    var feature = featureSubscription.feature;
+
+    var element = E("toggle-" + feature);
     updateToggleButton(feature, isSubscriptionEnabled(featureSubscription));
-    FilterNotifier.addListener(function(action)
-    {
-      if (/^(filter|subscription)\.(added|removed|disabled)$/.test(action))
-        updateToggleButton(feature, isSubscriptionEnabled(featureSubscription));

[Wladimir Palant, 2013/05/27 11:02:00]

How about adding a single listener that will update all subscription buttons
rather than one per subscription? Also, this listener needs to be removed when
the page unloads. And finally, why do you care about filter notifications?

[Thomas Greiner, 2013/05/27 13:03:16]

Not removing the listeners was a clear oversight, I agree.

I personally don't mind not having this listener but I thought it'd make it more
consistent with the settings in case they change and especially because it
didn't require any effort to add it. But I see why you'd want to remove it which
is why I removed it now.

[Thomas Greiner, 2013/05/27 16:39:15]

Readded with unload.

-    });
     element.addEventListener("click", function(event)
     {
-      var subscription = getSubscription(featureSubscription);
-      if (subscription == null)
-      {
-        subscription = Subscription.fromURL(featureSubscription.url);
+      var subscription = Subscription.fromURL(featureSubscription.url);
+      if (isSubscriptionEnabled(featureSubscription))
+        FilterStorage.removeSubscription(subscription);
+      else
+      {
         subscription.disabled = false;
         subscription.title = featureSubscription.title;
         subscription.homepage = featureSubscription.homepage;
         FilterStorage.addSubscription(subscription);
-        if (subscription instanceof DownloadableSubscription && !subscription.lastDownload)
+        if (!subscription.lastDownload)
           Synchronizer.execute(subscription);
       }
-      else if (!subscription.disabled)
-        FilterStorage.removeSubscription(subscription);
-      else
-        subscription.disabled = false;

[Wladimir Palant, 2013/05/27 11:02:00]

The logic should be simpler here, either it's currently enabled or it isn't:

    var subscription = Subscription.fromURL(featureSubscription.url);
    if (isSubscriptionEnabled(featureSubscription))
      FilterStorage.removeSubscription(subscription);
    else
    {
      subscription.disabled = false;
      subscription.title = featureSubscription.title;
      subscription.homepage = featureSubscription.homepage;
      FilterStorage.addSubscription(subscription);
      if (!subscription.lastDownload)
        Synchronizer.execute(subscription);
    }

[Thomas Greiner, 2013/05/27 13:03:16]

Done.

     }, false);
   }
 
   function scrollPage()
   {
     if (scrollTimer)
       stopScroll();
 
     scrollTimer = setInterval(function()
     {
       window.scrollBy(0, 5);
     }, 20);
   }
 
   function stopScroll()
   {
     clearTimeout(scrollTimer);
     scrollTimer = null;
   }
 
   function openSharePopup(url)
   {
-    var iframe = document.getElementById("share-popup");
-    var glassPane = document.getElementById("glass-pane");
+    var iframe = E("share-popup");
+    var glassPane = E("glass-pane");
     var popupMessageReceived = false;
 
     var popupMessageListener = function(event)
     {
       var originFilter = Filter.fromText("||adblockplus.org^");
       if (!originFilter.matches(event.origin, "OTHER", null, null))
         return;
 
       var width = event.data.width;
       var height = event.data.height;
@@ skipping 32 matching lines @@
 
     iframe.src = url;
     glassPane.className = "visible";
   }
 
   function initSocialLinks(variant)
   {
     var networks = ["twitter", "facebook", "gplus"];
     networks.forEach(function(network)
     {
-      var link = document.getElementById("share-" + network);
+      var link = E("share-" + network);
       link.addEventListener("click", function(e)
       {
         e.preventDefault();
-        openSharePopup(getDocLink("share-" + network) + "&variant=" + variant);
+        openSharePopup(Utils.getDocLink("share-" + network) + "&variant=" + variant);
       }, false);
     });
   }
 
   function setLinks(id)
   {
-    var element = document.getElementById(id);
+    var element = E(id);
     if (!element)
       return;
 
     var links = element.getElementsByTagName("a");
     for (var i = 0; i < links.length; i++)
     {
       if (typeof arguments[i + 1] == "string")
       {
         links[i].href = arguments[i + 1];
         links[i].setAttribute("target", "_blank");
       }
       else if (typeof arguments[i + 1] == "function")
       {
         links[i].href = "javascript:void(0);";
         links[i].addEventListener("click", arguments[i + 1], false);
       }
     }
   }
 
-  function getDocLink(page, anchor)
-  {
-    return Prefs.documentation_link
-                .replace(/%LINK%/g, page)
-                .replace(/%LANG%/g, Utils.appLocale) + (anchor ? "#" + anchor : "");
-  }

[Wladimir Palant, 2013/05/27 11:02:00]

The anchor parameter is unnecessary.

This is duplicating some code from ui.js in Firefox. Please add
Utils.getDocLink() to utils.js and make UI.loadDocLink() use it. In Chrome you
can add it to lib/utils.js as well.

-
   function openFilters()
   {
-    if (userAgent == "firefox")
+    if (typeof UI != "undefined")
       UI.openFiltersDialog();
-    else if (userAgent == "chrome")
+    else
     {
       backgroundPage.openOptions();

[Wladimir Palant, 2013/05/27 11:02:00]

Again, please don't go by user agents here:

if (typeof UI != "undefined")
  UI.openFiltersDialog();
else
  backgroundPage.openOptions();

In the long term we need a UI module for Chrome that's compatible with Firefox.

[Thomas Greiner, 2013/05/27 13:03:16]

Done.

     }
   }
 
   function updateToggleButton(feature, isEnabled)
   {
-    var button = document.getElementById("toggle-" + feature);
+    var button = E("toggle-" + feature);
     button.className = isEnabled ? "disable" : "enable";
     button.textContent = i18n.getMessage(isEnabled ? "firstRun_action_disable" : "firstRun_action_enable");
   }
 
-  document.addEventListener("DOMContentLoaded", loadScripts, false);
+  document.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();