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

firstRun.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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 25 matching lines @@
     {
       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.

+    ext.backgroundPage.sendMessage({
+      type: "app.get",
+      what: "doclink",
+      link: link
+    }, callback);
   }
 
   function onDOMLoaded()
   {
     // Set up URLs
     getDocLink("donate", function(link)
     {
       E("donate").href = link;
     });
 
     getDocLink("contributors", function(link)
     {
       E("contributors").href = link;
     });
 
     getDocLink("acceptable_ads_criteria", function(link)
     {
       setLinks("acceptableAdsExplanation", link, openFilters);
     });
 
     getDocLink("contribute", function(link)
     {
       setLinks("share-headline", link);
     });
 
-    ext.backgroundPage.sendMessage({type: "app.issues"}, function(issues)
+    ext.backgroundPage.sendMessage({
+      type: "app.get",
+      what: "issues"
+    }, function(issues)
     {
       // Show warning if data corruption was detected
       if (issues.seenDataCorruption)
       {
         E("dataCorruptionWarning").removeAttribute("hidden");
         getDocLink("knownIssuesChrome_filterstorage", function(link)
         {
           setLinks("dataCorruptionWarning", link);
         });
       }
 
       // Show warning if filterlists settings were reinitialized
       if (issues.filterlistsReinitialized)
       {
         E("filterlistsReinitializedWarning").removeAttribute("hidden");
         setLinks("filterlistsReinitializedWarning", openFilters);
       }
-    });
-
-    // Show warning if Safari version isn't supported
-    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"
-      ))
-      {
+
+      if (issues.legacySafariVersion)
         E("legacySafariWarning").removeAttribute("hidden");
-      }
     });
 
     // Set up feature buttons linked to subscriptions
     featureSubscriptions.forEach(initToggleSubscriptionButton);
     updateToggleButtons();
-    initSocialLinks();
+    updateSocialLinks();
 
     ext.onMessage.addListener(function(message)
     {
       if (message.type == "subscriptions.listen")
       {
         updateToggleButtons();
-        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.

+        updateSocialLinks();
+      }
+    });
+    ext.backgroundPage.sendMessage({
+      type: "subscriptions.listen",
+      filter: ["added", "removed", "disabled"]
+    });
   }
 
   function initToggleSubscriptionButton(featureSubscription)
   {
     var feature = featureSubscription.feature;
 
     var element = E("toggle-" + feature);
     element.addEventListener("click", function(event)
     {
       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)
     {
       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";
       popupMessageReceived = true;
       window.removeEventListener("message", popupMessageListener);
@@ skipping 21 matching lines @@
       }
 
       iframe.removeEventListener("load", popupLoadListener);
     };
     iframe.addEventListener("load", popupLoadListener, false);
 
     iframe.src = url;
     glassPane.className = "visible";
   }
 
-  function initSocialLinks()
+  function updateSocialLinks()
   {
     var networks = ["twitter", "facebook", "gplus"];
     networks.forEach(function(network)
     {
       var link = E("share-" + network);
       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)
   {
     event.preventDefault();
 
@@ skipping 23 matching lines @@
       else if (typeof arguments[i + 1] == "function")
       {
         links[i].href = "javascript:void(0);";
         links[i].addEventListener("click", arguments[i + 1], false);
       }
     }
   }
 
   function openFilters()
   {
-    ext.backgroundPage.sendMessage({type: "app.options"});
+    ext.backgroundPage.sendMessage({type: "app.open", what: "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)

+    ext.backgroundPage.sendMessage({
+      type: "subscriptions.get",
+      downloadable: true,
+      ignoreDisabled: true
+    }, function(subscriptions)
     {
       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
       button.classList.add("off");
   }
 
   document.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();