Bug 24966 – Move RangeBoundaryPoint off of Position

• Component: HTML Editing
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Move RangeBoundaryPoint off of Position

Darin suggested over IRC that we move RangeBoundaryPoint off of Position.  This will cause some additional ref()-churn when returning from rbp.position(), but allows us to keep moving Positions away from exposing their container/offset pair.

[Eric Seidel (no email)]

Created attachment 29136 [details]
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()

 WebCore/ChangeLog        |   21 +++++++++++++
 WebCore/dom/Position.cpp |   76 ++++++++++++++++++++++++++++++++++++++++++++++
 WebCore/dom/Position.h   |   17 ++++++++++
 3 files changed, 114 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Created attachment 29137 [details]
Move RangeBoundaryPoint off of Position, per Darin's suggestion

 WebCore/ChangeLog                |   33 +++++++++++
 WebCore/dom/Position.h           |   16 +++--
 WebCore/dom/Range.cpp            |    8 +-
 WebCore/dom/Range.h              |    4 +-
 WebCore/dom/RangeBoundaryPoint.h |  114 +++++++++++++++++++++-----------------
 5 files changed, 113 insertions(+), 62 deletions(-)

[Eric Seidel (no email)]

All layout tests pass.

[Darin Adler]

Comment on attachment 29137 [details]
Move RangeBoundaryPoint off of Position, per Darin's suggestion

Looks good.

r=me

Do we need RangeBoundaryPoint::toPosition()? I think RangeBoundaryPoint may be a lower level class than Position, so conversion to a Position might insteadCan we get rid of Range::startPosition and Range::endPosition?

[Eric Seidel (no email)]

Comment on attachment 29136 [details]
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()

Uploading an improved patch in a sec.

[Eric Seidel (no email)]

Created attachment 29151 [details]
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()

 WebCore/ChangeLog        |   21 +++++++++++
 WebCore/dom/Position.cpp |   85 ++++++++++++++++++++++++++++++++++++++++++++++
 WebCore/dom/Position.h   |   17 +++++++++
 3 files changed, 123 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Created attachment 29152 [details]
Move RangeBoundaryPoint off of Position, per Darin's suggestion

 WebCore/ChangeLog                |   33 +++++++++++
 WebCore/dom/Position.h           |   16 +++--
 WebCore/dom/Range.cpp            |    8 +-
 WebCore/dom/Range.h              |    4 +-
 WebCore/dom/RangeBoundaryPoint.h |  114 +++++++++++++++++++++-----------------
 5 files changed, 113 insertions(+), 62 deletions(-)

[Eric Seidel (no email)]

Created attachment 29153 [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)]

Comment on attachment 29152 [details]
Move RangeBoundaryPoint off of Position, per Darin's suggestion

Uploaded again by accident.

[Darin Adler]

Why are these other patches in this bug? Given the title, doesn’t make sense.

[Eric Seidel (no email)]

I did these patches all as one series.  I'm happy to put the other two patches on separate bugs.  The "add containerNode()..." patch is actually the first patch.  But again, I'm happy to put them on other bugs if that would make things easier.

[Darin Adler]

(In reply to comment #11)
> I did these patches all as one series. I'm happy to put the other two patches
> on separate bugs. The "add containerNode()..." patch is actually the first
> patch. But again, I'm happy to put them on other bugs if that would make
> things easier.

If you want to do this all in one bug report, that's fine. But please give the bug report a title that corresponds to the patches. These patches aren’t about "move RangeBoundaryPoint off of Position", and I have trouble understanding their context so I can give them a good review without a bug title explaining the high level task.

[Eric Seidel (no email)]

Comment on attachment 29151 [details]
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()

Adding these to other bugs per Darin's suggestion.

[Eric Seidel (no email)]

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

Moving this patch to a new bug (bug 25056) per Darin's suggestion.

[Eric Seidel (no email)]

I will land this once the first patch from bug 24763 is reviewed (as this patch was created as part of a series of patches and currently depends on that one... although doesn't in the end need to).

[Eric Seidel (no email)]

Thanks for the review darin.  Landed as:
        M       WebCore/ChangeLog
        M       WebCore/dom/Position.h
        M       WebCore/dom/Range.cpp
        M       WebCore/dom/Range.h
        M       WebCore/dom/RangeBoundaryPoint.h
Committed r42264