Bug 85368 - [webkit-patch] Add new chrome-channels to track down current chrome release channels for a committed bug.
Summary: [webkit-patch] Add new chrome-channels to track down current chrome release c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Peters
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-02 05:37 PDT by Gavin Peters
Modified: 2012-05-08 06:36 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.99 KB, patch)
2012-05-02 05:39 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (24.24 KB, patch)
2012-05-07 17:18 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (26.34 KB, patch)
2012-05-07 19:45 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2012-05-02 05:37:18 PDT
[webkit-patch] Add new chrome-channels to track down current chrome release channels for a committed bug.
Comment 1 Gavin Peters 2012-05-02 05:39:05 PDT
Created attachment 139793 [details]
Patch
Comment 2 Gavin Peters 2012-05-02 05:43:55 PDT
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?
Comment 3 Dirk Pranke 2012-05-02 14:37:51 PDT
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.
Comment 4 Dirk Pranke 2012-05-02 14:40:12 PDT
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.
Comment 5 Eric Seidel (no email) 2012-05-02 17:40:56 PDT
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.
Comment 6 Gavin Peters 2012-05-07 10:23:57 PDT
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.
Comment 7 Gavin Peters 2012-05-07 17:18:03 PDT
Created attachment 140630 [details]
Patch
Comment 8 Gavin Peters 2012-05-07 17:28:11 PDT
The new upload adds tests and a lot more documentation.  I also documented bug-search, since chrome-channels is a variation on it.
Comment 9 Eric Seidel (no email) 2012-05-07 17:30:53 PDT
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. :)
Comment 10 Gavin Peters 2012-05-07 19:45:41 PDT
Created attachment 140651 [details]
Patch
Comment 11 Gavin Peters 2012-05-07 19:50:08 PDT
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.
Comment 12 Eric Seidel (no email) 2012-05-07 19:56:15 PDT
Comment on attachment 140651 [details]
Patch

LGTM.
Comment 13 WebKit Review Bot 2012-05-08 06:36:43 PDT
Comment on attachment 140651 [details]
Patch

Clearing flags on attachment: 140651

Committed r116414: <http://trac.webkit.org/changeset/116414>
Comment 14 WebKit Review Bot 2012-05-08 06:36:48 PDT
All reviewed patches have been landed.  Closing bug.