Stop instantiating legacy editing positions in DeleteSelectionCommand, IndentOudentCommand, InsertLineBreakCommand, InsertListCOmmand.cpp, InsertParagraphSeparatorCommand, and htmlediting.cpp

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Cleanup in the following files:
Source/WebCore/editing/CompositeEditCommand.cpp
Source/WebCore/editing/DeleteSelectionCommand.cpp
Source/WebCore/editing/IndentOutdentCommand.cpp
Source/WebCore/editing/InsertLineBreakCommand.cpp
Source/WebCore/editing/InsertListCommand.cpp
Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp
Source/WebCore/editing/htmlediting.cpp

[Ryosuke Niwa]

Created attachment 79302 [details]
cleanup

[Eric Seidel (no email)]

Comment on attachment 79302 [details]
cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=79302&action=review

OK.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:111
>      // For HRs, we'll get a position at (HR,1) when hitting delete from the beginning of the previous line, or (HR,0) when forward deleting,

Seems we need to update this comment?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:321
> +    switch (position.anchorType()) {
> +    case Position::PositionIsOffsetInAnchor:
> +        if (position.containerNode() == node->parentNode() && static_cast<unsigned>(position.offsetInContainerNode()) > node->nodeIndex())
> +            position.moveToOffset(position.offsetInContainerNode() - 1);

I wonder if this method should move onto the Position class.

I also wonder if DeleteSelectionCommand wants to be using RangeEndPoint intsead of Position, since don't ranges do this sort of thing automatically?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:489
> +                    m_downstreamEnd.moveToOffset(m_downstreamEnd.deprecatedEditingOffset() - 1);

I guess moveToOffset does not ASSERT that the offset is valid?

Can we use something other than deprecatedEditingOffset() here? or is that a separate change?

[Ryosuke Niwa]

Comment on attachment 79302 [details]
cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=79302&action=review

Thanks for the review, Eric.  Unfortunately, I can't make further changes because of the chicken or the egg dilemma.

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:111
>>      // For HRs, we'll get a position at (HR,1) when hitting delete from the beginning of the previous line, or (HR,0) when forward deleting,
> 
> Seems we need to update this comment?

This comment is about start/end passed onto DeleteSelectionCommand, which is probably generated by upstream/downstream, and not about the positions we're expanding to.  So I will not remove.

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:321
>> +            position.moveToOffset(position.offsetInContainerNode() - 1);
> 
> I wonder if this method should move onto the Position class.
> 
> I also wonder if DeleteSelectionCommand wants to be using RangeEndPoint intsead of Position, since don't ranges do this sort of thing automatically?

You're right.  DeleteSelectionCommand should be using a Range but I guess that refactoring can only be done after we got rid of legacy editing positions because ranges aren't compatible with legacy editing positions.

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:489
>> +                    m_downstreamEnd.moveToOffset(m_downstreamEnd.deprecatedEditingOffset() - 1);
> 
> I guess moveToOffset does not ASSERT that the offset is valid?
> 
> Can we use something other than deprecatedEditingOffset() here? or is that a separate change?

I can call offsetInContainerNode but that isn't really correct in the sense that the code around it doesn't really consider positions before/after node, and they also call .node() all over the place.  I'm planning to remove all calls to Position::node(), and not calling deprecatedEditingOffset() here should be a part of that change.

[Ryosuke Niwa]

Committed r76057: <http://trac.webkit.org/changeset/76057>