Issue 3415 - Detect application based on UA for Chromium-based browsers

Issue 3415 - Detect application based on UA for Chromium-based browsers

[Sebastian Noack, 2015-12-15 16:36:58]

https://codereview.adblockplus.org/29332680/diff/29332687/chromeInfo.js.tmpl
File chromeInfo.js.tmpl (right):

https://codereview.adblockplus.org/29332680/diff/29332687/chromeInfo.js.tmpl#...
chromeInfo.js.tmpl:27: application = app == "OPR" ? "opera" : app.toLowerCase();
For reference, some UAs, I tested the logic against:

http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser51856...
http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser25589...
http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser19693...
http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser11563...
http://www.webapps-online.com/online-tools/user-agent-strings/dv/plugin55210/...

[kzar, 2015-12-15 17:26:01]

https://codereview.adblockplus.org/29332680/diff/29332687/chromeInfo.js.tmpl
File chromeInfo.js.tmpl (right):

https://codereview.adblockplus.org/29332680/diff/29332687/chromeInfo.js.tmpl#...
chromeInfo.js.tmpl:10: var regexp = /(\S+)\/(\S+)(?:\s*\(.*?\))?/g;
Nit: More descriptive name would be nice, perhaps app_ver_regexp?

https://codereview.adblockplus.org/29332680/diff/29332687/chromeInfo.js.tmpl#...
chromeInfo.js.tmpl:13: while (match = regexp.exec(navigator.userAgent))
Maybe a comment for this loop explaining that we're inspecting the browser's
user agent string to make our best guess of platform and version?

https://codereview.adblockplus.org/29332680/diff/29332687/chromeInfo.js.tmpl#...
chromeInfo.js.tmpl:27: application = app == "OPR" ? "opera" : app.toLowerCase();
On 2015/12/15 16:36:58, Sebastian Noack wrote:
> For reference, some UAs, I tested the logic against:
> 
>
http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser51856...
>
http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser25589...
>
http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser19693...
>
http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser11563...
>
http://www.webapps-online.com/online-tools/user-agent-strings/dv/plugin55210/...

Nit: Maybe just do `application = app.toLowerCase()` here and then later change
"opr" to "opera"?

[Sebastian Noack, 2015-12-15 17:31:31]

https://codereview.adblockplus.org/29332680/diff/29332687/chromeInfo.js.tmpl
File chromeInfo.js.tmpl (right):

https://codereview.adblockplus.org/29332680/diff/29332687/chromeInfo.js.tmpl#...
chromeInfo.js.tmpl:10: var regexp = /(\S+)\/(\S+)(?:\s*\(.*?\))?/g;
On 2015/12/15 17:26:00, kzar wrote:
> Nit: More descriptive name would be nice, perhaps app_ver_regexp?

Well, the context here is limitted. And after all we are used to call match
simply match as well.

https://codereview.adblockplus.org/29332680/diff/29332687/chromeInfo.js.tmpl#...
chromeInfo.js.tmpl:13: while (match = regexp.exec(navigator.userAgent))
On 2015/12/15 17:26:00, kzar wrote:
> Maybe a comment for this loop explaining that we're inspecting the browser's
> user agent string to make our best guess of platform and version?

Well, how could a comment make it more obvious that we inspect the user agent
string, while this line literally says execute a regular expression on
navigator.userAgent? ;) I think the less intuitive bits are already in the
comments below.

https://codereview.adblockplus.org/29332680/diff/29332687/chromeInfo.js.tmpl#...
chromeInfo.js.tmpl:27: application = app == "OPR" ? "opera" : app.toLowerCase();
On 2015/12/15 17:26:00, kzar wrote:
> On 2015/12/15 16:36:58, Sebastian Noack wrote:
> > For reference, some UAs, I tested the logic against:
> > 
> >
>
http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser51856...
> >
>
http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser25589...
> >
>
http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser19693...
> >
>
http://www.webapps-online.com/online-tools/user-agent-strings/dv/browser11563...
> >
>
http://www.webapps-online.com/online-tools/user-agent-strings/dv/plugin55210/...
> 
> Nit: Maybe just do `application = app.toLowerCase()` here and then later
change
> "opr" to "opera"?

I don't think that spreading these conversions across the code makes anything
easier to understand.

[kzar, 2015-12-15 17:40:26]

Don't want to argue about nits so LGTM