Issue 1663 - Made first-run page use an asynchronous messaging protocol

firstRun.js

Index: firstRun.js
===================================================================
--- a/firstRun.js
+++ b/firstRun.js
@@ -36,115 +36,118 @@
     {
       feature: "tracking",
       homepage: "https://easylist.adblockplus.org/",
       title: "EasyPrivacy",
       url: "https://easylist-downloads.adblockplus.org/easyprivacy.txt"
     }
   ];
 
+  function getDocLink(link, callback)
+  {
+    ext.backgroundPage.sendMessage({type: "app.doclink", args: [link]}, callback);

[Thomas Greiner, 2014/12/18 10:17:48]

Isn't that exactly what we didn't want to do, send a message for each single
information? One idea behind the new interface was to send one message to
request multiple things of the same kind.

e.g.

ext.backgroundPage.sendMessage({
  type: "app.doclink",
  args: [
    "donate",
    "contributors",
    "acceptableAdsExplanation",
    "share-headline"
  ]
}, function(values)
{
  if (values.donate)
    E("donate").href = values.donate;

  ...
});

[Wladimir Palant, 2014/12/18 19:31:35]

Sure, but that's more relevant for the scenarios where there is lots of data -
here it doesn't seem worth the added complexity. Documentation links are never
many.

+  }
+
   function onDOMLoaded()
   {
     // Set up URLs
-    var donateLink = E("donate");
-    donateLink.href = Utils.getDocLink("donate");
+    getDocLink("donate", function(link)
+    {
+      E("donate").href = link;
+    });
 
-    var contributors = E("contributors");
-    contributors.href = Utils.getDocLink("contributors");
+    getDocLink("contributors", function(link)
+    {
+      E("contributors").href = link;
+    });
 
-    setLinks("acceptableAdsExplanation", Utils.getDocLink("acceptable_ads_criteria"), openFilters);
-    setLinks("share-headline", Utils.getDocLink("contribute"));
+    getDocLink("acceptable_ads_criteria", function(link)
+    {
+      setLinks("acceptableAdsExplanation", link, openFilters);
+    });
 
-    if (typeof backgroundPage != "undefined")
+    getDocLink("contribute", function(link)
+    {
+      setLinks("share-headline", link);
+    });
+
+    ext.backgroundPage.sendMessage({type: "app.issues"}, function(issues)
     {
       // Show warning if data corruption was detected
-      if (backgroundPage.seenDataCorruption)
+      if (issues.seenDataCorruption)
       {
         E("dataCorruptionWarning").removeAttribute("hidden");
-        setLinks("dataCorruptionWarning", Utils.getDocLink("knownIssuesChrome_filterstorage"));
+        getDocLink("knownIssuesChrome_filterstorage", function(link)
+        {
+          setLinks("dataCorruptionWarning", link);
+        });
       }
 
       // Show warning if filterlists settings were reinitialized
-      if (backgroundPage.filterlistsReinitialized)
+      if (issues.filterlistsReinitialized)
       {
         E("filterlistsReinitializedWarning").removeAttribute("hidden");
         setLinks("filterlistsReinitializedWarning", openFilters);
       }
-    }
+    });
 
     // Show warning if Safari version isn't supported
