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

ext/common.js

Index: ext/common.js
diff --git a/ext/common.js b/ext/common.js
index c00cfffac710285aeef7b16823cd952067c7ea4c..28fbd651b6877451be77a74aaaab0d658c094a7b 100644
--- a/ext/common.js
+++ b/ext/common.js
@@ -17,6 +17,10 @@
 
 (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.

+
   if (!global.ext)
     global.ext = {};
 
@@ -82,6 +86,71 @@
     }
   };
 
+
+  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(/\..*?$/, "");
+
+    // Randomize URI to work around bug 719376
+    var stringBundle = Services.strings.createBundle("chrome://adblockplus/locale/" + pageName +
+      ".properties?" + Math.random());
+
+    function getI18nMessage(key)
+    {
+      return {
+        "message": stringBundle.GetStringFromName(key)
+      };
+    }
+
+    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);