Merge webkit.org at r61121: Initial merge by git.

Change-Id: Icd6db395c62285be384d137164d95d7466c98760

20 files changed, 335 insertions, 96 deletions

diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/irc_command.py b/WebKitTools/Scripts/webkitpy/tool/bot/irc_command.py
index c21fdc6..ee8c669 100644
--- a/WebKitTools/Scripts/webkitpy/tool/bot/irc_command.py
+++ b/WebKitTools/Scripts/webkitpy/tool/bot/irc_command.py
@@ -75,8 +75,35 @@ class Rollout(IRCCommand):
                     tool.bugs.bug_url_for_bug_id(bug_id))
 
 
+class Help(IRCCommand):
+    def execute(self, nick, args, tool, sheriff):
+        return "%s: Available commands: %s" % (nick, ", ".join(commands.keys()))
+
+
 class Hi(IRCCommand):
     def execute(self, nick, args, tool, sheriff):
         quips = tool.bugs.quips()
         quips.append('"Only you can prevent forest fires." -- Smokey the Bear')
         return random.choice(quips)
+
+
+class Eliza(IRCCommand):
+    therapist = None
+
+    def __init__(self):
+        if not self.therapist:
+            import webkitpy.thirdparty.autoinstalled.eliza as eliza
+            Eliza.therapist = eliza.eliza()
+
+    def execute(self, nick, args, tool, sheriff):
+        return "%s: %s" % (nick, self.therapist.respond(" ".join(args)))
+
+
+# FIXME: Lame.  We should have an auto-registering CommandCenter.
+commands = {
+    "last-green-revision": LastGreenRevision,
+    "restart": Restart,
+    "rollout": Rollout,
+    "help": Help,
+    "hi": Hi,
+}
diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/irc_command_unittest.py b/WebKitTools/Scripts/webkitpy/tool/bot/irc_command_unittest.py
new file mode 100644
index 0000000..7aeb6a0
--- /dev/null
+++ b/WebKitTools/Scripts/webkitpy/tool/bot/irc_command_unittest.py
@@ -0,0 +1,38 @@
+# 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.tool.bot.irc_command import *
+
+
+class IRCCommandTest(unittest.TestCase):
+    def test_eliza(self):
+        eliza = Eliza()
+        eliza.execute("tom", "hi", None, None)
+        eliza.execute("tom", "bye", None, None)
diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py b/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py
index ac7a760..a1a66a1 100644
--- a/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py
+++ b/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py
@@ -113,7 +113,7 @@ class QueueEngine:
                     # handled in the child process and we should just keep looping.
                     if e.exit_code == self.handled_error_code:
                         continue
-                    message = "Unexpected failure when landing patch!  Please file a bug against webkit-patch.\n%s" % e.message_with_output()
+                    message = "Unexpected failure when processing patch!  Please file a bug against webkit-patch.\n%s" % e.message_with_output()
                     self._delegate.handle_unexpected_error(work_item, message)
             except TerminateQueue, e:
                 log("\nTerminateQueue exception received.")
diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/sheriffircbot.py b/WebKitTools/Scripts/webkitpy/tool/bot/sheriffircbot.py
index 43aa9c3..de77222 100644
--- a/WebKitTools/Scripts/webkitpy/tool/bot/sheriffircbot.py
+++ b/WebKitTools/Scripts/webkitpy/tool/bot/sheriffircbot.py
@@ -52,14 +52,6 @@ class _IRCThreadTearoff(IRCBotDelegate):
 
 
 class SheriffIRCBot(object):
-    # FIXME: Lame.  We should have an auto-registering CommandCenter.
-    commands = {
-        "last-green-revision": irc_command.LastGreenRevision,
-        "restart": irc_command.Restart,
-        "rollout": irc_command.Rollout,
-        "hi": irc_command.Hi,
-    }
-
     def __init__(self, tool, sheriff):
         self._tool = tool
         self._sheriff = sheriff
@@ -75,15 +67,13 @@ class SheriffIRCBot(object):
         tokenized_request = request.strip().split(" ")
         if not tokenized_request:
             return
-        command = self.commands.get(tokenized_request[0])
+        command = irc_command.commands.get(tokenized_request[0])
+        args = tokenized_request[1:]
         if not command:
