Issue 4580 - Replace ext.i18n.getMessage with i18n.getMessage

Related: https://codereview.adblockplus.org/29569649/

[Manish Jethani, 2017-10-09 05:10:14]

Patch Set 1

Trying to get rid of ext.* as much as possible, here ext.i18n.getMessage.

https://codereview.adblockplus.org/29569659/diff/29569660/background.js
File background.js (left):

https://codereview.adblockplus.org/29569659/diff/29569660/background.js#oldco...
background.js:95: return parent.ext.i18n.locale.replace(/_/g, "-");
I'm not sure why it says parent here, but I've replaced it with window.chrome.

https://codereview.adblockplus.org/29569659/diff/29569660/i18n.js
File i18n.js (left):

https://codereview.adblockplus.org/29569659/diff/29569660/i18n.js#oldcode22
i18n.js:22: let {i18n} = ext;
This is no longer necessary because of the other changes in adblockpluschrome as
well as the new options pages here.

https://codereview.adblockplus.org/29569659/diff/29569660/i18n.js
File i18n.js (right):

https://codereview.adblockplus.org/29569659/diff/29569660/i18n.js#newcode40
i18n.js:40: setElementText(element, stringName, args)
Leaving setElementText here instead of sticking it on chrome.i18n since it's not
a browser API.

[Manish Jethani, 2017-10-09 05:20:09]

Patch Set 2: Rebase

options.js got renamed to desktop-options.js, that's all

[Thomas Greiner, 2017-10-09 17:43:31]

Unfortunately, I cannot review this without more context so what's the reason
for this change?

[Manish Jethani, 2017-10-09 17:46:18]

On 2017/10/09 17:43:31, Thomas Greiner wrote:
> Unfortunately, I cannot review this without more context so what's the reason
> for this change?

Related in adblockpluschrome: https://codereview.adblockplus.org/29569649/

Issue #5593

We're trying to get the popup to stop depending on the background page. It
cannot access the background page on Firefox in a private browsing window, and
anyway we can't do it with webpack. So the plan has been to get rid of the ext
namespace entirely and use native browser APIs wherever possible.

This change replaces uses of ext.i18n.getMessage with chrome.i18n.getMessage.

[Manish Jethani, 2017-10-09 17:57:26]

On 2017/10/09 17:46:18, Manish Jethani wrote:
> On 2017/10/09 17:43:31, Thomas Greiner wrote:
> > Unfortunately, I cannot review this without more context so what's the
reason
> > for this change?
> 
> Related in adblockpluschrome: https://codereview.adblockplus.org/29569649/
> 
> Issue #5593

To clarify, this change is not strictly required for fixing #5593, we could have
a ext.i18n still pointing to chrome.i18n and it would still be fixed. But this
is part of the larger effort to get rid of the ext namespace.

[Sebastian Noack, 2017-10-09 18:41:38]

On 2017/10/09 17:57:26, Manish Jethani wrote:
> On 2017/10/09 17:46:18, Manish Jethani wrote:
> > On 2017/10/09 17:43:31, Thomas Greiner wrote:
> > > Unfortunately, I cannot review this without more context so what's the
> reason
> > > for this change?
> > 
> > Related in adblockpluschrome: https://codereview.adblockplus.org/29569649/
> > 
> > Issue #5593
> 
> To clarify, this change is not strictly required for fixing #5593, we could
have
> a ext.i18n still pointing to chrome.i18n and it would still be fixed. But this
> is part of the larger effort to get rid of the ext namespace.

Also, unrelated of issue 5593, we want to get rid of ext.* all together,
eventually. The whole point of the abstraction layer, implemented in the ext.*
namespace, was to support Safari and Chrome with the same code. But with Safari
support phased out, and new target platforms (i.e. Firefox and Microsoft Edge)
providing APIs compatible with Chrome, the whole abstraction layer became
pointless. However, it is unrealstic to remove all of ext.* in one go. But
removing ext.i18n is one step in the right direction.

[Sebastian Noack, 2017-10-09 18:43:54]

https://codereview.adblockplus.org/29569659/diff/29570555/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29569659/diff/29570555/ext/common.js#newco...
ext/common.js:164: chrome.i18n = {
Shouldn't this rather be browser.i18n, now where we are going to use the
extension API, though the "browser" namespace (rather than "chrome")?

[Manish Jethani, 2017-10-09 18:47:02]

https://codereview.adblockplus.org/29569659/diff/29570555/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29569659/diff/29570555/ext/common.js#newco...
ext/common.js:164: chrome.i18n = {
On 2017/10/09 18:43:54, Sebastian Noack wrote:
> Shouldn't this rather be browser.i18n, now where we are going to use the
> extension API, though the "browser" namespace (rather than "chrome")?

Yes, assuming that one lands first.

[Thomas Greiner, 2017-10-10 12:05:14]

It'd be great if we could have some ticket to track all changes related to
removing `ext` rather than making those changes using Noissue commits. That way
we can transparently communicate why we want to make these changes.

Other than that, LGTM

[Manish Jethani, 2017-10-10 12:43:14]

On 2017/10/10 12:05:14, Thomas Greiner wrote:
> It'd be great if we could have some ticket to track all changes related to
> removing `ext` rather than making those changes using Noissue commits. That
way
> we can transparently communicate why we want to make these changes.
> 
> Other than that, LGTM

Thanks!

I found the issue:

https://issues.adblockplus.org/ticket/4580