Do Blink changes require a Bug? (Was: [blink-dev] Finding Blink contributions in the review system.)

[Eric Seidel]

I think the every-patch-needs-a-bug rule makes sense in WebKit where
the review system and patch are combined.

In Blink, like WebKit, every patch should have a review (which means
an entry in codereviews.chromium.org). I'm not sure it separately
also needs a bug, although I'm open to such a requirement if its
believed to add value.

Adding yourself to the WATCHLIST causes you to be CC'd on reviews, not
bugs. Bugs in Blink (and Chromium) are more about reports from users
and release tracking. Reviews are more of the nuts-and-bolts of
tracking a change, and making sure all the right people see it go by.

So I guess I'm with Ojan on this one, that reviews should be required,
but bugs not necessarily. Thoughts?

On Thu, Apr 4, 2013 at 1:36 PM, Ojan Vafai <oj...@chromium.org> wrote:
> On Thu, Apr 4, 2013 at 1:21 PM, Dominic Mazzoni <dmaz...@chromium.org>
> wrote:
>>
>> Code reviews are not categorized, but each code review should have an
>> associated bug.
>
>
> Is this true? I don't think there's any rule that we need a bug for each
> patch. In fact, I'd strongly prefer we not have that sort of make-work.
>
>> One feature I'd like to see: when someone uploads a code review with a bug
>> id, automatically cc everyone subscribed to that bug. (Or make that an
>> option - perhaps only developers want this, not external bug reporters.)
>
>
> Yeah, that would be nice.

[Nico Weber]

On Thu, Apr 4, 2013 at 1:42 PM, Eric Seidel <ese...@chromium.org> wrote:

I think the every-patch-needs-a-bug rule makes sense in WebKit where
the review system and patch are combined.

In Blink, like WebKit, every patch should have a review (which means
an entry in codereviews.chromium.org).  I'm not sure it separately
also needs a bug, although I'm open to such a requirement if its
believed to add value.

Adding yourself to the WATCHLIST causes you to be CC'd on reviews, not
bugs.  Bugs in Blink (and Chromium) are more about reports from users
and release tracking.  Reviews are more of the nuts-and-bolts of
tracking a change, and making sure all the right people see it go by.

So I guess I'm with Ojan on this one, that reviews should be required,
but bugs not necessarily.  Thoughts?

