Issue 5660 - Check for runtime.getBrowserInfo API support

Issue 5660 - Check for runtime.getBrowserInfo API support

[Manish Jethani, 2017-09-13 01:45:22]

Patch Set 1

I checked and we're only checking specifically for "fennec" so far. It should be
OK for application to be "unknown" and applicationVersion to be "0" on Firefox
50.

[Sebastian Noack, 2017-09-13 01:59:49]

I suppose we could just fall back to the user agent if runtime.getBrowserInfo is
not supported.

[Sebastian Noack, 2017-09-13 02:07:04]

> I checked and we're only checking specifically for "fennec" so far. It should
be
> OK for application to be "unknown" and applicationVersion to be "0" on Firefox
> 50.

We also include "application" and "applicationVersion" in the query string when
downloading filters list and notifications.

As for the application logic, I agree that it doesn't matter as we only check
for "fennec", and for other reasons Adblock Plus wouldn't load on Firefox Mobile
<55, anyway. But how likely is it that we will keep in mind that
application/applicationVersion are unknown on Firefox 50, as we add
functionality and workarounds?

[Manish Jethani, 2017-09-13 07:44:12]

Patch Set 2: Fall back on UA string

[Manish Jethani, 2017-09-13 07:47:06]

On 2017/09/13 01:59:49, Sebastian Noack wrote:
> I suppose we could just fall back to the user agent if runtime.getBrowserInfo
is
> not supported.

Yeah, I think this is a good idea.

Note that we could just use the UA string first and then overwrite the values
with getBrowserInfo once it's available, but I don't like the inconsistency that
this might introduce. I've separated out the UA string parsing into the else
block, and it's only run if getBrowserInfo is not available on the platform.
This change should then not affect any other versions of Firefox.

[Sebastian Noack, 2017-09-13 17:38:01]

https://codereview.adblockplus.org/29542859/diff/29543565/templates/geckoInfo...
File templates/geckoInfo.js.tmpl (right):

https://codereview.adblockplus.org/29542859/diff/29543565/templates/geckoInfo...
templates/geckoInfo.js.tmpl:43: exportApplicationInfo({name: browserInfo[1],
version: browserInfo[2]});
There is only one application this code is possibly running on, i.e. Firefox on
desktop. Firefox Mobile didn't support WebExtensions before version 54 (which
already has runtime.getBrowserInfo), and Adblock Plus won't load before version
55, anyway. Other Gecko applications don't have WebExtension support to date.

Even if this code could run on Firefox Mobile, your logic wouldn't detect it, as
the user agent string looks like "Mozilla/5.0 (Android 4.4; Mobile; rv:41.0)
Gecko/41.0 Firefox/41.0".

So it is safe (if not even more reliable) to hard-code Firefox as the
application here, which will also simplify the detection of the version:

  if ("getBrowserInfo" in browser.runtime)
  {
    browser.runtime.getBrowserInfo().then(browserInfo =>
    {
      exports.application = browserInfo.name.toLowerCase();
      exports.applicationVersion = browserInfo.version;
    });
  }
  else
  {
    exports.application = "firefox";
    let match = /\bFirefox\/(\S+)/.exec(avigator.userAgent);
    if (match)
      exports.applicationVersion = match[1];
  }

[Manish Jethani, 2017-09-14 04:27:12]

https://codereview.adblockplus.org/29542859/diff/29543565/templates/geckoInfo...
File templates/geckoInfo.js.tmpl (right):

https://codereview.adblockplus.org/29542859/diff/29543565/templates/geckoInfo...
templates/geckoInfo.js.tmpl:43: exportApplicationInfo({name: browserInfo[1],
version: browserInfo[2]});
On 2017/09/13 17:38:01, Sebastian Noack wrote:
> There is only one application this code is possibly running on, i.e. Firefox
on
> desktop. Firefox Mobile didn't support WebExtensions before version 54 (which
> already has runtime.getBrowserInfo), and Adblock Plus won't load before
version
> 55, anyway. Other Gecko applications don't have WebExtension support to date.

