Merge WebKit at r59636: Initial merge by git

Change-Id: I59b289c4e6b18425f06ce41cc9d34c522515de91

19 files changed, 288 insertions, 94 deletions

diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py b/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py
index 2c45ab6..ac7a760 100644
--- a/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py
+++ b/WebKitTools/Scripts/webkitpy/tool/bot/queueengine.py
@@ -77,8 +77,8 @@ class QueueEngine:
         self._output_tee = OutputTee()
 
     log_date_format = "%Y-%m-%d %H:%M:%S"
-    sleep_duration_text = "5 mins"
-    seconds_to_sleep = 300
+    sleep_duration_text = "2 mins"  # This could be generated from seconds_to_sleep
+    seconds_to_sleep = 120
     handled_error_code = 2
 
     # Child processes exit with a special code to the parent queue process can detect the error was handled.
@@ -142,8 +142,12 @@ class QueueEngine:
             self._output_tee.remove_log(self._work_log)
             self._work_log = None
 
+    def _now(self):
+        """Overriden by the unit tests to allow testing _sleep_message"""
+        return datetime.now()
+
     def _sleep_message(self, message):
-        wake_time = datetime.now() + timedelta(seconds=self.seconds_to_sleep)
+        wake_time = self._now() + timedelta(seconds=self.seconds_to_sleep)
         return "%s Sleeping until %s (%s)." % (message, wake_time.strftime(self.log_date_format), self.sleep_duration_text)
 
     def _sleep(self, message):
diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/queueengine_unittest.py b/WebKitTools/Scripts/webkitpy/tool/bot/queueengine_unittest.py
index 626181d..ec91bdb 100644
--- a/WebKitTools/Scripts/webkitpy/tool/bot/queueengine_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/bot/queueengine_unittest.py
@@ -26,6 +26,7 @@
 # (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 datetime
 import os
 import shutil
 import tempfile
@@ -160,6 +161,17 @@ class QueueEngineTest(unittest.TestCase):
         expected_callbacks.append('should_continue_work_queue')
         self.assertEquals(delegate._callbacks, expected_callbacks)
 
+    def test_now(self):
+        """Make sure there are no typos in the QueueEngine.now() method."""
+        engine = QueueEngine("test", None, None)
+        self.assertTrue(isinstance(engine._now(), datetime.datetime))
+
+    def test_sleep_message(self):
+        engine = QueueEngine("test", None, None)
+        engine._now = lambda: datetime.datetime(2010, 1, 1)
+        expected_sleep_message = "MESSAGE Sleeping until 2010-01-01 00:02:00 (2 mins)."
+        self.assertEqual(engine._sleep_message("MESSAGE"), expected_sleep_message)
+
     def setUp(self):
         self.temp_dir = tempfile.mkdtemp(suffix="work_queue_test_logs")
 
diff --git a/WebKitTools/Scripts/webkitpy/tool/bot/sheriff_unittest.py b/WebKitTools/Scripts/webkitpy/tool/bot/sheriff_unittest.py
index dd048a1..c375ff9 100644
--- a/WebKitTools/Scripts/webkitpy/tool/bot/sheriff_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/bot/sheriff_unittest.py
@@ -33,7 +33,7 @@ from webkitpy.common.net.buildbot import Builder
 from webkitpy.common.system.outputcapture import OutputCapture
 from webkitpy.thirdparty.mock import Mock
 from webkitpy.tool.bot.sheriff import Sheriff
-from webkitpy.tool.mocktool import MockTool, mock_builder
+from webkitpy.tool.mocktool import MockTool
 
 
 class MockSheriffBot(object):
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download.py b/WebKitTools/Scripts/webkitpy/tool/commands/download.py
index c66b95c..a283da9 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/download.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/download.py
@@ -260,8 +260,9 @@ class AbstractRolloutPrepCommand(AbstractSequencedCommand):
             #       of create-rollout for bug URLs.  It should do better
             #       parsing instead.
             log("Preparing rollout for bug %s." % commit_info.bug_id())
