[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..f15ec52b616328292e4618691bb784b31d41d3fa 100644
--- a/ext/common.js
+++ b/ext/common.js
@@ -17,6 +17,10 @@
 
 (function(global)
 {
+  const Cu = Components.utils;
+
+  let {Services} = Cu.import("resource://gre/modules/Services.jsm", {});
+
   if (!global.ext)
     global.ext = {};
 
@@ -82,6 +86,36 @@
     }
   };
 
+  let I18n = global.ext._I18n = function(pageName)

[Wladimir Palant, 2017/03/02 11:20:20]

Any reason why _I18n needs to be exported? IMHO this line should be changed
into:

  function I18N(pageName)

In fact, having a class here seems pointless altogether:

  let pageName = ...;
  let stringBundle = Services.strings.createBundle(...);
  global.ext.i18n = {
    getMessage(key)
    {
       return stringBundle.GetStringFromName(key);
    }
  };

[wspee, 2017/03/02 17:13:29]

Done, I thought bundling the state with that logic would make sense but as there
is no need to expose it this doesn't make much sense.

+  {
+    // Randomize URI to work around bug 719376
+    this.stringBundle = Services.strings.createBundle(
+      "chrome://adblockplus/locale/" + pageName + ".properties?" + Math.random());
+  };
+
+  I18n.prototype = {
+    getMessage(key)
+    {
+      try {
+        return this.stringBundle.GetStringFromName(key);

[Wladimir Palant, 2017/03/02 11:20:20]

Why did you remove placeholders support? While we currently don't seem to use
any placeholders, both adblockpluscore and Chrome implementation of this have
placeholders support. If we really decide that we don't want placeholders (or
not in this form) we should adjust all implementations to be consistent.

[wspee, 2017/03/02 17:13:29]

Because it didn't work, getI18nMessage never returned any placeholders and as
such the arguments would have never been used. Even if I would enable it here it
would still not work as expected because the order of the placeholders in the
string is not the same as the expected order in the args argument (if I
understood it correctly).

"msgid": {
  "message": "a text with two placeholders in reverse order $second$ $first$",
  "placeholders": {
    "first": {
      "content": "$1"
    },
    "second": {
      "content": "$2"
    }
  }
}

To make it work correctly I would also have to make changes to the gecko
packager. As placeholders are currently not used and we'll discontinue ff sooner
or late I'm not sure if this is worth it? I would of course do it if it is worth
it!

Thoughts? Hope I didn't miss anything ;).

[Wladimir Palant, 2017/03/06 08:36:40]

I see the issue now. I agree, making placeholders work in Firefox isn't worth
the effort. The question is mostly whether we should support placeholders in
adblockplusui then, probably not. But that's irrelevant for the review here.

+      }
+      catch(e)
+      {
+        // Don't report errors for special strings, these are expected to be
+        // missing.
+        if (key[0] != "@")
+          Cu.reportError(e);
+        return "";
+      }
+    }
+  }
+
+  let pageName = "global";
+  if (typeof location !== "undefined")
+    pageName = location.pathname.replace(/.*\//, "").replace(/\..*?$/, "");
+
+  global.ext.i18n = new I18n(pageName);
+
   if (typeof exports == "object")
     exports = global.ext;
 })(this);