Hypothetically, suppose other Gecko-based browsers start supporting
WebExtensions at some point, then for the UA string "Mozilla/5.0 (Macintosh;
Intel Mac OS X 10.5; rv:2.0.1) Gecko/20100101 Firefox/4.0.1 Camino/2.2.1", we
want the application/version to be Firefox 4.0.1 or Camino 2.2.1?

Anyway, if it's only going to run on Firefox for now then I guess it's OK to
just query the Firefox version number. I'll make the change.

[Sebastian Noack, 2017-09-14 04:33:41]

https://codereview.adblockplus.org/29542859/diff/29543565/templates/geckoInfo...
File templates/geckoInfo.js.tmpl (right):

https://codereview.adblockplus.org/29542859/diff/29543565/templates/geckoInfo...
templates/geckoInfo.js.tmpl:43: exportApplicationInfo({name: browserInfo[1],
version: browserInfo[2]});
On 2017/09/14 04:27:11, Manish Jethani wrote:
> On 2017/09/13 17:38:01, Sebastian Noack wrote:
> > There is only one application this code is possibly running on, i.e. Firefox
> on
> > desktop. Firefox Mobile didn't support WebExtensions before version 54
(which
> > already has runtime.getBrowserInfo), and Adblock Plus won't load before
> version
> > 55, anyway. Other Gecko applications don't have WebExtension support to
date.
> 
> Hypothetically, suppose other Gecko-based browsers start supporting
> WebExtensions at some point, then for the UA string "Mozilla/5.0 (Macintosh;
> Intel Mac OS X 10.5; rv:2.0.1) Gecko/20100101 Firefox/4.0.1 Camino/2.2.1", we
> want the application/version to be Firefox 4.0.1 or Camino 2.2.1?

This would only be relevant in the unlikely case that browser is using Gecko 50.
Older versions are not supported by anyway. On newer versions we'd use
runtime.getBrowserInfo().

[Manish Jethani, 2017-09-14 06:06:17]

Patch Set 3: Parse UA string for only version number

[Manish Jethani, 2017-09-14 06:06:51]

https://codereview.adblockplus.org/29542859/diff/29543565/templates/geckoInfo...
File templates/geckoInfo.js.tmpl (right):

https://codereview.adblockplus.org/29542859/diff/29543565/templates/geckoInfo...
templates/geckoInfo.js.tmpl:43: exportApplicationInfo({name: browserInfo[1],
version: browserInfo[2]});
On 2017/09/13 17:38:01, Sebastian Noack wrote:
> There is only one application this code is possibly running on, i.e. Firefox
on
> desktop. Firefox Mobile didn't support WebExtensions before version 54 (which
> already has runtime.getBrowserInfo), and Adblock Plus won't load before
version
> 55, anyway. Other Gecko applications don't have WebExtension support to date.
> 
> Even if this code could run on Firefox Mobile, your logic wouldn't detect it,
as
> the user agent string looks like "Mozilla/5.0 (Android 4.4; Mobile; rv:41.0)
> Gecko/41.0 Firefox/41.0".
> 
> So it is safe (if not even more reliable) to hard-code Firefox as the
> application here, which will also simplify the detection of the version:
> 
>   if ("getBrowserInfo" in browser.runtime)
>   {
>     browser.runtime.getBrowserInfo().then(browserInfo =>
>     {
>       exports.application = browserInfo.name.toLowerCase();
>       exports.applicationVersion = browserInfo.version;
>     });
>   }
>   else
>   {
>     exports.application = "firefox";
>     let match = /\bFirefox\/(\S+)/.exec(avigator.userAgent);
>     if (match)
>       exports.applicationVersion = match[1];
>   }

Done.

[Sebastian Noack, 2017-09-14 15:40:02]

LGTM