Bug 38021

Summary: complete rietveld bugzilla integration
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: Tools / TestsAssignee: Ojan Vafai <ojan>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, atwilson, eric, jamesr, jparent, levin, slightlyoff, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 38162    
Bug Blocks:    
Attachments:
Description Flags
Patch-11001 none

Description Ojan Vafai 2010-04-22 18:27:33 PDT 
We've gotten all the feedback we're going to get without moving forward. So, I think we should take the next steps. Some options:

1. Make --fancy-review the default. Pro: People who have the chrome extension installed and start doing real reviews and find the bugs. Con: Upload takes longer
2. Move the extension code into bugzilla.
3. Make upload put the rietveld issue in the ChangeLog so we can upload to the same issue instead of creating a new one each time.

Ideally we'd start working on all three of these immediately. 1 is the simplest. I volunteer to do that assuming there are no objections. Once we've done 1, that will let us get a better sense of what the outstanding issues are with the upload/reitveld process. Sound OK?
Comment 1 Adam Barth 2010-04-23 12:30:03 PDT 
Sounds good to me.
Comment 2 Ojan Vafai 2010-04-26 11:09:44 PDT 
Created attachment 54319 [details]
Patch-11001
Comment 3 Eric Seidel (no email) 2010-04-26 14:02:23 PDT 
I'm not sure I like that this is going to put rietveld bug review links in every bug I upload.
Comment 4 Ojan Vafai 2010-04-26 14:10:45 PDT 
(In reply to comment #3)
> I'm not sure I like that this is going to put rietveld bug review links in
> every bug I upload.

All it does is append the rietveld issue id to the patch description. It only shows a rietveld review link if you have the chrome extension installed. It no longer appends a review link to the bug comments, ever.
Comment 5 Ojan Vafai 2010-04-26 14:11:20 PDT 
Note that I uploaded this patch using --fancy-review, so this bug is a good example of what will change (i.e. very little).
Comment 6 Eric Seidel (no email) 2010-04-26 14:16:09 PDT 
Comment on attachment 54319 [details]
Patch-11001

Woh, ok.

We probably need to change the description name slightly.
Comment 7 Ojan Vafai 2010-04-26 14:52:50 PDT 
Committed r58265: <http://trac.webkit.org/changeset/58265>
Comment 8 Ojan Vafai 2010-04-26 14:59:53 PDT 
Filed bug 38143 and bug 38144 for items 2 and 3 above.
Comment 9 Adam Barth 2010-04-26 20:36:15 PDT 
Reverted r58265 for reason:

This change prevents me from uploading patches

Committed r58282: <http://trac.webkit.org/changeset/58282>
Comment 10 WebKit Review Bot 2010-04-26 21:08:29 PDT 
http://trac.webkit.org/changeset/58282 might have broken SnowLeopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/58281
http://trac.webkit.org/changeset/58282
Comment 11 Ojan Vafai 2010-04-27 08:16:45 PDT 
(In reply to comment #9)
> This change prevents me from uploading patches

Ugh. Sorry this broke you. What was the error you were getting? Also, how did it break sheriffbot?
Comment 12 Eric Seidel (no email) 2010-05-02 19:38:50 PDT 
Attachment 54319 [details] was posted by a committer and has review+, assigning to Ojan Vafai for commit.
Comment 13 Ojan Vafai 2010-05-02 21:08:43 PDT 
Comment on attachment 54319 [details]
Patch-11001

This has been committed and rolled back.
Comment 14 Ojan Vafai 2010-08-19 13:49:08 PDT 
This is obsolete. The rietveld work is going on in different patches.