Issue 5028 - Use browser namespace

Related: https://codereview.adblockplus.org/29573083/
Related: https://codereview.adblockplus.org/29573134/

[Manish Jethani, 2017-10-09 15:42:06]

Patch Set 1

[Manish Jethani, 2017-10-09 15:49:41]

https://codereview.adblockplus.org/29570614/diff/29570615/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29570614/diff/29570615/ext/background.js#n...
ext/background.js:167: // tabs. However, we have to keep relying on the
onUpdated event for
Fixed typo.

https://codereview.adblockplus.org/29570614/diff/29570615/ext/popup.js
File ext/popup.js (left):

https://codereview.adblockplus.org/29570614/diff/29570615/ext/popup.js#oldcode5
ext/popup.js:5: if (typeof chrome == "undefined" || typeof chrome.extension ==
"undefined")
This is redundant since it's already done in ext/common.js

https://codereview.adblockplus.org/29570614/diff/29570615/options.js
File options.js (right):

https://codereview.adblockplus.org/29570614/diff/29570615/options.js#newcode20
options.js:20: // Both Edge and Mozilla Web Extensions use the namespace
This is a duplication of Ollie's change, will be merged.

[Manish Jethani, 2017-10-09 15:53:45]

Patch Set 2: Fix indentation

[Manish Jethani, 2017-10-09 16:03:53]

Patch Set 3: Use callback-based API for storage

[Sebastian Noack, 2017-10-09 16:05:38]

BTW, this file in buildtools should be updated too:
https://hg.adblockplus.org/buildtools/file/d4ca9bfb82b5/chromeDevenvPoller__.js

https://codereview.adblockplus.org/29570614/diff/29570615/options.js
File options.js (right):

https://codereview.adblockplus.org/29570614/diff/29570615/options.js#newcode20
options.js:20: // Both Edge and Mozilla Web Extensions use the namespace
On 2017/10/09 15:49:41, Manish Jethani wrote:
> This is a duplication of Ollie's change, will be merged.

See my comment on Ollie's review. I don't think we should duplicate that
fallback here, but create a desperate file like polyfill.js.

[Manish Jethani, 2017-10-09 16:08:31]

On 2017/10/09 16:05:38, Sebastian Noack wrote:
> BTW, this file in buildtools should be updated too:
>
https://hg.adblockplus.org/buildtools/file/d4ca9bfb82b5/chromeDevenvPoller__.js

Yeah, will require some changes there as well as adblockplusui

[Manish Jethani, 2017-10-09 16:08:39]

https://codereview.adblockplus.org/29570614/diff/29570615/options.js
File options.js (right):

https://codereview.adblockplus.org/29570614/diff/29570615/options.js#newcode20
options.js:20: // Both Edge and Mozilla Web Extensions use the namespace
On 2017/10/09 16:05:37, Sebastian Noack wrote:
> On 2017/10/09 15:49:41, Manish Jethani wrote:
> > This is a duplication of Ollie's change, will be merged.
> 
> See my comment on Ollie's review. I don't think we should duplicate that
> fallback here, but create a desperate file like polyfill.js.

Yes, so I think I can do it as part of this change, since it's related. Ollie
could go ahead a commit that one.

[Sebastian Noack, 2017-10-09 16:14:19]

https://codereview.adblockplus.org/29570614/diff/29570615/options.js
File options.js (right):

https://codereview.adblockplus.org/29570614/diff/29570615/options.js#newcode20
options.js:20: // Both Edge and Mozilla Web Extensions use the namespace
On 2017/10/09 16:08:38, Manish Jethani wrote:
> On 2017/10/09 16:05:37, Sebastian Noack wrote:
> a desperate file like polyfill.js.

s/desperate/separate/

> Yes, so I think I can do it as part of this change, since it's related. Ollie
> could go ahead a commit that one.

Fine with me addressing it in here, and then dropping Ollie's patch.

[Manish Jethani, 2017-10-10 22:17:19]

Patch Set 5: Use polyfill.js

There are corresponding changes in adblockplusui

[Manish Jethani, 2017-10-10 22:26:39]

On 2017/10/10 22:17:19, Manish Jethani wrote:
> Patch Set 5: Use polyfill.js
> 
> There are corresponding changes in adblockplusui

https://codereview.adblockplus.org/29573083/

