Bug 63473 – Move m_isDirectional from FrameSelection to VisibleSelection

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Because m_isDirectional needs to preserved over undo/redo, it seems more natural for VisibleSelection to have m_isDirectional instead of FrameSelection.

[Ryosuke Niwa]

Created attachment 98805 [details]
refactoring, fixes a bug

[WebKit Review Bot]

Comment on attachment 98805 [details]
refactoring, fixes a bug

Attachment 98805 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8954087

[Ryosuke Niwa]

Created attachment 98810 [details]
fixed chromium build

[Ryosuke Niwa]

Ping reviewers

[Ryosuke Niwa]

Any reviewers?

[Justin Garcia]

(In reply to comment #0)
> Because m_isDirectional needs to preserved over undo/redo, it seems more natural for VisibleSelection to have m_isDirectional instead of FrameSelection.

In TextEdit, the directionality of the selection doesn’t appear to be preserved doing Undo. I typed:

hello world this is a test

Placed the caret before "world". Extended it a few characters to the right (modifying the end) with Shift+Right Arrow.
Cut
Undo
Shift+Arrow modifies the start of the selection, not the end.

Also it seems odd to me that there are m_frame NULL checks in something called FrameSelection.

[Justin Garcia]

Comment on attachment 98810 [details]
fixed chromium build

> In TextEdit, the directionality of the selection doesn’t appear to be preserved doing Undo.

Never mind. This is exactly what your patch fixes. r=me

[Ojan Vafai]

Can you hold off for a bit from committing this? I'd like to take a look through it, but I haven't had the time. I'll get to it today.

[Ojan Vafai]

Comment on attachment 98810 [details]
fixed chromium build

View in context: https://bugs.webkit.org/attachment.cgi?id=98810&action=review

Seems fine to me, except I think you need to change operator==. Thanks for holding off on committing.

Took me a bit to page all this code back in. I remembered removing granularity from VisibleSelection and wanted to make sure that's consistent with this change. I think it makes sense to say that the directionality is a part of the selection, but the granularity is not since maintaining the granularity only matters when you're in the middle of making a mouse-based selection.

> Source/WebCore/editing/VisibleSelection.h:141
>  inline bool operator==(const VisibleSelection& a, const VisibleSelection& b)

Shouldn't this be checking to make sure the directionality of the two selections is equal?

[Ryosuke Niwa]

Comment on attachment 98810 [details]
fixed chromium build

View in context: https://bugs.webkit.org/attachment.cgi?id=98810&action=review

>> Source/WebCore/editing/VisibleSelection.h:141
>>  inline bool operator==(const VisibleSelection& a, const VisibleSelection& b)
> 
> Shouldn't this be checking to make sure the directionality of the two selections is equal?

Huh, I'm surprised that we even have operator== and operator!=.  Can we just get rid of it?

[Ryosuke Niwa]

(In reply to comment #10)
> Huh, I'm surprised that we even have operator== and operator!=.  Can we just get rid of it?

I guess not.  Will fix before landing it.

And thank you for the review Justin & Ojan!

[Ryosuke Niwa]

Created attachment 99398 [details]
added rebaselines required by operator== change

[Ryosuke Niwa]

It turned out that modifying operator= adds one editing delete callbacks in many editing tests :(

[WebKit Review Bot]

Comment on attachment 99398 [details]
added rebaselines required by operator== change

Attachment 99398 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8970235

New failing tests:
editing/selection/anchor-focus2.html
editing/selection/anchor-focus3.html
editing/deleting/delete-br-011.html

[WebKit Review Bot]

Created attachment 99406 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Ryosuke Niwa]

(In reply to comment #14)
> (From update of attachment 99398 [details])
> Attachment 99398 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8970235
> 
> New failing tests:
> editing/selection/anchor-focus2.html
> editing/selection/anchor-focus3.html
> editing/deleting/delete-br-011.html

These tests probably have platform specific results.

[Ryosuke Niwa]

Justin & Ojan, could you review new patch?

[Ryosuke Niwa]

Created attachment 99497 [details]
Fixed per comment

[Ojan Vafai]

Comment on attachment 99497 [details]
Fixed per comment

Wrong bug.

[Ryosuke Niwa]

(In reply to comment #19)
> (From update of attachment 99497 [details])
> Wrong bug.

Oops, sorry. :(  Could you restore your r+ on other patch.

[Ryosuke Niwa]

Comment on attachment 99398 [details]
added rebaselines required by operator== change

Clearing flags on attachment: 99398

Committed r90275: <http://trac.webkit.org/changeset/90275>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.