Bug 110487 - Ctrl+Shift+Right in Windows should select the spacing after the word
: Ctrl+Shift+Right in Windows should select the spacing after the word
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: PC Windows 7
: P2 Normal
Assigned To:
:
:
: 111321 113383 113414 113424 113490
:
  Show dependency treegraph
 
Reported: 2013-02-21 09:38 PST by
Modified: 2013-04-23 13:38 PST (History)


Attachments
Patch (1.99 KB, patch)
2013-02-21 10:02 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (13.37 KB, patch)
2013-02-26 09:46 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (15.73 KB, patch)
2013-02-27 06:10 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (15.12 KB, patch)
2013-02-27 07:36 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.99 KB, patch)
2013-02-27 08:29 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.32 KB, patch)
2013-02-27 22:42 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (348.01 KB, patch)
2013-03-21 15:16 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (348.80 KB, patch)
2013-03-22 12:21 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch without tests, in order to assess effects of code changes. (5.17 KB, patch)
2013-03-26 06:21 PST, Claudio Saavedra
csaavedra: review-
Review Patch | Details | Formatted Diff | Diff
Patch without tests without format issues. (6.23 KB, patch)
2013-03-26 07:13 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (862.56 KB, application/zip)
2013-03-26 10:31 PST, Build Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64 (837.62 KB, application/zip)
2013-03-26 14:10 PST, WebKit Review Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-future (863.46 KB, application/zip)
2013-03-26 23:10 PST, Build Bot
no flags Details
Patch without tests (6.23 KB, patch)
2013-04-02 08:06 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 for chromium-linux-x86_64 (1.72 MB, application/zip)
2013-04-03 05:24 PST, WebKit Review Bot
no flags Details
Patch (88.38 KB, patch)
2013-04-17 10:33 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2013-02-21 09:38:27 PST
In Windows, moving (or extending the selection) to the right with Control (and Shift) + the right arrow should also select the spacing after the current word. WebKit currently only does this when moving to the right (Control+Right), not when extending the selection (Control+Shift+Right).

I have an initial patch that solves this but I'd appreciate some comments from someone more familiar with this area.
------- Comment #1 From 2013-02-21 10:02:44 PST -------
Created an attachment (id=189549) [details]
Patch
------- Comment #2 From 2013-02-21 10:47:12 PST -------
(From update of attachment 189549 [details])
Attachment 189549 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16696186

