Bug 32542 – website: Add screenshots and clearer instructions on how to submit a patch for review

• Component: WebKit Website
• Status: RESOLVED FIXED

[Chris Jerdonek]

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.

[Chris Jerdonek]

Created attachment 44853 [details]
Proposed patch

[Chris Jerdonek]

Created attachment 44857 [details]
Proposed patch 2

Added border around image and adjusted next section about responding to reviewers.

[Darin Adler]

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

[Chris Jerdonek]

(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.

[Chris Jerdonek]

Created attachment 45034 [details]
Proposed patch 3

[Chris Jerdonek]

Created attachment 45035 [details]
Proposed patch 4

Forgot to remove the final slash in the <img> tags.

[David Levin]

> 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.

[Chris Jerdonek]

(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?

[David Levin]

> 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.

[Chris Jerdonek]

*** Bug 32508 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

(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.

[Chris Jerdonek]

Manually committed r53679: http://trac.webkit.org/changeset/53679

Thanks for the review, David!