-    var info = require("info");
-    if (info.platform == "safari" && (
-      Services.vc.compare(info.platformVersion, "6.0")  < 0 ||  // beforeload breaks websites in Safari 5
-      Services.vc.compare(info.platformVersion, "6.1") == 0 ||  // extensions are broken in 6.1 and 7.0
-      Services.vc.compare(info.platformVersion, "7.0") == 0
-    ))
-      E("legacySafariWarning").removeAttribute("hidden");
+    ext.backgroundPage.sendMessage({type: "app.info"}, function(info)
+    {
+      if (info.platform == "safari" && (
+        parseInt(info.platformVersion, 10) < 6 ||  // beforeload breaks websites in Safari 5
+        info.platformVersion == "6.1" ||           // extensions are broken in 6.1 and 7.0

[Sebastian Noack, 2014/12/16 14:35:07]

The old code used Services.vc.compare() here to match 6.1.0 as well. Same below
for 7.0.0. Maybe we should do the check in the core and keep using
Services.vc.compare().

[Wladimir Palant, 2014/12/17 13:13:37]

That's way more message passing than this is worth. I checked and Apple doesn't
seem to use either "6.1.0" or "7.0.0" as version strings - it's "6.1" and "7.0".

[Sebastian Noack, 2014/12/17 13:45:22]

Not necessarily. You could just add a new message type like:

  ext.backgroundPage.sendMessage(
    {type: "app.isLegacySafariVersion"},
    function(isLegacySafariVersion)
    {
      if (!isLegacySafariVersion)
        ...
    }
  );

Or if you want to keep it more flexible:

  ext.backgroundPage.sendMessage(
    {
      type: "app.compareVersion",
      versions: [{lt: true, version: "6"}, "6.1.0", "7.0.0"]
    },
    function(isLegacySafariVersion)
    {
      if (!isLegacySafariVersion)
        ...
    }
  );

[Wladimir Palant, 2014/12/17 14:37:04]

Frankly, neither approach sounds like worth doing to me, not for a minor detail
like this one (I actually considered removing the 6.1/7.0 check altogether,
given that the issue affects only a handful of users by now). So this is up to
Thomas, he is both reviewer and module owner here.

[Sebastian Noack, 2014/12/17 15:08:55]

Well, the former approach wouldn't add any complexity, but merely moves existing
logic to the core.
Please don't. It affects all users on OS X Mavericks, which didn't install any
updates yet.

Anyway, it's not important, since you made sure that the code works. So LGTM.

[Thomas Greiner, 2014/12/18 10:17:48]

I could think of a nice middleway. The current implementation always returns the
full `info` object but since you can pass arguments with the message you could
pass a list of properties that you'd like to have and in addition also some
additional information like whether it's a legacy Safari version.

e.g.

ext.backgroundPage.sendMessage({
  type: "app.info",
  args: ["isLegacySafari"]
}, function(values)
{
  if (values.isLegacySafari)
    E("legacySafariWarning").removeAttribute("hidden");
});

[Wladimir Palant, 2014/12/18 19:31:35]

Changed that, as discussed with Thomas: app.info message is gone, added
legacySafariVersion to app.issues response.

+        info.platformVersion == "7.0"
+      ))
+      {
+        E("legacySafariWarning").removeAttribute("hidden");
+      }
+    });
 
     // Set up feature buttons linked to subscriptions
-    featureSubscriptions.forEach(setToggleSubscriptionButton);
-    var filterListener = function(action)
+    featureSubscriptions.forEach(initToggleSubscriptionButton);
+    updateToggleButtons();
+    initSocialLinks();
+
+    ext.onMessage.addListener(function(message)
     {
-      if (/^subscription\.(added|removed|disabled)$/.test(action))
+      if (message.type == "subscriptions.listen")
       {
-        for (var i = 0; i < featureSubscriptions.length; i++)
-        {
-          var featureSubscription = featureSubscriptions[i];
-          updateToggleButton(featureSubscription.feature, isSubscriptionEnabled(featureSubscription));
-        }
+        updateToggleButtons();
+        initSocialLinks();
       }
-    }
-    FilterNotifier.addListener(filterListener);
-    window.addEventListener("unload", function(event)
-    {
-      FilterNotifier.removeListener(filterListener);
-    }, false);
-
-    initSocialLinks();
+    });
+    ext.backgroundPage.sendMessage({type: "subscriptions.listen", filter: ["added", "removed", "disabled"]});

[Thomas Greiner, 2014/12/18 10:17:48]

Nit: This line doesn't need to be that long.

[Thomas Greiner, 2014/12/18 10:17:48]

By renaming "filter" to "args" it would become consistent with my other
comments.

[Wladimir Palant, 2014/12/18 19:31:35]

It would also become inconsistent with my objections ;)

As I said before, we want to send structured data, not an unstructured list of
arguments. Here we want to send a filter to be applied, and there might be more
parameters in future.

   }
 
-  function isSubscriptionEnabled(featureSubscription)
-  {
-    return featureSubscription.url in FilterStorage.knownSubscriptions
-      && !Subscription.fromURL(featureSubscription.url).disabled;
-  }
-
-  function setToggleSubscriptionButton(featureSubscription)
+  function initToggleSubscriptionButton(featureSubscription)
   {
     var feature = featureSubscription.feature;
 
     var element = E("toggle-" + feature);
-    updateToggleButton(feature, isSubscriptionEnabled(featureSubscription));
     element.addEventListener("click", function(event)
     {
-      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);
-      }
+      ext.backgroundPage.sendMessage({
+        type: "subscriptions.toggle",
+        url: featureSubscription.url,
+        title: featureSubscription.title,
+        homepage: featureSubscription.homepage
+      });

[Thomas Greiner, 2014/12/18 10:17:48]

I wouldn't put the reserved "type" property in the same level as the arguments
to avoid collisions.

e.g. (without considering comments from ext/content.js)

ext.backgroundPage.sendMessage({
  type: "subscriptions.toggle",
  args: [
    {
      homepage: featureSubscription.homepage,
      title: featureSubscription.title,
      url: featureSubscription.url
    }
  ]
});

[Wladimir Palant, 2014/12/18 19:31:35]

Fine with me if we have a new API that will accept the type as a separate
parameter and generate that structure automatically (and decompile it on the
receiving end again). However, until then I definitely don't want to specify
overly complicated data structures in the code - and we can easily avoid
collisions by not specifying parameters that are named "type".

[Thomas Greiner, 2014/12/19 10:53:38]

Sounds like a good compromise to me.

     }, false);
   }
 
   function openSharePopup(url)
   {
     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))
