RESOLVED FIXED 52676
Stop instantiating legacy editing positions in InsertTextCommand, MoveSelectionCommand, ReplaceSelectionCommand, SelectionController, SpellChecker, TypingCommand, and markup.cpp 
https://bugs.webkit.org/show_bug.cgi?id=52676
Summary Stop instantiating legacy editing positions in InsertTextCommand, MoveSelecti...
Ryosuke Niwa
Reported 2011-01-18 15:52:52 PST
This is a cleanup
Attachments
cleanup (19.98 KB, patch)
2011-01-18 15:55 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
cleanup (20.03 KB, patch)
2011-01-18 15:57 PST, Ryosuke Niwa 		eric: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-01-18 15:55:58 PST
Created attachment 79345 [details] cleanup
Ryosuke Niwa
Comment 2 2011-01-18 15:57:07 PST
Created attachment 79347 [details] cleanup
Ryosuke Niwa
Comment 3 2011-01-24 08:31:25 PST
Could someone review my patch?
Eric Seidel (no email)
Comment 4 2011-01-24 12:03:10 PST
Comment on attachment 79347 [details] cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=79347&action=review Seems sane. Please see comments. > Source/WebCore/editing/InsertTextCommand.cpp:175 > + endPosition = Position(textNode, offset + text.length(), Position::PositionIsOffsetInAnchor); This seems like it will be off the end of the node? > Source/WebCore/editing/MoveSelectionCommand.cpp:50 > + if (pos.anchorType() == Position::PositionIsOffsetInAnchor && selectionEnd.anchorType() == Position::PositionIsOffsetInAnchor We need a less cumbersome way to write this check. > Source/WebCore/editing/TypingCommand.cpp:493 > + setEndingSelection(VisibleSelection(positionAfterNode(table), endingSelection().start(), DOWNSTREAM)); After node?
Ryosuke Niwa
Comment 5 2011-01-24 12:25:46 PST
Comment on attachment 79347 [details] cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=79347&action=review >> Source/WebCore/editing/InsertTextCommand.cpp:175 >> + endPosition = Position(textNode, offset + text.length(), Position::PositionIsOffsetInAnchor); > > This seems like it will be off the end of the node? It won't be because we're inserting text at offset. So endPosition is basically the position immediately after the text we just inserted. >> Source/WebCore/editing/MoveSelectionCommand.cpp:50 >> + if (pos.anchorType() == Position::PositionIsOffsetInAnchor && selectionEnd.anchorType() == Position::PositionIsOffsetInAnchor > > We need a less cumbersome way to write this check. I totally agree but we can probably do that kind of cleanup after we're done with the first phase of cleanup.
Ryosuke Niwa
Comment 6 2011-01-24 18:22:04 PST
Committed r76560: <http://trac.webkit.org/changeset/76560>
Note You need to log in before you can comment on or make changes to this bug.