Merge Webkit at r70949: Initial merge by git.

Change-Id: I77b8645c083b5d0da8dba73ed01d4014aab9848e

10 files changed, 180 insertions, 165 deletions

diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download.py b/WebKitTools/Scripts/webkitpy/tool/commands/download.py
index ed5c604..541c9c4 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/download.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/download.py
@@ -106,8 +106,10 @@ land will NOT build and run the tests before committing, but you can use the --b
 If a bug id is provided, or one can be found in the ChangeLog land will update the bug after committing."""
 
     def _prepare_state(self, options, args, tool):
+        changed_files = self._tool.scm().changed_files(options.git_commit)
         return {
-            "bug_id": (args and args[0]) or tool.checkout().bug_id_for_this_commit(options.git_commit),
+            "changed_files": changed_files,
+            "bug_id": (args and args[0]) or tool.checkout().bug_id_for_this_commit(options.git_commit, changed_files),
         }
 
 
@@ -221,18 +223,6 @@ class BuildAndTestAttachment(AbstractPatchSequencingCommand, ProcessAttachmentsM
     ]
 
 
-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 6af1f64..bfca139 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
@@ -118,10 +118,6 @@ 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.
@@ -186,5 +182,6 @@ where ATTACHMENT_ID is the ID of this attachment.
 
     def test_rollout(self):
         expected_stderr = "Preparing rollout for bug 42.\nUpdating working directory\nRunning prepare-ChangeLog\nMOCK: user.open_url: file://...\nBuilding WebKit\n"
-        self.assert_execute_outputs(Rollout(), [852, "Reason"], options=self._default_options(), expected_stderr=expected_stderr)
+        expected_stdout = "Was that diff correct?\n"
+        self.assert_execute_outputs(Rollout(), [852, "Reason"], options=self._default_options(), expected_stdout=expected_stdout, expected_stderr=expected_stderr)
 
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py
index 5ec468e..3b53d1a 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem.py
@@ -50,8 +50,7 @@ class AbstractEarlyWarningSystem(AbstractReviewQueue):
                 self.port.flag(),
                 "--build-style=%s" % self._build_style,
                 "--force-clean",
-                "--no-update",
-                "--quiet"])
+                "--no-update"])
             return True
         except ScriptError, e:
             failure_log = self._log_from_script_error_for_upload(e)
@@ -81,6 +80,14 @@ class AbstractEarlyWarningSystem(AbstractReviewQueue):
             raise
 
     def review_patch(self, patch):
+        if patch.is_obsolete():
+            self._did_error(patch, "%s does not process obsolete patches." % self.name)
+            return False
+
+        if patch.bug().is_closed():
+            self._did_error(patch, "%s does not process patches on closed bugs." % self.name)
+            return False
+
         if not self._build(patch, first_run=True):
             if not self._can_build():
                 return False
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
index c400f81..830e11c 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
@@ -29,15 +29,54 @@
 import os
 
 from webkitpy.thirdparty.mock import Mock
+from webkitpy.common.system.outputcapture import OutputCapture
+from webkitpy.tool.bot.queueengine import QueueEngine
 from webkitpy.tool.commands.earlywarningsystem import *
 from webkitpy.tool.commands.queuestest import QueuesTest
+from webkitpy.tool.mocktool import MockTool, MockOptions
+
+
+class AbstractEarlyWarningSystemTest(QueuesTest):
+    def test_can_build(self):
+        # Needed to define port_name, used in AbstractEarlyWarningSystem.__init__
+        class TestEWS(AbstractEarlyWarningSystem):
+            port_name = "win"  # Needs to be a port which port/factory understands.
+
+        queue = TestEWS()
+        queue.bind_to_tool(MockTool(log_executive=True))
+        queue._options = MockOptions(port=None)
+        expected_stderr = "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--port=win', '--build-style=release', '--force-clean', '--no-update']\n"
+        OutputCapture().assert_outputs(self, queue._can_build, [], expected_stderr=expected_stderr)
+
+        def mock_run_webkit_patch(args):
+            raise ScriptError("MOCK script error")
+
+        queue.run_webkit_patch = mock_run_webkit_patch
+        expected_stderr = "MOCK: update_status: None Unable to perform a build\n"
+        OutputCapture().assert_outputs(self, queue._can_build, [], expected_stderr=expected_stderr)
+
+    # FIXME: This belongs on an AbstractReviewQueueTest object in queues_unittest.py
+    def test_subprocess_handled_error(self):
+        queue = AbstractReviewQueue()
+        queue.bind_to_tool(MockTool())
+
+        def mock_review_patch(patch):
+            raise ScriptError('MOCK script error', exit_code=QueueEngine.handled_error_code)
+
+        queue.review_patch = mock_review_patch
+        mock_patch = queue._tool.bugs.fetch_attachment(197)
+        expected_stderr = "MOCK: release_work_item: None 197\n"
+        OutputCapture().assert_outputs(self, queue.process_work_item, [mock_patch], expected_stderr=expected_stderr, expected_exception=ScriptError)
+
 
 class EarlyWarningSytemTest(QueuesTest):
     def test_failed_builds(self):
         ews = ChromiumLinuxEWS()
+        ews.bind_to_tool(MockTool())
         ews._build = lambda patch, first_run=False: False
         ews._can_build = lambda: True
-        ews.review_patch(Mock())
+        mock_patch = ews._tool.bugs.fetch_attachment(197)
+        ews.review_patch(mock_patch)
 
     def _default_expected_stderr(self, ews):
         string_replacemnts = {
@@ -46,20 +85,29 @@ class EarlyWarningSytemTest(QueuesTest):
             "watchers": ews.watchers,
         }
         expected_stderr = {
-            "begin_work_queue": self._default_begin_work_queue_stderr(ews.name, os.getcwd()),  # FIXME: Use of os.getcwd() is wrong, should be scm.checkout_root
+            "begin_work_queue": self._default_begin_work_queue_stderr(ews.name, ews._tool.scm().checkout_root),
             "handle_unexpected_error": "Mock error message\n",
             "next_work_item": "",
             "process_work_item": "MOCK: update_status: %(name)s Pass\nMOCK: release_work_item: %(name)s 197\n" % string_replacemnts,
-            "handle_script_error": "MOCK: update_status: %(name)s ScriptError error message\nMOCK bug comment: bug_id=142, cc=%(watchers)s\n--- Begin comment ---\nAttachment 197 did not build on %(port)s:\nBuild output: http://dummy_url\n--- End comment ---\n\n" % string_replacemnts,
+            "handle_script_error": "MOCK: update_status: %(name)s ScriptError error message\nMOCK bug comment: bug_id=42, cc=%(watchers)s\n--- Begin comment ---\nAttachment 197 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):
+        ews.bind_to_tool(MockTool())
         expected_exceptions = {
             "handle_script_error": SystemExit,
         }
         self.assert_queue_outputs(ews, expected_stderr=self._default_expected_stderr(ews), expected_exceptions=expected_exceptions)
 
+    def _test_committer_only_ews(self, ews):
+        ews.bind_to_tool(MockTool())
+        expected_stderr = self._default_expected_stderr(ews)
+        string_replacemnts = {"name": ews.name}
+        expected_stderr["process_work_item"] = "MOCK: update_status: %(name)s Error: %(name)s cannot process patches from non-committers :(\nMOCK: release_work_item: %(name)s 197\n" % string_replacemnts
+        expected_exceptions = {"handle_script_error": SystemExit}
+        self.assert_queue_outputs(ews, expected_stderr=expected_stderr, 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.
     def test_chromium_linux_ews(self):
@@ -78,19 +126,7 @@ class EarlyWarningSytemTest(QueuesTest):
         self._test_ews(EflEWS())
 
     def test_mac_ews(self):
-        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"
-        expected_exceptions = {
-            "handle_script_error": SystemExit,
-        }
-        self.assert_queue_outputs(ews, expected_stderr=expected_stderr, expected_exceptions=expected_exceptions)
+        self._test_committer_only_ews(MacEWS())
 
     def test_chromium_mac_ews(self):
-        ews = ChromiumMacEWS()
-        expected_stderr = self._default_expected_stderr(ews)
-        expected_stderr["process_work_item"] = "MOCK: update_status: cr-mac-ews Error: cr-mac-ews cannot process patches from non-committers :(\n"
-        expected_exceptions = {
-            "handle_script_error": SystemExit,
-        }
-        self.assert_queue_outputs(ews, expected_stderr=expected_stderr, expected_exceptions=expected_exceptions)
+        self._test_committer_only_ews(ChromiumMacEWS())
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
index 7b3002a..6b4213b 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
@@ -38,7 +38,7 @@ from datetime import datetime
 from optparse import make_option
 from StringIO import StringIO
 
-from webkitpy.common.net.bugzilla import CommitterValidator
+from webkitpy.common.net.bugzilla import CommitterValidator, Attachment
 from webkitpy.common.net.layouttestresults import path_for_layout_test, LayoutTestResults
 from webkitpy.common.net.statusserver import StatusServer
 from webkitpy.common.system.executive import ScriptError
@@ -47,7 +47,7 @@ from webkitpy.tool.commands.stepsequence import StepSequenceErrorHandler
 from webkitpy.tool.bot.commitqueuetask import CommitQueueTask, CommitQueueTaskDelegate
 from webkitpy.tool.bot.feeders import CommitQueueFeeder, EWSFeeder
 from webkitpy.tool.bot.queueengine import QueueEngine, QueueEngineDelegate
-from webkitpy.tool.grammar import pluralize
+from webkitpy.tool.grammar import pluralize, join_with_separators
 from webkitpy.tool.multicommandtool import Command, TryAgain
 
 
@@ -87,6 +87,11 @@ class AbstractQueue(Command, QueueEngineDelegate):
         if self._options.port:
             webkit_patch_args += ["--port=%s" % self._options.port]
         webkit_patch_args.extend(args)
+        # FIXME: There is probably no reason to use run_and_throw_if_fail anymore.
+        # run_and_throw_if_fail was invented to support tee'd output
+        # (where we write both to a log file and to the console at once),
+        # but the queues don't need live-progress, a dump-of-output at the
+        # end should be sufficient.
         return self._tool.executive.run_and_throw_if_fail(webkit_patch_args)
 
     def _log_directory(self):
@@ -196,24 +201,40 @@ class AbstractPatchQueue(AbstractQueue):
     def _update_status(self, message, patch=None, results_file=None):
         return self._tool.status_server.update_status(self.name, message, patch, results_file)
 
-    def _fetch_next_work_item(self):
-        return self._tool.status_server.next_work_item(self.name)
+    def _next_patch(self):
+        patch_id = self._tool.status_server.next_work_item(self.name)
+        if not patch_id:
+            return None
+        patch = self._tool.bugs.fetch_attachment(patch_id)
+        if not patch:
+            # FIXME: Using a fake patch because release_work_item has the wrong API.
+            # We also don't really need to release the lock (although that's fine),
+            # mostly we just need to remove this bogus patch from our queue.
+            # If for some reason bugzilla is just down, then it will be re-fed later.
+            patch = Attachment({'id': patch_id}, None)
+            self._release_work_item(patch)
+            return None
+        return patch
 
     def _release_work_item(self, patch):
         self._tool.status_server.release_work_item(self.name, patch)
 
     def _did_pass(self, patch):
         self._update_status(self._pass_status, patch)
+        self._release_work_item(patch)
 
     def _did_fail(self, patch):
         self._update_status(self._fail_status, patch)
+        self._release_work_item(patch)
 
     def _did_retry(self, patch):
         self._update_status(self._retry_status, patch)
+        self._release_work_item(patch)
 
     def _did_error(self, patch, reason):
         message = "%s: %s" % (self._error_status, reason)
         self._update_status(message, patch)
+        self._release_work_item(patch)
 
     def work_item_log_path(self, patch):
         return os.path.join(self._log_directory(), "%s.log" % patch.bug_id())
@@ -229,10 +250,7 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskD
         self.committer_validator = CommitterValidator(self._tool.bugs)
 
     def next_work_item(self):
-        patch_id = self._fetch_next_work_item()
-        if not patch_id:
-            return None
-        return self._tool.bugs.fetch_attachment(patch_id)
+        return self._next_patch()
 
     def should_proceed_with_work_item(self, patch):
         patch_text = "rollout patch" if patch.is_rollout() else "patch"
@@ -251,7 +269,6 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskD
             validator = CommitterValidator(self._tool.bugs)
             validator.reject_patch_from_commit_queue(patch.id(), self._error_message_for_bug(task.failure_status_id, e))
             self._did_fail(patch)
-        self._release_work_item(patch)
 
     def _error_message_for_bug(self, status_id, script_error):
         if not script_error.output:
@@ -297,13 +314,17 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskD
     def _author_emails_for_tests(self, flaky_tests):
         test_paths = map(path_for_layout_test, flaky_tests)
         commit_infos = self._tool.checkout().recent_commit_infos_for_files(test_paths)
-        return [commit_info.author().bugzilla_email() for commit_info in commit_infos if commit_info.author()]
+        return set([commit_info.author().bugzilla_email() for commit_info in commit_infos if commit_info.author()])
 
     def report_flaky_tests(self, patch, flaky_tests):
-        authors = self._author_emails_for_tests(flaky_tests)
-        cc_explaination = "  The author(s) of the test(s) have been CCed on this bug." if authors else ""
-        message = "The %s encountered the following flaky tests while processing attachment %s:\n\n%s\n\nPlease file bugs against the tests.%s  The commit-queue is continuing to process your patch." % (self.name, patch.id(), "\n".join(flaky_tests), cc_explaination)
-        self._tool.bugs.post_comment_to_bug(patch.bug_id(), message, cc=authors)
+        message = "The %s encountered the following flaky tests while processing attachment %s:" % (self.name, patch.id())
+        message += "\n\n%s\n\n" % ("\n".join(flaky_tests))
+        message += "Please file bugs against the tests.  "
+        author_emails = self._author_emails_for_tests(flaky_tests)
+        if author_emails:
+            message += "These tests were authored by %s.  " % (join_with_separators(sorted(author_emails)))
+        message += "The commit-queue is continuing to process your patch."
+        self._tool.bugs.post_comment_to_bug(patch.bug_id(), message)
 
     # StepSequenceErrorHandler methods
 
@@ -327,52 +348,6 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskD
         raise TryAgain()
 
 
-# FIXME: All the Rietveld code is no longer used and should be deleted.
-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, StepSequenceErrorHandler):
     """This is the base-class for the EWS queues and the style-queue."""
     def __init__(self, options=None):
@@ -387,10 +362,7 @@ class AbstractReviewQueue(AbstractPatchQueue, StepSequenceErrorHandler):
         AbstractPatchQueue.begin_work_queue(self)
 
     def next_work_item(self):
-        patch_id = self._fetch_next_work_item()
-        if not patch_id:
-            return None
-        return self._tool.bugs.fetch_attachment(patch_id)
+        return self._next_patch()
 
     def should_proceed_with_work_item(self, patch):
         raise NotImplementedError("subclasses must implement")
@@ -404,9 +376,11 @@ class AbstractReviewQueue(AbstractPatchQueue, StepSequenceErrorHandler):
         except ScriptError, e:
             if e.exit_code != QueueEngine.handled_error_code:
                 self._did_fail(patch)
+            else:
+                # The subprocess handled the error, but won't have released the patch, so we do.
+                # FIXME: We need to simplify the rules by which _release_work_item is called.
+                self._release_work_item(patch)
             raise e
-        finally:
-            self._release_work_item(patch)
 
     def handle_unexpected_error(self, patch, message):
         log(message)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
index b37b5dc..b45db7b 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
@@ -29,12 +29,13 @@
 import os
 
 from webkitpy.common.checkout.scm import CheckoutNeedsUpdate
+from webkitpy.common.config.committers import Committer
 from webkitpy.common.net.bugzilla import Attachment
 from webkitpy.common.system.outputcapture import OutputCapture
 from webkitpy.thirdparty.mock import Mock
 from webkitpy.tool.commands.commandtest import CommandsTest
 from webkitpy.tool.commands.queues import *
-from webkitpy.tool.commands.queuestest import QueuesTest, MockPatch
+from webkitpy.tool.commands.queuestest import QueuesTest
 from webkitpy.tool.commands.stepsequence import StepSequence
 from webkitpy.tool.mocktool import MockTool, MockSCM, MockStatusServer
 
@@ -51,11 +52,6 @@ class TestFeederQueue(FeederQueue):
     _sleep_duration = 0
 
 
-class MockRolloutPatch(MockPatch):
-    def is_rollout(self):
-        return True
-
-
 class AbstractQueueTest(CommandsTest):
     def test_log_directory(self):
         self.assertEquals(TestQueue()._log_directory(), "test-queue-logs")
@@ -144,15 +140,19 @@ MOCK: submit_to_ews: 103
 
 
 class AbstractPatchQueueTest(CommandsTest):
-    def test_fetch_next_work_item(self):
+    def test_next_patch(self):
         queue = AbstractPatchQueue()
         tool = MockTool()
         queue.bind_to_tool(tool)
         queue._options = Mock()
         queue._options.port = None
-        self.assertEquals(queue._fetch_next_work_item(), None)
-        tool.status_server = MockStatusServer(work_items=[2, 1, 3])
-        self.assertEquals(queue._fetch_next_work_item(), 2)
+        self.assertEquals(queue._next_patch(), None)
+        tool.status_server = MockStatusServer(work_items=[2, 197])
+        expected_stdout = "MOCK: fetch_attachment: 2 is not a known attachment id\n"  # A mock-only message to prevent us from making mistakes.
+        expected_stderr = "MOCK: release_work_item: None 2\n"
+        patch_id = OutputCapture().assert_outputs(self, queue._next_patch, [], expected_stdout=expected_stdout, expected_stderr=expected_stderr)
+        self.assertEquals(patch_id, None)  # 2 is an invalid patch id
+        self.assertEquals(queue._next_patch().id(), 197)
 
 
 class NeedsUpdateSequence(StepSequence):
@@ -198,6 +198,19 @@ class SecondThoughtsCommitQueue(CommitQueue):
         return Attachment(attachment_dictionary, None)
 
 
+# Creating fake CommitInfos is a pain, so we use a mock one here.
+class MockCommitInfo(object):
+    def __init__(self, author_email):
+        self._author_email = author_email
+
+    def author(self):
+        # It's definitely possible to have commits with authors who
+        # are not in our committers.py list.
+        if not self._author_email:
+            return None
+        return Committer("Mock Committer", self._author_email)
+
+
 class CommitQueueTest(QueuesTest):
     def test_commit_queue(self):
         expected_stderr = {
@@ -209,6 +222,7 @@ MOCK: update_status: commit-queue Built patch
 MOCK: update_status: commit-queue Passed tests
 MOCK: update_status: commit-queue Landed patch
 MOCK: update_status: commit-queue Pass
+MOCK: release_work_item: commit-queue 197
 """,
             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Rejecting patch 197 from commit-queue.' and additional comment 'Mock error message'\n",
             "handle_script_error": "ScriptError error message\n",
