[webkit-reviews] review granted: [Bug 35314] smartdelete should only occur after double-click : [Attachment 50616] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Mar 17 18:30:28 PDT 2010
David Levin <levin at chromium.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 35314: smartdelete should only occur after double-click
https://bugs.webkit.org/show_bug.cgi?id=35314
Attachment 50616: Patch
https://bugs.webkit.org/attachment.cgi?id=50616&action=review
------- Additional Comments from David Levin <levin at chromium.org>
Just a few minor things, which you can fix when landing it.
> diff --git a/WebCore/editing/VisibleSelection.h
b/WebCore/editing/VisibleSelection.h
> + void validate(TextGranularity granularity = CharacterGranularity);
Pls remove param name "granularity" as it adds no information here.
> + void
setStartAndEndFromBaseAndExtentRespectingGranularity(TextGranularity
granularity);
Pls remove param name "granularity" as it adds no information here.
> diff --git a/WebCore/page/DragController.cpp
b/WebCore/page/DragController.cpp
> + // NSTextView behavior is to always smartdelete on moving a
selection,
Rather than lower casing smartdelete, smartinsert, selectiongranularity,
wordgranularity (which I find hard to read), please consider either
UpperCasing both words or separating them or formatting as is done in the code
smartDelete.
Personally, I prefer smart delete, smart insert, selection granularity, word
granularity.
> + // but only to smartinsert if the selectiongranularity is
wordgranularity.
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> +2010-03-12 Ojan Vafai <ojan at chromium.org>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Need a short description and bug URL (OOPS!)
oops indeed :)
> +
> + * src/WebFrameImpl.cpp:
> + (WebKit::WebFrameImpl::selectWordAroundPosition):
> +
> diff --git a/WebKit/chromium/src/WebFrameImpl.cpp
b/WebKit/chromium/src/WebFrameImpl.cpp
> if (frame->shouldChangeSelection(selection))
As you mentioned, this should be:
if (frame->shouldChangeSelection(selection)) {
TextGranularity granularity = CharacterGranularity;
if (selection.isRange())
granularity = WordGranularity;
frame->selection()->setSelection(selection, granularity);
}
More information about the webkit-reviews
mailing list