Bug filed: http://code.google.com/p/chromium/issues/detail?id=17471
On Wed, Jul 22, 2009 at 1:49 PM, Jeremy Orlow <[email protected]> wrote: > Here comes the bike shedding...but what about this: > """ > > > BUG= > TEST= > RELEASE_NOTES= > """ > > I don't think any ALL CAPS TEXT at the top is going to make people put in > better descriptions. If you want bug to be a url, it should use > crbug.cominstead of the full string, but the convention so far has been a bar > bug > number which I think is fine. > > I don't really see why TEST is required...it's "none" much more often than > BUG. I think simply populating it by default will get people to annotate it > if they have anything to say. Otherwise they should just delete it. > > And I don't like the idea of needing to delete instructions on how to use > the change descriptions every time I make a CL. I think what I'm proposing > makes it obvious enough for newcomers while optimizing for people who submit > a lot of CLs. > > J > > On Wed, Jul 22, 2009 at 1:43 PM, Ojan Vafai <[email protected]> wrote: > >> I'm OK with not having the function list. I can see the advantages of the >> webkit approach as it means that comments in the code don't get outdated. >> But, then it forces you to look at the changelog history of every line of >> code. >> Anyways, how about we prepopulate gcl/git-cl change with this text: >> DETAILED_DESCRIPTION_HERE >> >> BUG=http://bugs.chromium.org/BUG_NUMBER_HERE >> TEST=required >> RELEASE_NOTES=optional >> >> >> On Wed, Jul 22, 2009 at 1:28 PM, Jeremy Orlow <[email protected]>wrote: >> >>> On Wed, Jul 22, 2009 at 1:22 PM, Peter Kasting <[email protected]>wrote: >>> >>>> On Wed, Jul 22, 2009 at 1:17 PM, Ojan Vafai <[email protected]> wrote: >>>> >>>>> >>>>>>> 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). >>>>>> >>>>> >>>>> The benefit of this is that you can give more detailed comments inline. >>>>> You see some WebKit commits do this (e.g. Darin Adler's) and it makes it >>>>> much more clear from reading the change description what happened. So, if >>>>> we're going to do it, it's not useful to do at commit time. It would be >>>>> great if people got in the habit of writing detailed descriptions like >>>>> this. >>>>> >>>> >>>> I really, really hate this style. >>>> >>>> I have never encountered a case where a combination of comments-in-code >>>> and good-change-description are not both sufficient, and far more readable >>>> than the above style. >>>> >>>> Here's an example of a Darin Adler commit where this leads to a nice >>>>> clear and detailed description. >>>>> >>>>> WebCore: >>>>> >>>>> 2009-07-15 Darin Adler < [email protected] <[email protected]>> >>>>> >>>>> Reviewed by John Sullivan. >>>>> >>>>> After double-clicking a word, using Shift-arrow to select behaves >>>>> unpredictably >>>>> >>>>> https://bugs.webkit.org/show_bug.cgi?id=27177<https://bugs.webkit.org/show_bug.cgi?id=27177> >>>>> rdar://problem/7034324 >>>>> >>>>> Test: editing/selection/extend-selection-after-double-click.html >>>>> >>>>> The bug was due to the m_lastChangeWasHorizontalExtension flag, which >>>>> was not >>>>> being cleared in many cases where it should have been. >>>>> >>>>> >>>>> - editing/SelectionController.cpp: >>>>> (WebCore::SelectionController::setSelection): Set >>>>> m_lastChangeWasHorizontalExtension to false. This catches all sorts of >>>>> cases >>>>> that don't flow through the modify function. Before, the flag would >>>>> reflect >>>>> the last call to the modify function, which was not necessarily the >>>>> last >>>>> selection change. >>>>> >>>>> This kind of thing should be a comment in the code, and the fact that >>>> WebKit does not generally comment code is much to their detriment. >>>> >>> >>> Yes. For some reason, WebKit likes to replace comments in the code with >>> comments in the ChangeLog which seems insane to me. If you need comments >>> this detailed in the ChangeLog then something is wrong with the code IMHO. >>> >>> I think there are very few cases where you need down to the function >>> level of detail. In most case, you should be putting comments in the code >>> or splitting your CL into multiple that each address one specific issue. >>> >>>> >>>>> - editing/selection/extend-selection-after-double-click.html: >>>>> Copied from LayoutTests/editing/selection/word-granularity.html. Then >>>>> turned >>>>> it into a new test. >>>>> >>>>> This kind of thing, if really important, should be a comment in the >>>> change description. >>>> >>>> PK >>>> >>> >>> >> > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
