[webkit-reviews] review granted: [Bug 52644] Stop instantiating legacy editing positions in DeleteSelectionCommand, IndentOudentCommand, InsertLineBreakCommand, InsertListCOmmand.cpp, InsertParagraphSeparatorCommand, and htmlediting.cpp : [Attachment 79302] cleanup

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 52644: Stop instantiating legacy editing positions in
DeleteSelectionCommand, IndentOudentCommand, InsertLineBreakCommand,
InsertListCOmmand.cpp, InsertParagraphSeparatorCommand, and htmlediting.cpp
https://bugs.webkit.org/show_bug.cgi?id=52644

Attachment 79302: cleanup
https://bugs.webkit.org/attachment.cgi?id=79302&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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?