@@ -243,15 +257,16 @@ MOCK: release_work_item: commit-queue 197
             "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue", MockSCM.fake_checkout_root),
             "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing patch\n",
             "next_work_item": "",
-            "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197]
+            "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--force-clean', '--non-interactive', 197]
 MOCK: update_status: commit-queue Applied patch
-MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build-style=both', '--quiet']
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build-style=both']
 MOCK: update_status: commit-queue Built patch
-MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
 MOCK: update_status: commit-queue Passed tests
-MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197]
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 197]
 MOCK: update_status: commit-queue Landed patch
 MOCK: update_status: commit-queue Pass
+MOCK: release_work_item: commit-queue 197
 """,
             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Rejecting patch 197 from commit-queue.' and additional comment 'Mock error message'\n",
             "handle_script_error": "ScriptError error message\n",
@@ -261,22 +276,22 @@ MOCK: update_status: commit-queue Pass
     def test_rollout_lands(self):
         tool = MockTool(log_executive=True)
         tool.buildbot.light_tree_on_fire()
-        rollout_patch = MockRolloutPatch()
+        rollout_patch = tool.bugs.fetch_attachment(106)  # _patch6, a rollout patch.
+        assert(rollout_patch.is_rollout())
         expected_stderr = {
             "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue", MockSCM.fake_checkout_root),
             "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing rollout patch\n",
             "next_work_item": "",
-            "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197]
+            "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--force-clean', '--non-interactive', 106]
 MOCK: update_status: commit-queue Applied patch
-MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build-style=both', '--quiet']
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build-style=both']
 MOCK: update_status: commit-queue Built patch
-MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']
-MOCK: update_status: commit-queue Passed tests
-MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197]
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 106]
 MOCK: update_status: commit-queue Landed patch
 MOCK: update_status: commit-queue Pass
+MOCK: release_work_item: commit-queue 106
 """,
