[webkit-reviews] review requested: [Bug 15643] Double-clicking word should select whitespace after the word : [Attachment 24822] Possible patch to issue 15643

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Glenn Wilson <gwilson at google.com>
Here's a revised patch for this issue.

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

That's fair.  I was thinking of it in terms of a work-around for smart
insert/delete, but it really is a feature.  I've changed the ChangeLog text to
reflect this.

>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.

Yes, agreed.  I put a comment in Editor.h, which I thought was high enough in
the stack to be accessible but low enough to be generally applicable to what
the underlying API is doing.  Let me know if this comment should say more or
less, or if there's a better place than Editor.h.  (Editor.cpp?)

> This line shouldn't be present in ChangeLogs.

Yep...gone.

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

Agreed...this was kind of confusing.  I've changed this to say
"isSelectTrailing..." everywhere.

> Maybe appendTrailingWhitespace would be slightly more clear?

I wanted to make this consistent with the other methods added here, so I
changed this to "selectTrailingWhitespace".  I thought that this has a chance
to be confusing, as it infers that it is selecting *just* the trailing
whitespace.  I also considered "includeTrailingWhitespace"...let me know if
you'd like to see a different name here.

> 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. 

Ok, I moved this into what I thought was the right interface in WebViewPrivate.
 I'm not sure if the interface is right (WebPendingPublic).  Should this be in
the WebPrivate interface?

> However, I believe that adding methods to the middle of IDL will break
nightly
> builds.

I wasn't sure what you meant here, so I moved the methods to the very end of
their interface in the IDL.   Did you mean that these have to be added to
something other than the IDL, or changing IDLs dooms the nightlies to fail for
a while?

Thanks again for the review!!