Bug 25056 – Make editing stop using Position::m_offset directly

• Status: NEW
• Component: HTML Editing

[Eric Seidel (no email)]

Make editing stop using Position::m_offset directly

In order for us to support neighbor-anchored positions (and to stop having confusion about what m_offset means in different sections of code) we need to make Position::m_offset private.  I've attached a patch to do that.

[Eric Seidel (no email)]

Created attachment 29273 [details]
Make Position::m_offset private and fix callers to use either deprecatedEditingOffset() or offsetInContainer()

 LayoutTests/ChangeLog                              |   11 ++
 .../assert-on-range-creation-expected.txt          |    1 +
 .../execCommand/assert-on-range-creation.html      |   12 ++
 WebCore/ChangeLog                                  |  140 ++++++++++++++++++++
 WebCore/WebCore.base.exp                           |    2 +
 WebCore/dom/Position.h                             |   19 ++-
 WebCore/dom/PositionIterator.h                     |    4 +-
 WebCore/dom/Range.cpp                              |   19 +++-
 WebCore/dom/Range.h                                |    4 +
 WebCore/editing/ApplyStyleCommand.cpp              |   90 +++++++------
 WebCore/editing/BreakBlockquoteCommand.cpp         |    8 +-
 WebCore/editing/CompositeEditCommand.cpp           |   34 +++---
 WebCore/editing/DeleteSelectionCommand.cpp         |   47 ++++----
 WebCore/editing/Editor.cpp                         |   14 +-
 WebCore/editing/InsertLineBreakCommand.cpp         |    6 +-
 .../editing/InsertParagraphSeparatorCommand.cpp    |   14 +-
 WebCore/editing/InsertTextCommand.cpp              |   12 +-
 WebCore/editing/MoveSelectionCommand.cpp           |    6 +-
 WebCore/editing/ReplaceSelectionCommand.cpp        |    2 +-
 WebCore/editing/SelectionController.cpp            |    4 +-
 WebCore/editing/TextIterator.cpp                   |    2 +-
 WebCore/editing/TypingCommand.cpp                  |   10 +-
 WebCore/editing/VisiblePosition.cpp                |   46 +++----
 WebCore/editing/VisibleSelection.cpp               |   12 +-
 WebCore/editing/htmlediting.cpp                    |   20 ++-
 WebCore/editing/visible_units.cpp                  |   40 +++---
 WebCore/page/AccessibilityObject.cpp               |    2 +-
 WebCore/page/AccessibilityRenderObject.cpp         |    4 +-
 WebCore/page/DOMSelection.cpp                      |   16 +-
 WebCore/page/EventHandler.cpp                      |    2 +-
 WebCore/page/Frame.cpp                             |    2 +-
 WebCore/page/mac/AccessibilityObjectWrapper.mm     |    4 +-
 WebCore/rendering/RenderTextControl.cpp            |    2 +-
 WebCore/rendering/RenderTreeAsText.cpp             |    6 +-
 WebKit/mac/ChangeLog                               |   11 ++
 WebKit/mac/WebView/WebFrame.mm                     |    7 +-
 36 files changed, 424 insertions(+), 211 deletions(-)

[Eric Seidel (no email)]

*** Bug 19221 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Is there anything I can do for you two gentlemen to make this easier to review?

I'm pretty much done with my SVG rendering tree re-write (at least I've fixed the bugs I went in to fix) and plan to start working on fixing position/selection objects in Editing again as of tomorrow.  Further work is at least partially blocked by this change.

[Darin Adler]

Comment on attachment 29273 [details]
Make Position::m_offset private and fix callers to use either deprecatedEditingOffset() or offsetInContainer()

I think you should first make a patch that changes all use of m_offset to deprecatedEditingOffset(), then a separate patch that moves to using the other functions such as computeOffsetInContainerNode. The renaming should be separate from the behavior change.

> +    // This silently converts the position to being parent relative before setting it on the Range

That's a strange comment. What else could the function do? What do you mean by "silently"?

>  short Range::compareBoundaryPoints(const Position& a, const Position& b)
>  {
> -    return compareBoundaryPoints(a.node(), a.m_offset, b.node(), b.m_offset);
> +    // FIXME: Some callers pass in sibling-anchored positions.
> +    // Those callers should move to a new comparePositions function, then
> +    // compareBoundaryPoints need only care about parent-anchored node/offset pairs.
> +    return compareBoundaryPoints(a.anchorNode(), a.deprecatedEditingOffset(), b.anchorNode(), b.deprecatedEditingOffset());
>  }

The change above seems wrong. Shouldn't this be using contaonerNode and computeOffsetInContainerNode?

> +    void setStart(const Position&, ExceptionCode&);
> +    void setEnd(const Position&, ExceptionCode&);

Your change makes Range lower level than Position. Range is based on raw DOM, and Position is coming into its own as a higher level editing building block.

Accordingly, I don't think these functions should be added to the Range class. We should remove any use of Position from Range.h and put helper functions for using Position with Range into Position.h or some other header.

> -    return Range::compareBoundaryPoints(node, 0, start.node(), start.m_offset) >= 0 &&
> +    return Range::compareBoundaryPoints(node, 0, start.anchorNode(), start.deprecatedEditingOffset()) >= 0 &&

> -    bool isFullyAfterEnd = Range::compareBoundaryPoints(node, 0, end.node(), end.m_offset) > 0;
> +    bool isFullyAfterEnd = Range::compareBoundaryPoints(node, 0, end.containerNode(), end.offsetInContainerNode()) > 0;

This seems strange to me. Two nearly identical cases, but in one you used anchorNode/deprecatedEditingOffset and in the other containerNode/offsetInContainerNode.

The patch is *way* too big for me to review.

[Darin Adler]

Comment on attachment 29273 [details]
Make Position::m_offset private and fix callers to use either deprecatedEditingOffset() or offsetInContainer()

I think the key here is to first create a patch that is guaranteed to not change behavior (using functions by new names, using functions instead of direct access to data members). And then have a much smaller patch for all the behavior changes.

Review should be straightforward for a patch that changes no behavior. And then we can carefully review the behavior changes without the distraction of the editorial-only changes.

[Eric Seidel (no email)]

Yes.  Sorry, I had meant to thank you for your previous comments.  I'm working on a new series of patches which should be easier to review.  Thank you again.