-            "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '197' with comment 'Rejecting patch 197 from commit-queue.' and additional comment 'Mock error message'\n",
+            "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '106' with comment 'Rejecting patch 106 from commit-queue.' and additional comment 'Mock error message'\n",
             "handle_script_error": "ScriptError error message\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=rollout_patch, expected_stderr=expected_stderr)
@@ -307,23 +322,36 @@ MOCK: update_status: commit-queue Passed tests
 MOCK: update_status: commit-queue Retry
 MOCK: release_work_item: commit-queue 197
 """
-        OutputCapture().assert_outputs(self, queue.process_work_item, [MockPatch()], expected_stderr=expected_stderr)
+        OutputCapture().assert_outputs(self, queue.process_work_item, [QueuesTest.mock_work_item], expected_stderr=expected_stderr)
+
+    def _assert_emails_for_tests(self, emails):
+        queue = CommitQueue()
+        tool = MockTool()
+        queue.bind_to_tool(tool)
+        commit_infos = [MockCommitInfo(email) for email in emails]
+        tool.checkout().recent_commit_infos_for_files = lambda paths: set(commit_infos)
+        self.assertEqual(queue._author_emails_for_tests([]), set(emails))
+
+    def test_author_emails_for_tests(self):
+        self._assert_emails_for_tests([])
+        self._assert_emails_for_tests(["test1@test.com", "test1@test.com"])
+        self._assert_emails_for_tests(["test1@test.com", "test2@test.com"])
 
     def test_report_flaky_tests(self):
         queue = CommitQueue()
         queue.bind_to_tool(MockTool())
-        expected_stderr = """MOCK bug comment: bug_id=142, cc=['abarth@webkit.org']
+        expected_stderr = """MOCK bug comment: bug_id=42, cc=None
 --- Begin comment ---
 The commit-queue encountered the following flaky tests while processing attachment 197:
 
 foo/bar.html
 bar/baz.html
 
-Please file bugs against the tests.  The author(s) of the test(s) have been CCed on this bug.  The commit-queue is continuing to process your patch.
+Please file bugs against the tests.  These tests were authored by abarth@webkit.org.  The commit-queue is continuing to process your patch.
 --- End comment ---
 
 """
