Bug 85368 – [webkit-patch] Add new chrome-channels to track down current chrome release channels for a committed bug.

• Component: New Bugs
• Status: RESOLVED FIXED

[Gavin Peters]

[webkit-patch] Add new chrome-channels to track down current chrome release channels for a committed bug.

[Gavin Peters]

Created attachment 139793 [details]
Patch

[Gavin Peters]

I'd like to eventually make this into a mini-bot that comments on bugs that have a keyword chrome-channel-reports on them.  But, for now, this new command will save me a lot of time. 

dpranke, WDYT?

[Dirk Pranke]

Comment on attachment 139793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139793&action=review

We typically require at least *some* form of test for anything that gets checked in, even if it is a "scripting hack" :)

> Tools/Scripts/webkitpy/common/net/omahaproxy.py:67
> +                    row = (int(version['base_webkit_revision']), version['channel'], platform['os'], version['date'])

Should this be a dict or an object instead of a tuple?

> Tools/Scripts/webkitpy/tool/commands/chromechannels.py:38
> +    help_text = "Lists which chrome channels the commit associated with a bug is included in."

I'm actually unclear on this ... is it figuring out when a bugfix is introduced, when a bug is introduced, or is really just querying revision numbers regardless of what the change in the revision was for?

Also, it looks like the usage is "webkit-patch chrome-channels bug" ... can we mention that bugno is a required argument somewhere and whether it is a bug number, or a URL to the bug, or ... ?

What happens if there were multiple commits for a single bug number? It's rare, but it certainly happens ... do you look for channels that have any of the commits? all of the commits? etc.

[Dirk Pranke]

I'm also a little torn on this functionality since it is so chromium-specific; I'm not sure how suitable this is for being in webkit at all.

That said, it does seem useful, so I'm on the side of including it.

[Eric Seidel (no email)]

Comment on attachment 139793 [details]
Patch

The more the merrier.  Chrome should feel welcome to add chrome-specific stuff to webkit-patch IMO.  The architecture was designed such that Apple could add sekret commands in some other directory for importing into Radar, etc.  I don't see why Chrome shouldn't be able to do the same.  We could move this command out of the main WebKit system at some point if needed.  I'm not sure Host should hold the OmahaProxy however, since this is the only command which needs one, and adding something to host means all webkitpy commands would load it.

[Gavin Peters]

Comment on attachment 139793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139793&action=review

>> Tools/Scripts/webkitpy/common/net/omahaproxy.py:67
>> +                    row = (int(version['base_webkit_revision']), version['channel'], platform['os'], version['date'])
> 
> Should this be a dict or an object instead of a tuple?

Yes. Done.

>> Tools/Scripts/webkitpy/tool/commands/chromechannels.py:38
>> +    help_text = "Lists which chrome channels the commit associated with a bug is included in."
> 
> I'm actually unclear on this ... is it figuring out when a bugfix is introduced, when a bug is introduced, or is really just querying revision numbers regardless of what the change in the revision was for?
> 
> Also, it looks like the usage is "webkit-patch chrome-channels bug" ... can we mention that bugno is a required argument somewhere and whether it is a bug number, or a URL to the bug, or ... ?
> 
> What happens if there were multiple commits for a single bug number? It's rare, but it certainly happens ... do you look for channels that have any of the commits? all of the commits? etc.

I've added long help, and also improved my search through the bug history to bail earlier when the history is too confusing.

[Gavin Peters]

Created attachment 140630 [details]
Patch

[Gavin Peters]

The new upload adds tests and a lot more documentation.  I also documented bug-search, since chrome-channels is a variation on it.

[Eric Seidel (no email)]

Comment on attachment 140630 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140630&action=review

This seems really cool. :)

> Tools/Scripts/webkitpy/common/net/omahaproxy.py:42
> +    chrome_platforms = {"linux": "Linux", "win": "Windows", "mac": "Mac", "cros": "Chrome OS", "cf": "Chrome Frame"}

IMO this dict would be more readable written out one pair per line.

> Tools/Scripts/webkitpy/common/net/omahaproxy.py:63
> +    def _get_json(self):
> +        try:
> +            return urllib2.urlopen(self._json_url()).read()
> +        except urllib2.HTTPError, e:
> +            if e.code == 404:
> +                return None
> +            raise e

I think we have something to do this for you.... NetworkRequest iirc?