-            self._tool.irc().post("%s: Available commands: %s" % (
-                                  nick, ", ".join(self.commands.keys())))
-            return
-        response = command().execute(nick,
-                                     tokenized_request[1:],
-                                     self._tool,
-                                     self._sheriff)
+            # Give the peoples someone to talk with.
+            command = irc_command.Eliza
+            args = tokenized_request
+        response = command().execute(nick, args, self._tool, self._sheriff)
         if response:
             self._tool.irc().post(response)
 
diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py b/WebKitTools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py
index d5116e4..21bff12 100644
--- a/WebKitTools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py
@@ -50,9 +50,9 @@ class SheriffIRCBotTest(unittest.TestCase):
         expected_stderr = 'MOCK: irc.post: "Only you can prevent forest fires." -- Smokey the Bear\n'
         OutputCapture().assert_outputs(self, run, args=["hi"], expected_stderr=expected_stderr)
 
-    def test_bogus(self):
-        expected_stderr = "MOCK: irc.post: mock_nick: Available commands: rollout, hi, restart, last-green-revision\n"
-        OutputCapture().assert_outputs(self, run, args=["bogus"], expected_stderr=expected_stderr)
+    def test_help(self):
+        expected_stderr = "MOCK: irc.post: mock_nick: Available commands: rollout, hi, help, restart, last-green-revision\n"
+        OutputCapture().assert_outputs(self, run, args=["help"], expected_stderr=expected_stderr)
 
     def test_lgr(self):
         expected_stderr = "MOCK: irc.post: mock_nick: http://trac.webkit.org/changeset/9479\n"
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download.py b/WebKitTools/Scripts/webkitpy/tool/commands/download.py
index a283da9..a85b09a 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/download.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/download.py
@@ -182,6 +182,18 @@ class BuildAttachment(AbstractPatchSequencingCommand, ProcessAttachmentsMixin):
     ]
 
 
