Bug 52676

Summary: Stop instantiating legacy editing positions in InsertTextCommand, MoveSelectionCommand, ReplaceSelectionCommand, SelectionController, SpellChecker, TypingCommand, and markup.cpp
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Enhancement CC: darin, eric, ojan, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 52099    
Attachments:
Description Flags
cleanup
none
cleanup eric: review+

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>