-            return commit_info
-        log("Unable to parse bug number from diff.")
+        else:
+            log("Unable to parse bug number from diff.")
+        return commit_info
 
     def _prepare_state(self, options, args, tool):
         revision = args[0]
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
index 926037c..4dd9d7f 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
@@ -26,9 +26,32 @@
 # (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.thirdparty.mock import Mock
 from webkitpy.tool.commands.commandtest import CommandsTest
 from webkitpy.tool.commands.download import *
+from webkitpy.tool.mocktool import MockTool
+
+
+class AbstractRolloutPrepCommandTest(unittest.TestCase):
+    def test_commit_info(self):
+        command = AbstractRolloutPrepCommand()
+        tool = MockTool()
+        command.bind_to_tool(tool)
+        output = OutputCapture()
+
+        expected_stderr = "Preparing rollout for bug 42.\n"
+        commit_info = output.assert_outputs(self, command._commit_info, [1234], expected_stderr=expected_stderr)
+        self.assertTrue(commit_info)
+
+        mock_commit_info = Mock()
+        mock_commit_info.bug_id = lambda: None
+        tool._checkout.commit_info_for_revision = lambda revision: mock_commit_info
+        expected_stderr = "Unable to parse bug number from diff.\n"
+        commit_info = output.assert_outputs(self, command._commit_info, [1234], expected_stderr=expected_stderr)
+        self.assertEqual(commit_info, mock_commit_info)
 
 
 class DownloadCommandsTest(CommandsTest):
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
index 4d23a4c..3d0ddd1 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
@@ -39,37 +39,37 @@ class EarlyWarningSytemTest(QueuesTest):
         ews._can_build = lambda: True
         ews.review_patch(Mock())
 
-    def test_chromium_linux_ews(self):
+    def _default_expected_stderr(self, ews):
+        string_replacemnts = {
+            "name": ews.name,
+            "checkout_dir": os.getcwd(),  # FIXME: Use of os.getcwd() is wrong, should be scm.checkout_root
+        }
         expected_stderr = {
-            "begin_work_queue": "CAUTION: chromium-ews will discard all local changes in \"%s\"\nRunning WebKit chromium-ews.\n" % os.getcwd(),
+            "begin_work_queue": "CAUTION: %(name)s will discard all local changes in \"%(checkout_dir)s\"\nRunning WebKit %(name)s.\n" % string_replacemnts,
             "handle_unexpected_error": "Mock error message\n",
+            "process_work_item": "MOCK: update_status: %(name)s Pass\n" % string_replacemnts,
         }
-        self.assert_queue_outputs(ChromiumLinuxEWS(), expected_stderr=expected_stderr)
+        return expected_stderr
+
+    def _test_ews(self, ews):
+        self.assert_queue_outputs(ews, expected_stderr=self._default_expected_stderr(ews))
+
+    # 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):
+        self._test_ews(ChromiumLinuxEWS())
 
     def test_chromium_windows_ews(self):
-        expected_stderr = {
-            "begin_work_queue": "CAUTION: cr-win-ews will discard all local changes in \"%s\"\nRunning WebKit cr-win-ews.\n" % os.getcwd(),
-            "handle_unexpected_error": "Mock error message\n",
-        }
-        self.assert_queue_outputs(ChromiumWindowsEWS(), expected_stderr=expected_stderr)
+        self._test_ews(ChromiumWindowsEWS())
 
     def test_qt_ews(self):
-        expected_stderr = {
-            "begin_work_queue": "CAUTION: qt-ews will discard all local changes in \"%s\"\nRunning WebKit qt-ews.\n" % os.getcwd(),
-            "handle_unexpected_error": "Mock error message\n",
-        }
-        self.assert_queue_outputs(QtEWS(), expected_stderr=expected_stderr)
+        self._test_ews(QtEWS())
 
     def test_gtk_ews(self):