+class PostAttachmentToRietveld(AbstractPatchSequencingCommand, ProcessAttachmentsMixin):
+    name = "post-attachment-to-rietveld"
+    help_text = "Uploads a bugzilla attachment to rietveld"
+    arguments_names = "ATTACHMENTID"
+    main_steps = [
+        steps.CleanWorkingDirectory,
+        steps.Update,
+        steps.ApplyPatch,
+        steps.PostCodeReview,
+    ]
+
+
 class AbstractPatchApplyingCommand(AbstractPatchSequencingCommand):
     prepare_steps = [
         steps.EnsureLocalCommitIfNeeded,
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
index 08a4377..958620a 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
@@ -108,6 +108,10 @@ class DownloadCommandsTest(CommandsTest):
         expected_stderr = "Processing 1 patch from 1 bug.\nUpdating working directory\nProcessing patch 197 from bug 42.\nBuilding WebKit\n"
         self.assert_execute_outputs(BuildAttachment(), [197], options=self._default_options(), expected_stderr=expected_stderr)
 
+    def test_post_attachment_to_rietveld(self):
+        expected_stderr = "Processing 1 patch from 1 bug.\nUpdating working directory\nProcessing patch 197 from bug 42.\nMOCK: Uploading patch to rietveld\nMOCK setting flag 'in-rietveld' to '+' on attachment '197' with comment 'None' and additional comment 'None'\n"
+        self.assert_execute_outputs(PostAttachmentToRietveld(), [197], options=self._default_options(), expected_stderr=expected_stderr)
+
     def test_land_attachment(self):
         # FIXME: This expected result is imperfect, notice how it's seeing the same patch as still there after it thought it would have cleared the flags.
         expected_stderr = """Processing 1 patch from 1 bug.
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
index 27e09ba..67393d8 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
@@ -43,17 +43,23 @@ class EarlyWarningSytemTest(QueuesTest):
         string_replacemnts = {
             "name": ews.name,
             "checkout_dir": os.getcwd(),  # FIXME: Use of os.getcwd() is wrong, should be scm.checkout_root
+            "port": ews.port_name,
+            "watchers": ews.watchers,
         }
         expected_stderr = {
             "begin_work_queue": "CAUTION: %(name)s will discard all local changes in \"%(checkout_dir)s\"\nRunning WebKit %(name)s.\n" % string_replacemnts,
             "handle_unexpected_error": "Mock error message\n",
             "next_work_item": "MOCK: update_work_items: %(name)s [103]\n" % string_replacemnts,
             "process_work_item": "MOCK: update_status: %(name)s Pass\n" % string_replacemnts,
+            "handle_script_error": "MOCK: update_status: %(name)s ScriptError error message\nMOCK bug comment: bug_id=345, cc=%(watchers)s\n--- Begin comment ---\\Attachment 1234 did not build on %(port)s:\nBuild output: http://dummy_url\n--- End comment ---\n\n" % string_replacemnts,
         }
         return expected_stderr
 
     def _test_ews(self, ews):
-        self.assert_queue_outputs(ews, expected_stderr=self._default_expected_stderr(ews))
+        expected_exceptions = {
+            "handle_script_error": SystemExit,
+        }
+        self.assert_queue_outputs(ews, expected_stderr=self._default_expected_stderr(ews), expected_exceptions=expected_exceptions)
 
     # FIXME: If all EWSes are going to output the same text, we
     # could test them all in one method with a for loop over an array.
@@ -73,4 +79,7 @@ class EarlyWarningSytemTest(QueuesTest):
         ews = MacEWS()
         expected_stderr = self._default_expected_stderr(ews)
         expected_stderr["process_work_item"] = "MOCK: update_status: mac-ews Error: mac-ews cannot process patches from non-committers :(\n"
-        self.assert_queue_outputs(ews, expected_stderr=expected_stderr)
+        expected_exceptions = {
+            "handle_script_error": SystemExit,
+        }
+        self.assert_queue_outputs(ews, expected_stderr=expected_stderr, expected_exceptions=expected_exceptions)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
index 08bd3aa..d14ac9e 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
@@ -121,7 +121,7 @@ class AbstractQueue(Command, QueueEngineDelegate):
 
     @classmethod
     def _update_status_for_script_error(cls, tool, state, script_error, is_error=False):
-        message = script_error.message
+        message = str(script_error)
         if is_error:
             message = "Error: %s" % message
         output = script_error.message_with_output(output_limit=1024*1024) # 1MB
@@ -289,7 +289,6 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
         self.committer_validator.reject_patch_from_commit_queue(patch.id(), message)
 
     # StepSequenceErrorHandler methods
-
     @staticmethod
     def _error_message_for_bug(tool, status_id, script_error):
         if not script_error.output:
@@ -304,6 +303,51 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
         validator.reject_patch_from_commit_queue(state["patch"].id(), cls._error_message_for_bug(tool, status_id, script_error))
 
 
+class RietveldUploadQueue(AbstractPatchQueue, StepSequenceErrorHandler):
+    name = "rietveld-upload-queue"
+
+    def __init__(self):
+        AbstractPatchQueue.__init__(self)
+
+    # AbstractPatchQueue methods
+
+    def next_work_item(self):
+        patch_id = self.tool.bugs.queries.fetch_first_patch_from_rietveld_queue()
+        if patch_id:
+            return patch_id
+        self._update_status("Empty queue")
+
+    def should_proceed_with_work_item(self, patch):
+        self._update_status("Uploading patch", patch)
+        return True
+
+    def process_work_item(self, patch):
+        try:
+            self.run_webkit_patch(["post-attachment-to-rietveld", "--force-clean", "--non-interactive", "--parent-command=rietveld-upload-queue", patch.id()])
+            self._did_pass(patch)
+            return True
+        except ScriptError, e:
+            if e.exit_code != QueueEngine.handled_error_code:
+                self._did_fail(patch)
+            raise e
+
+    @classmethod
+    def _reject_patch(cls, tool, patch_id):
+        tool.bugs.set_flag_on_attachment(patch_id, "in-rietveld", "-")
+
+    def handle_unexpected_error(self, patch, message):
+        log(message)
+        self._reject_patch(self.tool, patch.id())
+
+    # StepSequenceErrorHandler methods
+
+    @classmethod
+    def handle_script_error(cls, tool, state, script_error):
+        log(script_error.message_with_output())
+        cls._update_status_for_script_error(tool, state, script_error)
+        cls._reject_patch(tool, state["patch"].id())
+
+
 class AbstractReviewQueue(AbstractPatchQueue, PersistentPatchCollectionDelegate, StepSequenceErrorHandler):
     def __init__(self, options=None):
         AbstractPatchQueue.__init__(self, options)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
index a5d56da..b32dfa8 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
@@ -122,10 +122,13 @@ class CommitQueueTest(QueuesTest):
             # FIXME: The commit-queue warns about bad committers twice.  This is due to the fact that we access Attachment.reviewer() twice and it logs each time.
             "next_work_item" : """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
 Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
+MOCK setting flag 'commit-queue' to '-' on attachment '128' with comment 'Rejecting patch 128 from commit-queue.' and additional comment 'non-committer@example.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.\n\n- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.\n\n- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.'
 MOCK: update_work_items: commit-queue [106, 197]
 2 patches in commit-queue [106, 197]
 """,
             "process_work_item" : "MOCK: update_status: commit-queue Pass\n",
+            "handle_unexpected_error" : "MOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'Mock error message'\n",
+            "handle_script_error": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
         }
         self.assert_queue_outputs(CommitQueue(), expected_stderr=expected_stderr)
 
@@ -138,11 +141,14 @@ MOCK: update_work_items: commit-queue [106, 197]
             # FIXME: The commit-queue warns about bad committers twice.  This is due to the fact that we access Attachment.reviewer() twice and it logs each time.
             "next_work_item" : """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
 Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
+MOCK setting flag \'commit-queue\' to \'-\' on attachment \'128\' with comment \'Rejecting patch 128 from commit-queue.\' and additional comment \'non-committer@example.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.\n\n- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.\n\n- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.\'
 MOCK: update_work_items: commit-queue [106, 197]
 MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://build.webkit.org
 1 patch in commit-queue [106]
 """,
             "process_work_item" : "MOCK: update_status: commit-queue Builders [\"Builder2\"] are red. See http://build.webkit.org\n",
+            "handle_unexpected_error" : "MOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'Mock error message'\n",
+            "handle_script_error": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, expected_stderr=expected_stderr)
 
@@ -156,11 +162,14 @@ MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://buil
             # FIXME: The commit-queue warns about bad committers twice.  This is due to the fact that we access Attachment.reviewer() twice and it logs each time.
             "next_work_item": """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
 Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
+MOCK setting flag \'commit-queue\' to \'-\' on attachment \'128\' with comment \'Rejecting patch 128 from commit-queue.\' and additional comment \'non-committer@example.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.\n\n- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.\n\n- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.\'
 MOCK: update_work_items: commit-queue [106, 197]
 MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://build.webkit.org
 1 patch in commit-queue [106]
 """,
             "process_work_item": "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 76543]\nMOCK: update_status: commit-queue Pass\n",
+            "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '76543' with comment 'Rejecting patch 76543 from commit-queue.' and additional comment 'Mock error message'\n",
+            "handle_script_error": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=rollout_patch, expected_stderr=expected_stderr)
 
@@ -193,6 +202,18 @@ MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://buil
         self.assertEqual(attachments, expected_sort)
 
 
+class RietveldUploadQueueTest(QueuesTest):
+    def test_rietveld_upload_queue(self):
+        expected_stderr = {
+            "begin_work_queue": "CAUTION: rietveld-upload-queue will discard all local changes in \"%s\"\nRunning WebKit rietveld-upload-queue.\n" % MockSCM.fake_checkout_root,
+            "should_proceed_with_work_item": "MOCK: update_status: rietveld-upload-queue Uploading patch\n",
+            "process_work_item": "MOCK: update_status: rietveld-upload-queue Pass\n",
+            "handle_unexpected_error": "Mock error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '1234' with comment 'None' and additional comment 'None'\n",
+            "handle_script_error": "ScriptError error message\nMOCK: update_status: rietveld-upload-queue ScriptError error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '1234' with comment 'None' and additional comment 'None'\n",
+        }
+        self.assert_queue_outputs(RietveldUploadQueue(), expected_stderr=expected_stderr)
+
+
 class StyleQueueTest(QueuesTest):
     def test_style_queue(self):
         expected_stderr = {
@@ -201,5 +222,9 @@ class StyleQueueTest(QueuesTest):
             "should_proceed_with_work_item": "MOCK: update_status: style-queue Checking style\n",
             "process_work_item" : "MOCK: update_status: style-queue Pass\n",
             "handle_unexpected_error" : "Mock error message\n",
+            "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=345, cc=[]\n--- Begin comment ---\\Attachment 1234 did not pass style-queue:\n\nScriptError error message\n\nIf any of these errors are false positives, please file a bug against check-webkit-style.\n--- End comment ---\n\n",
+        }
+        expected_exceptions = {
+            "handle_script_error": SystemExit,
         }
-        self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr)
+        self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr, expected_exceptions=expected_exceptions)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
index bf7e32a..9e17c5c 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
@@ -30,6 +30,7 @@ import unittest
 
 from webkitpy.common.net.bugzilla import Attachment
 from webkitpy.common.system.outputcapture import OutputCapture
+from webkitpy.common.system.executive import ScriptError
 from webkitpy.thirdparty.mock import Mock
 from webkitpy.tool.mocktool import MockTool
 
@@ -42,6 +43,14 @@ class MockQueueEngine(object):
         pass
 
 
+class MockPatch():
+    def id(self):
+        return 1234
+
+    def bug_id(self):
+        return 345
+
+
 class QueuesTest(unittest.TestCase):
     mock_work_item = Attachment({
         "id": 1234,
@@ -50,7 +59,19 @@ class QueuesTest(unittest.TestCase):
         "attacher_email": "adam@example.com",
     }, None)
 
-    def assert_queue_outputs(self, queue, args=None, work_item=None, expected_stdout=None, expected_stderr=None, options=Mock(), tool=MockTool()):
+    def assert_outputs(self, func, func_name, args, expected_stdout, expected_stderr, expected_exceptions):
+        exception = None
+        if expected_exceptions and func_name in expected_exceptions:
+            exception = expected_exceptions[func_name]
+
+        OutputCapture().assert_outputs(self,
+                func,
+                args=args,
+                expected_stdout=expected_stdout.get(func_name, ""),
+                expected_stderr=expected_stderr.get(func_name, ""),
+                expected_exception=exception)
+
+    def assert_queue_outputs(self, queue, args=None, work_item=None, expected_stdout=None, expected_stderr=None, expected_exceptions=None, options=Mock(), tool=MockTool()):
         if not expected_stdout:
             expected_stdout = {}
         if not expected_stderr:
@@ -63,38 +84,12 @@ class QueuesTest(unittest.TestCase):
 
         queue.execute(options, args, tool, engine=MockQueueEngine)
 
-        OutputCapture().assert_outputs(self,
-                queue.queue_log_path,
-                expected_stdout=expected_stdout.get("queue_log_path", ""),
-                expected_stderr=expected_stderr.get("queue_log_path", ""))
-        OutputCapture().assert_outputs(self,
-                queue.work_item_log_path,
-                args=[work_item],
-                expected_stdout=expected_stdout.get("work_item_log_path", ""),
-                expected_stderr=expected_stderr.get("work_item_log_path", ""))
-        OutputCapture().assert_outputs(self,
-                queue.begin_work_queue,
-                expected_stdout=expected_stdout.get("begin_work_queue", ""),
-                expected_stderr=expected_stderr.get("begin_work_queue", ""))
-        OutputCapture().assert_outputs(self,
-                queue.should_continue_work_queue,
-                expected_stdout=expected_stdout.get("should_continue_work_queue", ""), expected_stderr=expected_stderr.get("should_continue_work_queue", ""))
-        OutputCapture().assert_outputs(self,
-                queue.next_work_item,
-                expected_stdout=expected_stdout.get("next_work_item", ""),
-                expected_stderr=expected_stderr.get("next_work_item", ""))
-        OutputCapture().assert_outputs(self,
-                queue.should_proceed_with_work_item,
-                args=[work_item],
-                expected_stdout=expected_stdout.get("should_proceed_with_work_item", ""),
-                expected_stderr=expected_stderr.get("should_proceed_with_work_item", ""))
-        OutputCapture().assert_outputs(self,
-                queue.process_work_item,
-                args=[work_item],
-                expected_stdout=expected_stdout.get("process_work_item", ""),
-                expected_stderr=expected_stderr.get("process_work_item", ""))
-        OutputCapture().assert_outputs(self,
-                queue.handle_unexpected_error,
-                args=[work_item, "Mock error message"],
-                expected_stdout=expected_stdout.get("handle_unexpected_error", ""),
-                expected_stderr=expected_stderr.get("handle_unexpected_error", ""))
+        self.assert_outputs(queue.queue_log_path, "queue_log_path", [], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.work_item_log_path, "work_item_log_path", [work_item], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.begin_work_queue, "begin_work_queue", [], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.should_continue_work_queue, "should_continue_work_queue", [], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.next_work_item, "next_work_item", [], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.should_proceed_with_work_item, "should_proceed_with_work_item", [work_item], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.process_work_item, "process_work_item", [work_item], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.handle_unexpected_error, "handle_unexpected_error", [work_item, "Mock error message"], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.handle_script_error, "handle_script_error", [tool, {"patch": MockPatch()}, ScriptError(message="ScriptError error message", script_args="MockErrorCommand")], expected_stdout, expected_stderr, expected_exceptions)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
index cf715b9..e682ca7 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
@@ -171,7 +171,6 @@ class Post(AbstractPatchUploadingCommand):
     steps = [
         steps.CheckStyle,
         steps.ConfirmDiff,
-        steps.PostCodeReview,
         steps.ObsoletePatches,
         steps.PostDiff,
     ]
@@ -215,7 +214,6 @@ 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 d52775b..8fef54a 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
@@ -56,8 +56,6 @@ class UploadCommandsTest(CommandsTest):
         options.request_commit = False
         options.review = True
         options.comment = None
-        # Rietveld upload code requires a real SCM checkout.
-        options.fancy_review = False
         options.cc = None
         expected_stderr = """Running check-webkit-style
 MOCK: user.open_url: file://...
@@ -87,8 +85,6 @@ MOCK: user.open_url: http://example.com/42
         options.request_commit = False
         options.review = True
         options.comment = None
-        # Rietveld upload code requires a real SCM checkout.
-        options.fancy_review = False
         options.cc = None
         expected_stderr = """Running check-webkit-style
 MOCK: user.open_url: file://...
diff --git a/WebKitTools/Scripts/webkitpy/tool/mocktool.py b/WebKitTools/Scripts/webkitpy/tool/mocktool.py
index 3934ea3..d88190f 100644
--- a/WebKitTools/Scripts/webkitpy/tool/mocktool.py
+++ b/WebKitTools/Scripts/webkitpy/tool/mocktool.py
@@ -86,6 +86,7 @@ _patch3 = {
     "name": "Patch3",
     "is_obsolete": False,
     "is_patch": True,
+    "in-rietveld": "?",
     "review": "?",
     "attacher_email": "eric@webkit.org",
 }
@@ -112,6 +113,7 @@ _patch5 = {
     "name": "Patch5",
     "is_obsolete": False,
     "is_patch": True,
+    "in-rietveld": "?",
     "review": "+",
     "reviewer_email": "foo@bar.com",
     "attacher_email": "eric@webkit.org",
@@ -125,6 +127,7 @@ _patch6 = { # Valid committer, but no reviewer.
     "name": "ROLLOUT of r3489",
     "is_obsolete": False,
     "is_patch": True,
+    "in-rietveld": "-",
     "commit-queue": "+",
     "committer_email": "foo@bar.com",
     "attacher_email": "eric@webkit.org",
@@ -138,6 +141,7 @@ _patch7 = { # Valid review, patch is marked obsolete.
     "name": "Patch7",
     "is_obsolete": True,
     "is_patch": True,
+    "in-rietveld": "+",
     "review": "+",
     "reviewer_email": "foo@bar.com",
     "attacher_email": "eric@webkit.org",
@@ -221,6 +225,12 @@ class MockBugzillaQueries(Mock):
     def fetch_patches_from_pending_commit_list(self):
         return sum([bug.reviewed_patches() for bug in self._all_bugs()], [])
 
+    def fetch_first_patch_from_rietveld_queue(self):
+        for bug in self._all_bugs():
+            patches = bug.in_rietveld_queue_patches()
+            if len(patches):
+                return patches[0]
+        raise Exception('No patches in the rietveld queue')
 
 # FIXME: Bugzilla is the wrong Mock-point.  Once we have a BugzillaNetwork
 #        class we should mock that instead.
@@ -287,6 +297,15 @@ class MockBugzilla(Mock):
             action_param = "&action=%s" % action
         return "%s/%s%s" % (self.bug_server_url, attachment_id, action_param)
 
+    def set_flag_on_attachment(self,
+                               attachment_id,
+                               flag_name,
+                               flag_value,
+                               comment_text=None,
+                               additional_comment_text=None):
+        log("MOCK setting flag '%s' to '%s' on attachment '%s' with comment '%s' and additional comment '%s'" % (
+            flag_name, flag_value, attachment_id, comment_text, additional_comment_text))
+
     def post_comment_to_bug(self, bug_id, comment_text, cc=None):
         log("MOCK bug comment: bug_id=%s, cc=%s\n--- Begin comment ---\%s\n--- End comment ---\n" % (
             bug_id, cc, comment_text))
@@ -453,6 +472,9 @@ class MockUser(object):
     def confirm(self, message=None):
         return True
 
+    def can_open_url(self):
+        return True
+
     def open_url(self, url):
         if url.startswith("file://"):
             log("MOCK: user.open_url: file://...")
@@ -490,6 +512,8 @@ class MockStatusServer(object):
     def update_svn_revision(self, svn_revision, broken_bot):
         return 191
 
+    def results_url_for_status(self, status_id):
+        return "http://dummy_url"
 
 class MockExecute(Mock):
     def __init__(self, should_log):
@@ -513,6 +537,15 @@ class MockExecute(Mock):
         return "MOCK output of child process"
 
 
+class MockRietveld():
+
+    def __init__(self, executive, dryrun=False):
+        pass
+
+    def post(self, diff, message=None, codereview_issue=None, cc=None):
+        log("MOCK: Uploading patch to rietveld")
+
+
 class MockTool():
 
     def __init__(self, log_executive=False):
@@ -526,7 +559,7 @@ class MockTool():
         self._checkout = MockCheckout()
         self.status_server = MockStatusServer()
         self.irc_password = "MOCK irc password"
-        self.codereview = Rietveld(self.executive)
+        self.codereview = MockRietveld(self.executive)
 
     def scm(self):
         return self._scm
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py b/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py
index abafe63..20f8bbf 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py
@@ -53,8 +53,9 @@ class AbstractStep(object):
         return self._port
 
     _well_known_keys = {
-        "diff": lambda self: self._tool.scm().create_patch(self._options.git_commit, self._options.squash),
-        "changelogs": lambda self: self._tool.checkout().modified_changelogs(self._options.git_commit, self._options.squash),
+        "diff": lambda self, state: self._tool.scm().create_patch(self._options.git_commit, self._options.squash),
+        "changelogs": lambda self, state: self._tool.checkout().modified_changelogs(self._options.git_commit, self._options.squash),
+        "bug_title": lambda self, state: self._tool.bugs.fetch_bug(state["bug_id"]).title(),
     }
 
     def cached_lookup(self, state, key, promise=None):
@@ -62,7 +63,7 @@ class AbstractStep(object):
             return state[key]
         if not promise:
             promise = self._well_known_keys.get(key)
-        state[key] = promise(self)
+        state[key] = promise(self, state)
         return state[key]
 
     @classmethod
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/confirmdiff.py b/WebKitTools/Scripts/webkitpy/tool/steps/confirmdiff.py
index 626fcf3..7e8e348 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/confirmdiff.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/confirmdiff.py
@@ -46,6 +46,9 @@ class ConfirmDiff(AbstractStep):
         ]
 
     def _show_pretty_diff(self, diff):
+        if not self._tool.user.can_open_url():
+            return None
+
         try:
             pretty_patch = PrettyPatch(self._tool.executive,
                                        self._tool.scm().checkout_root)
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/options.py b/WebKitTools/Scripts/webkitpy/tool/steps/options.py
index 186d292..fa36f73 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/options.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/options.py
@@ -41,7 +41,6 @@ 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)")
 # FIXME: Make commit ranges treat each commit separately instead of squashing them into one.
     git_commit = make_option("--git-commit", action="store", dest="git_commit", help="Local git commit to upload/land. If a range, the commits are squashed into one.")
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py b/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py
index 8397519..f9bc685 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py
@@ -36,33 +36,27 @@ class PostCodeReview(AbstractStep):
         return AbstractStep.options() + [
             Options.cc,
             Options.description,
-            Options.fancy_review,
         ]
 
     def run(self, state):
-        if not self._options.fancy_review:
-            return
+        patch = state.get("patch")
+        bug_id = patch.bug_id()
+        title = patch.name()
 
-        bug_id = state.get("bug_id")
-        if not bug_id:
-            raise ScriptError(message="Cannot upload a fancy review without a bug ID.")
-
-        message = self._options.description
-        if not message:
-            # If we have an issue number, then the message becomes the label
-            # of the new patch. Otherwise, it becomes the title of the whole
-            # issue.
-            if state.get("bug_title"):
-                # This is the common case for the the first "upload" command.
-                message = state.get("bug_title")
-            elif bug_id:
-                # This is the common case for the "post" command and
-                # subsequent runs of the "upload" command.
-                message = "Code review for %s" % self._tool.bugs.bug_url_for_bug_id(bug_id)
-            else:
-                # Unreachable with our current commands, but we might hit
-                # this case if we support bug-less code reviews.
-                message = "Code review"
+        # If the issue already exists, then the message becomes the label
+        # of the new patch. Otherwise, it becomes the title of the whole
+        # issue.
+        if title:
+            # This is the common case for the the first "upload" command.
+            message = title
+        elif bug_id:
+            # This is the common case for the "post" command and
+            # subsequent runs of the "upload" command.
+            message = "Code review for %s" % self._tool.bugs.bug_url_for_bug_id(bug_id)
+        else:
+            # Unreachable with our current commands, but we might hit
+            # this case if we support bug-less code reviews.
+            message = "Code review"
 
         # Use the bug ID as the rietveld issue number. This means rietveld code reviews
         # when there are multiple different patches on a bug will be a bit wonky, but
@@ -71,3 +65,5 @@ class PostCodeReview(AbstractStep):
                                                    message=message,
                                                    codereview_issue=bug_id,
                                                    cc=self._options.cc)
+
+        self._tool.bugs.set_flag_on_attachment(patch.id(), 'in-rietveld', '+')
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py
index 3a5c013..59048a3 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py
@@ -28,6 +28,7 @@
 
 import os
 
+from webkitpy.common.checkout.changelog import ChangeLog
 from webkitpy.common.system.executive import ScriptError
 from webkitpy.tool.steps.abstractstep import AbstractStep
 from webkitpy.tool.steps.options import Options
@@ -46,8 +47,21 @@ class PrepareChangeLog(AbstractStep):
             Options.squash,
         ]
 
+    def _ensure_bug_url(self, state):
+        if not state.get("bug_id"):
+            return
+        bug_id = state.get("bug_id")
+        changelogs = self.cached_lookup(state, "changelogs")
+        for changelog_path in changelogs:
+            changelog = ChangeLog(changelog_path)
+            if not changelog.latest_entry().bug_id():
+                changelog.set_short_description_and_bug_url(
+                    self.cached_lookup(state, "bug_title"),
+                    self._tool.bugs.bug_url_for_bug_id(bug_id))
+
     def run(self, state):
         if self.cached_lookup(state, "changelogs"):
+            self._ensure_bug_url(state)
             return
         os.chdir(self._tool.scm().checkout_root)
         args = [self.port().script_path("prepare-ChangeLog")]
@@ -56,7 +70,7 @@ class PrepareChangeLog(AbstractStep):
         if self._options.email:
             args.append("--email=%s" % self._options.email)
         if self._tool.scm().should_squash(self._options.squash):
-            args.append("--merge-base=%s" % self._tool.scm().svn_merge_base())
+            args.append("--merge-base=%s" % self._tool.scm().remote_merge_base())
         if self._options.git_commit:
             args.append("--git-commit=%s" % self._options.git_commit)
 
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog_unittest.py
new file mode 100644
index 0000000..1d0db75
--- /dev/null
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog_unittest.py
@@ -0,0 +1,55 @@
+# 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 unittest
+
+from webkitpy.common.checkout.changelog_unittest import ChangeLogTest
+from webkitpy.common.system.outputcapture import OutputCapture
+from webkitpy.thirdparty.mock import Mock
+from webkitpy.tool.mocktool import MockTool
+from webkitpy.tool.steps.preparechangelog import PrepareChangeLog
+
+
+class PrepareChangeLogTest(ChangeLogTest):
+    def test_ensure_bug_url(self):
+        capture = OutputCapture()
+        step = PrepareChangeLog(MockTool(), Mock())
+        changelog_contents = u"%s\n%s" % (self._new_entry_boilerplate, self._example_changelog)
+        changelog_path = self._write_tmp_file_with_contents(changelog_contents.encode("utf-8"))
+        state = {
+            "bug_title": "Example title",
+            "bug_id": 1234,
+            "changelogs": [changelog_path],
+        }
+        capture.assert_outputs(self, step.run, [state])
+        actual_contents = self._read_file_contents(changelog_path, "utf-8")
+        expected_message = "Example title\n        http://example.com/1234"
+        expected_contents = changelog_contents.replace("Need a short description and bug URL (OOPS!)", expected_message)
+        os.remove(changelog_path)
+        self.assertEquals(actual_contents, expected_contents)