[Manish Jethani, 2017-10-10 22:40:41]

Patch Set 6: Refactor polyfill.js

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

On 2017/10/09 16:05:38, Sebastian Noack wrote:
> BTW, this file in buildtools should be updated too:
>
https://hg.adblockplus.org/buildtools/file/d4ca9bfb82b5/chromeDevenvPoller__.js

https://codereview.adblockplus.org/29573134/

[Sebastian Noack, 2017-10-10 23:07:44]

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js#newcode28
polyfill.js:28: // for a "chrome" namespace with the extension APIs and assign
it to the
The part about Firefox is inaccurate, both the "browser" and "chrome" object on
Firefox can be used with either promises or callbacks. If no callback is given,
then a promise is returned.

I think this comment can be much shorter:

  Unlike Firefox and Microsoft Edge, Chrome doesn't have a "browser" object,
  but provides the extension API through the "chrome" namespace (non-standard).

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js#newcode30
polyfill.js:30: if (typeof chrome != "undefined" && "extension" in chrome)
I think we should just use the "browser" object whenever it is available. As
explained above, you can use the "browser" object also with callbacks on FF.

[Sebastian Noack, 2017-10-10 23:15:25]

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js#newcode28
polyfill.js:28: // for a "chrome" namespace with the extension APIs and assign
it to the
On 2017/10/10 23:07:43, Sebastian Noack wrote:
> both the "browser" and "chrome" object on Firefox
> can be used with either promises or callbacks.
> If no callback is given, then a promise is returned.

That is actually only true for the "browser" object (which supports both
callbacks and promises), while the "chrome" object indeed only supports
callbacks. Either way, no reason to alias browser=chrome on Firefox.

[Manish Jethani, 2017-10-11 01:09:20]

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js#newcode28
polyfill.js:28: // for a "chrome" namespace with the extension APIs and assign
it to the
On 2017/10/10 23:15:25, Sebastian Noack wrote:
> On 2017/10/10 23:07:43, Sebastian Noack wrote:
> > both the "browser" and "chrome" object on Firefox
> > can be used with either promises or callbacks.
> > If no callback is given, then a promise is returned.
> 
> That is actually only true for the "browser" object (which supports both
> callbacks and promises), while the "chrome" object indeed only supports
> callbacks. Either way, no reason to alias browser=chrome on Firefox.

There are two issues with using browser directly on Firefox.

Code that expects the API calls to return undefined might fail. For example, say
there's this code:

  doSomething(() =>
  {
    chrome.tabs.create({url});
  });

But it could also be written like so, since tabs.create returns undefined
anyway:

  doSomething(() => chrome.tabs.create({url}));

If doSomething actually checks the return value of the callback, let's say it
does something else if it returns true, then this will cause unintended side
effects when we change it to the browser namespace.

I'll admit this is a contrived example and maybe we never run into this, but
we'll have to keep this in mind every time we deal with the APIs.

The other issue is that browser is probably just a wrapper around chrome with
extra promise-related stuff, which means we'd be running extra code we
definitely don't benefit from ourselves.

What do you think?

[Sebastian Noack, 2017-10-11 01:40:06]

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js#newcode28
polyfill.js:28: // for a "chrome" namespace with the extension APIs and assign
it to the
On 2017/10/11 01:09:20, Manish Jethani wrote:
> On 2017/10/10 23:15:25, Sebastian Noack wrote:
> > On 2017/10/10 23:07:43, Sebastian Noack wrote:
> > > both the "browser" and "chrome" object on Firefox
> > > can be used with either promises or callbacks.
> > > If no callback is given, then a promise is returned.
> > 
> > That is actually only true for the "browser" object (which supports both
> > callbacks and promises), while the "chrome" object indeed only supports
> > callbacks. Either way, no reason to alias browser=chrome on Firefox.
> 
> There are two issues with using browser directly on Firefox.
> 
> Code that expects the API calls to return undefined might fail. For example,
say
> there's this code:
> 
>   doSomething(() =>
>   {
>     chrome.tabs.create({url});
>   });
> 
> But it could also be written like so, since tabs.create returns undefined
> anyway:
> 
>   doSomething(() => chrome.tabs.create({url}));
> 
> If doSomething actually checks the return value of the callback, let's say it
> does something else if it returns true, then this will cause unintended side
> effects when we change it to the browser namespace.
>
> I'll admit this is a contrived example and maybe we never run into this, but
> we'll have to keep this in mind every time we deal with the APIs.