-        expected_stderr = {
-            "begin_work_queue": "CAUTION: gtk-ews will discard all local changes in \"%s\"\nRunning WebKit gtk-ews.\n" % os.getcwd(),
-            "handle_unexpected_error": "Mock error message\n",
-        }
-        self.assert_queue_outputs(GtkEWS(), expected_stderr=expected_stderr)
+        self._test_ews(GtkEWS())
 
     def test_mac_ews(self):
-        expected_stderr = {
-            "begin_work_queue": "CAUTION: mac-ews will discard all local changes in \"%s\"\nRunning WebKit mac-ews.\n" % os.getcwd(),
-            "handle_unexpected_error": "Mock error message\n",
-        }
-        self.assert_queue_outputs(MacEWS(), expected_stderr=expected_stderr)
+        ews = MacEWS()
+        expected_stderr = self._default_expected_stderr(ews)
+        expected_stderr["process_work_item"] = "MOCK: update_status: mac-ews Error: mac-ews cannot process patches from non-committers :(\n"
+        self.assert_queue_outputs(ews, expected_stderr=expected_stderr)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
index 775aa44..78ca729 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
@@ -46,7 +46,6 @@ from webkitpy.tool.multicommandtool import Command
 
 class AbstractQueue(Command, QueueEngineDelegate):
     watchers = [
-        "webkit-bot-watchers@googlegroups.com",
     ]
 
     _pass_status = "Pass"
@@ -168,16 +167,17 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
         # Not using BugzillaQueries.fetch_patches_from_commit_queue() so we can reject patches with invalid committers/reviewers.
         bug_ids = self.tool.bugs.queries.fetch_bug_ids_from_commit_queue()
         all_patches = sum([self.tool.bugs.fetch_bug(bug_id).commit_queued_patches(include_invalid=True) for bug_id in bug_ids], [])
-        valid_patches = self.committer_validator.patches_after_rejecting_invalid_commiters_and_reviewers(all_patches)
-        if not self._builders_are_green():
-            return filter(lambda patch: patch.is_rollout(), valid_patches)
-        return valid_patches
+        return self.committer_validator.patches_after_rejecting_invalid_commiters_and_reviewers(all_patches)
 
     def next_work_item(self):
         patches = self._validate_patches_in_commit_queue()
+        builders_are_green = self._builders_are_green()
+        if not builders_are_green:
+            patches = filter(lambda patch: patch.is_rollout(), patches)
         # FIXME: We could sort the patches in a specific order here, was suggested by https://bugs.webkit.org/show_bug.cgi?id=33395
         if not patches:
-            self._update_status("Empty queue")
+            queue_text = "queue" if builders_are_green else "rollout queue"
+            self._update_status("Empty %s" % queue_text)
             return None
         # Only bother logging if we have patches in the queue.
         self.log_progress([patch.id() for patch in patches])
@@ -211,7 +211,8 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
         if not patch.is_rollout():
             if not self._builders_are_green():
                 return False
-        self._update_status("Landing patch", patch)
+        patch_text = "rollout patch" if patch.is_rollout() else "patch"
+        self._update_status("Landing %s" % patch_text, patch)
         return True
 
     def _land(self, patch, first_run=False):
@@ -226,7 +227,6 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
                 "land-attachment",
                 "--force-clean",
                 "--build",
-                "--test",
                 "--non-interactive",
                 # The master process is responsible for checking the status
                 # of the builders (see above call to _builders_are_green).
@@ -235,6 +235,9 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
                 "--quiet",
                 patch.id()
             ]
+            # We don't bother to run tests for rollouts as that makes them too slow.
+            if not patch.is_rollout():
+                args.append("--test")
             if not first_run:
                 # The first time through, we don't reject the patch from the
                 # commit queue because we want to make sure we can build and
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
index 16eb053..0bd42fb 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
@@ -118,11 +118,13 @@ class CommitQueueTest(QueuesTest):
     def test_commit_queue(self):
         expected_stderr = {
             "begin_work_queue" : "CAUTION: commit-queue will discard all local changes in \"%s\"\nRunning WebKit commit-queue.\n" % MockSCM.fake_checkout_root,
+            "should_proceed_with_work_item": "MOCK: update_status: commit-queue Landing patch\n",
             # FIXME: The commit-queue warns about bad committers twice.  This is due to the fact that we access Attachment.reviewer() twice and it logs each time.
             "next_work_item" : """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
 Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
 2 patches in commit-queue [197, 106]
 """,
+            "process_work_item" : "MOCK: update_status: commit-queue Pass\n",
         }
         self.assert_queue_outputs(CommitQueue(), expected_stderr=expected_stderr)
 
