Issue 5496 - Use omahaproxy to get chrome-versions

Issue 5496 - Use omahaproxy to get chrome-versions

[tlucas, 2017-08-14 08:04:30]

Patch Set 1

Changed get_chrome_versions to use the recommended omahaproxy.

[Sebastian Noack, 2017-08-14 11:54:31]

https://codereview.adblockplus.org/29515558/diff/29515559/globals/get_browser...
File globals/get_browser_versions.py (right):

https://codereview.adblockplus.org/29515558/diff/29515559/globals/get_browser...
globals/get_browser_versions.py:93: channels = {x['channel']: x for x in
data[0]['versions']}
It seems the logic can be simplified. All data we need for each channel is the
major version number. And if we extract it here already, we wouldn't need the
get_chrome_version() function.

  channels = {x['channel']: x['version'].split('.')[0]
              for x in data[0]['versions']}
  return {
      'current': channels['stable'],
      'unreleased': [channels['beta'], channels['dev']],
  }

[tlucas, 2017-08-14 12:08:28]

Patch Set 2

Removed "get_chrome_version" and moved the logic to the dict-comprehension

https://codereview.adblockplus.org/29515558/diff/29515559/globals/get_browser...
File globals/get_browser_versions.py (right):

https://codereview.adblockplus.org/29515558/diff/29515559/globals/get_browser...
globals/get_browser_versions.py:93: channels = {x['channel']: x for x in
data[0]['versions']}
On 2017/08/14 11:54:31, Sebastian Noack wrote:
> It seems the logic can be simplified. All data we need for each channel is the
> major version number. And if we extract it here already, we wouldn't need the
> get_chrome_version() function.
> 
>   channels = {x['channel']: x['version'].split('.')[0]
>               for x in data[0]['versions']}
>   return {
>       'current': channels['stable'],
>       'unreleased': [channels['beta'], channels['dev']],
>   }

Done.

[Sebastian Noack, 2017-08-14 12:41:28]

LGTM

[saroyanm, 2017-08-14 14:26:10]

https://codereview.adblockplus.org/29515558/diff/29515561/globals/get_browser...
File globals/get_browser_versions.py (right):

https://codereview.adblockplus.org/29515558/diff/29515561/globals/get_browser...
globals/get_browser_versions.py:18: CHROME_URL =
'https://omahaproxy.appspot.com/all.json?os=win'
The URL mentioned in the Issue tracker doesn't include os=win parameter. Is the
usage intentional ?

[Sebastian Noack, 2017-08-14 14:33:30]

https://codereview.adblockplus.org/29515558/diff/29515561/globals/get_browser...
File globals/get_browser_versions.py (right):

https://codereview.adblockplus.org/29515558/diff/29515561/globals/get_browser...
globals/get_browser_versions.py:18: CHROME_URL =
'https://omahaproxy.appspot.com/all.json?os=win'
On 2017/08/14 14:26:10, saroyanm wrote:
> The URL mentioned in the Issue tracker doesn't include os=win parameter. Is
the
> usage intentional ?

Note that this is consistent with the old implementation, using the update API,
where we set <os platform="win">. Since the requirements page doesn't account
for different Chrome version potentially being released for different operating
systems, we have to abstract that detail away. Using the version numbers of
Chrome for Windows seems reasonable as this is by far the most widely used
operating system. I've updated the issue description.

[saroyanm, 2017-08-14 14:34:48]

LGTM

https://codereview.adblockplus.org/29515558/diff/29515561/globals/get_browser...
File globals/get_browser_versions.py (right):

https://codereview.adblockplus.org/29515558/diff/29515561/globals/get_browser...
globals/get_browser_versions.py:18: CHROME_URL =
'https://omahaproxy.appspot.com/all.json?os=win'
On 2017/08/14 14:33:30, Sebastian Noack wrote:
> On 2017/08/14 14:26:10, saroyanm wrote:
> > The URL mentioned in the Issue tracker doesn't include os=win parameter. Is
> the
> > usage intentional ?
> 
> Note that this is consistent with the old implementation, using the update
API,
> where we set <os platform="win">. Since the requirements page doesn't account
> for different Chrome version potentially being released for different
operating
> systems, we have to abstract that detail away. Using the version numbers of
> Chrome for Windows seems reasonable as this is by far the most widely used
> operating system. I've updated the issue description.

Acknowledged.