Yes, that is why Dave and I were used to use braces for one-liner arrow
functions, where we don't intend to forward the return value, rather than
relying on the fact that the operation returns undefined. Hindsight, we should
have better stuck to that practice. The particular problem here might be just
one example, but generally it seems to be a good idea to avoid unnecessary
assumptions in your code.

Perhaps it would be still worth it to go through all one-liner arrow functions,
and add braces wherever the the return value doesn't need to be forwarded.

Sure, keep using the "chrome" object (under the name "browser"), on Firefox,
will be a less risky option. But it also partly defeats the purpose of this
change, which is to use the browser extension API in a more standard fashion. It
would be quite backwards and potentially confusing if "browser" is actually
"chrome", in particular in Firefox-only code that could otherwise use promises.

> The other issue is that browser is probably just a wrapper around chrome with
> extra promise-related stuff, which means we'd be running extra code we
> definitely don't benefit from ourselves.

It is actually the other way around. ;)
https://lists.w3.org/Archives/Public/public-browserext/2016Oct/0002.html

[Manish Jethani, 2017-10-11 03:26:44]

Patch Set 7: Promisify

First draft of the promisified implementation.

This brings the browser namespace on Chrome and Edge on par with Firefox and
allows us to migrate piecemeal over time.

https://codereview.adblockplus.org/29570614/diff/29573138/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29570614/diff/29573138/polyfill.js#newcode26
polyfill.js:26: "create"
Right now only covers tabs.create

[Sebastian Noack, 2017-10-11 03:45:33]

On 2017/10/11 03:26:44, Manish Jethani wrote:
> Patch Set 7: Promisify
> 
> First draft of the promisified implementation.
> 
> This brings the browser namespace on Chrome and Edge on par with Firefox and
> allows us to migrate piecemeal over time.

