Bug 63473 - Move m_isDirectional from FrameSelection to VisibleSelection
Summary: Move m_isDirectional from FrameSelection to VisibleSelection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 60529
  Show dependency treegraph
 
Reported: 2011-06-27 12:30 PDT by Ryosuke Niwa
Modified: 2011-07-01 14:10 PDT (History)
13 users (show)

See Also:


Attachments
refactoring, fixes a bug (18.67 KB, patch)
2011-06-27 15:46 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed chromium build (20.09 KB, patch)
2011-06-27 16:21 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
added rebaselines required by operator== change (64.24 KB, patch)
2011-06-30 16:58 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.28 MB, application/zip)
2011-06-30 17:29 PDT, WebKit Review Bot
no flags Details
Fixed per comment (3.51 KB, patch)
2011-07-01 11:59 PDT, Ryosuke Niwa
ojan: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-06-27 12:30:34 PDT
Because m_isDirectional needs to preserved over undo/redo, it seems more natural for VisibleSelection to have m_isDirectional instead of FrameSelection.
Comment 1 Ryosuke Niwa 2011-06-27 15:46:08 PDT
Created attachment 98805 [details]
refactoring, fixes a bug
Comment 2 WebKit Review Bot 2011-06-27 16:02:34 PDT
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
Comment 3 Ryosuke Niwa 2011-06-27 16:21:23 PDT
Created attachment 98810 [details]
fixed chromium build
Comment 4 Ryosuke Niwa 2011-06-28 15:04:13 PDT
Ping reviewers
Comment 5 Ryosuke Niwa 2011-06-30 10:40:17 PDT
Any reviewers?
Comment 6 Justin Garcia 2011-06-30 11:20:48 PDT
(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.
Comment 7 Justin Garcia 2011-06-30 11:23:53 PDT
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
Comment 8 Ojan Vafai 2011-06-30 12:07:28 PDT
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.
Comment 9 Ojan Vafai 2011-06-30 14:41:17 PDT
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?
Comment 10 Ryosuke Niwa 2011-06-30 15:10:47 PDT
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?
Comment 11 Ryosuke Niwa 2011-06-30 15:26:27 PDT
(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!
Comment 12 Ryosuke Niwa 2011-06-30 16:58:59 PDT
Created attachment 99398 [details]
added rebaselines required by operator== change
Comment 13 Ryosuke Niwa 2011-06-30 17:00:32 PDT
It turned out that modifying operator= adds one editing delete callbacks in many editing tests :(
Comment 14 WebKit Review Bot 2011-06-30 17:29:49 PDT
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
Comment 15 WebKit Review Bot 2011-06-30 17:29:54 PDT
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
Comment 16 Ryosuke Niwa 2011-06-30 17:30:48 PDT
(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.
Comment 17 Ryosuke Niwa 2011-07-01 10:27:42 PDT
Justin & Ojan, could you review new patch?
Comment 18 Ryosuke Niwa 2011-07-01 11:59:43 PDT
Created attachment 99497 [details]
Fixed per comment
Comment 19 Ojan Vafai 2011-07-01 12:25:14 PDT
Comment on attachment 99497 [details]
Fixed per comment

Wrong bug.
Comment 20 Ryosuke Niwa 2011-07-01 12:29:52 PDT
(In reply to comment #19)
> (From update of attachment 99497 [details])
> Wrong bug.

Oops, sorry. :(  Could you restore your r+ on other patch.
Comment 21 Ryosuke Niwa 2011-07-01 14:07:44 PDT
Comment on attachment 99398 [details]
added rebaselines required by operator== change

Clearing flags on attachment: 99398

Committed r90275: <http://trac.webkit.org/changeset/90275>
Comment 22 Ryosuke Niwa 2011-07-01 14:07:51 PDT
All reviewed patches have been landed.  Closing bug.