[webkit-reviews] review granted: [Bug 53288] Add webkit-patch roll-chromium-deps : [Attachment 80426] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 53288: Add webkit-patch roll-chromium-deps
https://bugs.webkit.org/show_bug.cgi?id=53288

Attachment 80426: Patch
https://bugs.webkit.org/attachment.cgi?id=80426&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=80426&action=review

In general this seems fine as a v1.  Thanks.

> Tools/Scripts/webkitpy/common/checkout/api.py:153
> +    def chromium_deps(self):
> +	   return DEPS(os.path.join(self._scm.checkout_root, "Source",
"WebKit", "chromium", "DEPS"))

This doesn't really seem to belong on Checkout, IMO.  But I guess it's OK.

> Tools/Scripts/webkitpy/tool/mocktool.py:498
> +class MockChromiumDEPS(object):

MockDEPs, no?

> Tools/Scripts/webkitpy/tool/commands/roll_unittest.py:48
> +	       self.assert_execute_outputs(RollChromiumDEPS(), [5764])

Just add an expected_exception argument and pass it along to OutputCapture, no?


> Tools/Scripts/webkitpy/tool/steps/preparechangelogfordepsroll.py:40
> +	       ChangeLog(changelog_path).update_with_unreviewed_message("Rolled
DEPS.\n\n")

In this case seems it should be rolled to latest known good revisoin (maybe
even with the link).  Exept that doesn't belong here.  You'd want to pass that
in from the command or something, no?  I guess this wants to pull a message off
of state?

Yes, that could be v2.

> Tools/Scripts/webkitpy/tool/steps/preparechangelogforrevert_unittest.py:41
> +class UpdateChangeLogsWithReviewerTest(unittest.TestCase):

None of these have reviewers.  Seems this is the wrong name for the class?

> Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:43
> +	   new_chromium_revision = state["chromium_revision"]

This needs a comment as to why you use [] instead of .get().

> Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:57
> +	   if new_chromium_revision < current_chromium_revision:
> +	       log("Current Chromium DEPS revision %s is newer than %s." %
(current_chromium_revision, new_chromium_revision))
> +	       new_chromium_revision = self._tool.user.prompt("Enter new
chromium revision (enter nothing to cancel):\n")
> +	       try:
> +		   new_chromium_revision = int(new_chromium_revision)
> +	       except ValueError, TypeError:
> +		   new_chromium_revision = None
> +	       if not new_chromium_revision:
> +		   error("Unable to update Chromium DEPS")

I would have split this out into some sort of _validate_blah_blah method.