[webkit-reviews] review denied: [Bug 38918] add a FancyReviewQueue to upload r? patches to rietveld : [Attachment 55908] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 38918: add a FancyReviewQueue to upload r? patches to rietveld
https://bugs.webkit.org/show_bug.cgi?id=38918

Attachment 55908: Patch
https://bugs.webkit.org/attachment.cgi?id=55908&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
Why do we support uploading directly to rietveld?  It seems like we want to
remove that code in favor of always using the bot.

It's a little strange that we keep track of the "this patch is in rietveld"
state twice: once in a bugzilla flag and again in AppEngine.  What if these get
out of sync?  I think we only want to keep one copy of this information.

WebKitTools/Scripts/webkitpy/tool/commands/download.py:185
 +  # FIXME: Write a test for this code. Testing it is hard since it using
Rietveld's upload.py,
Huh?  We should just mock that out.

WebKitTools/Scripts/webkitpy/tool/commands/download.py:188
 +	name = "post-attachment-fancy"
We need a better name...  send-attachment-to-rietveld ?
post-code-review-for-attachment ?

WebKitTools/Scripts/webkitpy/tool/commands/download.py:190
 +	arguments_names = "[ATTACHMENTID]"
Why is this argument optional?

WebKitTools/Scripts/webkitpy/tool/commands/queues.py:375
 +	name = "fancy-review-queue"
Again, we need a better name here.  This name is stored persistently on the
server so we're stuck with it once we start.

WebKitTools/Scripts/webkitpy/tool/commands/queues.py:385
 +	    self.run_webkit_patch(["post-attachment-fancy", "--fancy-review",
"--force-clean", "--non-interactive", "--parent-command=style-queue",
patch.id()])
why is the style-queue the parent command?

WebKitTools/Scripts/webkitpy/tool/commands/queues.py:386
 +	    return True
Don't we need an error handler here?  What do you want to happen if we
encounter an error?

WebKitTools/Scripts/webkitpy/tool/commands/upload.py: 
 +  
Why did you remove this line?

WebKitTools/Scripts/webkitpy/tool/mocktool.py:509
 +  class MockRietveld(Rietveld):
We don't have the mocks inherit from the real objects.	It's too dangerous. 
The calls might leak through and do real things.

WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py:50
 +		self.tool.bugs.set_flag_on_attachment(patch.id, 'in-rietveld',
'+', 'Patch has been upload to rietveld')
Where does this text go?  Is it spammed in email?