+      if (!/[.\/]adblockplus\.org$/.test(event.origin))

[Sebastian Noack, 2014/12/16 14:35:07]

This would match http://example.com/www.adblockplus.org. How about
|/(^|\.)adblockplus\.org$/.test(document.domain)| ?

[Wladimir Palant, 2014/12/16 15:17:18]

event.origin isn't a URL - see
https://developer.mozilla.org/en-US/docs/Web/API/Window.postMessage#The_dispa...

[Sebastian Noack, 2014/12/16 15:25:33]

Got ya. Feel free to ignore this comment then.

         return;
 
       var width = event.data.width;
       var height = event.data.height;
       iframe.width = width;
       iframe.height = height;
       iframe.style.marginTop = -height/2 + "px";
       iframe.style.marginLeft = -width/2 + "px";
@@ -182,29 +185,42 @@
   }
 
   function initSocialLinks()
   {
     var networks = ["twitter", "facebook", "gplus"];
     networks.forEach(function(network)
     {
       var link = E("share-" + network);
-      link.addEventListener("click", onSocialLinkClick, false);
+      var message = {

[Thomas Greiner, 2014/12/18 10:17:48]

Same as above regarding "type" property collisions.

+        type: "filters.blocked",
+        url: link.getAttribute("data-script"),
+        requestType: "SCRIPT",
+        docDomain: "adblockplus.org",
+        thirdParty: true
+      };
+      ext.backgroundPage.sendMessage(message, function(blocked)
+      {
+        // Don't open the share page if the sharing script would be blocked
+        if (blocked)
+          link.removeEventListener("click", onSocialLinkClick, false);

[Thomas Greiner, 2014/12/18 10:17:48]

No need to remove the event listener because it won't be added unless `blocked`
is `false`

[Wladimir Palant, 2014/12/18 19:31:35]

This function is being called multiple times, whenever user's subscriptions
change. So the event listener *can* be there, added by the previous invocation
of this function.

This is a somewhat awkward construct but the user can add the subscription to
block social buttons right on this page. If he chooses to do that before
clicking the social links we should adapt. And we cannot just query the filters
when the button is clicked because this will introduce delays and we won't be
able to open the original link target in a new tab any more (pop-up blocker).

[Thomas Greiner, 2014/12/19 10:53:38]

Ok and thanks for adapting the function name to reflect that.

+        else
+          link.addEventListener("click", onSocialLinkClick, false);
+      });
     });
   }
 
   function onSocialLinkClick(event)
   {
-    // Don't open the share page if the sharing script would be blocked
-    var filter = defaultMatcher.matchesAny(event.target.getAttribute("data-script"), "SCRIPT", "adblockplus.org", true);
-    if (!(filter instanceof BlockingFilter))
+    event.preventDefault();
+
+    getDocLink(event.target.id, function(link)
     {
-      event.preventDefault();
-      openSharePopup(Utils.getDocLink(event.target.id));
-    }
+      openSharePopup(link);
+    });
   }
 
   function setLinks(id)
   {
     var element = E(id);
     if (!element)
     {
       return;
@@ -224,22 +240,29 @@
         links[i].href = "javascript:void(0);";
         links[i].addEventListener("click", arguments[i + 1], false);
       }
     }
   }
 
   function openFilters()
   {
-    if (typeof UI != "undefined")
-      UI.openFiltersDialog();
-    else
+    ext.backgroundPage.sendMessage({type: "app.options"});
+  }
+
+  function updateToggleButtons()
+  {
+    ext.backgroundPage.sendMessage({type: "subscriptions.get"}, function(subscriptions)

[Thomas Greiner, 2014/12/18 10:17:48]

What you want here is only downloadable subscriptions therefore I'd suggest
specifying "downloadable" as an argument (as specified in the interface
proposal)

     {
-      backgroundPage.openOptions();
-    }
+      for (var i = 0; i < featureSubscriptions.length; i++)
+      {
+        var featureSubscription = featureSubscriptions[i];
+        updateToggleButton(featureSubscription.feature, subscriptions.indexOf(featureSubscription.url) >= 0);
+      }
+    });
   }
 
   function updateToggleButton(feature, isEnabled)
   {
     var button = E("toggle-" + feature);
     if (isEnabled)
       button.classList.remove("off");
     else