[webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]

[Ryosuke Niwa]

On Fri, Nov 16, 2012 at 7:16 AM, <noam.rosenthal at nokia.com> wrote:

>
> From: ext Ryosuke Niwa <rniwa at webkit.org>
> > r+ and r- flags are supposed to be set only by reviewers. If you wanted
> to withdraw your patch from the review queue, then you should be clearing
>  r? flag, instead of setting r-. If you’re uploading a WIP patch, then it
> should not bear either r?, r-, or r+
> > flags. You can accomplish this by either not setting the flag when you
> upload a patch on Bugzilla, clearing flag on the Bugzilla, or using
> --no-review option on webkit-patch.
>
> Regarding WIP patches, what I've seen a few times is us reviewers adding
> an r- flag to a WIP patch with no r?, when we think it's horribly wrong…
> I think the flip side of the guideline for non-reviewers to avoid r- is to
> have reviewers use r- only when the patch is up for review. This will
> encourage people to use no flags instead of putting r- for WIP patches.
>

Yeah, I agree that's misleading as well.

On Fri, Nov 16, 2012 at 7:42 AM, Julien Chaffraix <
julien.chaffraix at gmail.com> wrote:

> > Seconded. I also think only the one who submitted the patch can clear
> > the r? flag. Others should NOT do that, please, even you are a
> > reviewer. You can r- the patch if you believe it is bad.
>
> I disagree with that. You seem to think that patches falls into either
> good or bad. However the reality is more complex and there are levels
> of goodness and badness. I use r- for patches that I really think are
> not in the right direction or shouldn't be landed: it is a statement
> in this direction. Clearing the flag is for patches that are close
> enough but still not up to our standards and that I want to kick off
> the review queue.
>

I don't think reviewers should be clearing r? flags unless the patch has
become obsolete (and/or the author is no longer actively working on it) and
we want to get it off of the review queue.

I would leave a comment and leave r? or I would r+ it with comments when I
feel a patch is at the borderline condition. If you believe the patch
should not be landed as is, then you should just r- the patch. If anything,
the author can set r? flag back after arguing against your objection.

- R.Niwa
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20121116/d95322b3/attachment.html>