Issue 29376555: [adblockplus] Issue 4915 - Expose ext.i18n for background pages (Closed)
|
Created:
Feb. 20, 2017, 1:40 p.m. by wspee Modified:
May 22, 2017, 7:56 a.m. Reviewers:
Wladimir Palant CC:
Thomas Greiner, Sebastian Noack Visibility:
Public. |
DescriptionIssue 4915 - Expose ext.i18n for background pages
Patch Set 1 #
Total comments: 15
Patch Set 2 : Removed obsolete code addressed review comments #
Total comments: 5
Patch Set 3 : Removed I18n class #
MessagesTotal messages: 6
https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js#newco... ext/common.js:92: if (typeof location == "undefined") 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?!
https://codereview.adblockplus.org/29376555/diff/29376556/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29376555/diff/29376556/ext/background.js#n... ext/background.js:21: let {_EventTarget: EventTarget, i18n: i18n} = require("ext_common"); You don't have to specify both destination and origin when they are the same during destructuring. let {_EventTarget: EventTarget, i18n} = require("ext_common"); https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js#newco... ext/common.js:22: var Services = Cu.import("resource://gre/modules/Services.jsm", {}).Services; Please use let in favor of var in new code. (Same below.) https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js#newco... ext/common.js:90: global.ext.i18n = (function() 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. https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js#newco... ext/common.js:90: global.ext.i18n = (function() It seems you forgot to remove ext.i18n from ext/content.js. So we have two redundant implementations now. https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js#newco... ext/common.js:92: if (typeof location == "undefined") On 2017/02/20 13:53:16, wspee wrote: > 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?! I might misunderstand, but isn't the "global" bundle exactly what we need here? https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js#newco... ext/common.js:110: var text = message.message; 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. https://codereview.adblockplus.org/29376555/diff/29376556/metadata.gecko File metadata.gecko (right): https://codereview.adblockplus.org/29376555/diff/29376556/metadata.gecko#newc... metadata.gecko:67: lib/antiadblockInit.js = adblockplusui/lib/antiadblockInit.js This change seems to be unrelated, or do I miss something?
(Moved Sebastian to cc since he's not available) https://codereview.adblockplus.org/29376555/diff/29376556/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29376555/diff/29376556/ext/background.js#n... ext/background.js:21: let {_EventTarget: EventTarget, i18n: i18n} = require("ext_common"); On 2017/02/20 16:53:20, Sebastian Noack wrote: > You don't have to specify both destination and origin when they are the same > during destructuring. > > let {_EventTarget: EventTarget, i18n} = require("ext_common"); Done. https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js#newco... ext/common.js:22: var Services = Cu.import("resource://gre/modules/Services.jsm", {}).Services; On 2017/02/20 16:53:20, Sebastian Noack wrote: > Please use let in favor of var in new code. (Same below.) Done. https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js#newco... ext/common.js:90: global.ext.i18n = (function() On 2017/02/20 16:53:20, Sebastian Noack wrote: > 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. Done. https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js#newco... ext/common.js:90: global.ext.i18n = (function() On 2017/02/20 16:53:20, Sebastian Noack wrote: > It seems you forgot to remove ext.i18n from ext/content.js. So we have two > redundant implementations now. Done. https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js#newco... ext/common.js:92: if (typeof location == "undefined") On 2017/02/20 16:53:20, Sebastian Noack wrote: > On 2017/02/20 13:53:16, wspee wrote: > > 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?! > > I might misunderstand, but isn't the "global" bundle exactly what we need here? 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). https://codereview.adblockplus.org/29376555/diff/29376556/ext/common.js#newco... ext/common.js:110: var text = message.message; On 2017/02/20 16:53:20, Sebastian Noack wrote: > 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. I have removed getText and getI18nMessage altogether. The function getI18nMessage never returns placeholders so the code is needless. I might be missing something thought?! https://codereview.adblockplus.org/29376555/diff/29376556/metadata.gecko File metadata.gecko (right): https://codereview.adblockplus.org/29376555/diff/29376556/metadata.gecko#newc... metadata.gecko:67: lib/antiadblockInit.js = adblockplusui/lib/antiadblockInit.js On 2017/02/20 16:53:20, Sebastian Noack wrote: > This change seems to be unrelated, or do I miss something? Done.
https://codereview.adblockplus.org/29376555/diff/29377741/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29376555/diff/29377741/ext/common.js#newco... ext/common.js:89: let I18n = global.ext._I18n = function(pageName) 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); } }; https://codereview.adblockplus.org/29376555/diff/29377741/ext/common.js#newco... ext/common.js:100: return this.stringBundle.GetStringFromName(key); 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.
https://codereview.adblockplus.org/29376555/diff/29377741/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29376555/diff/29377741/ext/common.js#newco... ext/common.js:89: let I18n = global.ext._I18n = function(pageName) On 2017/03/02 11:20:20, Wladimir Palant wrote: > 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); > } > }; 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. https://codereview.adblockplus.org/29376555/diff/29377741/ext/common.js#newco... ext/common.js:100: return this.stringBundle.GetStringFromName(key); On 2017/03/02 11:20:20, Wladimir Palant wrote: > 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. 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 ;).
LGTM https://codereview.adblockplus.org/29376555/diff/29377741/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29376555/diff/29377741/ext/common.js#newco... ext/common.js:100: return this.stringBundle.GetStringFromName(key); On 2017/03/02 17:13:29, wspee wrote: > 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). 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. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
