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

Change-Id: Icd6db395c62285be384d137164d95d7466c98760

8 files changed, 130 insertions, 47 deletions

diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download.py b/WebKitTools/Scripts/webkitpy/tool/commands/download.py
index a283da9..a85b09a 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/download.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/download.py
@@ -182,6 +182,18 @@ class BuildAttachment(AbstractPatchSequencingCommand, ProcessAttachmentsMixin):
     ]
 
 
+class PostAttachmentToRietveld(AbstractPatchSequencingCommand, ProcessAttachmentsMixin):
+    name = "post-attachment-to-rietveld"
+    help_text = "Uploads a bugzilla attachment to rietveld"
+    arguments_names = "ATTACHMENTID"
+    main_steps = [
+        steps.CleanWorkingDirectory,
+        steps.Update,
+        steps.ApplyPatch,
+        steps.PostCodeReview,
+    ]
+
+
 class AbstractPatchApplyingCommand(AbstractPatchSequencingCommand):
     prepare_steps = [
         steps.EnsureLocalCommitIfNeeded,
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
index 08a4377..958620a 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
@@ -108,6 +108,10 @@ class DownloadCommandsTest(CommandsTest):
         expected_stderr = "Processing 1 patch from 1 bug.\nUpdating working directory\nProcessing patch 197 from bug 42.\nBuilding WebKit\n"
         self.assert_execute_outputs(BuildAttachment(), [197], options=self._default_options(), expected_stderr=expected_stderr)
 
+    def test_post_attachment_to_rietveld(self):
+        expected_stderr = "Processing 1 patch from 1 bug.\nUpdating working directory\nProcessing patch 197 from bug 42.\nMOCK: Uploading patch to rietveld\nMOCK setting flag 'in-rietveld' to '+' on attachment '197' with comment 'None' and additional comment 'None'\n"
+        self.assert_execute_outputs(PostAttachmentToRietveld(), [197], options=self._default_options(), expected_stderr=expected_stderr)
+
     def test_land_attachment(self):
         # FIXME: This expected result is imperfect, notice how it's seeing the same patch as still there after it thought it would have cleared the flags.
         expected_stderr = """Processing 1 patch from 1 bug.
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
index 27e09ba..67393d8 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
@@ -43,17 +43,23 @@ class EarlyWarningSytemTest(QueuesTest):
         string_replacemnts = {
             "name": ews.name,
             "checkout_dir": os.getcwd(),  # FIXME: Use of os.getcwd() is wrong, should be scm.checkout_root
+            "port": ews.port_name,
+            "watchers": ews.watchers,
         }
         expected_stderr = {
             "begin_work_queue": "CAUTION: %(name)s will discard all local changes in \"%(checkout_dir)s\"\nRunning WebKit %(name)s.\n" % string_replacemnts,
             "handle_unexpected_error": "Mock error message\n",
             "next_work_item": "MOCK: update_work_items: %(name)s [103]\n" % string_replacemnts,
             "process_work_item": "MOCK: update_status: %(name)s Pass\n" % string_replacemnts,
+            "handle_script_error": "MOCK: update_status: %(name)s ScriptError error message\nMOCK bug comment: bug_id=345, cc=%(watchers)s\n--- Begin comment ---\\Attachment 1234 did not build on %(port)s:\nBuild output: http://dummy_url\n--- End comment ---\n\n" % string_replacemnts,
         }
         return expected_stderr
 
     def _test_ews(self, ews):
-        self.assert_queue_outputs(ews, expected_stderr=self._default_expected_stderr(ews))
+        expected_exceptions = {
+            "handle_script_error": SystemExit,
+        }
+        self.assert_queue_outputs(ews, expected_stderr=self._default_expected_stderr(ews), expected_exceptions=expected_exceptions)
 
     # FIXME: If all EWSes are going to output the same text, we
     # could test them all in one method with a for loop over an array.
@@ -73,4 +79,7 @@ class EarlyWarningSytemTest(QueuesTest):
         ews = MacEWS()
         expected_stderr = self._default_expected_stderr(ews)
         expected_stderr["process_work_item"] = "MOCK: update_status: mac-ews Error: mac-ews cannot process patches from non-committers :(\n"
-        self.assert_queue_outputs(ews, expected_stderr=expected_stderr)
+        expected_exceptions = {
+            "handle_script_error": SystemExit,
+        }
+        self.assert_queue_outputs(ews, expected_stderr=expected_stderr, expected_exceptions=expected_exceptions)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
index 08bd3aa..d14ac9e 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues.py
@@ -121,7 +121,7 @@ class AbstractQueue(Command, QueueEngineDelegate):
 
     @classmethod
     def _update_status_for_script_error(cls, tool, state, script_error, is_error=False):
-        message = script_error.message
+        message = str(script_error)
         if is_error:
             message = "Error: %s" % message
         output = script_error.message_with_output(output_limit=1024*1024) # 1MB
@@ -289,7 +289,6 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
         self.committer_validator.reject_patch_from_commit_queue(patch.id(), message)
 
     # StepSequenceErrorHandler methods
-
     @staticmethod
     def _error_message_for_bug(tool, status_id, script_error):
         if not script_error.output:
@@ -304,6 +303,51 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
         validator.reject_patch_from_commit_queue(state["patch"].id(), cls._error_message_for_bug(tool, status_id, script_error))
 
 
+class RietveldUploadQueue(AbstractPatchQueue, StepSequenceErrorHandler):
+    name = "rietveld-upload-queue"
+
+    def __init__(self):
+        AbstractPatchQueue.__init__(self)
+
+    # AbstractPatchQueue methods
+
+    def next_work_item(self):
+        patch_id = self.tool.bugs.queries.fetch_first_patch_from_rietveld_queue()
+        if patch_id:
+            return patch_id
+        self._update_status("Empty queue")
+
+    def should_proceed_with_work_item(self, patch):
+        self._update_status("Uploading patch", patch)
+        return True
+
+    def process_work_item(self, patch):
+        try:
+            self.run_webkit_patch(["post-attachment-to-rietveld", "--force-clean", "--non-interactive", "--parent-command=rietveld-upload-queue", patch.id()])
+            self._did_pass(patch)
+            return True
+        except ScriptError, e:
+            if e.exit_code != QueueEngine.handled_error_code:
+                self._did_fail(patch)
+            raise e
+
+    @classmethod
+    def _reject_patch(cls, tool, patch_id):
+        tool.bugs.set_flag_on_attachment(patch_id, "in-rietveld", "-")
+
+    def handle_unexpected_error(self, patch, message):
+        log(message)
+        self._reject_patch(self.tool, patch.id())
+
+    # StepSequenceErrorHandler methods
+
+    @classmethod
+    def handle_script_error(cls, tool, state, script_error):
+        log(script_error.message_with_output())
+        cls._update_status_for_script_error(tool, state, script_error)
+        cls._reject_patch(tool, state["patch"].id())
+
+
 class AbstractReviewQueue(AbstractPatchQueue, PersistentPatchCollectionDelegate, StepSequenceErrorHandler):
     def __init__(self, options=None):
         AbstractPatchQueue.__init__(self, options)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
index a5d56da..b32dfa8 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
@@ -122,10 +122,13 @@ class CommitQueueTest(QueuesTest):
             # FIXME: The commit-queue warns about bad committers twice.  This is due to the fact that we access Attachment.reviewer() twice and it logs each time.
             "next_work_item" : """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
 Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
+MOCK setting flag 'commit-queue' to '-' on attachment '128' with comment 'Rejecting patch 128 from commit-queue.' and additional comment 'non-committer@example.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.\n\n- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.\n\n- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.'
 MOCK: update_work_items: commit-queue [106, 197]
 2 patches in commit-queue [106, 197]
 """,
             "process_work_item" : "MOCK: update_status: commit-queue Pass\n",
+            "handle_unexpected_error" : "MOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'Mock error message'\n",
+            "handle_script_error": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
         }
         self.assert_queue_outputs(CommitQueue(), expected_stderr=expected_stderr)
 
@@ -138,11 +141,14 @@ MOCK: update_work_items: commit-queue [106, 197]
             # FIXME: The commit-queue warns about bad committers twice.  This is due to the fact that we access Attachment.reviewer() twice and it logs each time.
             "next_work_item" : """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
 Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
+MOCK setting flag \'commit-queue\' to \'-\' on attachment \'128\' with comment \'Rejecting patch 128 from commit-queue.\' and additional comment \'non-committer@example.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.\n\n- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.\n\n- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.\'
 MOCK: update_work_items: commit-queue [106, 197]
 MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://build.webkit.org
 1 patch in commit-queue [106]
 """,
             "process_work_item" : "MOCK: update_status: commit-queue Builders [\"Builder2\"] are red. See http://build.webkit.org\n",
+            "handle_unexpected_error" : "MOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'Mock error message'\n",
+            "handle_script_error": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, expected_stderr=expected_stderr)
 
@@ -156,11 +162,14 @@ MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://buil
             # FIXME: The commit-queue warns about bad committers twice.  This is due to the fact that we access Attachment.reviewer() twice and it logs each time.
             "next_work_item": """Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
 Warning, attachment 128 on bug 42 has invalid committer (non-committer@example.com)
+MOCK setting flag \'commit-queue\' to \'-\' on attachment \'128\' with comment \'Rejecting patch 128 from commit-queue.\' and additional comment \'non-committer@example.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.\n\n- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.\n\n- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.\'
 MOCK: update_work_items: commit-queue [106, 197]
 MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://build.webkit.org
 1 patch in commit-queue [106]
 """,
             "process_work_item": "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 76543]\nMOCK: update_status: commit-queue Pass\n",
+            "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '76543' with comment 'Rejecting patch 76543 from commit-queue.' and additional comment 'Mock error message'\n",
+            "handle_script_error": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=rollout_patch, expected_stderr=expected_stderr)
 
@@ -193,6 +202,18 @@ MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://buil
         self.assertEqual(attachments, expected_sort)
 
 
+class RietveldUploadQueueTest(QueuesTest):
+    def test_rietveld_upload_queue(self):
+        expected_stderr = {
+            "begin_work_queue": "CAUTION: rietveld-upload-queue will discard all local changes in \"%s\"\nRunning WebKit rietveld-upload-queue.\n" % MockSCM.fake_checkout_root,
+            "should_proceed_with_work_item": "MOCK: update_status: rietveld-upload-queue Uploading patch\n",
+            "process_work_item": "MOCK: update_status: rietveld-upload-queue Pass\n",
+            "handle_unexpected_error": "Mock error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '1234' with comment 'None' and additional comment 'None'\n",
+            "handle_script_error": "ScriptError error message\nMOCK: update_status: rietveld-upload-queue ScriptError error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '1234' with comment 'None' and additional comment 'None'\n",
+        }
+        self.assert_queue_outputs(RietveldUploadQueue(), expected_stderr=expected_stderr)
+
+
 class StyleQueueTest(QueuesTest):
     def test_style_queue(self):
         expected_stderr = {
@@ -201,5 +222,9 @@ class StyleQueueTest(QueuesTest):
             "should_proceed_with_work_item": "MOCK: update_status: style-queue Checking style\n",
             "process_work_item" : "MOCK: update_status: style-queue Pass\n",
             "handle_unexpected_error" : "Mock error message\n",
+            "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=345, cc=[]\n--- Begin comment ---\\Attachment 1234 did not pass style-queue:\n\nScriptError error message\n\nIf any of these errors are false positives, please file a bug against check-webkit-style.\n--- End comment ---\n\n",
+        }
+        expected_exceptions = {
+            "handle_script_error": SystemExit,
         }
-        self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr)
+        self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr, expected_exceptions=expected_exceptions)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
index bf7e32a..9e17c5c 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
@@ -30,6 +30,7 @@ import unittest
 
 from webkitpy.common.net.bugzilla import Attachment
 from webkitpy.common.system.outputcapture import OutputCapture
+from webkitpy.common.system.executive import ScriptError
 from webkitpy.thirdparty.mock import Mock
 from webkitpy.tool.mocktool import MockTool
 
@@ -42,6 +43,14 @@ class MockQueueEngine(object):
         pass
 
 
+class MockPatch():
+    def id(self):
+        return 1234
+
+    def bug_id(self):
+        return 345
+
+
 class QueuesTest(unittest.TestCase):
     mock_work_item = Attachment({
         "id": 1234,
@@ -50,7 +59,19 @@ class QueuesTest(unittest.TestCase):
         "attacher_email": "adam@example.com",
     }, None)
 
-    def assert_queue_outputs(self, queue, args=None, work_item=None, expected_stdout=None, expected_stderr=None, options=Mock(), tool=MockTool()):
+    def assert_outputs(self, func, func_name, args, expected_stdout, expected_stderr, expected_exceptions):
+        exception = None
+        if expected_exceptions and func_name in expected_exceptions:
+            exception = expected_exceptions[func_name]
+
+        OutputCapture().assert_outputs(self,
+                func,
+                args=args,
+                expected_stdout=expected_stdout.get(func_name, ""),
+                expected_stderr=expected_stderr.get(func_name, ""),
+                expected_exception=exception)
+
+    def assert_queue_outputs(self, queue, args=None, work_item=None, expected_stdout=None, expected_stderr=None, expected_exceptions=None, options=Mock(), tool=MockTool()):
         if not expected_stdout:
             expected_stdout = {}
         if not expected_stderr:
@@ -63,38 +84,12 @@ class QueuesTest(unittest.TestCase):
 
         queue.execute(options, args, tool, engine=MockQueueEngine)
 
-        OutputCapture().assert_outputs(self,
-                queue.queue_log_path,
-                expected_stdout=expected_stdout.get("queue_log_path", ""),
-                expected_stderr=expected_stderr.get("queue_log_path", ""))
-        OutputCapture().assert_outputs(self,
-                queue.work_item_log_path,
-                args=[work_item],
-                expected_stdout=expected_stdout.get("work_item_log_path", ""),
-                expected_stderr=expected_stderr.get("work_item_log_path", ""))
-        OutputCapture().assert_outputs(self,
-                queue.begin_work_queue,
-                expected_stdout=expected_stdout.get("begin_work_queue", ""),
-                expected_stderr=expected_stderr.get("begin_work_queue", ""))
-        OutputCapture().assert_outputs(self,
-                queue.should_continue_work_queue,
-                expected_stdout=expected_stdout.get("should_continue_work_queue", ""), expected_stderr=expected_stderr.get("should_continue_work_queue", ""))
-        OutputCapture().assert_outputs(self,
-                queue.next_work_item,
-                expected_stdout=expected_stdout.get("next_work_item", ""),
-                expected_stderr=expected_stderr.get("next_work_item", ""))
-        OutputCapture().assert_outputs(self,
-                queue.should_proceed_with_work_item,
-                args=[work_item],
-                expected_stdout=expected_stdout.get("should_proceed_with_work_item", ""),
-                expected_stderr=expected_stderr.get("should_proceed_with_work_item", ""))
-        OutputCapture().assert_outputs(self,
-                queue.process_work_item,
-                args=[work_item],
-                expected_stdout=expected_stdout.get("process_work_item", ""),
-                expected_stderr=expected_stderr.get("process_work_item", ""))
-        OutputCapture().assert_outputs(self,
-                queue.handle_unexpected_error,
-                args=[work_item, "Mock error message"],
-                expected_stdout=expected_stdout.get("handle_unexpected_error", ""),
-                expected_stderr=expected_stderr.get("handle_unexpected_error", ""))
+        self.assert_outputs(queue.queue_log_path, "queue_log_path", [], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.work_item_log_path, "work_item_log_path", [work_item], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.begin_work_queue, "begin_work_queue", [], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.should_continue_work_queue, "should_continue_work_queue", [], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.next_work_item, "next_work_item", [], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.should_proceed_with_work_item, "should_proceed_with_work_item", [work_item], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.process_work_item, "process_work_item", [work_item], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.handle_unexpected_error, "handle_unexpected_error", [work_item, "Mock error message"], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.handle_script_error, "handle_script_error", [tool, {"patch": MockPatch()}, ScriptError(message="ScriptError error message", script_args="MockErrorCommand")], expected_stdout, expected_stderr, expected_exceptions)
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
index cf715b9..e682ca7 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload.py
@@ -171,7 +171,6 @@ class Post(AbstractPatchUploadingCommand):
     steps = [
         steps.CheckStyle,
         steps.ConfirmDiff,
-        steps.PostCodeReview,
         steps.ObsoletePatches,
         steps.PostDiff,
     ]
@@ -215,7 +214,6 @@ class Upload(AbstractPatchUploadingCommand):
         steps.PrepareChangeLog,
         steps.EditChangeLog,
         steps.ConfirmDiff,
-        steps.PostCodeReview,
         steps.ObsoletePatches,
         steps.PostDiff,
     ]
diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
index d52775b..8fef54a 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
@@ -56,8 +56,6 @@ class UploadCommandsTest(CommandsTest):
         options.request_commit = False
         options.review = True
         options.comment = None
-        # Rietveld upload code requires a real SCM checkout.
-        options.fancy_review = False
         options.cc = None
         expected_stderr = """Running check-webkit-style
 MOCK: user.open_url: file://...
@@ -87,8 +85,6 @@ MOCK: user.open_url: http://example.com/42
         options.request_commit = False
         options.review = True
         options.comment = None
-        # Rietveld upload code requires a real SCM checkout.
-        options.fancy_review = False
         options.cc = None
         expected_stderr = """Running check-webkit-style
 MOCK: user.open_url: file://...