Issue 5028 - Use browser namespace

Issue 5028 - Use browser namespace

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

[Manish Jethani, 2017-10-10 22:25:45]

Patch Set 1

[Sebastian Noack, 2017-10-10 23:23:08]

https://codereview.adblockplus.org/29573083/diff/29573084/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29573083/diff/29573084/ext/common.js#newco...
ext/common.js:161: browser.i18n = {
I think this code should be moved to polyfill.js now.

https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js#newcode20
polyfill.js:20: if (typeof browser == "undefined")
This check seems unnecessary. This script is only included when running the
standalone test server within adblockplusui.

[Manish Jethani, 2017-10-11 11:29:06]

Patch Set 2: Move i18n code into polyfill.js

https://codereview.adblockplus.org/29573083/diff/29573084/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29573083/diff/29573084/ext/common.js#newco...
ext/common.js:161: browser.i18n = {
On 2017/10/10 23:23:08, Sebastian Noack wrote:
> I think this code should be moved to polyfill.js now.

Done.

https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js#newcode20
polyfill.js:20: if (typeof browser == "undefined")
On 2017/10/10 23:23:08, Sebastian Noack wrote:
> This check seems unnecessary. This script is only included when running the
> standalone test server within adblockplusui.

But if the developer is testing on Chrome, the browser object won't be available
and consequently they won't be able to test, right?

https://codereview.adblockplus.org/29573083/diff/29573641/background.html
File background.html (right):

https://codereview.adblockplus.org/29573083/diff/29573641/background.html#new...
background.html:23: <script type="text/javascript" src="ext/common.js"></script>
This is needed for ext.Page

[Sebastian Noack, 2017-10-13 02:48:17]

https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js#newcode20
polyfill.js:20: if (typeof browser == "undefined")
On 2017/10/11 11:29:06, Manish Jethani wrote:
> On 2017/10/10 23:23:08, Sebastian Noack wrote:
> > This check seems unnecessary. This script is only included when running the
> > standalone test server within adblockplusui.
> 
> But if the developer is testing on Chrome, the browser object won't be
available
> and consequently they won't be able to test, right?

The browser object will never be available here, as this code doesn't run in the
context of an extension, but in the context of a web page, when testing
adblockplusui standalone, which is the only case in which this script runs.

[Manish Jethani, 2017-10-13 05:00:34]

Patch Set 3: Remove check for window.browser

https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js#newcode20
polyfill.js:20: if (typeof browser == "undefined")
On 2017/10/13 02:48:17, Sebastian Noack wrote:
> On 2017/10/11 11:29:06, Manish Jethani wrote:
> > On 2017/10/10 23:23:08, Sebastian Noack wrote:
> > > This check seems unnecessary. This script is only included when running
the
> > > standalone test server within adblockplusui.
> > 
> > But if the developer is testing on Chrome, the browser object won't be
> available
> > and consequently they won't be able to test, right?
> 
> The browser object will never be available here, as this code doesn't run in
the
> context of an extension, but in the context of a web page, when testing
> adblockplusui standalone, which is the only case in which this script runs.

Ah, you're right.

Done.

[Sebastian Noack, 2017-10-13 21:39:24]

https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js#newcode22
polyfill.js:22: window.browser = {};
Nit: Perhaps we should just define the "browser" object inline below?

  window.browser = {
    i18n: {
      ...
    }
  };

[Manish Jethani, 2017-10-13 22:30:24]

https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js#newcode22
polyfill.js:22: window.browser = {};
On 2017/10/13 21:39:23, Sebastian Noack wrote:
> Nit: Perhaps we should just define the "browser" object inline below?
> 
>   window.browser = {
>     i18n: {
>       ...
>     }
>   };

There will be more polyfills in this file (see
https://codereview.adblockplus.org/29573905/) so I think it's easier to group
them with their helper functions. Here browser.i18n needs getLocaleCandidates,
later browser.runtime would need its sendRawMessage (as well as some variables,
like the message ID for tracking, etc.). I'm OK with doing it either way, but it
just seems better to group all the i18n code together here.

[Sebastian Noack, 2017-10-13 22:36:01]

LGTM

[saroyanm, 2017-10-16 19:24:21]

Seems like we are still using chrome.runtime.sendMessage, which probably also
needs to be updated.

There were just couple of notes to take into consideration while rebasing.

https://codereview.adblockplus.org/29573083/diff/29574722/desktop-options.js
File desktop-options.js (right):

https://codereview.adblockplus.org/29573083/diff/29574722/desktop-options.js#...
desktop-options.js:546: ext.backgroundPage.sendMessage(message, (errors) =>
Seems like this was replaced by chrome.runtime.sendMessage ->
https://hg.adblockplus.org/adblockplusui/rev/441e39c75d99#l1.13

I think this should be rebased.

https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js#newcode29
polyfill.js:29: let defaultLocale = "en-US";
Heads-up for the rebase: There were couple of changes that affected this code ->
https://hg.adblockplus.org/adblockplusui/rev/070c26efe1c1

[Manish Jethani, 2017-10-16 21:35:22]

Patch Set 4: Rebase

https://codereview.adblockplus.org/29573083/diff/29574722/desktop-options.js
File desktop-options.js (right):

https://codereview.adblockplus.org/29573083/diff/29574722/desktop-options.js#...
desktop-options.js:546: ext.backgroundPage.sendMessage(message, (errors) =>
On 2017/10/16 19:24:20, saroyanm wrote:
> Seems like this was replaced by chrome.runtime.sendMessage ->
> https://hg.adblockplus.org/adblockplusui/rev/441e39c75d99#l1.13
> 
> I think this should be rebased.

Done.

https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js#newcode29
polyfill.js:29: let defaultLocale = "en-US";
On 2017/10/16 19:24:20, saroyanm wrote:
> Heads-up for the rebase: There were couple of changes that affected this code
->
> https://hg.adblockplus.org/adblockplusui/rev/070c26efe1c1

Done.

[saroyanm, 2017-10-17 11:09:52]

We still have some occurrences of chrome.runtime.sendMessage

https://codereview.adblockplus.org/29573083/diff/29580675/devtools-panel.js
File devtools-panel.js (right):

https://codereview.adblockplus.org/29573083/diff/29580675/devtools-panel.js#n...
devtools-panel.js:22: chrome.runtime.sendMessage({type: "types.get"},
There are still couple of chrome.runtime.sendMessag occurrences in current file.

https://codereview.adblockplus.org/29573083/diff/29580675/firstRun.html
File firstRun.html (right):

https://codereview.adblockplus.org/29573083/diff/29580675/firstRun.html#newco...
firstRun.html:28: <script type="text/javascript" src="ext/content.js"></script>
ext/content.js has couple of occurrences of "chrome.runtime.sendMessage"

https://codereview.adblockplus.org/29573083/diff/29580675/firstRun.html#newco...
firstRun.html:30: <script type="text/javascript" src="i18n.js"></script>
i18n.js has an occurrence of "chrome.runtime.sendMessage"

https://codereview.adblockplus.org/29573083/diff/29580675/firstRun.html#newco...
firstRun.html:31: <script type="text/javascript" src="firstRun.js"></script>
firstRun.js has couple of occurrences of "chrome.runtime.sendMessage"

https://codereview.adblockplus.org/29573083/diff/29580675/mobile-options.js
File mobile-options.js (right):

https://codereview.adblockplus.org/29573083/diff/29580675/mobile-options.js#n...
mobile-options.js:79: chrome.runtime.sendMessage(
We still use chrome.runtime.sendMessag in current file, same applies to all
other instances in current file.

[Manish Jethani, 2017-10-17 11:35:35]

Patch Set 5: Make more changes from chrome.runtime to browser.runtime

https://codereview.adblockplus.org/29573083/diff/29580675/devtools-panel.js
File devtools-panel.js (right):

https://codereview.adblockplus.org/29573083/diff/29580675/devtools-panel.js#n...
devtools-panel.js:22: chrome.runtime.sendMessage({type: "types.get"},
On 2017/10/17 11:09:51, saroyanm wrote:
> There are still couple of chrome.runtime.sendMessag occurrences in current
file.

Really sorry about this.

Done.

https://codereview.adblockplus.org/29573083/diff/29580675/firstRun.html
File firstRun.html (right):

https://codereview.adblockplus.org/29573083/diff/29580675/firstRun.html#newco...
firstRun.html:28: <script type="text/javascript" src="ext/content.js"></script>
On 2017/10/17 11:09:51, saroyanm wrote:
> ext/content.js has couple of occurrences of "chrome.runtime.sendMessage"

Done.

https://codereview.adblockplus.org/29573083/diff/29580675/firstRun.html#newco...
firstRun.html:30: <script type="text/javascript" src="i18n.js"></script>
On 2017/10/17 11:09:51, saroyanm wrote:
> i18n.js has an occurrence of "chrome.runtime.sendMessage"

Done.

https://codereview.adblockplus.org/29573083/diff/29580675/firstRun.html#newco...
firstRun.html:31: <script type="text/javascript" src="firstRun.js"></script>
On 2017/10/17 11:09:51, saroyanm wrote:
> firstRun.js has couple of occurrences of "chrome.runtime.sendMessage"

Done.

https://codereview.adblockplus.org/29573083/diff/29580675/mobile-options.js
File mobile-options.js (right):

https://codereview.adblockplus.org/29573083/diff/29580675/mobile-options.js#n...
mobile-options.js:79: chrome.runtime.sendMessage(
On 2017/10/17 11:09:52, saroyanm wrote:
> We still use chrome.runtime.sendMessag in current file, same applies to all
> other instances in current file.

Done.

[saroyanm, 2017-10-17 12:13:59]

Just a small notice.

https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js#newcode18
polyfill.js:18: "use strict";
While this polyfill file will be only used in the test environment and we want
to overwrite this file during the build, I'll suggest to move it under
ext/polyfill.js as it's described in the Readme See ->
https://hg.adblockplus.org/adblockplusui/file/tip/README.md#l29 otherwise we
might want to update Readme accordingly.

As far as I can see pollyfill.js in adblockpluschrome repository is being used
in the root directory instead of "ext" ->
https://codereview.adblockplus.org/29570614/ I'm not sure whether it was
intentional or not, so depending on that we might want to move pollyfill in both
repositories into "ext" folder to make it consistent with other implementations
"ext/background.js", "ext/common.js" and etc. or we might want to make a note
about pollyfill.js in the Readme.

In case I'm not missing something.

[Manish Jethani, 2017-10-17 12:19:50]

https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js#newcode18
polyfill.js:18: "use strict";
On 2017/10/17 12:13:58, saroyanm wrote:
> While this polyfill file will be only used in the test environment and we want
> to overwrite this file during the build, I'll suggest to move it under
> ext/polyfill.js as it's described in the Readme See ->
> https://hg.adblockplus.org/adblockplusui/file/tip/README.md#l29 otherwise we
> might want to update Readme accordingly.
> 
> As far as I can see pollyfill.js in adblockpluschrome repository is being used
> in the root directory instead of "ext" ->
> https://codereview.adblockplus.org/29570614/ I'm not sure whether it was
> intentional or not, so depending on that we might want to move pollyfill in
both
> repositories into "ext" folder to make it consistent with other
implementations
> "ext/background.js", "ext/common.js" and etc. or we might want to make a note
> about pollyfill.js in the Readme.
> 
> In case I'm not missing something.

Hmm ... ext/ was supposed to have the ext namespace, while we're moving away
from it to use the standard browser extension APIs. At the same time, it does
make sense to separate out files that will be overwritten by product-specific
versions. We could make an exception for polyfill.js here, considering it to be
a special file, and update the README accordingly, or we could move it to ext/

Sebastian, what do you think?

[Manish Jethani, 2017-10-17 12:38:17]

Patch Set 6: Add polyfill.js to README.md

https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js#newcode18
polyfill.js:18: "use strict";
On 2017/10/17 12:19:50, Manish Jethani wrote:
> On 2017/10/17 12:13:58, saroyanm wrote:
> > While this polyfill file will be only used in the test environment and we
want
> > to overwrite this file during the build, I'll suggest to move it under
> > ext/polyfill.js as it's described in the Readme See ->
> > https://hg.adblockplus.org/adblockplusui/file/tip/README.md#l29 otherwise we
> > might want to update Readme accordingly.
> > 
> > As far as I can see pollyfill.js in adblockpluschrome repository is being
used
> > in the root directory instead of "ext" ->
> > https://codereview.adblockplus.org/29570614/ I'm not sure whether it was
> > intentional or not, so depending on that we might want to move pollyfill in
> both
> > repositories into "ext" folder to make it consistent with other
> implementations
> > "ext/background.js", "ext/common.js" and etc. or we might want to make a
note
> > about pollyfill.js in the Readme.
> > 
> > In case I'm not missing something.
> 
> Hmm ... ext/ was supposed to have the ext namespace, while we're moving away
> from it to use the standard browser extension APIs. At the same time, it does
> make sense to separate out files that will be overwritten by product-specific
> versions. We could make an exception for polyfill.js here, considering it to
be
> a special file, and update the README accordingly, or we could move it to ext/
> 
> Sebastian, what do you think?

So I see that there are a few top-level files that are also not to be imported,
like background.js, subscriptions.xml, and so on. In that case the decision is
easier. Let's keep pollfill.js where it is and just update the README file, in
my opinion.

Done.

[saroyanm, 2017-10-17 12:41:12]

LGTM

https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js#newcode18
polyfill.js:18: "use strict";
On 2017/10/17 12:38:17, Manish Jethani wrote:
> On 2017/10/17 12:19:50, Manish Jethani wrote:
> > On 2017/10/17 12:13:58, saroyanm wrote:
> > > While this polyfill file will be only used in the test environment and we
> want
> > > to overwrite this file during the build, I'll suggest to move it under
> > > ext/polyfill.js as it's described in the Readme See ->
> > > https://hg.adblockplus.org/adblockplusui/file/tip/README.md#l29 otherwise
we
> > > might want to update Readme accordingly.
> > > 
> > > As far as I can see pollyfill.js in adblockpluschrome repository is being
> used
> > > in the root directory instead of "ext" ->
> > > https://codereview.adblockplus.org/29570614/ I'm not sure whether it was
> > > intentional or not, so depending on that we might want to move pollyfill
in
> > both
> > > repositories into "ext" folder to make it consistent with other
> > implementations
> > > "ext/background.js", "ext/common.js" and etc. or we might want to make a
> note
> > > about pollyfill.js in the Readme.
> > > 
> > > In case I'm not missing something.
> > 
> > Hmm ... ext/ was supposed to have the ext namespace, while we're moving away
> > from it to use the standard browser extension APIs. At the same time, it
does
> > make sense to separate out files that will be overwritten by
product-specific
> > versions. We could make an exception for polyfill.js here, considering it to
> be
> > a special file, and update the README accordingly, or we could move it to
ext/
> > 
> > Sebastian, what do you think?
> 
> So I see that there are a few top-level files that are also not to be
imported,
> like background.js, subscriptions.xml, and so on. In that case the decision is
> easier. Let's keep pollfill.js where it is and just update the README file, in
> my opinion.
> 
> Done.

I agree