Merge Webkit at r70949: Initial merge by git.

Change-Id: I77b8645c083b5d0da8dba73ed01d4014aab9848e

21 files changed, 297 insertions, 271 deletions

diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py
index 02e203c..ea12702 100644
--- a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py
+++ b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py
@@ -85,7 +85,6 @@ class CommitQueueTask(object):
             "apply-attachment",
             "--force-clean",
             "--non-interactive",
-            "--quiet",
             self._patch.id(),
         ],
         "Applied patch",
@@ -97,7 +96,6 @@ class CommitQueueTask(object):
             "--no-clean",
             "--no-update",
             "--build-style=both",
-            "--quiet",
         ],
         "Built patch",
         "Patch does not build")
@@ -108,7 +106,6 @@ class CommitQueueTask(object):
             "--force-clean",
             "--no-update",
             "--build-style=both",
-            "--quiet",
         ],
         "Able to build without patch",
         "Unable to build without patch")
@@ -120,7 +117,6 @@ class CommitQueueTask(object):
             "--no-update",
             # Notice that we don't pass --build, which means we won't build!
             "--test",
-            "--quiet",
             "--non-interactive",
         ],
         "Passed tests",
@@ -133,7 +129,6 @@ class CommitQueueTask(object):
             "--no-update",
             "--build",
             "--test",
-            "--quiet",
             "--non-interactive",
         ],
         "Able to pass tests without patch",
@@ -146,11 +141,11 @@ class CommitQueueTask(object):
         return results.failing_tests()
 
     def _land(self):
+        # Unclear if this should pass --quiet or not.  If --parent-command always does the reporting, then it should.
         return self._run_command([
             "land-attachment",
             "--force-clean",
             "--ignore-builders",
-            "--quiet",
             "--non-interactive",
             "--parent-command=commit-queue",
             self._patch.id(),
diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py
index 6fa0667..15a4a6b 100644
--- a/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py
@@ -75,13 +75,13 @@ class CommitQueueTaskTest(unittest.TestCase):
 
     def test_success_case(self):
         commit_queue = MockCommitQueue([])
-        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197]
+        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197]
 command_passed: success_message='Applied patch' patch='197'
-run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet']
+run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='197'
-run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']
+run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
 command_passed: success_message='Passed tests' patch='197'
-run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197]
+run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 197]
 command_passed: success_message='Landed patch' patch='197'
 """
         self._run_through_task(commit_queue, expected_stderr)
@@ -90,7 +90,7 @@ command_passed: success_message='Landed patch' patch='197'
         commit_queue = MockCommitQueue([
             ScriptError("MOCK apply failure"),
         ])
-        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197]
+        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197]
 command_failed: failure_message='Patch does not apply' script_error='MOCK apply failure' patch='197'
 """
         self._run_through_task(commit_queue, expected_stderr, ScriptError)
@@ -100,11 +100,11 @@ command_failed: failure_message='Patch does not apply' script_error='MOCK apply
             None,
             ScriptError("MOCK build failure"),
         ])
-        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197]
+        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197]
 command_passed: success_message='Applied patch' patch='197'
-run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet']
+run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_failed: failure_message='Patch does not build' script_error='MOCK build failure' patch='197'
-run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both', '--quiet']
+run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Able to build without patch' patch='197'
 """
         self._run_through_task(commit_queue, expected_stderr, ScriptError)
@@ -115,11 +115,11 @@ command_passed: success_message='Able to build without patch' patch='197'
             ScriptError("MOCK build failure"),
             ScriptError("MOCK clean build failure"),
         ])
-        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197]
+        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197]
 command_passed: success_message='Applied patch' patch='197'
-run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet']
+run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_failed: failure_message='Patch does not build' script_error='MOCK build failure' patch='197'
-run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both', '--quiet']
+run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both']
 command_failed: failure_message='Unable to build without patch' script_error='MOCK clean build failure' patch='197'
 """
         self._run_through_task(commit_queue, expected_stderr)
@@ -130,16 +130,16 @@ command_failed: failure_message='Unable to build without patch' script_error='MO
             None,
             ScriptError("MOCK tests failure"),
         ])
