[adblockplus] Issue 4915 - Expose ext.i18n for background pages

ext/common.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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/>.
  */
 
 (function(global)
 {
   const Cu = Components.utils;
 
-  var Services = Cu.import("resource://gre/modules/Services.jsm", {}).Services;

[Sebastian Noack, 2017/02/20 16:53:20]

Please use let in favor of var in new code. (Same below.)

[wspee, 2017/03/01 14:54:08]

Done.

+  let {Services} = Cu.import("resource://gre/modules/Services.jsm", {});
 
   if (!global.ext)
     global.ext = {};
 
   var wrapperSymbol = Symbol("ext-wrapper");
 
   function wrapFrames(frames)
   {
     if (!frames.length)
       return null;
@@ skipping 46 matching lines @@
       this._port.on("ext_message", wrapper);
     },
 
     removeListener: function(listener)
     {
       if (listener[wrapperSymbol])
         this._port.off("ext_message", listener[wrapperSymbol]);
     }
   };
 
+  let pageName = "global";
+  if (typeof location !== "undefined")
+    pageName = location.pathname.replace(/.*\//, "").replace(/\..*?$/, "");
 
-  global.ext.i18n = (function()

[Sebastian Noack, 2017/02/20 16:53:20]

There is no need for any IIFE in modern code. The function only exists to
prevent leaking local variables. The same can be achieved with block-scoping:

{
  ...

  global.ext.i18n = {
    ...
  };
}

Or just omit the block altogether, as that script is fairly small. Up to you.

[Sebastian Noack, 2017/02/20 16:53:20]

It seems you forgot to remove ext.i18n from ext/content.js. So we have two
redundant implementations now.

[wspee, 2017/03/01 14:54:08]

Done.

[wspee, 2017/03/01 14:54:08]

Done.

-  {
-    if (typeof location == "undefined")

[wspee, 2017/02/20 13:53:16]

This is probably a bad idea, but I'm not sure what to do here.

Ideally we would only load the bundles currently neccessary so we could keep the
location approach for content pages and load all bundles for background use. But
then how to know all available bundles and should we pick the first matching
translation?!

[Sebastian Noack, 2017/02/20 16:53:20]

I might misunderstand, but isn't the "global" bundle exactly what we need here?

[wspee, 2017/03/01 14:54:08]

Yes it is, it does the right thing.

I was thinking that a module that behaves differently depending on where you
include is a bad idea (but this is no different from how it behaved previously).

-      var pageName = "global";
-    else
-      var pageName = location.pathname.replace(/.*\//, "").replace(/\..*?$/, "");
+  let stringBundle = Services.strings.createBundle(
+    "chrome://adblockplus/locale/" + pageName + ".properties?" + Math.random());
 
-    // Randomize URI to work around bug 719376
-    var stringBundle = Services.strings.createBundle("chrome://adblockplus/locale/" + pageName +
-      ".properties?" + Math.random());
-
-    function getI18nMessage(key)
+  global.ext.i18n = {
+    getMessage(key, args)
     {
-      return {
-        "message": stringBundle.GetStringFromName(key)
-      };
+      try {
+        return stringBundle.GetStringFromName(key);
+      }
+      catch(e)
+      {
+        // Don't report errors for special strings, these are expected to be
+        // missing.
+        if (key[0] != "@")
+          Cu.reportError(e);
+        return "";
+      }
     }
-
-    function getText(message, args)
-    {
-      var text = message.message;

[Sebastian Noack, 2017/02/20 16:53:20]

Perhaps you rather want to use restructuring arguments:

  function getText({message, placeholders}, args)
  {
    ...

ESLint with our configuration would also complain when you store the property of
an object into a variable like you do here, instead of using destructuring.

[wspee, 2017/03/01 14:54:08]

I have removed getText and getI18nMessage altogether. The function
getI18nMessage never returns placeholders so the code is needless. I might be
missing something thought?!

-      var placeholders = message.placeholders;
-
-      if (!args || !placeholders)
-        return text;
-
-      for (var key in placeholders)
-      {
-        var content = placeholders[key].content;
-        if (!content)
-          continue;
-
-        var index = parseInt(content.slice(1), 10);
-        if (isNaN(index))
-          continue;
-
-        var replacement = args[index - 1];
-        if (typeof replacement === "undefined")
-          continue;
-
-        text = text.split("$" + key + "$").join(replacement);
-      }
-      return text;
-    }
-
-    return {
-      getMessage: function(key, args)
-      {
-        try{
-          var message = getI18nMessage(key);
-          return getText(message, args);
-        }
-        catch(e)
-        {
-          // Don't report errors for special strings, these are expected to be
-          // missing.
-          if (key[0] != "@")
-            Cu.reportError(e);
-          return "";
-        }
-      }
-    };
-  })();
+  };
 
   if (typeof exports == "object")
     exports = global.ext;
 })(this);