Bug 32542 - website: Add screenshots and clearer instructions on how to submit a patch for review
: website: Add screenshots and clearer instructions on how to submit a patch fo...
Status: RESOLVED FIXED
: WebKit
WebKit Website
: 528+ (Nightly build)
: All All
: P4 Minor
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-12-15 01:13 PST by
Modified: 2010-01-21 22:41 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2009-12-15 02:24:54 PST -------
Created an attachment (id=44853) [details]
Proposed patch
------- Comment #2 From 2009-12-15 02:51:01 PST -------
Created an attachment (id=44857) [details]
Proposed patch 2

Added border around image and adjusted next section about responding to reviewers.
------- Comment #3 From 2009-12-16 10:29:53 PST -------
(From update of attachment 44857 [details])
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 From 2009-12-16 18:24:55 PST -------
(In reply to comment #3)
> (From update of attachment 44857 [details] [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 From 2009-12-16 21:08:04 PST -------
Created an attachment (id=45034) [details]
Proposed patch 3
------- Comment #6 From 2009-12-16 21:14:14 PST -------
Created an attachment (id=45035) [details]
Proposed patch 4

Forgot to remove the final slash in the <img> tags.
------- Comment #7 From 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 From 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 From 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 From 2009-12-19 21:56:35 PST -------
*** Bug 32508 has been marked as a duplicate of this bug. ***
------- Comment #11 From 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 From 2010-01-21 22:41:29 PST -------
Manually committed r53679: http://trac.webkit.org/changeset/53679

Thanks for the review, David!