-        OutputCapture().assert_outputs(self, queue.report_flaky_tests, [MockPatch(), ["foo/bar.html", "bar/baz.html"]], expected_stderr=expected_stderr)
+        OutputCapture().assert_outputs(self, queue.report_flaky_tests, [QueuesTest.mock_work_item, ["foo/bar.html", "bar/baz.html"]], expected_stderr=expected_stderr)
 
     def test_layout_test_results(self):
         queue = CommitQueue()
@@ -333,17 +361,6 @@ Please file bugs against the tests.  The author(s) of the test(s) have been CCed
         queue._read_file_contents = lambda path: ""
         self.assertEquals(queue.layout_test_results(), None)
 
-class RietveldUploadQueueTest(QueuesTest):
-    def test_rietveld_upload_queue(self):
-        expected_stderr = {
-            "begin_work_queue": self._default_begin_work_queue_stderr("rietveld-upload-queue", 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 '197' 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 '197' 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):
@@ -353,7 +370,7 @@ 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\nMOCK: release_work_item: style-queue 197\n",
             "handle_unexpected_error": "Mock error message\n",
-            "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=142, cc=[]\n--- Begin comment ---\nAttachment 197 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",
+            "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=42, cc=[]\n--- Begin comment ---\nAttachment 197 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,
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
index 379d380..6455617 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
@@ -44,25 +44,9 @@ class MockQueueEngine(object):
         pass
 
 
-class MockPatch():
-    def id(self):
-        return 197
-
-    def bug_id(self):
-        return 142
-
-    def is_rollout(self):
-        return False
-
-
 class QueuesTest(unittest.TestCase):
-    # Ids match patch1 in mocktool.py
-    mock_work_item = Attachment({
-        "id": 197,
-        "bug_id": 142,
-        "name": "Patch",
-        "attacher_email": "adam@example.com",
-    }, None)
+    # This is _patch1 in mocktool.py
+    mock_work_item = MockTool().bugs.fetch_attachment(197)
 
     def assert_outputs(self, func, func_name, args, expected_stdout, expected_stderr, expected_exceptions):
         exception = None
@@ -108,4 +92,4 @@ class QueuesTest(unittest.TestCase):
         self.assert_outputs(queue.handle_unexpected_error, "handle_unexpected_error", [work_item, "Mock error message"], expected_stdout, expected_stderr, expected_exceptions)
         # Should we have a different function for testing StepSequenceErrorHandlers?
         if isinstance(queue, StepSequenceErrorHandler):
-            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)
+            self.assert_outputs(queue.handle_script_error, "handle_script_error", [tool, {"patch": self.mock_work_item}, ScriptError(message="ScriptError error message", script_args="MockErrorCommand")], expected_stdout, expected_stderr, expected_exceptions)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py
index 32eb016..4db463e 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py
@@ -38,9 +38,10 @@ class SheriffBotTest(QueuesTest):
     builder2 = MockBuilder("Builder2")
 
     def test_sheriff_bot(self):
