Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
2012/11/15 Ryosuke Niwa rn...@webkit.org: On Thu, Nov 15, 2012 at 4:28 PM, Mike Lawther mikelawt...@google.com wrote: On 16 November 2012 09:59, Ryosuke Niwa rn...@webkit.org wrote: While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html I see that page says 'Note that you should not put r+ nor r- on patches in such unofficial reviews' with respect to a non-reviewer doing a shadow review. I can't see the extrapolation from that to 'you can't r- your own patches'. I thought r-'ing your own patch was a relatively common practice when uploading a WIP patch, as a signal that 'I have no intention of landing this patch', and as a courtesy so a reviewer will not waste any time looking at it (unless specifically asked). 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. A patch with r- should be a patch rejected by a reviewer, not a WIP patch the contributor decided not to proceed with. Otherwise, reviewers can’t differentiate the patches rejected by other reviewers and patches authors didn’t like (unless you recognize author’s IRC nickname). - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev 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. -yong ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
From: ext Ryosuke Niwa rn...@webkit.orgmailto:rn...@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. No'am ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
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. This is my reasoning on that and other people likely have different views. However I don't think it's unreasonable to clear the flag instead of r-'ing in some contexts. Julien ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On Fri, Nov 16, 2012 at 7:42 AM, Julien Chaffraix julien.chaffr...@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. This is my reasoning on that and other people likely have different views. However I don't think it's unreasonable to clear the flag instead of r-'ing in some contexts. I would be very confused if anyone but me cleared the r? on one of my bugs, unless it was obvious that the patch was old and buried and I had just forgotten about it. If I think a patch has a few flaws but is on the right track (and I just have questions), I will often leave the r? as-is and expect the contributor to figure it out (meaning that someone else might then be okay to r+ it. I will usually only r- a patch if I think the patch should not be landed as-is and I don't want someone else to r+ it. -- Dirk ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On Fri, Nov 16, 2012 at 7:16 AM, noam.rosent...@nokia.com wrote: From: ext Ryosuke Niwa rn...@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.chaffr...@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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
2012/11/16 Julien Chaffraix julien.chaffr...@gmail.com: 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 True. I was inaccurate in that statement. 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. In this case you should still r- it. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On 16 November 2012 09:59, Ryosuke Niwa rn...@webkit.org wrote: While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html I see that page says 'Note that you should not put r+ nor r- on patches in such unofficial reviews' with respect to a non-reviewer doing a shadow review. I can't see the extrapolation from that to 'you can't r- your own patches'. I thought r-'ing your own patch was a relatively common practice when uploading a WIP patch, as a signal that 'I have no intention of landing this patch', and as a courtesy so a reviewer will not waste any time looking at it (unless specifically asked). I don't see why I wouldn't be allowed to r- my own patch? mike ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On Thu, Nov 15, 2012 at 4:28 PM, Mike Lawther mikelawt...@google.com wrote: On 16 November 2012 09:59, Ryosuke Niwa rn...@webkit.org wrote: While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html I see that page says 'Note that you should not put r+ nor r- on patches in such unofficial reviews' with respect to a non-reviewer doing a shadow review. I can't see the extrapolation from that to 'you can't r- your own patches'. I thought r-'ing your own patch was a relatively common practice when uploading a WIP patch, as a signal that 'I have no intention of landing this patch', and as a courtesy so a reviewer will not waste any time looking at it (unless specifically asked). I don't see why I wouldn't be allowed to r- my own patch? It seems fine to r- your own patch. You can also clear the review flag if you're not interested in having someone review your patch. If you want to upload a work-in-progress patch, one pattern I use is the following: webkit-patch upload --no-review -m Work-in-progress That will avoid setting the review flag and will label the patch as a work in progress. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On Thu, Nov 15, 2012 at 4:28 PM, Mike Lawther mikelawt...@google.comwrote: On 16 November 2012 09:59, Ryosuke Niwa rn...@webkit.org wrote: While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html I see that page says 'Note that you should not put r+ nor r- on patches in such unofficial reviews' with respect to a non-reviewer doing a shadow review. I can't see the extrapolation from that to 'you can't r- your own patches'. I thought r-'ing your own patch was a relatively common practice when uploading a WIP patch, as a signal that 'I have no intention of landing this patch', and as a courtesy so a reviewer will not waste any time looking at it (unless specifically asked). 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. A patch with r- should be a patch rejected by a reviewer, not a WIP patch the contributor decided not to proceed with. Otherwise, reviewers can’t differentiate the patches rejected by other reviewers and patches authors didn’t like (unless you recognize author’s IRC nickname). - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On Nov 15, 2012, at 4:56 PM, Ryosuke Niwa rn...@webkit.org wrote: On Thu, Nov 15, 2012 at 4:28 PM, Mike Lawther mikelawt...@google.com wrote: On 16 November 2012 09:59, Ryosuke Niwa rn...@webkit.org wrote: While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html I see that page says 'Note that you should not put r+ nor r- on patches in such unofficial reviews' with respect to a non-reviewer doing a shadow review. I can't see the extrapolation from that to 'you can't r- your own patches'. I thought r-'ing your own patch was a relatively common practice when uploading a WIP patch, as a signal that 'I have no intention of landing this patch', and as a courtesy so a reviewer will not waste any time looking at it (unless specifically asked). 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. A patch with r- should be a patch rejected by a reviewer, not a WIP patch the contributor decided not to proceed with. Otherwise, reviewers can’t differentiate the patches rejected by other reviewers and patches authors didn’t like (unless you recognize author’s IRC nickname). Personally I think it is ok to r- your own patch (it doesn't really violate the spirit of the rules about setting the review flag), but it's probably better practice to unset the review flag for the sake of clarity. Regards, Maciej___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
