Unattributed FIXMEs.

[Mike West]

As Peter correctly noted in a recent review[1], the prevailing style in Blink for noting that some piece of code isn't as awesome as it could be is an unattributed FIXME ("// FIXME: Foo should be Bar, see crbug.com/XXX").

I much prefer Chromium style attributions ("// TODO(mkwst): ..."), as they make it clear to whom I might talk without diving into layers of 'git blame'.

Would anyone have strong objections to changing the style guide going forward to prefer the latter format?

-mike

[1]: https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/public/WebDeviceMotionReader.h#newcode38

[Eric Seidel]

I don't feel like the (name) part of TODO offers much value. git
blame shows me the same information (and more context of when it was
added).

If we switched to recommending a name, I would still keep the FIXME
style, as FIXME is still the string to grep for in most of the Blink
code. :)

[Mike West]

Assuming that the code hasn't been moved around, I agree with you. I think we're in the process of refactoring _all_ the code to get Blink up and running, so I expect the level of effort to dig for a typical FIXME's historical context to increase.

I'd be equally happy with strong cultural encouragement to file bugs for FIXME comments, if that's more appealing to you. :)

-mike

[Paweł Hajdan, Jr.]

If unification with Chromium style is something to consider, bringing this closer to Chromium style would be a win.

Note that I'm not really working on Blink, so I don't want to bikeshed much - but thought the above perspective could be useful.

Paweł

[Brett Wilson]

This is a case where there's not a strong technical reason to do one
thing or another, and the existing code uses a number of styles. On
the other side, there's a very specific style mandated by the Google
style guide and widely used in Chrome.

In this type of case, I think we should always be trying to converge,
so having new code using the Google/Chrome way would be a benefit. I
don't think it's worthwhile to do silly things like converting
everything over.

Brett

On Tue, Apr 9, 2013 at 10:52 AM, Eric Seidel <ese...@chromium.org> wrote:

[Mike Lawther]

I have been using the practice of filing a bug, and referencing the bug, eg (from WebCore/platform/CalculationValue.cpp)

   // FIXME calc https://webkit.org/b/80411 : result is NaN when there is a division

   // by zero which isn't found at parse time.

There are a bunch of other examples in WebKit code where this practice has been followed.

This serves the purpose of knowing who is responsible with a single click (no need to spelunk, tracking code moves etc), as well as acting as a repo for any discussion about the issue in question. Plus filing bugs for what is really an open issue shines more light on the code, making it easier to use our tools to see how many open bugs we have. It really is super useful.

Can we please consider adopting this? 

    mike

ps - I don't mind whether it's TODO or FIXME, it's the bug link I care about

[Brett Wilson]

On Tue, Apr 9, 2013 at 5:07 PM, Mike Lawther <mikel...@chromium.org> wrote:
> I have been using the practice of filing a bug, and referencing the bug, eg
> (from WebCore/platform/CalculationValue.cpp)
>
> // FIXME calc https://webkit.org/b/80411 : result is NaN when there is a
> division
> // by zero which isn't found at parse time.
>
> There are a bunch of other examples in WebKit code where this practice has
> been followed.
>
> This serves the purpose of knowing who is responsible with a single click
> (no need to spelunk, tracking code moves etc), as well as acting as a repo
> for any discussion about the issue in question. Plus filing bugs for what is
> really an open issue shines more light on the code, making it easier to use
> our tools to see how many open bugs we have. It really is super useful.
>
> Can we please consider adopting this?

In Chrome before launch we used to very strongly encourage bugs to be
filed for all TODOs, and even went through a fixit to do this, so I
agree with you. We seem to have gotten lax about this in the last 4
years or so :)

We used something like "TODO(brettw) bug 12345 blah blah blah". There
was some effort to standardize the exact format so it would be easier
for tools to understand, but that never really caught on and I suspect
it's a waste of time. But I think it's a good idea to recommend in
code reviews that people file bugs for their TODOs, although I don't
think it's something that's worth requiring in the style guide.

Brett

[Darin Fisher]

The blink style guide (presently a copy of the webkit style guide) indicates that FIXME should be used.

FIXME is called out here:

http://www.chromium.org/blink/coding-style#TOC-Comments

While I'm a fan of TODO(user), I think we should approach style guide changes conservatively, or maybe at least not until some of the dust settles from the fork.

-Darin

[Brett Wilson]

Ah, I thought this was a new rule that was being proposed. We should
be careful that changes we make should always incrementally bring
things into better harmony, but I agree that it's best to be
conservative about changing existing rules for now.

Brett

[Bem Jones-Bey]

It's funny, I thought that bugs were required for WebKit FIXME comments, since that's what everyone always told me I needed. (And I figured that the comments without a bug were legacy)

So I was rather surprised that this thread was talking about it like it was a new thing until I looked again and realized that the style guide doesn't actually require a bug.

+1 on requiring a bug for a FIXME, I think it's a very useful practice.