On Wed, Jul 22, 2009 at 11:57 AM, Ojan Vafai <o...@chromium.org> wrote:
> On Wed, Jul 22, 2009 at 10:07 AM, Darin Fisher <da...@chromium.org> wrote:
>
>> On Wed, Jul 22, 2009 at 10:03 AM, Adam Langley <a...@chromium.org> wrote:
>>
>>> * By having the ChangeLog in the review, reviewers can critique it.
>>>
>>
>>> Many of our commit messages are little brief. Some are far too brief.
>>> But, the commit message should match the change message on the code
>>> review, so our reviewers can already critique it. So, this would
>>> appear to be a review issue rather than a technological issue.
>>>
>>
>> I think we need to remember to review the CL description. It is easy to
>> overlook when reviewing an issue.
>>
>
> This is an understatement. We really do a poor job with commit
> descriptions. There is a lot to be gained by having better commit
> descriptions. We can learn from WebKit process here without adding the
> burden of ChangeLog files. They have great change descriptions and it's
> frequently very useful.
>
TOTALLY agree. We're too lax in reviewing CL descriptions in Chrome.
WebKit reviewers scrutinizes them as much as the code itself, and that's a
good thing.
> To be a bit more concrete, gcl change could autopopulate your change
> description with something like the following that has the list of modified
> functions:
> DETAILED DESCRIPTION HERE
>
> BUG=required
> TEST=required
> RELEASE_NOTES=optional
>
> Autogenerated list of modified methods here.
>
>
> So here's what an example would look like (poached from AGL's recent webkit
> commit):
>
> Chromium Linux: Allow for setting the global font rendering preferences.
> Added static functions for setting hinting, anti-alias and subpixel glyph
> preferences.
>
> BUG=12345
> TEST=none
> RELEASE_NOTES=none
>
I feel like RELEASE_NOTES should only be there if it matters (i.e. isn't
'none'). Honestly, I'd say the same for TEST.
Maybe the default CL description when you first run 'gcl change' should add
"BUG=\n TEST=\n RELEASE_NOTES=" but then have the convention that we should
delete them if they don't apply to our change?
> platform/graphics/chromium/FontPlatformDataLinux.cpp:
> (WebCore::FontPlatformData::setHinting):
> (WebCore::FontPlatformData::setAntiAlias):
> (WebCore::FontPlatformData::setSubpixelGlyphs):
> (WebCore::FontPlatformData::setupPaint): Modified to do something super
> special.
> platform/graphics/chromium/FontPlatformDataLinux.h:
>
Is it worth putting this into the change list? If so, maybe 'gcl commit'
can do it (since doing it any earlier risks it getting stale as the CL
changes). I know a couple of WebKit developers find it helpful, but I'm not
sure it's worth the overhead. Especially if people are used to it
currently.
Also note that when a function or file warrants a special note, there's
nothing stopping someone from making that note whether or not we have a
convention of always listing the files/functions modified.
J
--~--~---------~--~----~------------~-------~--~----~
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~----------~----~----~----~------~----~------~--~---