> Tools/Scripts/webkitpy/common/net/omahaproxy.py:71
> +                    row = {"commit": int(version["base_webkit_revision"]),

I would normally have put the key/value pairs indented to 4c insetad of right after the {.  I'm not sure what official pep8 is.

> Tools/Scripts/webkitpy/common/net/omahaproxy.py:75
> +                           }

I would have unintended this all the way to match "row".  I'm not sure what official pep8 style is.

> Tools/Scripts/webkitpy/common/net/omahaproxy_unittest.py:44
> +    example_omahaproxy_json = """[{"os": "win", "versions": [{"base_webkit_revision": "116185", "v8_ver": "3.10.8.1", "wk_ver": "536.11", "base_trunk_revision": 135598, "prev_version": "20.0.1128.0", "version": "20.0.1129.0", "date": "05\/07\/12", "prev_date": "05\/06\/12", "true_branch": "trunk", "channel": "canary", "branch_revision": "NA"}, {"base_webkit_revision": "115687", "v8_ver": "3.10.6.0", "wk_ver": "536.10", "base_trunk_revision": 134666, "prev_version": "20.0.1123.1", "version": "20.0.1123.4", "date": "05\/04\/12", "prev_date": "05\/02\/12", "true_branch": "1123", "channel": "dev", "branch_revision": 135092}]},{"os": "linux", "versions": [{"base_webkit_revision": "115688", "v8_ver": "3.10.6.0", "wk_ver": "536.10", "base_trunk_revision": 134666, "prev_version": "20.0.1123.2", "version": "20.0.1123.4", "date": "05\/04\/12", "prev_date": "05\/02\/12", "true_branch": "1123", "channel": "dev", "branch_revision": 135092}, {"base_webkit_revision": "112327", "v8_ver": "3.9.24.17", "wk_ver": "536.5", "base_trunk_revision": 129376, "prev_version": "19.0.1084.36", "version": "19.0.1084.41", "date": "05\/03\/12", "prev_date": "04\/25\/12", "true_branch": "1084", "channel": "beta", "branch_revision": 134854}, {"base_webkit_revision": "*", "v8_ver": "3.9.24.17", "wk_ver": "536.5", "base_trunk_revision": 129376, "prev_version": "19.0.1084.36", "version": "19.0.1084.41", "date": "05\/03\/12", "prev_date": "04\/25\/12", "true_branch": "1084", "channel": "release", "branch_revision": 134854}]}]"""

Is this from the horses mouth?  Should we pretty-print this here?

> Tools/Scripts/webkitpy/common/net/omahaproxy_unittest.py:46
> +    expected_revisions = [{"commit": 116185, "channel": "canary", "platform": "Windows", "date": "05/07/12"},

Again, this is kind odd indent for webkitpy.  Again, not sure what pep8's official style for multi-line lists or dicts is.

> Tools/Scripts/webkitpy/tool/commands/chromechannels.py:61
> +    def execute(self, options, args, tool):

You could break this function up into smaller pieces which would be more testable. :)  For example, having a function which processed one bug at a time would allow you to test that function with a mock bug. :)

[Gavin Peters]

Created attachment 140651 [details]
Patch

[Gavin Peters]

Comment on attachment 140630 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140630&action=review

This version should be more testable.

>> Tools/Scripts/webkitpy/common/net/omahaproxy.py:42
>> +    chrome_platforms = {"linux": "Linux", "win": "Windows", "mac": "Mac", "cros": "Chrome OS", "cf": "Chrome Frame"}
> 
> IMO this dict would be more readable written out one pair per line.

Done.

>> Tools/Scripts/webkitpy/common/net/omahaproxy.py:63
>> +            raise e
> 
> I think we have something to do this for you.... NetworkRequest iirc?

NetworkTransaction.  Done.

>> Tools/Scripts/webkitpy/common/net/omahaproxy.py:71
>> +                    row = {"commit": int(version["base_webkit_revision"]),
> 
> I would normally have put the key/value pairs indented to 4c insetad of right after the {.  I'm not sure what official pep8 is.

Done.

>> Tools/Scripts/webkitpy/common/net/omahaproxy.py:75
>> +                           }
> 
> I would have unintended this all the way to match "row".  I'm not sure what official pep8 style is.

Done.

>> Tools/Scripts/webkitpy/common/net/omahaproxy_unittest.py:44
>> +    example_omahaproxy_json = """[{"os": "win", "versions": [{"base_webkit_revision": "116185", "v8_ver": "3.10.8.1", "wk_ver": "536.11", "base_trunk_revision": 135598, "prev_version": "20.0.1128.0", "version": "20.0.1129.0", "date": "05\/07\/12", "prev_date": "05\/06\/12", "true_branch": "trunk", "channel": "canary", "branch_revision": "NA"}, {"base_webkit_revision": "115687", "v8_ver": "3.10.6.0", "wk_ver": "536.10", "base_trunk_revision": 134666, "prev_version": "20.0.1123.1", "version": "20.0.1123.4", "date": "05\/04\/12", "prev_date": "05\/02\/12", "true_branch": "1123", "channel": "dev", "branch_revision": 135092}]},{"os": "linux", "versions": [{"base_webkit_revision": "115688", "v8_ver": "3.10.6.0", "wk_ver": "536.10", "base_trunk_revision": 134666, "prev_version": "20.0.1123.2", "version": "20.0.1123.4", "date": "05\/04\/12", "prev_date": "05\/02\/12", "true_branch": "1123", "channel": "dev", "branch_revision": 135092}, {"base_webkit_revision": "112327", "v8_ver": "3.9.24.17", "wk_ver": "536.5", "base_trunk_revision": 129376, "prev_version": "19.0.1084.36", "version": "19.0.1084.41", "date": "05\/03\/12", "prev_date": "04\/25\/12", "true_branch": "1084", "channel": "beta", "branch_revision": 134854}, {"base_webkit_revision": "*", "v8_ver": "3.9.24.17", "wk_ver": "536.5", "base_trunk_revision": 129376, "prev_version": "19.0.1084.36", "version": "19.0.1084.41", "date": "05\/03\/12", "prev_date": "04\/25\/12", "true_branch": "1084", "channel": "release", "branch_revision": 134854}]}]"""
> 
> Is this from the horses mouth?  Should we pretty-print this here?

Done.

>> Tools/Scripts/webkitpy/common/net/omahaproxy_unittest.py:46
>> +    expected_revisions = [{"commit": 116185, "channel": "canary", "platform": "Windows", "date": "05/07/12"},
> 
> Again, this is kind odd indent for webkitpy.  Again, not sure what pep8's official style for multi-line lists or dicts is.

Done.

>> Tools/Scripts/webkitpy/tool/commands/chromechannels.py:61
>> +    def execute(self, options, args, tool):
> 
> You could break this function up into smaller pieces which would be more testable. :)  For example, having a function which processed one bug at a time would allow you to test that function with a mock bug. :)

Done.

[Eric Seidel (no email)]

Comment on attachment 140651 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 140651 [details]
Patch

Clearing flags on attachment: 140651

Committed r116414: <http://trac.webkit.org/changeset/116414>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.