[webkit-reviews] review denied: [Bug 15643] Double-clicking word should select whitespace after the word : [Attachment 24782] Improved possible patch to issue 15643

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Glenn Wilson
<gwilson at google.com>'s request for review:
Bug 15643: Double-clicking word should select whitespace after the word
https://bugs.webkit.org/show_bug.cgi?id=15643

Attachment 24782: Improved possible patch to issue 15643
https://bugs.webkit.org/attachment.cgi?id=24782&action=edit

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 Added support for clients that wish to disable smart insert/delete
+	 and enable the "trailing whitespace selection" work-around.

I'm not sure why you call this a work-around, it sounds like a normal feature
to me.

It may make sense to add comments somewhere in code explaining the relationship
between the two. Currently, one needs to go all the way to WebView to find out
that the features are mutually exclusive, and that's a long way.

We normally explain individual changes (per-file or per-function) in
ChangeLogs, but maybe this patch is obvious enough that there is no need to. 

+	 * ChangeLog:

This line shouldn't be present in ChangeLogs.

+bool Editor::selectTrailingWhitespaceEnabled()

This starts with a verb, but doesn't select anything. I suggest naming this
isSelectTrailingWhitespaceEnabled().

+void Selection::includeTrailingWhitespace()

Maybe appendTrailingWhitespace would be slightly more clear?

+	 * WebView/WebView.h:

WebView.h is an Mac OS X API header, and I'm pretty sure that we don't want to
expose this as API. Please modify WebViewPrivate.h instead. 

It is somewhat less of an issue on Windows, as we don't have a supported API
there, so I'm not sure whether I should insist on doing the same on Windows.
However, I believe that adding methods to the middle of IDL will break nightly
builds.

r- for WebView.h, and for breaking nightly builds. Looks good otherwise!