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

ext/content.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
  * 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/>.
  */
 
-this.ext = function(ext)
+(function(global)
 {
-  function getURLParameters(data)

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

This name is not reflecting what the function is doing. I'd suggest changing it
to "overwriteResponse", "considerURLParameters" or even
"modifyResponseWithURLParameters".

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

I went with updateFromURL() which isn't quite as verbose.

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

Thanks, that's great.

+  if (!global.ext)
+    global.ext = {};
+
+  function updateFromURL(data)
   {
     if (window.location.search)
     {
       var params = window.location.search.substr(1).split("&");
       for (var i = 0; i < params.length; i++)
       {
         var parts = params[i].split("=", 2);
         if (parts.length == 2 && parts[0] in data)
           data[parts[0]] = decodeURIComponent(parts[1]);
       }
     }
   }
 
   var subscriptions =[
     "https://easylist-downloads.adblockplus.org/easylistgermany+easylist.txt",
     "https://easylist-downloads.adblockplus.org/exceptionrules.txt",
     "https://easylist-downloads.adblockplus.org/fanboy-social.txt"
   ];
 
   var listenerFilter = null;
 
-  ext.backgroundPage = {
+  global.ext.backgroundPage = {
     sendMessage: function(message, responseCallback)
     {
       var respond = function(response)
       {
         setTimeout(responseCallback.bind(responseCallback, response), 0);
       };
 
       var dispatchListenerNotification = function(action)

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

Nit: Since this is only used for subscription notification I'd suggest renaming
this function to "dispatchSubscriptionNotification".

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

I'm ignoring this nit because that callback is being renamed into onFilterChange
by the next patch anyway.

       {
         var match = /^subscription\.(.*)/.exec(action);

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

In addition to my comment above, by directly passing in "added" or "removed" you
could avoid having to match the string.

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

Same here, this is no longer relevant because the next patch implements a
"proper" FilterNotifier.

         if (match && listenerFilter && listenerFilter.indexOf(match[1]) >= 0)
         {
-          ext.onMessage._dispatch({
+          global.ext.onMessage._dispatch({
             type: "subscriptions.listen",
             action: match[1],
             args: Array.prototype.slice.call(arguments, 1)
           });
         }
       };
 
       switch (message.type)

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

Introducing new method names should not be the norm. Therefore following the
interface proposal I'd suggest the following changes:

- app.info -> app.info with arguments "platform", "platformVersion",
"application" and "applicationVersion"
- app.issues -> app.get with argument "issues"
- app.options -> app.open with argument "options"
- subscriptions.toggle -> subscriptions.set with argument {disabled: BOOLEAN,
url: STRING}

The general idea is that the method defines what should be done to allow
grouping together similar messages. Whereas the arguments define what an action
should be applied to and also to pass further information if necessary.

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

I've mostly changed it like this. However, I'm not working with an array of
parameters but an object - so I decided to use what: "issues" and what:
"options". Also, subscription.toggle is being kept as discussed before.

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

Looks good. Not sure about whether an array for "app.get" at least might not be
the better choice since instead of `message.what == "issues"` it's just as easy
to do `message.what.indexOf("issues") > -1`.

However, I do agree with your argument that this might be unnecessary complexity
for now so fine with me not to use an array until it becomes necessary (see
http://codereview.adblockplus.org/6180766664884224/diff2/5741031244955648:575...)

[Wladimir Palant, 2014/12/19 13:32:37]

The problem isn't checking what was requested - the problem is rather sending
the result back in a meaningful way, as this API allows requesting multiple
things at the same time. After considering the possibilities here, all of them
seem unnecessarily complicated - particularly given that using app.get to
request multiple things at the same time seems rather unusual.

       {
-        case "app.doclink":
-          respond("https://adblockplus.org/redirect?link=" + encodeURIComponent(message.args[0]));

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

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

Coding style: "Lines can be longer than the limit, if limiting line length [to
80 characters] would hurt readability in a particular case."

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

This code is being rewritten by the next patch, no longer too long there.

+        case "app.get":
+          if (message.what == "issues")
+          {
+            var response = {seenDataCorruption: false, filterlistsReinitialized: false};
+            updateFromURL(response);
+
+            var info = {platform: "gecko", platformVersion: "34.0", application: "firefox", applicationVersion: "34.0"};
+            updateFromURL(info);
+            response.legacySafariVersion = (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
+              info.platformVersion == "7.0"));
+
+            respond(response);
+          }
+          else if (message.what == "doclink")
+            respond("https://adblockplus.org/redirect?link=" + encodeURIComponent(message.link));
+          else
+            respond(null);
           break;
-        case "app.info":
-          var response = {platform: "gecko", platformVersion: "34.0", application: "firefox", applicationVersion: "34.0"};

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

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

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

This code is being rewritten by the next patch, no longer too long there.

-          getURLParameters(response);
-          respond(response);
-          break;
-        case "app.issues":
-          var response = {seenDataCorruption: false, filterlistsReinitialized: false};

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

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

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

This code is being rewritten by the next patch, no longer too long there.

-          getURLParameters(response);
-          respond(response);
-          break;
-        case "app.options":
-          window.open("http://example.com/options.html", "_blank");
+        case "app.open":
+          if (message.what == "options")
+            window.open("http://example.com/options.html", "_blank");
           break;
         case "subscriptions.get":
           respond(subscriptions);
           break;
         case "filters.blocked":
           var params = {blockedURLs: ""};
-          getURLParameters(params);
+          updateFromURL(params);
           var blocked = params.blockedURLs.split(",");
           respond(blocked.indexOf(message.url) >= 0);
           break;
         case "subscriptions.toggle":
           var index = subscriptions.indexOf(message.url);
           if (index >= 0)
           {
             subscriptions.splice(index, 1);
             dispatchListenerNotification("subscription.removed", message.url);
           }
@@ skipping 35 matching lines @@
       {
         result = this._listeners[i].apply(null, arguments);
 
         if (this._cancelable && result === false)
           break;
       }
 
       return result;
     }
   };
-  ext.onMessage = new EventTarget();
-
-  return ext;
-}(this.ext || {});
+  global.ext.onMessage = new EventTarget();
+})(this);