-        mock_work_item = MockFailureMap(MockTool().buildbot)
+        tool = MockTool()
+        mock_work_item = MockFailureMap(tool.buildbot)
         expected_stderr = {
-            "begin_work_queue": self._default_begin_work_queue_stderr("sheriff-bot", os.getcwd()),
+            "begin_work_queue": self._default_begin_work_queue_stderr("sheriff-bot", tool.scm().checkout_root),
             "next_work_item": "",
             "process_work_item": """MOCK: irc.post: abarth, darin, eseidel: http://trac.webkit.org/changeset/29837 might have broken Builder1
 MOCK bug comment: bug_id=42, cc=['abarth@webkit.org', 'eric@webkit.org']
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
index 107d8db..ed91f5a 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
@@ -152,7 +152,9 @@ class AbstractPatchUploadingCommand(AbstractSequencedCommand):
         # Perfer a bug id passed as an argument over a bug url in the diff (i.e. ChangeLogs).
         bug_id = args and args[0]
         if not bug_id:
-            bug_id = tool.checkout().bug_id_for_this_commit(options.git_commit)
+            changed_files = self._tool.scm().changed_files(options.git_commit)
+            state["changed_files"] = changed_files
+            bug_id = tool.checkout().bug_id_for_this_commit(options.git_commit, changed_files)
         return bug_id
 
     def _prepare_state(self, options, args, tool):
@@ -171,6 +173,7 @@ class Post(AbstractPatchUploadingCommand):
         steps.CheckStyle,
         steps.ConfirmDiff,
         steps.ObsoletePatches,
+        steps.SuggestReviewers,
         steps.PostDiff,
     ]
 
@@ -219,6 +222,7 @@ class Upload(AbstractPatchUploadingCommand):
         steps.EditChangeLog,
         steps.ConfirmDiff,
         steps.ObsoletePatches,
+        steps.SuggestReviewers,
         steps.PostDiff,
     ]
     long_help = """upload uploads the current diff to bugs.webkit.org.
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
index 0d096b6..bd1fbd6 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.suggest_reviewers = False
         expected_stderr = """Running check-webkit-style
 MOCK: user.open_url: file://...
 Obsoleting 2 old patches on bug 42
@@ -67,7 +68,8 @@ None
 -- End comment --
 MOCK: user.open_url: http://example.com/42
 """
-        self.assert_execute_outputs(Post(), [42], options=options, expected_stderr=expected_stderr)
+        expected_stdout = "Was that diff correct?\n"
+        self.assert_execute_outputs(Post(), [42], options=options, expected_stdout=expected_stdout, expected_stderr=expected_stderr)
 
     def test_land_safely(self):
         expected_stderr = "Obsoleting 2 old patches on bug 42\nMOCK add_patch_to_bug: bug_id=42, description=Patch for landing, mark_for_review=False, mark_for_commit_queue=False, mark_for_landing=True\n-- Begin comment --\nNone\n-- End comment --\n"
@@ -88,6 +90,7 @@ MOCK: user.open_url: http://example.com/42
         options.description = "MOCK description"
         options.request_commit = False
         options.review = True
+        options.suggest_reviewers = False
         expected_stderr = """Running check-webkit-style
 MOCK: user.open_url: file://...
 Obsoleting 2 old patches on bug 42
@@ -97,7 +100,8 @@ None
 -- End comment --
 MOCK: user.open_url: http://example.com/42
 """
-        self.assert_execute_outputs(Upload(), [42], options=options, expected_stderr=expected_stderr)
+        expected_stdout = "Was that diff correct?\n"
+        self.assert_execute_outputs(Upload(), [42], options=options, expected_stdout=expected_stdout, expected_stderr=expected_stderr)
 
     def test_mark_bug_fixed(self):
         tool = MockTool()
@@ -106,7 +110,8 @@ MOCK: user.open_url: http://example.com/42
         options.bug_id = 42
         options.comment = "MOCK comment"
         expected_stderr = "Bug: <http://example.com/42> Bug with two r+'d and cq+'d patches, one of which has an invalid commit-queue setter.\nRevision: 9876\nMOCK: user.open_url: http://example.com/42\nAdding comment to Bug 42.\nMOCK bug comment: bug_id=42, cc=None\n--- Begin comment ---\nMOCK comment\n\nCommitted r9876: <http://trac.webkit.org/changeset/9876>\n--- End comment ---\n\n"
-        self.assert_execute_outputs(MarkBugFixed(), [], expected_stderr=expected_stderr, tool=tool, options=options)
+        expected_stdout = "Is this correct?\n"
+        self.assert_execute_outputs(MarkBugFixed(), [], expected_stdout=expected_stdout, expected_stderr=expected_stderr, tool=tool, options=options)
 
     def test_edit_changelog(self):
         self.assert_execute_outputs(EditChangeLogs(), [])