[Webkit-unassigned] [Bug 37418] Add experimental prototype Rietveld integration to webkit-patch upload

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Apr 11 16:53:33 PDT 2010


https://bugs.webkit.org/show_bug.cgi?id=37418





--- Comment #6 from Chris Jerdonek <cjerdonek at webkit.org>  2010-04-11 16:53:33 PST ---
(In reply to comment #3)
> Created an attachment (id=53114)
 --> (https://bugs.webkit.org/attachment.cgi?id=53114) [details]
> Patch

Thanks for getting the ball rolling on this, Adam!  It's amazing you were able
to do this with so little code.

Here are some quick, random reactions rather than a review.  I haven't looked
much at the webkit-patch code before, so some (though not all) of these
comments may be somewhat speculative.

--- a/WebKitTools/Scripts/webkitpy/common/config/__init__.py
+++ b/WebKitTools/Scripts/webkitpy/common/config/__init__.py
@@ -1 +1,7 @@
 # Required for Python to search this directory for module files
+
+import re
+
+codereview_server_host = "codereview.appspot.com"
+codereview_server_regex = "https?://%s/" % re.sub('\.', '\\.',
codereview_server_host)
+codereview_server_url = "https://%s/" % codereview_server_host

Until we figure out how we want to use our __init__ files, I would recommend
leaving the file empty and putting this information in a separate file.  Also,
I've mentioned this before, but I have mixed feelings about putting settings
for a range of areas in a single config folder because it increases the number
of couplings.  I think it might be better, for example, to put Rietveld-related
settings in a rietveld/ folder.  To illustrate with another example, I think
putting style settings like these in the config folder would make the style
code harder to understand and maintain:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=57119#L71

Keeping the settings close to the code where they're used (but clearly
separated off) would I think be easier.  We can always expose parameters on the
API that allow values other than the settings to be passed in, for more
flexibility, etc.  That approach would also make it a bit simpler to drop new
functionality in and out.

b/WebKitTools/Scripts/webkitpy/common/net/rietveld.py
new file mode 100644

+from webkitpy.common.system.deprecated_logging import log
+from webkitpy.common.system.executive import ScriptError

Should we still be using deprecated_logging in new code?

b/WebKitTools/Scripts/webkitpy/thirdparty/rietveld/__init__.py
new file mode 100644
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at

Is the Apache license compatible with the WebKit project?  I once asked re:
Apache 1.1 on webkit-dev, but never got a response:

https://lists.webkit.org/pipermail/webkit-dev/2010-January/011461.html

diff --git a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
index a425e90..24b4ef7 100644
--- a/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
@@ -68,6 +68,7 @@ class AbstractQueueTest(CommandsTest):
     def _assert_run_webkit_patch(self, run_args):
         queue = TestQueue()
         tool = MockTool()
+        tool.executive = Mock()
         queue.bind_to_tool(tool)

See remarks below.

diff --git a/WebKitTools/Scripts/webkitpy/tool/main.py
b/WebKitTools/Scripts/webkitpy/tool/main.py
index 629e86f..dd7652d 100755
--- a/WebKitTools/Scripts/webkitpy/tool/main.py
+++ b/WebKitTools/Scripts/webkitpy/tool/main.py
@@ -36,6 +36,7 @@ from webkitpy.common.checkout.api import Checkout
 from webkitpy.common.checkout.scm import detect_scm_system
 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
@@ -70,6 +71,7 @@ class WebKitPatch(MultiCommandTool):
         self._scm = None
         self._checkout = None
         self.status_server = StatusServer()
+        self.codereview = Rietveld(self.executive)

Given that WebKitPatch is meant to be a generic all-purpose tool, I wonder if
there's a way to design things so the class doesn't itself have to be aware of
all its capabilities a priori (e.g. if there is some sort of generic
pass-through design where you can add more functionality to WebKitPatch without
having to change its class structure/attributes each time).

@@ -488,13 +490,18 @@ class MockStatusServer(object):
         return 191


+class MockExecute(Mock):
+    def run_and_throw_if_fail(self, args, quiet=False):
+        return "MOCK output of child process"
+
+
 class MockTool():

     def __init__(self, log_executive=False):
         self.wakeup_event = threading.Event()
         self.bugs = MockBugzilla()
         self.buildbot = MockBuildBot()
-        self.executive = Mock()
+        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._irc = None
@@ -503,6 +510,7 @@ class MockTool():
         self._checkout = MockCheckout()
         self.status_server = MockStatusServer()
         self.irc_password = "MOCK irc password"
+        self.codereview = Rietveld(self.executive)

Same remark here as above re: knowing about all its capabilities. 

Also, what's the rationale for using MockExecute() here but Mock() in
queues_unittest.py?  It seems like another approach could be to make MockTool
the "bare bones" object, and then add more complex values for the data
attributes as needed to test particular functionality.  With the
queues_unittest change above, it seems like the reverse might be happening,
where you are obscuring previously set attributes with simpler variants.  It
seems like keeping things simpler by default may result in fewer changes to
dependent tests going forward.

+class PostCodeReview(AbstractStep):
+    @classmethod
+    def options(cls):
+        return [
+            Options.cc,
+            Options.description,
+            Options.fancy_review,
+            Options.review,
+        ]
+
+    def run(self, state):
+        if not self._options.fancy_review:
+            return
+        if not self._options.review:
+            return
+        # FIXME: This will always be None because we don't retrieve the issue
+        #        number frmo the ChangeLog yet.
+        codereview_issue = state.get("codereview_issue")

Where is this string coming from -- is there a way to avoid having it
hard-coded in more than one spot?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list