New failing tests:
editing/selection/extend-selection-enclosing-block.html
editing/selection/extend-selection-word.html
------- Comment #3 From 2013-02-21 11:12:54 PST -------
(From update of attachment 189549 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review

r- due to lack of tests and failing tests.  Why did this change the results of a test on Linux?

> Source/WebCore/editing/FrameSelection.cpp:603
> +#if USE(ICU_UNICODE)

Why does this depend on ICU_UNICODE?
------- Comment #4 From 2013-02-21 11:48:00 PST -------
(From update of attachment 189549 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review

> Source/WebCore/editing/FrameSelection.cpp:605
> +            bool skipsSpaceWhenMovingRight = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight();
> +            pos = rightWordPosition(pos, skipsSpaceWhenMovingRight);

This should be testable. Also, be sure to test this with bidirectional text.
Furthermore, there should be an equivalent change in modifyExtendingLeft.
------- Comment #5 From 2013-02-21 11:53:39 PST -------
(In reply to comment #4)
> (From update of attachment 189549 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review
> 
> > Source/WebCore/editing/FrameSelection.cpp:605
> > +            bool skipsSpaceWhenMovingRight = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight();
> > +            pos = rightWordPosition(pos, skipsSpaceWhenMovingRight);
> 
> This should be testable. Also, be sure to test this with bidirectional text.
> Furthermore, there should be an equivalent change in modifyExtendingLeft.

I tested the ctrl+shift+left case on my Windows machine (notepad.exe) and it does not include the preceding space.
------- Comment #6 From 2013-02-21 13:45:43 PST -------
(In reply to comment #3)
> (From update of attachment 189549 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review
> 
> r- due to lack of tests and failing tests.  Why did this change the results of a test on Linux?
> 
> > Source/WebCore/editing/FrameSelection.cpp:603
> > +#if USE(ICU_UNICODE)
> 
> Why does this depend on ICU_UNICODE?

It is commented somewhere else in the same file. I think there's one method that is not available for all platforms.
------- Comment #7 From 2013-02-21 13:46:41 PST -------
(In reply to comment #5)
> (In reply to comment #4)

> > 
> > This should be testable. Also, be sure to test this with bidirectional text.
> > Furthermore, there should be an equivalent change in modifyExtendingLeft.
> 
> I tested the ctrl+shift+left case on my Windows machine (notepad.exe) and it does not include the preceding space.

It probably does if your text is RTL. Ryosuke is probably right.
------- Comment #8 From 2013-02-21 14:06:59 PST -------
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> 
> > > 
> > > This should be testable. Also, be sure to test this with bidirectional text.
> > > Furthermore, there should be an equivalent change in modifyExtendingLeft.
> > 
> > I tested the ctrl+shift+left case on my Windows machine (notepad.exe) and it does not include the preceding space.
> 
> It probably does if your text is RTL. Ryosuke is probably right.

Ah, yes, if the text is RTL, it probably should select the trailing space when pressing ctrl+shift+left.
------- Comment #9 From 2013-02-26 01:45:10 PST -------
(In reply to comment #3)
> (From update of attachment 189549 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review
> 
> r- due to lack of tests and failing tests.  Why did this change the results of a test on Linux?

This changed the results of the tests in Linux because what my initial patch did was to use rightWordPosition() for the cases where the enclosing paragraph is LTR. Now, this fails because when the text is using RTL, this is wrong.

It seems to me that the proper fix would be to change nextWordPosition() to take into account whether spacing should be skipped, just like it was done with rightWordPosition().
------- Comment #10 From 2013-02-26 09:46:21 PST -------
Created an attachment (id=190316) [details]
Patch
------- Comment #11 From 2013-02-26 10:10:11 PST -------
(From update of attachment 190316 [details])
Attachment 190316 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16770324
------- Comment #12 From 2013-02-26 10:13:10 PST -------
(From update of attachment 190316 [details])
Attachment 190316 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16773304
------- Comment #13 From 2013-02-26 11:16:02 PST -------
(From update of attachment 190316 [details])
Attachment 190316 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16778291
------- Comment #14 From 2013-02-26 12:11:30 PST -------
(From update of attachment 190316 [details])
Attachment 190316 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16780269
------- Comment #15 From 2013-02-26 13:12:33 PST -------
(From update of attachment 190316 [details])
Attachment 190316 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16775390
------- Comment #16 From 2013-02-26 14:39:47 PST -------
(From update of attachment 190316 [details])
Attachment 190316 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16774461

New failing tests:
editing/execCommand/remove-formatting-2.html
editing/execCommand/toggle-unlink.html
editing/execCommand/createLink.html
editing/execCommand/button.html
editing/pasteboard/8145-2.html
editing/inserting/insert-paragraph-03.html
editing/pasteboard/5006779.html
editing/selection/4932260-2.html
editing/inserting/4960120-2.html
editing/deleting/list-item-1.html
editing/deleting/smart-delete-across-editable-boundaries.html
editing/execCommand/remove-format-multiple-elements.html
editing/execCommand/toggle-link.html
editing/execCommand/hilitecolor.html
editing/inserting/insert-before-link-1.html
editing/pasteboard/4806874.html
editing/deleting/delete-cell-contents.html
editing/deleting/smart-editing-disabled.html
editing/execCommand/unlink.html
editing/execCommand/indent-pre.html
editing/deleting/delete-by-word-001.html
editing/deleting/non-smart-delete.html
editing/deleting/delete-by-word-002.html
editing/inserting/insert-paragraph-04.html
editing/deleting/smart-delete-002.html
editing/pasteboard/bad-placeholder.html
editing/pasteboard/4242293-1.html
http/tests/cache/subresource-failover-to-network.html
editing/execCommand/query-command-state.html
------- Comment #17 From 2013-02-27 06:10:25 PST -------
Created an attachment (id=190503) [details]
Patch
------- Comment #18 From 2013-02-27 06:24:34 PST -------
(In reply to comment #17)
> Created an attachment (id=190503) [details] [details]
> Patch

This will probably fail tests but it should build in Mac and Qt.
------- Comment #19 From 2013-02-27 07:02:42 PST -------
(From update of attachment 190503 [details])
Attachment 190503 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16789278
------- Comment #20 From 2013-02-27 07:36:29 PST -------
Created an attachment (id=190519) [details]
Patch
------- Comment #21 From 2013-02-27 07:37:24 PST -------
(In reply to comment #20)
> Created an attachment (id=190519) [details] [details]
> Patch

And another attempt at fixing the build in mac.
------- Comment #22 From 2013-02-27 08:12:26 PST -------
(From update of attachment 190519 [details])
Attachment 190519 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16807280
------- Comment #23 From 2013-02-27 08:29:28 PST -------
Created an attachment (id=190528) [details]
Patch
------- Comment #24 From 2013-02-27 09:21:14 PST -------
(From update of attachment 190528 [details])
Attachment 190528 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16810045
------- Comment #25 From 2013-02-27 10:31:57 PST -------
(From update of attachment 190528 [details])
Attachment 190528 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16803101
------- Comment #26 From 2013-02-27 10:58:07 PST -------
(From update of attachment 190528 [details])
Attachment 190528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16835089

New failing tests:
editing/execCommand/remove-formatting-2.html
editing/execCommand/toggle-unlink.html
editing/execCommand/createLink.html
editing/execCommand/button.html
editing/pasteboard/8145-2.html
editing/inserting/insert-paragraph-03.html
editing/pasteboard/5006779.html
editing/selection/4932260-2.html
editing/inserting/4960120-2.html
editing/deleting/list-item-1.html
editing/deleting/smart-delete-across-editable-boundaries.html
editing/execCommand/remove-format-multiple-elements.html
editing/execCommand/toggle-link.html
editing/inserting/paragraph-outside-nested-divs.html
editing/inserting/insert-before-link-1.html
editing/pasteboard/4806874.html
editing/deleting/delete-cell-contents.html
editing/deleting/smart-editing-disabled.html
editing/deleting/non-smart-delete.html
editing/execCommand/unlink.html
editing/execCommand/indent-pre.html
editing/deleting/delete-by-word-001.html
editing/execCommand/hilitecolor.html
editing/pasteboard/bad-placeholder.html
editing/inserting/insert-paragraph-04.html
editing/deleting/smart-delete-002.html
editing/deleting/delete-by-word-002.html
editing/pasteboard/4242293-1.html
http/tests/cache/subresource-failover-to-network.html
editing/execCommand/query-command-state.html
------- Comment #27 From 2013-02-27 11:59:09 PST -------
(From update of attachment 190528 [details])
Attachment 190528 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16779485
------- Comment #28 From 2013-02-27 22:42:51 PST -------
Created an attachment (id=190658) [details]
Patch
------- Comment #29 From 2013-02-27 23:14:12 PST -------
(From update of attachment 190658 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review

> Source/WebCore/platform/text/TextBoundaries.cpp:65
> -int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward)
> +int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward, bool skipSpaces)

It's not a good idea to modify findNextWordFromIndex like this. Please follow what we did for visualWordPosition.
------- Comment #30 From 2013-02-27 23:31:29 PST -------
(In reply to comment #29)
> (From update of attachment 190658 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review
> 
> > Source/WebCore/platform/text/TextBoundaries.cpp:65
> > -int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward)
> > +int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward, bool skipSpaces)
> 
> It's not a good idea to modify findNextWordFromIndex like this. Please follow what we did for visualWordPosition.

Thanks for the comments. Not sure I understand though. visualWordPosition() is used from left/rightWordPosition(). For the platforms where the wordbreak is not available, the fallback is going to lead to findNextWordFromIndex() too.. would you mind clarifying what exactly you mean?
------- Comment #31 From 2013-02-28 00:01:45 PST -------
(From update of attachment 190658 [details])
Attachment 190658 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16822318

New failing tests:
editing/execCommand/remove-formatting-2.html
editing/execCommand/toggle-unlink.html
editing/execCommand/createLink.html
editing/execCommand/button.html
editing/pasteboard/8145-2.html
editing/inserting/insert-paragraph-03.html
editing/pasteboard/5006779.html
editing/selection/4932260-2.html
editing/inserting/4960120-2.html
editing/deleting/list-item-1.html
editing/deleting/smart-delete-across-editable-boundaries.html
editing/execCommand/remove-format-multiple-elements.html
editing/execCommand/toggle-link.html
editing/execCommand/hilitecolor.html
editing/pasteboard/copy-text-with-backgroundcolor.html
editing/inserting/insert-before-link-1.html
editing/pasteboard/4806874.html
editing/deleting/delete-cell-contents.html
editing/deleting/smart-editing-disabled.html
editing/execCommand/unlink.html
editing/execCommand/indent-pre.html
editing/deleting/delete-by-word-001.html
editing/deleting/non-smart-delete.html
editing/deleting/delete-by-word-002.html
editing/inserting/insert-paragraph-04.html
editing/deleting/smart-delete-002.html
editing/pasteboard/bad-placeholder.html
editing/pasteboard/4242293-1.html
http/tests/cache/subresource-failover-to-network.html
editing/execCommand/query-command-state.html
------- Comment #32 From 2013-02-28 00:22:32 PST -------
(In reply to comment #30)
> (In reply to comment #29)
> > (From update of attachment 190658 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review
> > 
> > > Source/WebCore/platform/text/TextBoundaries.cpp:65
> > > -int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward)
> > > +int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward, bool skipSpaces)
> > 
> > It's not a good idea to modify findNextWordFromIndex like this. Please follow what we did for visualWordPosition.
> 
> Thanks for the comments. Not sure I understand though. visualWordPosition() is used from left/rightWordPosition(). For the platforms where the wordbreak is not available, the fallback is going to lead to findNextWordFromIndex() too.. would you mind clarifying what exactly you mean?

The technique we used there is to move forward twice and go back.
------- Comment #33 From 2013-02-28 00:44:01 PST -------
(In reply to comment #32)
> (In reply to comment #30)
> > (In reply to comment #29)

> The technique we used there is to move forward twice and go back.

I might be overlooking something but that's not what I read in the code. What I see is that in visualWordPosition() it is iterated until finding the start or end of the next word, depending on whether we're in LTR/RTL and whether it is desired to skip spaces or not. I don't think there's any moving forward twice and then back..
------- Comment #34 From 2013-03-04 05:35:55 PST -------
(In reply to comment #3)
> (From update of attachment 189549 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review
> 
> r- due to lack of tests and failing tests.  Why did this change the results of a test on Linux?

Some of the tests break in cr-linux because Chromium's TestRunner uses WebSettings::EditingBehaviorWin in all platforms but Linux. Since this changes the outcome of extending the selection in Windows, many test expectations will need to be updated.
------- Comment #35 From 2013-03-04 06:04:45 PST -------
(In reply to comment #34)
> (In reply to comment #3)
> > (From update of attachment 189549 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review
> > 
> > r- due to lack of tests and failing tests.  Why did this change the results of a test on Linux?
> 
> Chromium's TestRunner uses WebSettings::EditingBehaviorWin in all platforms but Linux.

Excuse my thinko -- in all platforms but Mac.
------- Comment #36 From 2013-03-04 15:06:24 PST -------
(From update of attachment 190658 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review

> Source/WebCore/editing/FrameSelection.cpp:609
> +        if (directionOfEnclosingBlock() == LTR) {
> +#if PLATFORM(MAC)
>              pos = nextWordPosition(pos);
> -        else
> +#else
> +            bool skipsSpaceWhenMovingForward = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight();
> +            pos = nextWordPosition(pos, skipsSpaceWhenMovingForward);
> +#endif

I don't want to see all these if-defs here.
------- Comment #37 From 2013-03-05 12:12:49 PST -------
(In reply to comment #36)
> (From update of attachment 190658 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review
> 
> > Source/WebCore/editing/FrameSelection.cpp:609
> > +        if (directionOfEnclosingBlock() == LTR) {
> > +#if PLATFORM(MAC)
> >              pos = nextWordPosition(pos);
> > -        else
> > +#else
> > +            bool skipsSpaceWhenMovingForward = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight();
> > +            pos = nextWordPosition(pos, skipsSpaceWhenMovingForward);
> > +#endif
> 
> I don't want to see all these if-defs here.

I am not sure how to implement a version of findNextWordFromIndex() for mac that would take into account whether spaces should be skipped or not. The existing method seems to use NSAttributedString attributes. Since Mac doesn't follow the Windows behavior, this doesn't seem absolutely mandatory, but if some dev from the Mac port is kind enough to jump in we might not need the ifdef's.
------- Comment #38 From 2013-03-05 12:53:43 PST -------
(From update of attachment 190658 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review

I think that Ryosuke's request is not to implement Windows behavior using NSString APIs, but to make sure that editing behavior is not determined at compile time.

We want to test all platforms behavior in regression tests, using a window.internals switch to choose editing behavior in each test. This way, running tests locally would be a better indication that a patch is correct.

Since the patch still does not have tests, it's automatically r-.

> Source/WTF/wtf/unicode/icu/UnicodeIcu.h:179
> +inline bool isSpace(UChar32 c)
> +{
> +    return !!u_isWhitespace(c);
> +}

This is not a good name for this function (isWhitespace is not so great either, but it's OK in ICU context).

There are multiple definitions of whitespace used in the Web platform. They may or may not include Unicode spaces, form feeds etc.
------- Comment #39 From 2013-03-05 22:44:49 PST -------
Thanks for your comments!

(In reply to comment #38)
> (From update of attachment 190658 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review
> 
> I think that Ryosuke's request is not to implement Windows behavior using NSString APIs, but to make sure that editing behavior is not determined at compile time.

Editing behavior is not determined at compile-time. I'll explain why there are ifdef's more carefully: the way the next stop is determined, in all platforms, is through a call to the findNextWordFromIndex() method. What my patch is doing is to add a boolean parameter to the signature of this method, so that it looks like this:

  findNextWordFromIndex(UChar const* buffer, int len, int position, bool forward, bool skipSpaces);

This way, the calling methods can decide whether to request from this method to skip spaces when searching for the next word. Calling methods, of course, rely on the run-time property EditingBehavior. As you can see, the editing behavior is never determined at run-time.

As to the ifdefs: This method has different implementations: a generic one, one for the Qt port (using QString), and one for the Mac one (using NSString). My patch modifies both the generic implementation and the one for the Qt port. To make the patch complete one would need to modify the Mac implementation as well so that it works as we now need. Admittedly, since I am not familiar with NSString at all I've left this out. I am open to suggestions as to how to complete this patch so that findNextWordFromIndex() in Mac works in the same way and the ifdefs are not necessary.


> 
> We want to test all platforms behavior in regression tests, using a window.internals switch to choose editing behavior in each test. This way, running tests locally would be a better indication that a patch is correct.
> 
> Since the patch still does not have tests, it's automatically r-.

I will add tests soon, of course. However, at this point I am mostly concerned in what we're discussing above.

> 
> > Source/WTF/wtf/unicode/icu/UnicodeIcu.h:179
> > +inline bool isSpace(UChar32 c)
> > +{
> > +    return !!u_isWhitespace(c);
> > +}
> 
> This is not a good name for this function (isWhitespace is not so great either, but it's OK in ICU context).
> 
> There are multiple definitions of whitespace used in the Web platform. They may or may not include Unicode spaces, form feeds etc.

Do you have any suggestion? Maybe just isWhitespace() or isSpacing()?
------- Comment #40 From 2013-03-06 12:06:46 PST -------
(In reply to comment #39)
> Thanks for your comments!
>
> Editing behavior is not determined at compile-time. I'll explain why there are ifdef's more carefully: the way the next stop is determined, in all platforms, is through a call to the findNextWordFromIndex() method. What my patch is doing is to add a boolean parameter to the signature of this method, so that it looks like this:
> 
>   findNextWordFromIndex(UChar const* buffer, int len, int position, bool forward, bool skipSpaces);
> 
> This way, the calling methods can decide whether to request from this method to skip spaces when searching for the next word. Calling methods, of course, rely on the run-time property EditingBehavior. As you can see, the editing behavior is never determined at run-time.

What I've been telling you ever since this this bug is filed is that we shouldn't do this. Instead, call nextWordPosition twice and call previousWordPosition that'll move the caret to the position after the whitespace. This way, new behavior can be implemented and exposed in DRT everywhere.
------- Comment #41 From 2013-03-21 15:16:22 PST -------
Created an attachment (id=194359) [details]
Patch
------- Comment #42 From 2013-03-22 11:05:58 PST -------
(From update of attachment 194359 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=194359&action=review

> Source/WebCore/editing/FrameSelection.cpp:598
> +            pos = nextWordPosition(pos);

There is a bug here if c is in the middle of the word. In that case pos will be in a previous position and this won't work. I will attach a new patch correcting this.
------- Comment #43 From 2013-03-22 12:21:29 PST -------
Created an attachment (id=194615) [details]
Patch
------- Comment #44 From 2013-03-25 17:19:48 PST -------
(From update of attachment 194615 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=194615&action=review

I would have preferred test changes to be submitted separately on a separate bug so that we can assess the effect of the code change more closely.

> Source/WebCore/editing/FrameSelection.cpp:584
> +VisiblePosition FrameSelection::nextWordPositionForPlatform(const VisiblePosition &c)

Please don't use one-letter variables like c.

> Source/WebCore/editing/FrameSelection.cpp:586
> +    VisiblePosition pos = nextWordPosition(c);

Please don't use abbreviations like "pos". Also, "position" is a very generic name that doesn't really tell us the semantics.
How about "currentPosition"?

> Source/WebCore/editing/FrameSelection.cpp:590
> +    // In Windows, moving by word moves until the beginning of the
> +    // next word. We emulate that behavior here by moving twice
> +    // forward (intermediatePosition) and then once back.

I'm not sure if this comment is all that helpful. Given that this is an editing behavior, it's no longer tied to Windows or that it may change in the future.
Given that the code already tells us we're moving forward twice and going backwards once, we're probably repeating ourselves needlessly.
In WebKit, we try to avoid adding comments like this that merely explains what the code does as much as possible.
If we could explain *why* we do it, that's useful.

> Source/WebCore/editing/FrameSelection.cpp:592
> +        VisiblePosition intermediatePosition = nextWordPosition(pos);

"intermediate" is not a descriptive name either. Something like positionAfterCurrentWord would better. In particular, this emphasizes the fact it's not after the whitespace.

> Source/WebCore/editing/FrameSelection.cpp:597
> +        // When moving back, it is possible that we end up in the
> +        // start of the current word. In that case, just move until
> +        // the intermediate position.

Instead of adding a long comment line this, we can give the variable a more describe name.

> Source/WebCore/editing/FrameSelection.cpp:599
> +        VisiblePosition wordStart = previousWordPosition(nextWordPosition(c));
> +        if (pos == wordStart)

e.g. we can do something like
bool movingBackwardsTwiceMovedPositionToStartOfCurrentWord = intermiediatePosition == previousWordPosition(nextWordPosition(c));

> LayoutTests/ChangeLog:8
> +        This is a massive review of all tests that use word selection for

Massive review? or massive rebaseline?

> LayoutTests/ChangeLog:28
> +        uncovered a bug in execCommand(), reported now as wkb112240, that

Please use a regular URL like http://webkit.org/b/112240. "wkb" is not a thing.

> LayoutTests/ChangeLog:32
> +        * editing/deleting/smart-editing-disabled-mac-expected.txt: Copied from LayoutTests/editing/deleting/smart-editing-disabled-expected.txt.
> +        * editing/deleting/smart-editing-disabled-mac.html: Copied from LayoutTests/editing/deleting/smart-editing-disabled.html.

These lines are really long. Please line break somewhere. And ditto for the following lines.

> LayoutTests/TestExpectations:12
> +# pending bug fix
> +webkit.org/b/112240 editing/execCommand/remove-format-multiple-elements-win.html [ Skip ]

Please don't skip an existing test on all platforms. r- because of this change.
You may add [ Failure ] expectation on some platform where this test fails but it's not acceptable to disable a test on all platforms.
If we need to fix a test, then please fix it before submitting a patch on this bug.
------- Comment #45 From 2013-03-26 06:10:27 PST -------
(From update of attachment 194615 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=194615&action=review

Thanks for the review. A few comments follow.

>> Source/WebCore/editing/FrameSelection.cpp:599
>> +        if (pos == wordStart)
> 
> e.g. we can do something like
> bool movingBackwardsTwiceMovedPositionToStartOfCurrentWord = intermiediatePosition == previousWordPosition(nextWordPosition(c));

It would be movingBackwardsMovedPositionToStasrtOfCurrentWord, as we never move backward twice, only once.

>> LayoutTests/ChangeLog:8
>> +        This is a massive review of all tests that use word selection for
> 
> Massive review? or massive rebaseline?

Some tests have been rebaselined, others fixed differently.

>> LayoutTests/TestExpectations:12
>> +webkit.org/b/112240 editing/execCommand/remove-format-multiple-elements-win.html [ Skip ]
> 
> Please don't skip an existing test on all platforms. r- because of this change.
> You may add [ Failure ] expectation on some platform where this test fails but it's not acceptable to disable a test on all platforms.
> If we need to fix a test, then please fix it before submitting a patch on this bug.

It is not an existing test. It is a new test (see the ChangeLog entry fot that test.) It fails because of a completely different bug I have found that is unrelated to this one (bug 112240, already existing and affecting many editor commands). The test fails in all platforms because, as specified by the filename, the test forces a windows editing behavior regardless of the platform where it runs.

Does Failure runs the test anyway? I think that would make sense, but as I said this will fail in all platforms until the bug is addressed. Another option is not to add it or to add an -expected file that takes into consideration the failure.
------- Comment #46 From 2013-03-26 06:21:21 PST -------
Created an attachment (id=195074) [details]
Patch without tests, in order to assess effects of code changes.

This patch has no tests so I don't expect it to be committed. It addresses your comments regarding variable naming and code comments. We can check with this what breaks.
------- Comment #47 From 2013-03-26 07:13:19 PST -------
Created an attachment (id=195080) [details]
Patch without tests without format issues.
------- Comment #48 From 2013-03-26 10:15:27 PST -------
(From update of attachment 195080 [details])
Attachment 195080 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17309297
------- Comment #49 From 2013-03-26 10:16:49 PST -------
(In reply to comment #45)
> >> LayoutTests/TestExpectations:12
> >> +webkit.org/b/112240 editing/execCommand/remove-format-multiple-elements-win.html [ Skip ]
> > 
> > Please don't skip an existing test on all platforms. r- because of this change.
> > You may add [ Failure ] expectation on some platform where this test fails but it's not acceptable to disable a test on all platforms.
> > If we need to fix a test, then please fix it before submitting a patch on this bug.
> 
> It is not an existing test. It is a new test (see the ChangeLog entry fot that test.) It fails because of a completely different bug I have found that is unrelated to this one (bug 112240, already existing and affecting many editor commands). The test fails in all platforms because, as specified by the filename, the test forces a windows editing behavior regardless of the platform where it runs.

No. The test was passing in Windows editing behavior prior to this patch. This patch introduces new behavior and reveals the bug that had previously not encountered on Windows. Unfortunately, that's not acceptable in WebKit. We need to fix the said bug prior to fixing this bug.
------- Comment #50 From 2013-03-26 10:31:10 PST -------
(From update of attachment 195080 [details])
Attachment 195080 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17318227

New failing tests:
editing/selection/selection-extend-should-not-move-across-caret-on-mac.html
fast/repaint/japanese-rl-selection-repaint-in-regions.html
editing/execCommand/query-command-state.html
------- Comment #51 From 2013-03-26 10:31:15 PST -------
Created an attachment (id=195110) [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
------- Comment #52 From 2013-03-26 14:10:00 PST -------
(From update of attachment 195080 [details])
Attachment 195080 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17112914

New failing tests:
editing/execCommand/remove-formatting-2.html
editing/inserting/paragraph-separator-01.html
editing/execCommand/createLink.html
editing/execCommand/button.html
editing/pasteboard/8145-2.html
editing/inserting/insert-paragraph-03.html
editing/pasteboard/5006779.html
editing/selection/4932260-2.html
editing/inserting/4960120-2.html
editing/deleting/list-item-1.html
editing/deleting/smart-delete-across-editable-boundaries.html
editing/execCommand/remove-format-multiple-elements.html
editing/execCommand/toggle-link.html
editing/inserting/paragraph-outside-nested-divs.html
editing/inserting/insert-before-link-1.html
editing/pasteboard/4806874.html
editing/deleting/delete-cell-contents.html
editing/deleting/smart-editing-disabled.html
editing/deleting/non-smart-delete.html
editing/execCommand/unlink.html
editing/execCommand/indent-pre.html
editing/deleting/delete-by-word-001.html
editing/execCommand/hilitecolor.html
editing/pasteboard/bad-placeholder.html
editing/inserting/insert-paragraph-04.html
editing/deleting/smart-delete-002.html
editing/deleting/delete-by-word-002.html
editing/pasteboard/4242293-1.html
http/tests/cache/subresource-failover-to-network.html
editing/execCommand/query-command-state.html
------- Comment #53 From 2013-03-26 14:10:05 PST -------
Created an attachment (id=195158) [details]
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
------- Comment #54 From 2013-03-26 23:10:25 PST -------
(From update of attachment 195080 [details])
Attachment 195080 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17221808

New failing tests:
editing/selection/selection-extend-should-not-move-across-caret-on-mac.html
editing/execCommand/query-command-state.html
------- Comment #55 From 2013-03-26 23:10:30 PST -------
Created an attachment (id=195230) [details]
Archive of layout-test-results from webkit-ews-04 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-future  Platform: Mac OS X 10.8.2
------- Comment #56 From 2013-04-01 23:47:09 PST -------
(From update of attachment 195080 [details])
Flipping to get updated test results after the patches to tests that have landed.
------- Comment #57 From 2013-04-02 08:06:34 PST -------
Created an attachment (id=196141) [details]
Patch without tests

Reuploaded to get EWS to check it again.
------- Comment #58 From 2013-04-03 05:24:30 PST -------
(From update of attachment 196141 [details])
Attachment 196141 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17304705

New failing tests:
editing/style/remove-underline-across-paragraph-in-bold.html
editing/selection/selection-extend-should-not-move-across-caret-on-mac.html
editing/execCommand/createLink.html
editing/style/remove-underline-from-stylesheet.html
editing/execCommand/button.html
editing/pasteboard/merge-end-table-2.html
editing/pasteboard/page-zoom.html
editing/style/remove-underline-across-paragraph.html
editing/inserting/insert-before-link-1.html
editing/style/remove-underline.html
editing/deleting/delete-cell-contents.html
editing/undo/replace-text-in-node-preserving-markers-crash.html
editing/execCommand/unlink.html
editing/execCommand/indent-pre.html
editing/style/unbold-in-bold.html
editing/style/apply-through-end-of-document.html
editing/selection/extend-selection-word.html
editing/pasteboard/merge-end-list-2.html
editing/deleting/delete-by-word-002.html
editing/pasteboard/display-block-on-spans.html
editing/style/remove-underline-in-bold.html
editing/execCommand/query-command-state.html
------- Comment #59 From 2013-04-03 05:24:37 PST -------
Created an attachment (id=196331) [details]
Archive of layout-test-results from gce-cr-linux-07 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
------- Comment #60 From 2013-04-17 05:55:34 PST -------
(In reply to comment #49)
> (In reply to comment #45)

> > It is not an existing test. It is a new test (see the ChangeLog entry fot that test.) It fails because of a completely different bug I have found that is unrelated to this one (bug 112240, already existing and affecting many editor commands). The test fails in all platforms because, as specified by the filename, the test forces a windows editing behavior regardless of the platform where it runs.
> 
> No. The test was passing in Windows editing behavior prior to this patch. This patch introduces new behavior and reveals the bug that had previously not encountered on Windows. Unfortunately, that's not acceptable in WebKit. We need to fix the said bug prior to fixing this bug.

Bug 112240 was fixed! I'll prepare a new patch updating the tests now. It might take a bit of time with all the changes in the WK tree post-blink.
------- Comment #61 From 2013-04-17 10:33:31 PST -------
Created an attachment (id=198583) [details]
Patch
------- Comment #62 From 2013-04-23 13:38:20 PST -------
(From update of attachment 198583 [details])
Clearing flags on attachment: 198583

Committed r148987: <http://trac.webkit.org/changeset/148987>
------- Comment #63 From 2013-04-23 13:38:28 PST -------
All reviewed patches have been landed.  Closing bug.