-        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197]
+        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197]
 command_passed: success_message='Applied patch' patch='197'
-run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet']
+run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='197'
-run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']
+run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
 command_failed: failure_message='Patch does not pass tests' script_error='MOCK tests failure' patch='197'
-run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']
+run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
 command_passed: success_message='Passed tests' patch='197'
 report_flaky_tests: patch='197' flaky_tests='None'
-run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197]
+run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 197]
 command_passed: success_message='Landed patch' patch='197'
 """
         self._run_through_task(commit_queue, expected_stderr)
@@ -151,15 +151,15 @@ command_passed: success_message='Landed patch' patch='197'
             ScriptError("MOCK test failure"),
             ScriptError("MOCK test failure again"),
         ])
-        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197]
+        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197]
 command_passed: success_message='Applied patch' patch='197'
-run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet']
+run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='197'
-run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']
+run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
 command_failed: failure_message='Patch does not pass tests' script_error='MOCK test failure' patch='197'
-run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']
+run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
 command_failed: failure_message='Patch does not pass tests' script_error='MOCK test failure again' patch='197'
-run_webkit_patch: ['build-and-test', '--force-clean', '--no-update', '--build', '--test', '--quiet', '--non-interactive']
+run_webkit_patch: ['build-and-test', '--force-clean', '--no-update', '--build', '--test', '--non-interactive']
 command_passed: success_message='Able to pass tests without patch' patch='197'
 """
         self._run_through_task(commit_queue, expected_stderr, ScriptError)
@@ -172,15 +172,15 @@ command_passed: success_message='Able to pass tests without patch' patch='197'
             ScriptError("MOCK test failure again"),
             ScriptError("MOCK clean test failure"),
         ])
-        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197]
+        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197]
 command_passed: success_message='Applied patch' patch='197'
-run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet']
+run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='197'
-run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']
+run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
 command_failed: failure_message='Patch does not pass tests' script_error='MOCK test failure' patch='197'
-run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']
+run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
 command_failed: failure_message='Patch does not pass tests' script_error='MOCK test failure again' patch='197'
-run_webkit_patch: ['build-and-test', '--force-clean', '--no-update', '--build', '--test', '--quiet', '--non-interactive']
+run_webkit_patch: ['build-and-test', '--force-clean', '--no-update', '--build', '--test', '--non-interactive']
 command_failed: failure_message='Unable to pass tests without patch (tree is red?)' script_error='MOCK clean test failure' patch='197'
 """
         self._run_through_task(commit_queue, expected_stderr)
@@ -192,13 +192,13 @@ command_failed: failure_message='Unable to pass tests without patch (tree is red
             None,
             ScriptError("MOCK land failure"),
         ])
-        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', '--quiet', 197]
+        expected_stderr = """run_webkit_patch: ['apply-attachment', '--force-clean', '--non-interactive', 197]
 command_passed: success_message='Applied patch' patch='197'
-run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both', '--quiet']
+run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='197'
-run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']
+run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
 command_passed: success_message='Passed tests' patch='197'
