Add experimental prototype Rietveld integration to webkit-patch upload

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Add experimental prototype Rietveld integration to webkit-patch upload

[Adam Barth]

Created attachment 53113 [details]
Patch

[Adam Barth]

Comment on attachment 53113 [details]
Patch

Let me try a slightly tweak.  One sec.

[Adam Barth]

Created attachment 53114 [details]
Patch

Feel free to provide comments at https://codereview.appspot.com/818047

[Chris Jerdonek]

(In reply to comment #3)
> Created an attachment (id=53114) [details]
> Patch
> 
> Feel free to provide comments at https://codereview.appspot.com/818047

Was this posted via the code itself?  It looks like only part of the patch got posted, or maybe part of a diff of a later patch of this report with an earlier patch.  Otherwise I would review it there.

[Adam Barth]

> Was this posted via the code itself?

Yes.

> It looks like only part of the patch got
> posted, or maybe part of a diff of a later patch of this report with an earlier
> patch.  Otherwise I would review it there.

Yeah, it seems to be diffing against the index instead of against HEAD.  I'll have to look into that, although I'd like to do that in a second iteration.

[Chris Jerdonek]

(In reply to comment #3)
> Created an attachment (id=53114) [details]
> Patch

Thanks for getting the ball rolling on this, Adam!  It's amazing you were able to do this with so little code.

Here are some quick, random reactions rather than a review.  I haven't looked much at the webkit-patch code before, so some (though not all) of these comments may be somewhat speculative.

--- a/WebKitTools/Scripts/webkitpy/common/config/__init__.py
+++ b/WebKitTools/Scripts/webkitpy/common/config/__init__.py
@@ -1 +1,7 @@
 # Required for Python to search this directory for module files
+
+import re
+
+codereview_server_host = "codereview.appspot.com"
+codereview_server_regex = "https?://%s/" % re.sub('\.', '\\.', codereview_server_host)
+codereview_server_url = "https://%s/" % codereview_server_host

Until we figure out how we want to use our __init__ files, I would recommend leaving the file empty and putting this information in a separate file.  Also, I've mentioned this before, but I have mixed feelings about putting settings for a range of areas in a single config folder because it increases the number of couplings.  I think it might be better, for example, to put Rietveld-related settings in a rietveld/ folder.  To illustrate with another example, I think putting style settings like these in the config folder would make the style code harder to understand and maintain:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=57119#L71

Keeping the settings close to the code where they're used (but clearly separated off) would I think be easier.  We can always expose parameters on the API that allow values other than the settings to be passed in, for more flexibility, etc.  That approach would also make it a bit simpler to drop new functionality in and out.

b/WebKitTools/Scripts/webkitpy/common/net/rietveld.py
new file mode 100644

+from webkitpy.common.system.deprecated_logging import log
+from webkitpy.common.system.executive import ScriptError

Should we still be using deprecated_logging in new code?

b/WebKitTools/Scripts/webkitpy/thirdparty/rietveld/__init__.py
new file mode 100644
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at

Is the Apache license compatible with the WebKit project?  I once asked re: Apache 1.1 on webkit-dev, but never got a response:

https://lists.webkit.org/pipermail/webkit-dev/2010-January/011461.html

diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
index a425e90..24b4ef7 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
@@ -68,6 +68,7 @@ class AbstractQueueTest(CommandsTest):
     def _assert_run_webkit_patch(self, run_args):
         queue = TestQueue()
         tool = MockTool()
+        tool.executive = Mock()
         queue.bind_to_tool(tool)
 
See remarks below.

diff --git a/WebKitTools/Scripts/webkitpy/tool/main.py b/WebKitTools/Scripts/webkitpy/tool/main.py
index 629e86f..dd7652d 100755
--- a/WebKitTools/Scripts/webkitpy/tool/main.py
+++ b/WebKitTools/Scripts/webkitpy/tool/main.py
@@ -36,6 +36,7 @@ from webkitpy.common.checkout.api import Checkout
 from webkitpy.common.checkout.scm import detect_scm_system
 from webkitpy.common.net.bugzilla import Bugzilla
 from webkitpy.common.net.buildbot import BuildBot
+from webkitpy.common.net.rietveld import Rietveld
 from webkitpy.common.net.irc.ircproxy import IRCProxy
 from webkitpy.common.system.executive import Executive
 from webkitpy.common.system.user import User
@@ -70,6 +71,7 @@ class WebKitPatch(MultiCommandTool):
         self._scm = None
         self._checkout = None
         self.status_server = StatusServer()
+        self.codereview = Rietveld(self.executive)

Given that WebKitPatch is meant to be a generic all-purpose tool, I wonder if there's a way to design things so the class doesn't itself have to be aware of all its capabilities a priori (e.g. if there is some sort of generic pass-through design where you can add more functionality to WebKitPatch without having to change its class structure/attributes each time).
 
@@ -488,13 +490,18 @@ class MockStatusServer(object):
         return 191
 
 
+class MockExecute(Mock):
+    def run_and_throw_if_fail(self, args, quiet=False):
+        return "MOCK output of child process"
+
+
 class MockTool():
 
     def __init__(self, log_executive=False):
         self.wakeup_event = threading.Event()
         self.bugs = MockBugzilla()
         self.buildbot = MockBuildBot()
-        self.executive = Mock()
+        self.executive = MockExecute()
         if log_executive:
             self.executive.run_and_throw_if_fail = lambda args: log("MOCK run_and_throw_if_fail: %s" % args)
         self._irc = None
@@ -503,6 +510,7 @@ class MockTool():
         self._checkout = MockCheckout()
         self.status_server = MockStatusServer()
         self.irc_password = "MOCK irc password"
+        self.codereview = Rietveld(self.executive)

Same remark here as above re: knowing about all its capabilities. 

Also, what's the rationale for using MockExecute() here but Mock() in queues_unittest.py?  It seems like another approach could be to make MockTool the "bare bones" object, and then add more complex values for the data attributes as needed to test particular functionality.  With the queues_unittest change above, it seems like the reverse might be happening, where you are obscuring previously set attributes with simpler variants.  It seems like keeping things simpler by default may result in fewer changes to dependent tests going forward.

+class PostCodeReview(AbstractStep):
+    @classmethod
+    def options(cls):
+        return [
+            Options.cc,
+            Options.description,
+            Options.fancy_review,
+            Options.review,
+        ]
+
+    def run(self, state):
+        if not self._options.fancy_review:
+            return
+        if not self._options.review:
+            return
+        # FIXME: This will always be None because we don't retrieve the issue
+        #        number frmo the ChangeLog yet.
+        codereview_issue = state.get("codereview_issue")

Where is this string coming from -- is there a way to avoid having it hard-coded in more than one spot?

[Adam Barth]

All good comments, thanks.

> Thanks for getting the ball rolling on this, Adam!  It's amazing you were able
> to do this with so little code.

We have lots of infrastructure now that makes adding features pretty easy.

> +++ b/WebKitTools/Scripts/webkitpy/common/config/__init__.py

> Until we figure out how we want to use our __init__ files, I would recommend
> leaving the file empty and putting this information in a separate file.

Ok.  Maybe servers.py in the config folder?  I tried that with the irc settings, but its slightly awkward to import:

import webkitpy.common.config.servers as config_servers

???

>  Also,
> I've mentioned this before, but I have mixed feelings about putting settings
> for a range of areas in a single config folder because it increases the number
> of couplings.  I think it might be better, for example, to put Rietveld-related
> settings in a rietveld/ folder.

For Bugzilla, we store this information directly in bugzilla.py, but there's a comment there that Eric added about wanting to move this to a more generic location.  I think Eric has the idea that someone might want to take webkitpy and use it for their own project.  They might want a central place to point all the URLs, etc, to the ones for their project.  I'm not sure what's best here.

> Should we still be using deprecated_logging in new code?

No, but I don't know how to use the real logging.  I need to read up more on it.

> Is the Apache license compatible with the WebKit project?

My understanding is that it is because it's essentially the same as the BSD license.

> Given that WebKitPatch is meant to be a generic all-purpose tool, I wonder if
> there's a way to design things so the class doesn't itself have to be aware of
> all its capabilities a priori (e.g. if there is some sort of generic
> pass-through design where you can add more functionality to WebKitPatch without
> having to change its class structure/attributes each time).

That would be nice.  We do something like that with commands and it works nicely.  We just haven't gotten around to build that for the main tool object.

> Also, what's the rationale for using MockExecute() here but Mock() in
> queues_unittest.py?

We could use MockExecutive in queues_unittest.  I could have just used None in that case (and maybe should have) because the method I'm testing doesn't interact with the executive.

I eventually want to build a nice fancy MockExecutive and have more of the tool-related classes use that as their mock point instead of always mocking at the tool.  Rietveld is a bit of an experiment in that regard.  I just did the minimum necessary to make that work in this patch.

> +        codereview_issue = state.get("codereview_issue")
> 
> Where is this string coming from -- is there a way to avoid having it
> hard-coded in more than one spot?

The state dictionary is a bit of an experiment.  We probably should make it into an object and document it properly.  As the moment, this is a hook for future extension.  I wanted to do try remembering the issue number, but I wasn't sure of the bets design and so pushed it off to the next iteration.

[Adam Barth]

Comment on attachment 53114 [details]
Patch

Chris is right.  Apache is not a legit license.

[Ojan Vafai]

Created attachment 53295 [details]
Patch

[Chris Jerdonek]

(In reply to comment #9)
> Created an attachment (id=53295) [details]
> Patch

I don't see the actual autoinstall code in the patch.

+import webkitpy.thirdparty.autoinstalled.rietveld_upload as rietveld_upload

Could we autoinstall upload.py to this instead?

autoinstalled/rietveld/upload.py

This will preserve the original file name.

[Chris Jerdonek]

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=53295) [details] [details]
> > Patch
> 
> I don't see the actual autoinstall code in the patch.

Oh, I see the code now.  The file is just not mentioned in the ChangeLog.  You can look at the ircbot/irclib autoinstall code for an example of creating an intermediate directory.

[Ojan Vafai]

Created attachment 53300 [details]
Patch

[David Levin]

Comment on attachment 53300 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 5f55f45..1ce3af2 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,31 @@
> [phone]  Ojan Vafai  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add experimental prototype Rietveld integration to webkit-patch upload
> +        https://bugs.webkit.org/show_bug.cgi?id=37418
> +
> +        This patch adds bare-bones integrtion with Rietveld for code reviews.
> +        The behavior is hidden behind the --fancy-review command line flag.
> +        Currently, there's no support for uploading more than one patch per
> +        issue (which is a nice feature of Rietveld).  The plan is to play with
> +        this for a bit and see if it's useful.
> +
> +        Modified from Adam's original patch to autoinstall the rietveld upload script.
> +
> +        * Scripts/webkitpy/common/config/__init__.py:
> +        * Scripts/webkitpy/common/net/rietveld.py: Added.
> +        * Scripts/webkitpy/common/net/rietveld_unitttest.py: Added.
> +        * Scripts/webkitpy/tool/commands/queues_unittest.py:
> +        * Scripts/webkitpy/tool/commands/upload.py:
> +        * Scripts/webkitpy/tool/commands/upload_unittest.py:
> +        * Scripts/webkitpy/tool/main.py:
> +        * Scripts/webkitpy/tool/mocktool.py:
> +        * Scripts/webkitpy/tool/steps/__init__.py:
> +        * Scripts/webkitpy/tool/steps/options.py:
> +        * Scripts/webkitpy/tool/steps/postcodereview.py: Added.
> +        * Scripts/webkitpy/tool/steps/postdiff.py:
> +
>  2010-04-13  Csaba Osztrogonác  <[e-mail]>
>  
>          Unreviewed buildfix after r57537.
> diff --git a/WebKitTools/Scripts/webkitpy/common/config/__init__.py b/WebKitTools/Scripts/webkitpy/common/config/__init__.py
> index ef65bee..03f1bc7 100644
> --- a/WebKitTools/Scripts/webkitpy/common/config/__init__.py
> +++ b/WebKitTools/Scripts/webkitpy/common/config/__init__.py
> @@ -1 +1,7 @@
>  # Required for Python to search this directory for module files
> +
> +import re
> +
> +codereview_server_host = "wkrietveld.appspot.com"
> +codereview_server_regex = "https?://%s/" % re.sub('\.', '\\.', codereview_server_host)
> +codereview_server_url = "https://%s/" % codereview_server_host
> diff --git a/WebKitTools/Scripts/webkitpy/common/net/rietveld.py b/WebKitTools/Scripts/webkitpy/common/net/rietveld.py
> new file mode 100644
> index 0000000..96ceb2b
> --- /dev/null
> +++ b/WebKitTools/Scripts/webkitpy/common/net/rietveld.py
> @@ -0,0 +1,89 @@
> +# Copyright (c) 2010 Google Inc. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +#     * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#     * Redistributions in binary form must reproduce the above
> +# copyright notice, this list of conditions and the following disclaimer
> +# in the documentation and/or other materials provided with the
> +# distribution.
> +#     * Neither the name of Google Inc. nor the names of its
> +# contributors may be used to endorse or promote products derived from
> +# this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +import os
> +import re
> +import stat
> +
> +import webkitpy.common.config as config
> +from webkitpy.common.system.deprecated_logging import log
> +from webkitpy.common.system.executive import ScriptError
> +import webkitpy.thirdparty.autoinstalled.rietveld.upload as upload
> +
> +
> +def parse_codereview_issue(message):
> +    if not message:
> +        return None
> +    match = re.search(config.codereview_server_regex +
> +                      "(?P<codereview_issue>\d+)",
> +                      message)
> +    if match:
> +        return int(match.group('codereview_issue'))
> +
> +
> +class Rietveld(object):
> +    def __init__(self, executive, dryrun=False):
> +        self.dryrun = dryrun
> +        self._executive = executive
> +        self._upload_py = upload.__file__
> +        # Chop of the last character so we modify permissions on the py file instead of the pyc.
s/Chop of/Chop off/

> +        if os.path.splitext(self._upload_py)[1] == ".pyc":
> +            self._upload_py = self._upload_py[:-1]
> +        os.chmod(self._upload_py, os.stat(self._upload_py).st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
> +
> +    def url_for_issue(self, codereview_issue):
> +        if not codereview_issue:
> +            return None
> +        return "%s%s" % (config.codereview_server_url, codereview_issue)
> +
> +    def post(self, message=None, codereview_issue=None, cc=None):
> +        if not message:
> +            raise ScriptError("Rietveld requires a message.")
> +
> +        args = [
> +            self._upload_py,
> +            "--assume_yes",
> +            "--server=%s" % config.codereview_server_host,
> +            "--message=%s" % message,
> +        ]
> +        if codereview_issue:
> +            args.append("--issue=%s" % codereview_issue)
> +        if cc:
> +            args.append("--cc=%s" % cc)
> +
> +        if self.dryrun:
> +            log("Would have run %s" % args)
> +            return
> +
> +        output = self._executive.run_and_throw_if_fail(args)
> +        match = re.search("Issue created\. URL: " +
> +                          config.codereview_server_regex +
> +                          "(?P<codereview_issue>\d+)",
> +                          output)
> +        if match:
> +            return int(match.group('codereview_issue'))
> diff --git a/WebKitTools/Scripts/webkitpy/common/net/rietveld_unittest.py b/WebKitTools/Scripts/webkitpy/common/net/rietveld_unittest.py
> new file mode 100644
> index 0000000..da5e0f5
> --- /dev/null
> +++ b/WebKitTools/Scripts/webkitpy/common/net/rietveld_unittest.py
> @@ -0,0 +1,39 @@
> +# Copyright (c) 2010 Google Inc. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +#     * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#     * Redistributions in binary form must reproduce the above
> +# copyright notice, this list of conditions and the following disclaimer
> +# in the documentation and/or other materials provided with the
> +# distribution.
> +#     * Neither the name of Google Inc. nor the names of its
> +# contributors may be used to endorse or promote products derived from
> +# this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +import unittest
> +
> +from webkitpy.common.net.rietveld import Rietveld
> +from webkitpy.thirdparty.mock import Mock
> +
> +
> +class RietveldTest(unittest.TestCase):
> +    def test_url_for_issue(self):
> +        rietveld = Rietveld(Mock())
> +        self.assertEqual(rietveld.url_for_issue(34223),
> +                         "https://codereview.appspot.com/34223")
> diff --git a/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py b/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py
> index 312da53..f1e5334 100644
> --- a/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py
> +++ b/WebKitTools/Scripts/webkitpy/thirdparty/__init__.py
> @@ -69,6 +69,13 @@ installer.install(url="http://pypi.python.org/packages/source/m/mechanize/mechan
>  installer.install(url="http://pypi.python.org/packages/source/p/pep8/pep8-0.5.0.tar.gz#md5=512a818af9979290cd619cce8e9c2e2b",
>                    url_subpath="pep8-0.5.0/pep8.py")
>  
> +
> +rietveld_dir = os.path.join(autoinstalled_dir, "rietveld")
> +installer = AutoInstaller(target_dir=rietveld_dir)
> +installer.install(url="http://webkit-rietveld.googlecode.com/svn/trunk/static/upload.py",
> +                  target_name="upload.py")
> +
> +
>  # Since irclib and ircbot are two top-level packages, we need to import
>  # them separately.  We group them into an irc package for better
>  # organization purposes.
> diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
> index 88a37f3..0906c70 100644
> --- a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
> +++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
> @@ -71,6 +71,7 @@ class AbstractQueueTest(CommandsTest):
>      def _assert_run_webkit_patch(self, run_args):
>          queue = TestQueue()
>          tool = MockTool()
> +        tool.executive = Mock()
>          queue.bind_to_tool(tool)
>  
>          queue.run_webkit_patch(run_args)
> diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
> index 6e68fbe..dd0ac13 100644
> --- a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
> +++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
> @@ -164,6 +164,7 @@ class Post(AbstractPatchUploadingCommand):
>      steps = [
>          steps.CheckStyle,
>          steps.ConfirmDiff,
> +        steps.PostCodeReview,
>          steps.ObsoletePatches,
>          steps.PostDiff,
>      ]
> @@ -208,6 +209,7 @@ class Upload(AbstractPatchUploadingCommand):
>          steps.PrepareChangeLog,
>          steps.EditChangeLog,
>          steps.ConfirmDiff,
> +        steps.PostCodeReview,
>          steps.ObsoletePatches,
>          steps.PostDiff,
>      ]
> diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
> index 4e987c9..9766e19 100644
> --- a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
> +++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
> @@ -58,6 +58,7 @@ class UploadCommandsTest(CommandsTest):
>          options.description = "MOCK description"
>          options.request_commit = False
>          options.review = True
> +        options.cc = None
>          expected_stderr = "Running check-webkit-style\nObsoleting 2 old patches on bug 42\nMOCK add_patch_to_bug: bug_id=42, description=MOCK description, mark_for_review=True, mark_for_commit_queue=False, mark_for_landing=False\n-- Begin comment --\nNone\n-- End comment --\nMOCK: user.open_url: http://example.com/42\n"
>          self.assert_execute_outputs(Post(), [42], options=options, expected_stderr=expected_stderr)
>  
> @@ -77,6 +78,7 @@ class UploadCommandsTest(CommandsTest):
>          options.description = "MOCK description"
>          options.request_commit = False
>          options.review = True
> +        options.cc = None
>          expected_stderr = "Running check-webkit-style\nObsoleting 2 old patches on bug 42\nMOCK add_patch_to_bug: bug_id=42, description=MOCK description, mark_for_review=True, mark_for_commit_queue=False, mark_for_landing=False\n-- Begin comment --\nNone\n-- End comment --\nMOCK: user.open_url: http://example.com/42\n"
>          self.assert_execute_outputs(Upload(), [42], options=options, expected_stderr=expected_stderr)
>  
> diff --git a/WebKitTools/Scripts/webkitpy/tool/main.py b/WebKitTools/Scripts/webkitpy/tool/main.py
> index 629e86f..dd7652d 100755
> --- a/WebKitTools/Scripts/webkitpy/tool/main.py
> +++ b/WebKitTools/Scripts/webkitpy/tool/main.py
> @@ -36,6 +36,7 @@ from webkitpy.common.checkout.api import Checkout
>  from webkitpy.common.checkout.scm import detect_scm_system
>  from webkitpy.common.net.bugzilla import Bugzilla
>  from webkitpy.common.net.buildbot import BuildBot
> +from webkitpy.common.net.rietveld import Rietveld
>  from webkitpy.common.net.irc.ircproxy import IRCProxy
>  from webkitpy.common.system.executive import Executive
>  from webkitpy.common.system.user import User
> @@ -70,6 +71,7 @@ class WebKitPatch(MultiCommandTool):
>          self._scm = None
>          self._checkout = None
>          self.status_server = StatusServer()
> +        self.codereview = Rietveld(self.executive)
>  
>      def scm(self):
>          # Lazily initialize SCM to not error-out before command line parsing (or when running non-scm commands).
> @@ -121,6 +123,7 @@ class WebKitPatch(MultiCommandTool):
>          if options.dry_run:
>              self.scm().dryrun = True
>              self.bugs.dryrun = True
> +            self.codereview.dryrun = True
>          if options.status_host:
>              self.status_server.set_host(options.status_host)
>          if options.irc_password:
> diff --git a/WebKitTools/Scripts/webkitpy/tool/mocktool.py b/WebKitTools/Scripts/webkitpy/tool/mocktool.py
> index 89fde6a..6e4b64f 100644
> --- a/WebKitTools/Scripts/webkitpy/tool/mocktool.py
> +++ b/WebKitTools/Scripts/webkitpy/tool/mocktool.py
> @@ -33,6 +33,7 @@ from webkitpy.common.config.committers import CommitterList, Reviewer
>  from webkitpy.common.checkout.commitinfo import CommitInfo
>  from webkitpy.common.checkout.scm import CommitMessage
>  from webkitpy.common.net.bugzilla import Bug, Attachment
> +from webkitpy.common.net.rietveld import Rietveld
>  from webkitpy.thirdparty.mock import Mock
>  from webkitpy.common.system.deprecated_logging import log
>  
> @@ -43,6 +44,7 @@ def _id_to_object_dictionary(*objects):
>          dictionary[thing["id"]] = thing
>      return dictionary
>  
> +# Testing
>  
>  # FIXME: The ids should be 1, 2, 3 instead of crazy numbers.
>  
> @@ -488,13 +490,18 @@ class MockStatusServer(object):
>          return 191
>  
>  
> +class MockExecute(Mock):
> +    def run_and_throw_if_fail(self, args, quiet=False):
> +        return "MOCK output of child process"
> +
> +
>  class MockTool():
>  
>      def __init__(self, log_executive=False):
>          self.wakeup_event = threading.Event()
>          self.bugs = MockBugzilla()
>          self.buildbot = MockBuildBot()
> -        self.executive = Mock()
> +        self.executive = MockExecute()
>          if log_executive:
>              self.executive.run_and_throw_if_fail = lambda args: log("MOCK run_and_throw_if_fail: %s" % args)
>          self._irc = None
> @@ -503,6 +510,7 @@ class MockTool():
>          self._checkout = MockCheckout()
>          self.status_server = MockStatusServer()
>          self.irc_password = "MOCK irc password"
> +        self.codereview = Rietveld(self.executive)
>  
>      def scm(self):
>          return self._scm
> diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py b/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py
> index 8e8ab15..d59cdc5 100644
> --- a/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py
> +++ b/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py
> @@ -44,6 +44,7 @@ from webkitpy.tool.steps.ensurebuildersaregreen import EnsureBuildersAreGreen
>  from webkitpy.tool.steps.ensurelocalcommitifneeded import EnsureLocalCommitIfNeeded
>  from webkitpy.tool.steps.obsoletepatches import ObsoletePatches
>  from webkitpy.tool.steps.options import Options
> +from webkitpy.tool.steps.postcodereview import PostCodeReview
>  from webkitpy.tool.steps.postdiff import PostDiff
>  from webkitpy.tool.steps.postdiffforcommit import PostDiffForCommit
>  from webkitpy.tool.steps.postdiffforrevert import PostDiffForRevert
> diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/options.py b/WebKitTools/Scripts/webkitpy/tool/steps/options.py
> index 40a6680..7f76f55 100644
> --- a/WebKitTools/Scripts/webkitpy/tool/steps/options.py
> +++ b/WebKitTools/Scripts/webkitpy/tool/steps/options.py
> @@ -40,6 +40,7 @@ class Options(object):
>      confirm = make_option("--no-confirm", action="store_false", dest="confirm", default=True, help="Skip confirmation steps.")
>      description = make_option("-m", "--description", action="store", type="string", dest="description", help="Description string for the attachment (default: \"patch\")")
>      email = make_option("--email", action="store", type="string", dest="email", help="Email address to use in ChangeLogs.")
> +    fancy_review = make_option("--fancy-review", action="store_true", dest="fancy_review", default=False, help="(Experimental) Upload the patch to Rietveld code review tool.")
>      force_clean = make_option("--force-clean", action="store_true", dest="force_clean", default=False, help="Clean working directory before applying patches (removes local changes and commits)")
>      local_commit = make_option("--local-commit", action="store_true", dest="local_commit", default=False, help="Make a local commit for each applied patch")
>      non_interactive = make_option("--non-interactive", action="store_true", dest="non_interactive", default=False, help="Never prompt the user, fail as fast as possible.")
> diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py b/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py
> new file mode 100644
> index 0000000..670ce6c
> --- /dev/null
> +++ b/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py
> @@ -0,0 +1,76 @@
> +# Copyright (C) 2010 Google Inc. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +#     * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#     * Redistributions in binary form must reproduce the above
> +# copyright notice, this list of conditions and the following disclaimer
> +# in the documentation and/or other materials provided with the
> +# distribution.
> +#     * Neither the name of Google Inc. nor the names of its
> +# contributors may be used to endorse or promote products derived from
> +# this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +from webkitpy.tool.steps.abstractstep import AbstractStep
> +from webkitpy.tool.steps.options import Options
> +
> +
> +class PostCodeReview(AbstractStep):
> +    @classmethod
> +    def options(cls):
> +        return [
> +            Options.cc,
> +            Options.description,
> +            Options.fancy_review,
> +            Options.review,
> +        ]
> +
> +    def run(self, state):
> +        if not self._options.fancy_review:
> +            return
> +        if not self._options.review:
> +            return
> +        # FIXME: This will always be None because we don't retrieve the issue
> +        #        number frmo the ChangeLog yet.
typo: frmo

> +        codereview_issue = state.get("codereview_issue")
> +        message = self._options.description
> +        if not message:
> +            # If we have an issue number, then the message because the label

s/because/becomes/

> +            # of the new patch. Otherwise, it becomes the title of the whole
> +            # issue.
> +            if codereview_issue:
> +                message = "Updated patch"
> +            elif state.get("bug_title"):
> +                # This is the common case for the the first "upload" command.
> +                message = state.get("bug_title")
> +            elif state.get("bug_id"):
> +                # This is the common case for the "post" command and
> +                # subsequent runs of the "upload" command.  In this case,
> +                # I'd rather add the new patch to the existing issue, but
> +                # that's not implemented yet.
> +                message = "Code review for %s" % self._tool.bugs.bug_url_for_bug_id(state["bug_id"])
> +            else:
> +                # Unreachable with our current commands, but we might hit
> +                # this case if we support bug-less code reviews.
> +                message = "Code review"
> +        created_issue = self._tool.codereview.post(message=message,
> +                                                   codereview_issue=codereview_issue,
> +                                                   cc=self._options.cc)
> +        if created_issue:
> +            # FIXME: Record the issue number in the ChangeLog.
> +            state["codereview_issue"] = created_issue
>

[Ojan Vafai]

Committed r57552: <http://trac.webkit.org/changeset/57552>

[Eric Seidel (no email)]

======================================================================
FAIL: test_url_for_issue (webkitpy.common.net.rietveld_unittest.RietveldTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Projects/WebKit/WebKitTools/Scripts/webkitpy/common/net/rietveld_unittest.py", line 39, in test_url_for_issue
    "https://codereview.appspot.com/34223")
AssertionError: 'https://wkrietveld.appspot.com/34223' != 'https://codereview.appspot.com/34223'

----------------------------------------------------------------------
Ran 369 tests in 2.627s

Looks like a new failure from this change.

[Eric Seidel (no email)]

Created attachment 53309 [details]
Patch