Bug 32542 - website: Add screenshots and clearer instructions on how to submit a patch for review
Summary: website: Add screenshots and clearer instructions on how to submit a patch fo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Website (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P4 Minor
Assignee: Chris Jerdonek
URL:
Keywords:
: 32508 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-12-15 01:13 PST by Chris Jerdonek
Modified: 2010-01-21 22:41 PST (History)
5 users (show)

See Also:


Attachments
Proposed patch (67.54 KB, patch)
2009-12-15 02:24 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 2 (68.76 KB, patch)
2009-12-15 02:51 PST, Chris Jerdonek
darin: review-
Details | Formatted Diff | Diff
Proposed patch 3 (69.48 KB, patch)
2009-12-16 21:08 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 4 (69.48 KB, patch)
2009-12-16 21:14 PST, Chris Jerdonek
levin: review+
cjerdonek: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2009-12-15 01:13:41 PST
It was remarked in the following comment that there is often confusion among new contributors on how to mark a patch for review:

https://bugs.webkit.org/show_bug.cgi?id=25877#c2

This is a placeholder for a forthcoming patch.
Comment 1 Chris Jerdonek 2009-12-15 02:24:54 PST
Created attachment 44853 [details]
Proposed patch
Comment 2 Chris Jerdonek 2009-12-15 02:51:01 PST
Created attachment 44857 [details]
Proposed patch 2

Added border around image and adjusted next section about responding to reviewers.
Comment 3 Darin Adler 2009-12-16 10:29:53 PST
Comment on attachment 44857 [details]
Proposed patch 2

Thanks for taking this on.

> +<p>WebKit uses <a href="https://bugs.webkit.org/">Bugzilla</a>

I personally don't think that using the name Bugzilla so much is helpful. I know that's the source of the bug database, but I don't think it's the best name to refer to the WebKit bug database -- there are lots of Bugzilla installations. I would call it the WebKit bug database, not Bugzilla.

I feel the same way about our bug tool being named "bugzilla-tool".

> +Every contribution must have an associated bug report in Bugzilla.
> +This is true even if the contribution is not a bug fix.

I think this is wrong. Every contribution must have review. And bugs.webkit.org is the one of the best ways to get review. But there is no rule against having contributions go in without ever involving bugs.webkit.org as long as it is reviewed.

Using bugs.webkit.org is best practice, but not required.

> +<p><img src="images/bugzilla_add_attachment.png"
> +alt='Bugzilla "Add an attachment" link' /></p>

I believe these "/>" at the end of the "<img>" tags aren't needed or helpful.

> +<p>Checking the patch checkbox and marking your patch <tt>review:?</tt>
> +are necessary for getting your patch reviewed by a WebKit reviewer.
> +Setting the review flag notifies WebKit reviewers by sending them an
> +automatic e-mail through the
> +<a href="http://lists.webkit.org/mailman/listinfo/webkit-reviews">
> +webkit-reviews</a> mailing list.
> +</p>

This implies that all reviewers subscript to the webkit-reviews mailing list, but that is not true. Different reviewers have different ways of discovering patches that might be reviewed.

review- for now so you can consider some of my feedback
Comment 4 Chris Jerdonek 2009-12-16 18:24:55 PST
(In reply to comment #3)
> (From update of attachment 44857 [details])
> Thanks for taking this on.

No problem.  Thanks for reviewing.  The process of submitting a patch gives me a chance to probe and understand the process better.

> > +<p>WebKit uses <a href="https://bugs.webkit.org/">Bugzilla</a>
> 
> I personally don't think that using the name Bugzilla so much is helpful.

Great point -- I agree.  I'll even change the file names.

> > +Every contribution must have an associated bug report in Bugzilla.
> > +This is true even if the contribution is not a bug fix.
> 
> I think this is wrong. Every contribution must have review. And bugs.webkit.org
> is the one of the best ways to get review. But there is no rule against having
> contributions go in without ever involving bugs.webkit.org as long as it is
> reviewed.

That's interesting.  What are some of the other ways of getting a reviews?  And out of curiosity, what percent of reviews would you estimate are done using one of those other processes?

> I believe these "/>" at the end of the "<img>" tags aren't needed or helpful.

I didn't know that -- thanks.  I'm surprised it validated as HTML strict.
Comment 5 Chris Jerdonek 2009-12-16 21:08:04 PST
Created attachment 45034 [details]
Proposed patch 3
Comment 6 Chris Jerdonek 2009-12-16 21:14:14 PST
Created attachment 45035 [details]
Proposed patch 4

Forgot to remove the final slash in the <img> tags.
Comment 7 David Levin 2009-12-17 15:32:39 PST
> That's interesting.  What are some of the other ways of getting a reviews? 

In person reviews (if you happen to be near a WK reviewer), reviews using pastebin, lisppaste, etc.

> out of curiosity, what percent of reviews would you estimate are done using one
> of those other processes?

imo, these are much, much less frequent than bug db style review but they occur.
Comment 8 Chris Jerdonek 2009-12-17 18:05:23 PST
(In reply to comment #7)
> > That's interesting.  What are some of the other ways of getting a reviews? 
> 
> In person reviews (if you happen to be near a WK reviewer), reviews using
> pastebin, lisppaste, etc.

In particular, it's possible to commit patches that at most one other person could possibly have seen in advance (even if someone was constantly online monitoring for pending patches).  Is that correct?
Comment 9 David Levin 2009-12-17 18:17:34 PST
> In particular, it's possible to commit patches that at most one other person
> could possibly have seen in advance (even if someone was constantly online
> monitoring for pending patches).  Is that correct?

Yes.
Comment 10 Chris Jerdonek 2009-12-19 21:56:35 PST
*** Bug 32508 has been marked as a duplicate of this bug. ***
Comment 11 Eric Seidel (no email) 2009-12-21 00:27:14 PST
(In reply to comment #9)
> > In particular, it's possible to commit patches that at most one other person
> > could possibly have seen in advance (even if someone was constantly online
> > monitoring for pending patches).  Is that correct?
> 
> Yes.

We're trying to discourage this type of review however.  I think it's fine for our docs to discourage them.  I was in fact just discussing things related to this with Maciej in #webkit this evening.
Comment 12 Chris Jerdonek 2010-01-21 22:41:29 PST
Manually committed r53679: http://trac.webkit.org/changeset/53679

Thanks for the review, David!