Bug 52676 - Stop instantiating legacy editing positions in InsertTextCommand, MoveSelectionCommand, ReplaceSelectionCommand, SelectionController, SpellChecker, TypingCommand, and markup.cpp
Summary: Stop instantiating legacy editing positions in InsertTextCommand, MoveSelecti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 52099
  Show dependency tree / graph
 
Reported: 2011-01-18 15:52 PST by Ryosuke Niwa
Modified: 2011-01-24 18:22 PST (History)
4 users (show)

See Also:

Attachments
cleanup (19.98 KB, patch)
2011-01-18 15:55 PST, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
cleanup (20.03 KB, patch)
2011-01-18 15:57 PST, Ryosuke Niwa 		eric: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-01-18 15:52:52 PST 
This is a cleanup
Comment 1 Ryosuke Niwa 2011-01-18 15:55:58 PST 
Created attachment 79345 [details]
cleanup
Comment 2 Ryosuke Niwa 2011-01-18 15:57:07 PST 
Created attachment 79347 [details]
cleanup
Comment 3 Ryosuke Niwa 2011-01-24 08:31:25 PST 
Could someone review my patch?
Comment 4 Eric Seidel (no email) 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?
Comment 5 Ryosuke Niwa 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.
Comment 6 Ryosuke Niwa 2011-01-24 18:22:04 PST 
Committed r76560: <http://trac.webkit.org/changeset/76560>