Created attachment 79302 [details] cleanup

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?

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.

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