[webkit-dev] Review Queue

[Ojan Vafai]

It makes no sense to me to r- a patch because reviewers don't have time to
review it. It put incentive in the wrong place. There are other solutions to
this problem that put incentive in the right place (i.e. with reviewers). I
don't necessarily think these are good ideas, but I'll throw them out there.
1. If the review queue gets too large, close the tree to commits until the
queue is at 80% of that too large number.
2. Automate landing commits (e.g. click a button off the commit queue) so
that it's easier to land commits and keep the commit queue small.
3. Give some kind of very visible notice on the waterfall when the review
queue gets too large (e.g. make the background color of the whole page red).
4. Auto-assign reviews that haven't gotten reviewed after two weeks to a
reviewer and have that person be responsible for reviewing it in a timely
manner (this gives personal responsibility over the queue length instead of
the current distributed responsibility).

I'm sure there are a ton of other possible solutions to this problem that
don't punish people sending patches who have little control over this
situation. I imagine doing 3 and 4 would make a significant difference.

Ojan

On Fri, May 22, 2009 at 1:52 PM, Eric Seidel <eric at webkit.org> wrote:

> Sorry I missed on on IRC, I would have been happy to chat further.
>
>
> Our current defacto policy requires involvement on both sides.
> Submitters need to be involved in finding people to review their
> patches.  Posting patches to the review queue does not automatically
> get you a review, except occasionally by Darin Adler or myself.
>
> If a bug totally stalls, and is sitting in the review queue untouched
> I view that as the responsible reviewers' implicit rejection of the
> patch.  I, as a responsible reviewer, am simply making explicit that
> implicit rejection.  Personally, I'd rather get an r- on my patches
> than have them sit ignored for multiple weeks at a time.
>
> -eric
>
> http://webkit.org/coding/contributing.html:
> Patch review
> Once you have a patch file, it must be reviewed by one of the approved
> WebKit reviewers. To request a review, attach the patch to the bug
> report, and mark the patch with the flag review:?. The reviewer will
> typically either approve the patch (by responding with an r=me in the
> bug report or in e-mail and marking the patch review:+) or request
> revisions to the patch (and mark the patch review:-). In rare cases a
> patch may be permanently rejected, meaning that the reviewer believes
> the feature should never be committed to the tree. The review process
> can consist of multiple iterations between you and the reviewer as
> revisions are made to your patch.
>
> On Fri, May 22, 2009 at 1:22 PM, Maciej Stachowiak <mjs at apple.com> wrote:
> >
> > On May 21, 2009, at 7:57 PM, Eric Seidel wrote:
> >
> >> I think it's better to get things out of the queue then to leave them
> rot.
> >
> > But it's not the patch submitter's fault if the reviewers have been
> > delinquent in review. And making them resubmit the same patch after a
> > blanket r- is useless busywork. Per our policy you shouldn't mark a patch
> r-
> > unless you are either requesting revisions or rejecting the whole concept
> of
> > the patch <http://webkit.org/coding/contributing.html>. So if you r-'d
> any
> > patches without giving review comments, please restore them.
> >
> > Regards,
> > Maciej
> >
> >
> >>
> >>
> >> Your review are most welcome. :)
> >>
> >> -eric
> >>
> >> On Fri, May 22, 2009 at 12:48 PM, Maciej Stachowiak <mjs at apple.com>
> wrote:
> >>>
> >>> On May 21, 2009, at 7:27 PM, Eric Seidel wrote:
> >>>
> >>>> Our review process seems to be failing.  As a reviewer, let me extend
> >>>> my apologies to the WebKit community as I am part of this failure.
> >>>>
> >>>> We have over 100 patches in the review queue at the moment:
> >>>> http://webkit.org/pending-review
> >>>>
> >>>> I've started going through the list and reviewing what patches I can.
> >>>> I'm also marking r- all patches I can't review which have had no
> >>>> comments in the last 2 weeks.
> >>>
> >>> While it's great to get more reviews done (and I encourage other
> >>> reviewers
> >>> to get cracking as well), I don't think it's appropriate to r- patches
> >>> that
> >>> you don't know how to review, simply because no one else has reviewed
> >>> them
> >>> yet. It is better to have an honest backlog than to sweep things under
> >>> the
> >>> carpet.
> >>>
> >>> Regards,
> >>> Maciej
> >>>
> >>>
> >
> >
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20090522/cc134b7e/attachment-0001.html>