On Wed, Jul 22, 2009 at 12:26 PM, Jeremy Orlow <[email protected]> wrote:
> On Wed, Jul 22, 2009 at 11:57 AM, Ojan Vafai <[email protected]> wrote: > >> 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. > Forgot to say, if someone felt really motivated to make this scrutinization happen, a change to reitveld to allow commenting on the change description and making it part of the list of things to review would work great for this. > 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. 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 <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. (WebCore::SelectionController::willBeModified): Rearrange function for clarity. Remove code that sets m_lastChangeWasHorizontalExtension; that is now handled elsewhere. (WebCore::SelectionController::modify): Call setLastChangeWasHorizontalExtension after setSelection when setting up a trial selection controller, since setSelection now clears that flag. Also changed both trial selection controller cases to set the flag, although it's not strictly necessary in both cases. Added code to set m_lastChangeWasHorizontalExtension when extending the selection, which used to be handled in willBeModified. Now we need to do it after the selection change. LayoutTests: 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 <rdar://problem/7034324> - editing/selection/extend-selection-after-double-click-expected.txt: Added. - editing/selection/extend-selection-after-double-click.html: Copied from LayoutTests/editing/selection/word-granularity.html. Then turned it into a new test. --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