While I'm all for enabling a progressive migration, can we please keep the
efforts separated. This patch is already becoming unwieldy enough, and there is
already a separate issue for targeting the promise-based API through polyfilling
(#4579).

Or is there any reason, I'm missing, while we shouldn't start using the
"browser" namespace (directly) without adding a polyfill to enable promises on
Chrome and Edge (other than if there is bug caused by implicitly forwarded
return values, it will explode on all platforms, and not just on Firefox)?

[Manish Jethani, 2017-10-11 10:46:41]

On 2017/10/11 03:45:33, Sebastian Noack wrote:
>
> While I'm all for enabling a progressive migration, can we please keep the
> efforts separated. This patch is already becoming unwieldy enough, and there
is
> already a separate issue for targeting the promise-based API through
polyfilling
> (#4579).

You're right, it's probably best as a separate patch set.

[Manish Jethani, 2017-10-11 10:59:20]

Patch Set 8: Use browser object directly when available

https://codereview.adblockplus.org/29570614/diff/29570615/options.js
File options.js (right):

https://codereview.adblockplus.org/29570614/diff/29570615/options.js#newcode20
options.js:20: // Both Edge and Mozilla Web Extensions use the namespace
On 2017/10/09 16:14:19, Sebastian Noack wrote:
> On 2017/10/09 16:08:38, Manish Jethani wrote:
> > On 2017/10/09 16:05:37, Sebastian Noack wrote:
> > a desperate file like polyfill.js.
> 
> s/desperate/separate/
> 
> > Yes, so I think I can do it as part of this change, since it's related.
Ollie
> > could go ahead a commit that one.
> 
> Fine with me addressing it in here, and then dropping Ollie's patch.

Done.

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js#newcode28
polyfill.js:28: // for a "chrome" namespace with the extension APIs and assign
it to the
On 2017/10/11 01:40:05, Sebastian Noack wrote:
> On 2017/10/11 01:09:20, Manish Jethani wrote:
> [...]
> 
> Sure, keep using the "chrome" object (under the name "browser"), on Firefox,
> will be a less risky option. But it also partly defeats the purpose of this
> change, which is to use the browser extension API in a more standard fashion.
It
> would be quite backwards and potentially confusing if "browser" is actually
> "chrome", in particular in Firefox-only code that could otherwise use
promises.

Alright, let's use the browser object directly, it makes more sense if we're
going to migrate to a promise-based API anyway.

> > The other issue is that browser is probably just a wrapper around chrome
with
> > extra promise-related stuff, which means we'd be running extra code we
> > definitely don't benefit from ourselves.
> 
> It is actually the other way around. ;)
> https://lists.w3.org/Archives/Public/public-browserext/2016Oct/0002.html

Ack.

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js#newcode28
polyfill.js:28: // for a "chrome" namespace with the extension APIs and assign
it to the
On 2017/10/10 23:15:25, Sebastian Noack wrote:
> On 2017/10/10 23:07:43, Sebastian Noack wrote:
> > both the "browser" and "chrome" object on Firefox
> > can be used with either promises or callbacks.
> > If no callback is given, then a promise is returned.
> 
> That is actually only true for the "browser" object (which supports both
> callbacks and promises), while the "chrome" object indeed only supports
> callbacks. Either way, no reason to alias browser=chrome on Firefox.

Acknowledged.

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js#newcode30
polyfill.js:30: if (typeof chrome != "undefined" && "extension" in chrome)
On 2017/10/10 23:07:43, Sebastian Noack wrote:
> I think we should just use the "browser" object whenever it is available. As
> explained above, you can use the "browser" object also with callbacks on FF.

Done.

[Manish Jethani, 2017-10-11 18:36:12]

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js#newcode28
polyfill.js:28: // for a "chrome" namespace with the extension APIs and assign
it to the
On 2017/10/11 10:59:19, Manish Jethani wrote:
> On 2017/10/11 01:40:05, Sebastian Noack wrote:
> > On 2017/10/11 01:09:20, Manish Jethani wrote:
> > [...]
> > 
> > Sure, keep using the "chrome" object (under the name "browser"), on Firefox,
> > will be a less risky option. But it also partly defeats the purpose of this
> > change, which is to use the browser extension API in a more standard
fashion.
> It
> > would be quite backwards and potentially confusing if "browser" is actually
> > "chrome", in particular in Firefox-only code that could otherwise use
> promises.
> 
> Alright, let's use the browser object directly, it makes more sense if we're
> going to migrate to a promise-based API anyway.

I'm going through the APIs that we actually use and I've just found one case
where browser and chrome differ (Firefox before version 52):

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage/Storage...

I haven't checked if this affects us, but I will do it.

But maybe it is safer to use chrome after all.

[Sebastian Noack, 2017-10-13 02:59:49]

LGTM

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js#newcode28
polyfill.js:28: // for a "chrome" namespace with the extension APIs and assign
it to the
On 2017/10/11 18:36:11, Manish Jethani wrote:
> On 2017/10/11 10:59:19, Manish Jethani wrote:
> > On 2017/10/11 01:40:05, Sebastian Noack wrote:
> > > On 2017/10/11 01:09:20, Manish Jethani wrote:
> > > [...]
> > > 
> > > Sure, keep using the "chrome" object (under the name "browser"), on
Firefox,
> > > will be a less risky option. But it also partly defeats the purpose of
this
> > > change, which is to use the browser extension API in a more standard
> fashion.
> > It
> > > would be quite backwards and potentially confusing if "browser" is
actually
> > > "chrome", in particular in Firefox-only code that could otherwise use
> > promises.
> > 
> > Alright, let's use the browser object directly, it makes more sense if we're
> > going to migrate to a promise-based API anyway.
> 
> I'm going through the APIs that we actually use and I've just found one case
> where browser and chrome differ (Firefox before version 52):
> 
>
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage/Storage...
> 
> I haven't checked if this affects us, but I will do it.
> 
> But maybe it is safer to use chrome after all.

It says that this inconsistency only exists in content scripts. We don't use
browser.storage/chrome.storage from content scripts.

[Manish Jethani, 2017-10-13 04:27:21]

On 2017/10/13 02:59:49, Sebastian Noack wrote:
> LGTM

Thanks!

[Manish Jethani, 2017-10-13 04:27:31]

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29570614/diff/29573098/polyfill.js#newcode28
polyfill.js:28: // for a "chrome" namespace with the extension APIs and assign
it to the
On 2017/10/13 02:59:49, Sebastian Noack wrote:
> On 2017/10/11 18:36:11, Manish Jethani wrote:
> > On 2017/10/11 10:59:19, Manish Jethani wrote:
> > > On 2017/10/11 01:40:05, Sebastian Noack wrote:
> > > > On 2017/10/11 01:09:20, Manish Jethani wrote:
> > > > [...]
> > > > 
> > > > Sure, keep using the "chrome" object (under the name "browser"), on
> Firefox,
> > > > will be a less risky option. But it also partly defeats the purpose of
> this
> > > > change, which is to use the browser extension API in a more standard
> > fashion.
> > > It
> > > > would be quite backwards and potentially confusing if "browser" is
> actually
> > > > "chrome", in particular in Firefox-only code that could otherwise use
> > > promises.
> > > 
> > > Alright, let's use the browser object directly, it makes more sense if
we're
> > > going to migrate to a promise-based API anyway.
> > 
> > I'm going through the APIs that we actually use and I've just found one case
> > where browser and chrome differ (Firefox before version 52):
> > 
> >
>
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage/Storage...
> > 
> > I haven't checked if this affects us, but I will do it.
> > 
> > But maybe it is safer to use chrome after all.
> 
> It says that this inconsistency only exists in content scripts. We don't use
> browser.storage/chrome.storage from content scripts.

Acknowledged.

[Manish Jethani, 2017-10-13 04:55:38]

Patch Set 9: Rebase

[Sebastian Noack, 2017-10-13 05:21:55]

LGTM

[Manish Jethani, 2017-10-13 20:01:02]

Patch Set 10: Rebase

[Sebastian Noack, 2017-10-13 23:17:38]

Still LGTM

[kzar, 2017-10-15 12:45:34]

As mentioned in IRC I recommend you update the buildtools dependency to
ba74948f8f01 as part of this change, since you switched to the browser namespace
for the devenv poller there. Take care to include the updated version of
ensure_dependencies.py. Note please don't include the webpack buildtools changes
which landed afterwards, since we'll end up creating a package-lock file again.

I'll update the buildtools dependency again afterwards to include the webpack
changes.

[kzar, 2017-10-16 10:35:52]

I've tested out these changes along with my webpack ones. They mostly seem to
work fine, except the devtools pane. I guess these changes depend on some
adblockplusui ones? If so could you link the review(s) in the description of
this review?

I've also checked and your changes pass linting, and mostly look OK to me except
the one nit below. Once the dependency updates are sorted this looks good to me.

https://codereview.adblockplus.org/29570614/diff/29575696/lib/notificationHel...
File lib/notificationHelper.js (right):

https://codereview.adblockplus.org/29570614/diff/29575696/lib/notificationHel...
lib/notificationHelper.js:215: "notification_without_buttons");
Nit: IMO it looks weird having the indentation like this without putting the
closing parenthesis on the next line. (Same below.)

