Issue 5587- Use dedicated mobile options page in Firefox Mobile

Fix for Edge not supporting 'chrome' namespace introduced in 3c76db9d2029

[Manish Jethani, 2017-10-09 12:03:52]

https://codereview.adblockplus.org/29570575/diff/29570576/options.js
File options.js (right):

https://codereview.adblockplus.org/29570575/diff/29570576/options.js#newcode29
options.js:29: if (typeof chrome == "undefined" || typeof chrome.runtime ==
"undefined")
LGTM

I would move it to the top of the file though, right after "use strict", but
your call.

[Manish Jethani, 2017-10-09 12:04:54]

You could also CC Dave on this one.

[kzar, 2017-10-09 14:02:42]

(Yes, please Cc me on Platform (and core) issues, thanks Manish.)

Nice catch, but as mentioned in IRC please could you open an issue for this? I
think it warrants one, so the QA guys know what to test and to provide some more
context about why the change was necessary.

Also as mentioned in IRC did you check if this is a problem elsewhere, for
example in the popup?

[Manish Jethani, 2017-10-09 15:23:39]

I think the commit message should reference issue #5587 but it should say what
the change is doing specifically rather than "Use dedicated mobile options page
in Firefox Mobile". The text in the description here, for example.

Otherwise LGTM

[Sebastian Noack, 2017-10-09 15:29:28]

https://codereview.adblockplus.org/29570575/diff/29570646/options.js
File options.js (right):

https://codereview.adblockplus.org/29570575/diff/29570646/options.js#newcode23
options.js:23: window.chrome = window.browser;
This fallback is duplicated in ext/common.js. However, since we don't want to
increase the usage of ext, but eventually get rid of it, I suggest to move the
code from ext/common.js that isn't specific to ext, into a separate script and
is included everywhere where ext/common.js is used and in options.js. An
appropriate name for such a script would be polyfill.js, and its content should
look like this:

  // In Edge the namespace for extension APIs is 'browser'. The 'chrome' object,
  // while available does not contain the runtime object needed below.
  if (typeof chrome == "undefined" || typeof chrome.extension == "undefined")
    window.chrome = window.browser;

  // Workaround since HTMLCollection, NodeList, StyleSheet and
  // CSSRuleList didn't have iterator support before Chrome 51.
  // https://bugs.chromium.org/p/chromium/issues/detail?id=401699
  let arrayIterator = Array.prototype[Symbol.iterator];
  if (!(Symbol.iterator in HTMLCollection.prototype))
    HTMLCollection.prototype[Symbol.iterator] = arrayIterator;
  if (!(Symbol.iterator in NodeList.prototype))
    NodeList.prototype[Symbol.iterator] = arrayIterator;
  if (!(Symbol.iterator in StyleSheetList.prototype))
    StyleSheetList.prototype[Symbol.iterator] = arrayIterator;
  if (!(Symbol.iterator in CSSRuleList.prototype))
    CSSRuleList.prototype[Symbol.iterator] = arrayIterator;

[Sebastian Noack, 2017-10-09 15:36:21]

On 2017/10/09 14:02:42, kzar wrote:
> Nice catch, but as mentioned in IRC please could you open an issue for this? I
> think it warrants one, so the QA guys know what to test and to provide some
more
> context about why the change was necessary.

To be fair, since this a regression introduced by #5587, that was never visible
in any development builds, QA should be covered by that issue. So I'm fine with
relating this change to #5587 (as Ollie did now). But please use a more distinct
commit message / review title.

[Manish Jethani, 2017-10-09 16:18:43]

On 2017/10/09 15:36:21, Sebastian Noack wrote:
> On 2017/10/09 14:02:42, kzar wrote:
> > Nice catch, but as mentioned in IRC please could you open an issue for this?
I
> > think it warrants one, so the QA guys know what to test and to provide some
> more
> > context about why the change was necessary.
> 
> To be fair, since this a regression introduced by #5587, that was never
visible
> in any development builds, QA should be covered by that issue. So I'm fine
with
> relating this change to #5587 (as Ollie did now). But please use a more
distinct
> commit message / review title.

Ollie, I'm making a bigger change here:

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

I found myself writing similar code to cover this case, but in the opposite
direction (browser points to chrome on non-Edge platforms). Since Sebastian has
suggestion that we put this in a separate file called polyfill.js, I think I can
do all of this as part of that big change. We can close this issue then.