[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
@@ skipping 68 matching lines @@
       this._port.on("ext_message", wrapper);
     },
 
     removeListener: function(listener)
     {
       if (listener[wrapperSymbol])
         this._port.off("ext_message", listener[wrapperSymbol]);
     }
   };
 
-  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());
-  };
+  let pageName = "global";
+  if (typeof location !== "undefined")
+    pageName = location.pathname.replace(/.*\//, "").replace(/\..*?$/, "");
 
-  I18n.prototype = {
-    getMessage(key)
+  let stringBundle = Services.strings.createBundle(
+    "chrome://adblockplus/locale/" + pageName + ".properties?" + Math.random());
+
+  global.ext.i18n = {
+    getMessage(key, args)
     {
       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.

+        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 "";
       }
     }
-  }
-
-  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);