[Manish Jethani, 2017-10-16 12:17:36]

Patch Set 11: Update buildtools dependency

https://codereview.adblockplus.org/29570614/diff/29575696/lib/notificationHel...
File lib/notificationHelper.js (right):

https://codereview.adblockplus.org/29570614/diff/29575696/lib/notificationHel...
lib/notificationHelper.js:215: "notification_without_buttons");
On 2017/10/16 10:35:51, kzar wrote:
> Nit: IMO it looks weird having the indentation like this without putting the
> closing parenthesis on the next line. (Same below.)

Done.

[Manish Jethani, 2017-10-16 12:18:10]

Patch Set 12: Update ensure_dependencies.py

[Manish Jethani, 2017-10-16 12:19:33]

On 2017/10/16 10:35:52, kzar wrote:
> I've tested out these changes along with my webpack ones. They mostly seem to
> work fine, except the devtools pane. I guess these changes depend on some
> adblockplusui ones? If so could you link the review(s) in the description of
> this review?

Done, it's in the description now.

The adblockplusui review is https://codereview.adblockplus.org/29573083/

[kzar, 2017-10-16 12:24:22]

LGTM once adblockplusui dependency is updated.

[Manish Jethani, 2017-10-17 13:11:51]

On 2017/10/16 12:24:22, kzar wrote:
> LGTM once adblockplusui dependency is updated.

Thanks!

https://hg.adblockplus.org/adblockpluschrome/rev/f9dfddb26058