Bug 38021 - complete rietveld bugzilla integration
Summary: complete rietveld bugzilla integration
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Ojan Vafai
Depends on: 38162
  Show dependency treegraph
Reported: 2010-04-22 18:27 PDT by Ojan Vafai
Modified: 2010-08-19 13:49 PDT (History)
8 users (show)

See Also:

Patch-11001 (2.25 KB, patch)
2010-04-26 11:09 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
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]

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:
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]

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.