In Chromium, the guideline is that ever commit needs to be understandable by someone not familiar with the change – and that includes the motivation for the commit. If it's a fairly simple change, you can put everything in the bug description. If it's something that's more complicated or something that's split over several patches, you probably want to file a bug (that's a better place to discuss why a patch was reverted, to note that a patch improved performance on some platform, etc).

I suggest that blink follows chromium's guidelines.

Nico

[Avi Drissman]

I'm a huge fan of bugs, but not to the extent that they're make-work.

For simple fixes (oops, we added instead of subtracted), sure, don't worry about a bug. And for huge features, of course a tracking bug is needed to tie all the changes together.

But bugs are useful for all kinds of things, like discussions, dependency tracking, launch bits. I don't want official rules, but I'd prefer to see Blink behave much as Chromium does here.

[Dirk Pranke]

On Thu, Apr 4, 2013 at 1:47 PM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Apr 4, 2013 at 1:42 PM, Eric Seidel <ese...@chromium.org> wrote:

I think the every-patch-needs-a-bug rule makes sense in WebKit where
the review system and patch are combined.

In Blink, like WebKit, every patch should have a review (which means
an entry in codereviews.chromium.org).  I'm not sure it separately
also needs a bug, although I'm open to such a requirement if its
believed to add value.

Adding yourself to the WATCHLIST causes you to be CC'd on reviews, not
bugs.  Bugs in Blink (and Chromium) are more about reports from users
and release tracking.  Reviews are more of the nuts-and-bolts of
tracking a change, and making sure all the right people see it go by.

So I guess I'm with Ojan on this one, that reviews should be required,
but bugs not necessarily.  Thoughts?

In Chromium, the guideline is that ever commit needs to be understandable by someone not familiar with the change – and that includes the motivation for the commit. If it's a fairly simple change, you can put everything in the bug description. If it's something that's more complicated or something that's split over several patches, you probably want to file a bug (that's a better place to discuss why a patch was reverted, to note that a patch improved performance on some platform, etc).

I suggest that blink follows chromium's guidelines.

Nico

You mean "If it's a fairly simple change, yo ucan put everything in the Rietveld issue", right?

Otherwise I also agree that we should follow Chromium's practice here and not require a bug for every Rietveld change, only when it makes sense for substantive issues.

-- Dirk

[Jochen Eisinger]

On Thu, Apr 4, 2013 at 10:42 PM, Eric Seidel <ese...@chromium.org> wrote:

I think the every-patch-needs-a-bug rule makes sense in WebKit where
the review system and patch are combined.

In Blink, like WebKit, every patch should have a review (which means
an entry in codereviews.chromium.org).  I'm not sure it separately
also needs a bug, although I'm open to such a requirement if its
believed to add value.

tl;dr if the patch is non-trivial, we should require a bug.

If the patch is non-trivial, I would recommend to have a bug. We might want to merge it to a branch, which requires a bug, you can add additional information what this change fixes etc.. which will be useful for e.g. sheriffs that have to decide whether or not your patch caused a certain failure, if somebody tries to triage a bug, having a bit more information than the commit message is super helpful.

If the patch works towards a feature, we should have a tracking bug for it (or even a launch bug), and either reference that bug directly, or have the bug for the individual commit block the tracking bug.

In the chromium world, it's also ok to reference a single bug from several commits

best

-jochen

[Dominic Mazzoni]

On Thu, Apr 4, 2013 at 1:47 PM, Nico Weber <tha...@chromium.org> wrote:

So I guess I'm with Ojan on this one, that reviews should be required,
but bugs not necessarily.  Thoughts?

In Chromium, the guideline is that ever commit needs to be understandable by someone not familiar with the change – and that includes the motivation for the commit. If it's a fairly simple change, you can put everything in the bug description. If it's something that's more complicated or something that's split over several patches, you probably want to file a bug (that's a better place to discuss why a patch was reverted, to note that a patch improved performance on some platform, etc).

Cleanups, whitespace changes, and build fixes rarely have bugs. On the other hand, if you want your change merged to a release branch, you must have a bug - the release process is built around the bug tracker. For anything in-between, you can use your best judgement.

Note that you don't have to file a new bug for each change. It's common to create a single bug to capture the general issue, then split the work into 2 - 3 code reviews that all refer to the same bug number. So it's really not that much of a burden.

- Dominic

[Nico Weber]

On Thu, Apr 4, 2013 at 1:50 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Thu, Apr 4, 2013 at 1:47 PM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Apr 4, 2013 at 1:42 PM, Eric Seidel <ese...@chromium.org> wrote:

I think the every-patch-needs-a-bug rule makes sense in WebKit where
the review system and patch are combined.

In Blink, like WebKit, every patch should have a review (which means
an entry in codereviews.chromium.org).  I'm not sure it separately
also needs a bug, although I'm open to such a requirement if its
believed to add value.

Adding yourself to the WATCHLIST causes you to be CC'd on reviews, not
bugs.  Bugs in Blink (and Chromium) are more about reports from users
and release tracking.  Reviews are more of the nuts-and-bolts of
tracking a change, and making sure all the right people see it go by.

So I guess I'm with Ojan on this one, that reviews should be required,
but bugs not necessarily.  Thoughts?

In Chromium, the guideline is that ever commit needs to be understandable by someone not familiar with the change – and that includes the motivation for the commit. If it's a fairly simple change, you can put everything in the bug description. If it's something that's more complicated or something that's split over several patches, you probably want to file a bug (that's a better place to discuss why a patch was reverted, to note that a patch improved performance on some platform, etc).

I suggest that blink follows chromium's guidelines.

Nico

You mean "If it's a fairly simple change, yo ucan put everything in the Rietveld issue", right?

Err, yes I mean "If it's a fairly simple change, you can put everything in the patch description" :-)

[Elliott Sprehn]

Can someone who's not a committer comment on a random reitvelt issue? If we don't have bugs we lose out on all the valuable input we used to get from commenters on bugs that were not necessarily committers (ex. Hixie could see things go by).

It also made it much easier for outside people to join in on discussions.

[Dirk Pranke]

Yes.

[Thiago Farina]

On Thu, Apr 4, 2013 at 5:52 PM, Dominic Mazzoni <dmaz...@chromium.org> wrote:
> Cleanups, whitespace changes, and build fixes rarely have bugs. On the other
> hand, if you want your change merged to a release branch, you must have a
> bug - the release process is built around the bug tracker. For anything
> in-between, you can use your best judgement.
>
> Note that you don't have to file a new bug for each change. It's common to
> create a single bug to capture the general issue, then split the work into 2
> - 3 code reviews that all refer to the same bug number. So it's really not
> that much of a burden.
>

IMO that captures exactly how we do in Chromium. And if possible I
think Blink could follow that. i.e, if your patch is trivial, one time
shot, it may not need a bug associated with, as Dominic said use your
best judgment.

[Eric Seidel]

Sounds like we need to write this down in some Wiki if it isn't
already. I'm all for Blink and Chromium being the same here.

[Alex Komoroske]

+1, we should write this down somewhere. If it's not already written down for Chromium, we should make a sub-page under chromium.org/blink .