Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(293)

Issue 29376555: [adblockplus] Issue 4915 - Expose ext.i18n for background pages (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 7 months ago by wspee
Modified:
2 years, 4 months ago
Reviewers:
Wladimir Palant
CC:
Thomas Greiner, Sebastian Noack
Visibility:
Public.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -62 lines) Patch
M ext/background.js View 1 1 chunk +2 lines, -1 line 0 comments Download
M ext/common.js View 1 2 2 chunks +28 lines, -0 lines 0 comments Download
M ext/content.js View 1 1 chunk +0 lines, -61 lines 0 comments Download

Messages

Total messages: 6
wspee
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#newcode92 ext/common.js:92: if (typeof location == "undefined") This is probably a ...
2 years, 7 months ago (2017-02-20 13:53:16 UTC) #1
Sebastian Noack
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#newcode21 ext/background.js:21: let {_EventTarget: EventTarget, i18n: i18n} = require("ext_common"); You don't ...
2 years, 7 months ago (2017-02-20 16:53:20 UTC) #2
wspee
(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#newcode21 ext/background.js:21: let ...
2 years, 6 months ago (2017-03-01 14:54:08 UTC) #3
Wladimir Palant
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#newcode89 ext/common.js:89: let I18n = global.ext._I18n = function(pageName) Any reason why ...
2 years, 6 months ago (2017-03-02 11:20:20 UTC) #4
wspee
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#newcode89 ext/common.js:89: let I18n = global.ext._I18n = function(pageName) On 2017/03/02 11:20:20, ...
2 years, 6 months ago (2017-03-02 17:13:29 UTC) #5
Wladimir Palant
2 years, 6 months ago (2017-03-06 08:36:41 UTC) #6
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5