Issue 5028 - Use browser namespace

polyfill.js

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-present eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+"use strict";
+
+// On Edge, the extension APIs are in the "browser" namespace, whereas on
+// both Firefox and Chrome they're in the "chrome" namespace. Firefox has a
+// "browser" namespace too, but it is slightly incompatible in that the
+// implementation uses promises as a fallback in the absence of a callback
+// parameter. Edge sometimes has a "chrome" namespace, but with no extension
+// APIs.
+//
+// In order to make the same code run on all three platforms, we simply look
+// for a "chrome" namespace with the extension APIs and assign it to the

[Sebastian Noack, 2017/10/10 23:07:43]

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).

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

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]

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:05]

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.
It is actually the other way around. ;)
https://lists.w3.org/Archives/Public/public-browserext/2016Oct/0002.html

[Manish Jethani, 2017/10/11 10:59:19]

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

[Manish Jethani, 2017/10/11 10:59:19]

Acknowledged.

[Manish Jethani, 2017/10/11 18:36:11]

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]

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:31]

Acknowledged.

+// "browser" namespace.
+if (typeof chrome != "undefined" && "extension" in chrome)

[Sebastian Noack, 2017/10/10 23:07:43]

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.

[Manish Jethani, 2017/10/11 10:59:18]

Done.

+  window.browser = chrome;
+
+// Workaround since HTMLCollection, NodeList, StyleSheetList, and CSSRuleList
+// didn't have iterator support before Chrome 51.
+// https://bugs.chromium.org/p/chromium/issues/detail?id=401699
+for (let object of [HTMLCollection, NodeList, StyleSheetList, CSSRuleList])
+{
+  if (!(Symbol.iterator in object.prototype))
+    object.prototype[Symbol.iterator] = Array.prototype[Symbol.iterator];
+}