@@ -131,11 +133,14 @@ Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.c
         tool.buildbot.light_tree_on_fire()
         expected_stderr = {
             "begin_work_queue" : "CAUTION: commit-queue will discard all local changes in \"%s\"\nRunning WebKit commit-queue.\n" % MockSCM.fake_checkout_root,
+            "should_proceed_with_work_item": "MOCK: update_status: commit-queue Builders [\"Builder2\"] are red. See http://build.webkit.org\n",
             # FIXME: The commit-queue warns about bad committers twice.  This is due to the fact that we access Attachment.reviewer() twice and it logs each time.
             "next_work_item" : """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
 Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
+MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://build.webkit.org
 1 patch in commit-queue [106]
 """,
+            "process_work_item" : "MOCK: update_status: commit-queue Builders [\"Builder2\"] are red. See http://build.webkit.org\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, expected_stderr=expected_stderr)
 
@@ -145,20 +150,33 @@ Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.c
         rollout_patch = MockPatch()
         expected_stderr = {
             "begin_work_queue": "CAUTION: commit-queue will discard all local changes in \"%s\"\nRunning WebKit commit-queue.\n" % MockSCM.fake_checkout_root,
+            "should_proceed_with_work_item": "MOCK: update_status: commit-queue Landing rollout patch\n",
             # FIXME: The commit-queue warns about bad committers twice.  This is due to the fact that we access Attachment.reviewer() twice and it logs each time.
             "next_work_item": """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
 Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
+MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://build.webkit.org
 1 patch in commit-queue [106]
 """,
-            "process_work_item": "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 76543]\n",
+            "process_work_item": "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 76543]\nMOCK: update_status: commit-queue Pass\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=rollout_patch, expected_stderr=expected_stderr)
 
+    def test_can_build_and_test(self):
+        queue = CommitQueue()
+        tool = MockTool()
+        tool.executive = Mock()
+        queue.bind_to_tool(tool)
+        self.assertTrue(queue._can_build_and_test())
+        expected_run_args = ["echo", "--status-host=example.com", "build-and-test", "--force-clean", "--build", "--test", "--non-interactive", "--no-update", "--build-style=both", "--quiet"]
+        tool.executive.run_and_throw_if_fail.assert_called_with(expected_run_args)
+
 
 class StyleQueueTest(QueuesTest):
     def test_style_queue(self):
         expected_stderr = {
             "begin_work_queue" : "CAUTION: style-queue will discard all local changes in \"%s\"\nRunning WebKit style-queue.\n" % MockSCM.fake_checkout_root,
+            "should_proceed_with_work_item": "MOCK: update_status: style-queue Checking style\n",
+            "process_work_item" : "MOCK: update_status: style-queue Pass\n",
             "handle_unexpected_error" : "Mock error message\n",
         }
         self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py
index eb80d8f..24c8517 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py
@@ -57,17 +57,46 @@ class SheriffBot(AbstractQueue, StepSequenceErrorHandler):
     def work_item_log_path(self, new_failures):
         return os.path.join("%s-logs" % self.name, "%s.log" % new_failures.keys()[0])
 
-    def next_work_item(self):
-        self._irc_bot.process_pending_messages()
-        self._update()
+    def _new_failures(self, revisions_causing_failures, old_failing_svn_revisions):
+        # We ignore failures that might have been caused by svn_revisions that
+        # we've already complained about.  This is conservative in the sense
+        # that we might be ignoring some new failures, but our experience has
+        # been that skipping this check causes a lot of spam for builders that
+        # take a long time to cycle.
+        old_failing_builder_names = []
+        for svn_revision in old_failing_svn_revisions:
+            old_failing_builder_names.extend(
+                [builder.name() for builder in revisions_causing_failures[svn_revision]])
+
         new_failures = {}
-        revisions_causing_failures = self.tool.buildbot.revisions_causing_failures()
         for svn_revision, builders in revisions_causing_failures.items():
-            if self.tool.status_server.svn_revision(svn_revision):
+            if svn_revision in old_failing_svn_revisions:
                 # FIXME: We should re-process the work item after some time delay.
                 # https://bugs.webkit.org/show_bug.cgi?id=36581
                 continue
-            new_failures[svn_revision] = builders
+            new_builders = [builder for builder in builders
+                            if builder.name() not in old_failing_builder_names]
+            if new_builders:
+                new_failures[svn_revision] = new_builders
+
+        return new_failures
+
+    def next_work_item(self):
+        self._irc_bot.process_pending_messages()
+        self._update()
+
+        # We do one read from buildbot to ensure a consistent view.
+        revisions_causing_failures = self.tool.buildbot.revisions_causing_failures()
+
+        # Similarly, we read once from our the status_server.
+        old_failing_svn_revisions = []
+        for svn_revision in revisions_causing_failures.keys():
+            if self.tool.status_server.svn_revision(svn_revision):
+                old_failing_svn_revisions.append(svn_revision)
+
+        new_failures = self._new_failures(revisions_causing_failures,
+                                          old_failing_svn_revisions)
+
         self._sheriff.provoke_flaky_builders(revisions_causing_failures)
         return new_failures
 
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py
index f121eda..4b4b8b6 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py
@@ -30,18 +30,44 @@ import os
 
 from webkitpy.tool.commands.queuestest import QueuesTest
 from webkitpy.tool.commands.sheriffbot import SheriffBot
-from webkitpy.tool.mocktool import mock_builder
+from webkitpy.tool.mocktool import MockBuilder
 
 
 class SheriffBotTest(QueuesTest):
+    builder1 = MockBuilder("Builder1")
+    builder2 = MockBuilder("Builder2")
+
     def test_sheriff_bot(self):
         mock_work_item = {
-            29837: [mock_builder],
+            29837: [self.builder1],
         }
         expected_stderr = {
             "begin_work_queue": "CAUTION: sheriff-bot will discard all local changes in \"%s\"\nRunning WebKit sheriff-bot.\n" % os.getcwd(),
             "next_work_item": "",
-            "process_work_item": "MOCK: irc.post: abarth, darin, eseidel: http://trac.webkit.org/changeset/29837 might have broken Mock builder name (Tests)\nMOCK bug comment: bug_id=42, cc=['webkit-bot-watchers@googlegroups.com', 'abarth@webkit.org', 'eric@webkit.org']\n--- Begin comment ---\\http://trac.webkit.org/changeset/29837 might have broken Mock builder name (Tests)\n--- End comment ---\n\n",
+            "process_work_item": "MOCK: irc.post: abarth, darin, eseidel: http://trac.webkit.org/changeset/29837 might have broken Builder1\nMOCK bug comment: bug_id=42, cc=['abarth@webkit.org', 'eric@webkit.org']\n--- Begin comment ---\\http://trac.webkit.org/changeset/29837 might have broken Builder1\n--- End comment ---\n\n",
             "handle_unexpected_error": "Mock error message\n"
         }
         self.assert_queue_outputs(SheriffBot(), work_item=mock_work_item, expected_stderr=expected_stderr)
+
+    revisions_causing_failures = {
+        1234: [builder1],
+        1235: [builder1, builder2],
+    }
+
+    def test_new_failures(self):
+        old_failing_svn_revisions = []
+        self.assertEquals(SheriffBot()._new_failures(self.revisions_causing_failures,
+                                                     old_failing_svn_revisions),
+                          self.revisions_causing_failures)
+
+    def test_new_failures_with_old_revisions(self):
+        old_failing_svn_revisions = [1234]
+        self.assertEquals(SheriffBot()._new_failures(self.revisions_causing_failures,
+                                                     old_failing_svn_revisions),
+                          {1235: [builder2]})
+
+    def test_new_failures_with_old_revisions(self):
+        old_failing_svn_revisions = [1235]
+        self.assertEquals(SheriffBot()._new_failures(self.revisions_causing_failures,
+                                                     old_failing_svn_revisions),
+                          {})
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
index 99d45a6..4797ef6 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
@@ -46,14 +46,23 @@ from webkitpy.tool.comments import bug_comment_from_svn_revision
 from webkitpy.tool.multicommandtool import AbstractDeclarativeCommand
 from webkitpy.common.system.deprecated_logging import error, log
 
+
 class CommitMessageForCurrentDiff(AbstractDeclarativeCommand):
     name = "commit-message"
     help_text = "Print a commit message suitable for the uncommitted changes"
 
+    def __init__(self):
+        options = [
+            steps.Options.squash,
+            steps.Options.git_commit,
+        ]
+        AbstractDeclarativeCommand.__init__(self, options=options)
+
     def execute(self, options, args, tool):
         # This command is a useful test to make sure commit_message_for_this_commit
         # always returns the right value regardless of the current working directory.
-        print "%s" % tool.checkout().commit_message_for_this_commit().message()
+        print "%s" % tool.checkout().commit_message_for_this_commit(options.git_commit, options.squash).message()
+
 
 class CleanPendingCommit(AbstractDeclarativeCommand):
     name = "clean-pending-commit"
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
index eec3751..8f6483a 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
@@ -34,10 +34,7 @@ from webkitpy.tool.mocktool import MockTool
 class UploadCommandsTest(CommandsTest):
     def test_commit_message_for_current_diff(self):
         tool = MockTool()
-        mock_commit_message_for_this_commit = Mock()
-        mock_commit_message_for_this_commit.message = lambda: "Mock message"
-        tool._checkout.commit_message_for_this_commit = lambda: mock_commit_message_for_this_commit
-        expected_stdout = "Mock message\n"
+        expected_stdout = "This is a fake commit message that is at least 50 characters.\n"
         self.assert_execute_outputs(CommitMessageForCurrentDiff(), [], expected_stdout=expected_stdout, tool=tool)
 
     def test_clean_pending_commit(self):
diff --git a/WebKitTools/Scripts/webkitpy/tool/mocktool.py b/WebKitTools/Scripts/webkitpy/tool/mocktool.py
index 128362a..47fff1b 100644
--- a/WebKitTools/Scripts/webkitpy/tool/mocktool.py
+++ b/WebKitTools/Scripts/webkitpy/tool/mocktool.py
@@ -184,15 +184,6 @@ _bug4 = {
 }
 
 
-class MockBuilder(object):
-
-    def name(self):
-        return "Mock builder name (Tests)"
-
-
-mock_builder = MockBuilder()
-
-
 class MockBugzillaQueries(Mock):
 
     def __init__(self, bugzilla):
@@ -319,6 +310,9 @@ class MockBuilder(object):
     def __init__(self, name):
         self._name = name
 
+    def name(self):
+        return self._name
+
     def force_build(self, username, comments):
         log("MOCK: force_build: name=%s, username=%s, comments=%s" % (
             self._name, username, comments))
@@ -369,7 +363,7 @@ class MockBuildBot(object):
 
     def revisions_causing_failures(self):
         return {
-            "29837": [mock_builder]
+            "29837": [self.builder_with_name("Builder1")],
         }
 
 
@@ -484,6 +478,7 @@ class MockStatusServer(object):
         return None
 
     def update_status(self, queue_name, status, patch=None, results_file=None):
+        log("MOCK: update_status: %s %s" % (queue_name, status))
         return 187
 
     def update_svn_revision(self, svn_revision, broken_bot):
@@ -491,7 +486,12 @@ class MockStatusServer(object):
 
 
 class MockExecute(Mock):
+    def __init__(self, should_log):
+        self._should_log = should_log
+
     def run_and_throw_if_fail(self, args, quiet=False):
+        if self._should_log:
+            log("MOCK run_and_throw_if_fail: %s" % args)
         return "MOCK output of child process"
 
     def run_command(self,
@@ -502,6 +502,8 @@ class MockExecute(Mock):
                     return_exit_code=False,
                     return_stderr=True,
                     decode_output=False):
+        if self._should_log:
+            log("MOCK run_command: %s" % args)
         return "MOCK output of child process"
 
 
@@ -511,9 +513,7 @@ class MockTool():
         self.wakeup_event = threading.Event()
         self.bugs = MockBugzilla()
         self.buildbot = MockBuildBot()
-        self.executive = MockExecute()
-        if log_executive:
-            self.executive.run_and_throw_if_fail = lambda args: log("MOCK run_and_throw_if_fail: %s" % args)
+        self.executive = MockExecute(should_log=log_executive)
         self._irc = None
         self.user = MockUser()
         self._scm = MockSCM()
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle.py b/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle.py
index 7b2be99..93e6215 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle.py
@@ -48,16 +48,17 @@ class CheckStyle(AbstractStep):
         if not self._options.check_style:
             return
         os.chdir(self._tool.scm().checkout_root)
-        try:
-            args = []
-            if self._options.git_commit:
-                args.append("--git-commit")
-                args.append(self._options.git_commit)
-            if self._tool.scm().should_squash(self._options.squash):
-                args.append("--squash")
-            else:
-                args.append("--no-squash")
 
+        args = []
+        if self._options.git_commit:
+            args.append("--git-commit")
+            args.append(self._options.git_commit)
+        if self._tool.scm().should_squash(self._options.squash):
+            args.append("--squash")
+        else:
+            args.append("--no-squash")
+
+        try:
             self._run_script("check-webkit-style", args)
         except ScriptError, e:
             if self._options.non_interactive:
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle_unittest.py
new file mode 100644
index 0000000..a23ea1b
--- /dev/null
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/checkstyle_unittest.py
@@ -0,0 +1,46 @@
+# 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.executive import ScriptError
+from webkitpy.thirdparty.mock import Mock
+from webkitpy.tool.mocktool import MockTool
+from webkitpy.tool.steps.checkstyle import CheckStyle
+
+
+class CheckStyleTest(unittest.TestCase):
+    def test_should_squash_error(self):
+        """should_squash can throw an error. That error should not be eaten by CheckStyle."""
+        def should_squash(squash):
+            raise ScriptError(message="Dummy error")
+
+        tool = MockTool()
+        tool._scm.should_squash = should_squash
+        step = CheckStyle(tool, Mock())
+        self.assertRaises(ScriptError, step.run, [])
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py b/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py
index 198cfce..8397519 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py
@@ -42,33 +42,32 @@ class PostCodeReview(AbstractStep):
     def run(self, state):
         if not self._options.fancy_review:
             return
-        # FIXME: This will always be None because we don't retrieve the issue
-        #        number from the ChangeLog yet.
-        codereview_issue = state.get("codereview_issue")
+
+        bug_id = state.get("bug_id")
+        if not bug_id:
+            raise ScriptError(message="Cannot upload a fancy review without a bug ID.")
+
         message = self._options.description
         if not message:
             # If we have an issue number, then the message becomes the label
             # of the new patch. Otherwise, it becomes the title of the whole
             # issue.
-            if codereview_issue:
-                message = "Updated patch"
-            elif state.get("bug_title"):
+            if state.get("bug_title"):
                 # This is the common case for the the first "upload" command.
                 message = state.get("bug_title")
-            elif state.get("bug_id"):
+            elif bug_id:
                 # This is the common case for the "post" command and
-                # subsequent runs of the "upload" command.  In this case,
-                # I'd rather add the new patch to the existing issue, but
-                # that's not implemented yet.
-                message = "Code review for %s" % self._tool.bugs.bug_url_for_bug_id(state["bug_id"])
+                # 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=codereview_issue,
+                                                   codereview_issue=bug_id,
                                                    cc=self._options.cc)
-        if created_issue:
-            # FIXME: Record the issue number in the ChangeLog.
-            state["codereview_issue"] = created_issue
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/postdiff.py b/WebKitTools/Scripts/webkitpy/tool/steps/postdiff.py
index a542dba..79739cd 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/postdiff.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/postdiff.py
@@ -44,11 +44,6 @@ class PostDiff(AbstractStep):
         diff = self.cached_lookup(state, "diff")
         description = self._options.description or "Patch"
         comment_text = None
-        codereview_issue = state.get("codereview_issue")
-        # Include codereview issue number in patch name. This is a bit of a hack,
-        # but it makes doing the rietveld integration a lot easier.
-        if codereview_issue:
-            description += "-%s" % state["codereview_issue"]
         self._tool.bugs.add_patch_to_bug(state["bug_id"], diff, description, comment_text=comment_text, mark_for_review=self._options.review, mark_for_commit_queue=self._options.request_commit)
         if self._options.open_bug:
             self._tool.user.open_url(self._tool.bugs.bug_url_for_bug_id(state["bug_id"]))
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py b/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py
index b1c2d3b..0734d8f 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/runtests.py
@@ -57,6 +57,11 @@ class RunTests(AbstractStep):
         if self._options.non_interactive:
             args.append("--no-launch-safari")
             args.append("--exit-after-n-failures=1")
+            # FIXME: Hack to work around https://bugs.webkit.org/show_bug.cgi?id=38912
+            # when running the commit-queue on a mac leopard machine.
+            if self.port().name() == "Mac" and self.port().is_leopard():
+                args.extend(["--ignore-tests", "compositing/iframes"])
+
         if self._options.quiet:
             args.append("--quiet")
         self._tool.executive.run_and_throw_if_fail(args)
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py b/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py
index 5abfc6d..eee183b 100644
--- a/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py
@@ -29,9 +29,11 @@
 import unittest
 
 from webkitpy.common.system.outputcapture import OutputCapture
+from webkitpy.common.config.ports import WebKitPort
 from webkitpy.thirdparty.mock import Mock
 from webkitpy.tool.mocktool import MockTool
 from webkitpy.tool.steps.update import Update
+from webkitpy.tool.steps.runtests import RunTests
 from webkitpy.tool.steps.promptforbugortitle import PromptForBugOrTitle
 
 
@@ -55,3 +57,27 @@ class StepsTest(unittest.TestCase):
         tool = MockTool()
         tool.user.prompt = lambda message: 42
         self._run_step(PromptForBugOrTitle, tool=tool)
+
+    def test_runtests_leopard_commit_queue_hack(self):
+        expected_stderr = "Running Python unit tests\nRunning Perl unit tests\nRunning JavaScriptCore tests\nRunning run-webkit-tests\n"
+        OutputCapture().assert_outputs(self, self._run_step, [RunTests], expected_stderr=expected_stderr)
+
+    def test_runtests_leopard_commit_queue_hack(self):
+        mock_options = Mock()
+        mock_options.non_interactive = True
+        step = RunTests(MockTool(log_executive=True), mock_options)
+        # FIXME: We shouldn't use a real port-object here, but there is too much to mock at the moment.
+        mock_port = WebKitPort()
+        mock_port.name = lambda: "Mac"
+        mock_port.is_leopard = lambda: True
+        step.port = lambda: mock_port
+        expected_stderr = """Running Python unit tests
+MOCK run_and_throw_if_fail: ['WebKitTools/Scripts/test-webkitpy']
+Running Perl unit tests
+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', '--ignore-tests', 'compositing/iframes', '--quiet']
+"""
+        OutputCapture().assert_outputs(self, step.run, [{}], expected_stderr=expected_stderr)