-run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 197]
+run_webkit_patch: ['land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 197]
 command_failed: failure_message='Unable to land patch' script_error='MOCK land failure' patch='197'
 """
         self._run_through_task(commit_queue, expected_stderr, ScriptError)
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(), [])
diff --git a/WebKitTools/Scripts/webkitpy/tool/main.py b/WebKitTools/Scripts/webkitpy/tool/main.py
index ce6666e..e0862c5 100755
--- a/WebKitTools/Scripts/webkitpy/tool/main.py
+++ b/WebKitTools/Scripts/webkitpy/tool/main.py
@@ -37,7 +37,6 @@ from webkitpy.common.checkout.scm import default_scm
 from webkitpy.common.config.ports import WebKitPort
 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
@@ -79,7 +78,6 @@ class WebKitPatch(MultiCommandTool):
         self._scm = None
         self._checkout = None
         self.status_server = StatusServer()
-        self.codereview = Rietveld(self.executive)
         self.port_factory = port.factory
 
     def scm(self):
@@ -126,7 +124,6 @@ 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.bot_id:
diff --git a/WebKitTools/Scripts/webkitpy/tool/mocktool.py b/WebKitTools/Scripts/webkitpy/tool/mocktool.py
index 05b30dd..af232d9 100644
--- a/WebKitTools/Scripts/webkitpy/tool/mocktool.py
+++ b/WebKitTools/Scripts/webkitpy/tool/mocktool.py
@@ -33,7 +33,6 @@ 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
 
@@ -86,7 +85,6 @@ _patch3 = {
     "name": "Patch3",
     "is_obsolete": False,
     "is_patch": True,
-    "in-rietveld": "?",
     "review": "?",
     "attacher_email": "eric@webkit.org",
 }
@@ -113,7 +111,6 @@ _patch5 = {
     "name": "Patch5",
     "is_obsolete": False,
     "is_patch": True,
-    "in-rietveld": "?",
     "review": "+",
     "reviewer_email": "foo@bar.com",
     "attacher_email": "eric@webkit.org",
@@ -127,7 +124,6 @@ _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",
@@ -141,7 +137,6 @@ _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",
@@ -192,6 +187,7 @@ _bug4 = {
 }
 
 
+# FIXME: This should not inherit from Mock
 class MockBugzillaQueries(Mock):
 
     def __init__(self, bugzilla):
@@ -229,18 +225,14 @@ 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')
+
+_mock_reviewer = Reviewer("Foo Bar", "foo@bar.com")
+
 
 # FIXME: Bugzilla is the wrong Mock-point.  Once we have a BugzillaNetwork
 #        class we should mock that instead.
 # Most of this class is just copy/paste from Bugzilla.
-
-
+# FIXME: This should not inherit from Mock
 class MockBugzilla(Mock):
 
     bug_server_url = "http://example.com"
@@ -258,7 +250,7 @@ class MockBugzilla(Mock):
     def __init__(self):
         Mock.__init__(self)
         self.queries = MockBugzillaQueries(self)
-        self.committers = CommitterList(reviewers=[Reviewer("Foo Bar", "foo@bar.com")])
+        self.committers = CommitterList(reviewers=[_mock_reviewer])
         self._override_patch = None
 
     def create_bug(self,
@@ -288,8 +280,10 @@ class MockBugzilla(Mock):
         if self._override_patch:
             return self._override_patch
 
-        # This could be changed to .get() if we wish to allow failed lookups.
-        attachment_dictionary = self.attachment_cache[attachment_id]
+        attachment_dictionary = self.attachment_cache.get(attachment_id)
+        if not attachment_dictionary:
+            print "MOCK: fetch_attachment: %s is not a known attachment id" % attachment_id
+            return None
         bug = self.fetch_bug(attachment_dictionary["bug_id"])
         for attachment in bug.attachments(include_obsolete=True):
             if attachment.id() == int(attachment_id):
@@ -418,6 +412,7 @@ class MockBuildBot(object):
         return MockFailureMap(self)
 
 
+# FIXME: This should not inherit from Mock
 class MockSCM(Mock):
 
     fake_checkout_root = os.path.realpath("/tmp") # realpath is needed to allow for Mac OS X's /private/tmp
@@ -480,7 +475,7 @@ class MockCheckout(object):
         # that LandDiff will try to actually read the patch from disk!
         return []
 
-    def commit_message_for_this_commit(self, git_commit):
+    def commit_message_for_this_commit(self, git_commit, changed_files=None):
         commit_message = Mock()
         commit_message.message = lambda:"This is a fake commit message that is at least 50 characters."
         return commit_message
@@ -491,6 +486,9 @@ class MockCheckout(object):
     def apply_reverse_diff(self, revision):
         pass
 
+    def suggested_reviewers(self, git_commit, changed_files=None):
+        return [_mock_reviewer]
+
 
 class MockUser(object):
 
@@ -508,6 +506,7 @@ class MockUser(object):
         pass
 
     def confirm(self, message=None, default='y'):
+        print message
         return default == 'y'
 
     def can_open_url(self):
@@ -545,7 +544,7 @@ class MockStatusServer(object):
     def next_work_item(self, queue_name):
         if not self._work_items:
             return None
-        return self._work_items[0]
+        return self._work_items.pop(0)
 
     def release_work_item(self, queue_name, patch):
         log("MOCK: release_work_item: %s %s" % (queue_name, patch.id()))
@@ -568,7 +567,8 @@ class MockStatusServer(object):
         return "http://dummy_url"
 
 
-class MockExecute(Mock):
+# FIXME: This should not inherit from Mock
+class MockExecutive(Mock):
     def __init__(self, should_log):
         self._should_log = should_log
 
@@ -603,47 +603,37 @@ class MockOptions(object):
             self.__dict__[key] = value
 
 
-class MockRietveld():
-
-    def __init__(self, executive, dryrun=False):
-        pass
-
-    def post(self, diff, patch_id, codereview_issue, message=None, cc=None):
-        log("MOCK: Uploading patch to rietveld")
-
-
-class MockTestPort1():
+class MockTestPort1(object):
 
     def skips_layout_test(self, test_name):
         return test_name in ["media/foo/bar.html", "foo"]
 
 
-class MockTestPort2():
+class MockTestPort2(object):
 
     def skips_layout_test(self, test_name):
         return test_name == "media/foo/bar.html"
 
 
-class MockPortFactory():
+class MockPortFactory(object):
 
     def get_all(self, options=None):
         return {"test_port1": MockTestPort1(), "test_port2": MockTestPort2()}
 
 
-class MockTool():
+class MockTool(object):
 
     def __init__(self, log_executive=False):
         self.wakeup_event = threading.Event()
         self.bugs = MockBugzilla()
         self.buildbot = MockBuildBot()
-        self.executive = MockExecute(should_log=log_executive)
+        self.executive = MockExecutive(should_log=log_executive)
         self._irc = None
         self.user = MockUser()
         self._scm = MockSCM()
         self._checkout = MockCheckout()
         self.status_server = MockStatusServer()
         self.irc_password = "MOCK irc password"
-        self.codereview = MockRietveld(self.executive)
         self.port_factory = MockPortFactory()
 
     def scm(self):
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py b/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py
index d59cdc5..64d9d05 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/__init__.py
@@ -44,7 +44,6 @@ 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
@@ -54,6 +53,7 @@ from webkitpy.tool.steps.promptforbugortitle import PromptForBugOrTitle
 from webkitpy.tool.steps.reopenbugafterrollout import ReopenBugAfterRollout
 from webkitpy.tool.steps.revertrevision import RevertRevision
 from webkitpy.tool.steps.runtests import RunTests
+from webkitpy.tool.steps.suggestreviewers import SuggestReviewers
 from webkitpy.tool.steps.updatechangelogswithreviewer import UpdateChangeLogsWithReviewer
 from webkitpy.tool.steps.update import Update
 from webkitpy.tool.steps.validatereviewer import ValidateReviewer
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/options.py b/WebKitTools/Scripts/webkitpy/tool/steps/options.py
index 835fdba..4f17dd3 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/options.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/options.py
@@ -54,5 +54,6 @@ class Options(object):
     request_commit = make_option("--request-commit", action="store_true", dest="request_commit", default=False, help="Mark the patch as needing auto-commit after review.")
     review = make_option("--no-review", action="store_false", dest="review", default=True, help="Do not mark the patch for review.")
     reviewer = make_option("-r", "--reviewer", action="store", type="string", dest="reviewer", help="Update ChangeLogs to say Reviewed by REVIEWER.")
+    suggest_reviewers = make_option("--suggest-reviewers", action="store_true", default=False, help="Offer to CC appropriate reviewers.")
     test = make_option("--test", action="store_true", dest="test", default=False, help="Run run-webkit-tests before committing.")
     update = make_option("--no-update", action="store_false", dest="update", default=True, help="Don't update the working directory.")
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/revertrevision.py b/WebKitTools/Scripts/webkitpy/tool/steps/revertrevision.py
index 81b6bcb..bbb794c 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/revertrevision.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/revertrevision.py
@@ -32,3 +32,4 @@ from webkitpy.tool.steps.abstractstep import AbstractStep
 class RevertRevision(AbstractStep):
     def run(self, state):
         self._tool.checkout().apply_reverse_diff(state["revision"])
+        self.did_modify_checkout(state)
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py b/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py
index dcbfc44..282e381 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py
@@ -57,6 +57,7 @@ class RunTests(AbstractStep):
         log("Running run-webkit-tests")
         args = self._tool.port().run_webkit_tests_command()
         if self._options.non_interactive:
+            args.append("--no-new-test-results")
             args.append("--no-launch-safari")
             args.append("--exit-after-n-failures=1")
             args.append("--wait-for-httpd")
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py
index 7eb8e3a..eabb656 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py
@@ -87,6 +87,6 @@ MOCK run_and_throw_if_fail: ['WebKitTools/Scripts/test-webkitperl']
 Running JavaScriptCore tests
 MOCK run_and_throw_if_fail: ['WebKitTools/Scripts/run-javascriptcore-tests']
 Running run-webkit-tests
-MOCK run_and_throw_if_fail: ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']
+MOCK run_and_throw_if_fail: ['WebKitTools/Scripts/run-webkit-tests', '--no-new-test-results', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']
 """
         OutputCapture().assert_outputs(self, step.run, [{}], expected_stderr=expected_stderr)
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py b/WebKitTools/Scripts/webkitpy/tool/steps/suggestreviewers.py
index d2f79f3..76bef35 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/suggestreviewers.py
@@ -30,41 +30,22 @@ from webkitpy.tool.steps.abstractstep import AbstractStep
 from webkitpy.tool.steps.options import Options
 
 
-class PostCodeReview(AbstractStep):
+class SuggestReviewers(AbstractStep):
     @classmethod
     def options(cls):
         return AbstractStep.options() + [
-            Options.cc,
-            Options.description,
+            Options.git_commit,
+            Options.suggest_reviewers,
         ]
 
     def run(self, state):
-        patch = state.get("patch")
-        bug_id = patch.bug_id()
-        title = patch.name()
+        if not self._options.suggest_reviewers:
+            return
 
-        # 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
-        # webkit-patch assumes one-patch-per-bug.
-        created_issue = self._tool.codereview.post(diff=self.cached_lookup(state, "diff"),
-                                                   message=message,
-                                                   codereview_issue=bug_id,
-                                                   cc=self._options.cc,
-                                                   patch_id=patch.id())
-
-        self._tool.bugs.set_flag_on_attachment(patch.id(), 'in-rietveld', '+')
+        reviewers = self._tool.checkout().suggested_reviewers(self._options.git_commit, self._changed_files(state))
+        print "The following reviewers have recently modified files in your patch:"
+        print "\n".join([reviewer.full_name for reviewer in reviewers])
+        if not self._tool.user.confirm("Would you like to CC them?"):
+            return
+        reviewer_emails = [reviewer.bugzilla_email() for reviewer in reviewers]
+        self._tool.bugs.add_cc_to_bug(state['bug_id'], reviewer_emails)
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/suggestreviewers_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/suggestreviewers_unittest.py
new file mode 100644
index 0000000..0c86535
--- /dev/null
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/suggestreviewers_unittest.py
@@ -0,0 +1,45 @@
+# Copyright (C) 2009 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.system.outputcapture import OutputCapture
+from webkitpy.tool.mocktool import MockOptions, MockTool
+from webkitpy.tool.steps.suggestreviewers import SuggestReviewers
+
+
+class SuggestReviewersTest(unittest.TestCase):
+    def test_disabled(self):
+        step = SuggestReviewers(MockTool(), MockOptions(suggest_reviewers=False))
+        OutputCapture().assert_outputs(self, step.run, [{}])
+
+    def test_basic(self):
+        capture = OutputCapture()
+        step = SuggestReviewers(MockTool(), MockOptions(suggest_reviewers=True, git_commit=None))
+        expected_stdout = "The following reviewers have recently modified files in your patch:\nFoo Bar\nWould you like to CC them?\n"
+        capture.assert_outputs(self, step.run, [{"bug_id": "123"}], expected_stdout=expected_stdout)