Because m_isDirectional needs to preserved over undo/redo, it seems more natural for VisibleSelection to have m_isDirectional instead of FrameSelection.
Created attachment 98805 [details] refactoring, fixes a bug
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
Created attachment 98810 [details] fixed chromium build
Ping reviewers
Any reviewers?
(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 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
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 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 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?
(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!
Created attachment 99398 [details] added rebaselines required by operator== change
It turned out that modifying operator= adds one editing delete callbacks in many editing tests :(
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
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
(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.
Justin & Ojan, could you review new patch?
Created attachment 99497 [details] Fixed per comment
Comment on attachment 99497 [details] Fixed per comment Wrong bug.
(In reply to comment #19) > (From update of attachment 99497 [details]) > Wrong bug. Oops, sorry. :( Could you restore your r+ on other patch.
Comment on attachment 99398 [details] added rebaselines required by operator== change Clearing flags on attachment: 99398 Committed r90275: <http://trac.webkit.org/changeset/90275>
All reviewed patches have been landed. Closing bug.