Bug 24966

Summary: Move RangeBoundaryPoint off of Position
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, jparent, justin.garcia, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Bug Depends on: 24763    
Bug Blocks:    
Attachments:
Description Flags
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()
none
Move RangeBoundaryPoint off of Position, per Darin's suggestion
darin: review+
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()
none
Move RangeBoundaryPoint off of Position, per Darin's suggestion
none
Make Position::m_offset private and fix callers to use either deprecatedEditingOffset() or offsetInContainer() none

Description Eric Seidel (no email) 2009-03-31 15:17:57 PDT
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.
Comment 1 Eric Seidel (no email) 2009-03-31 15:27:26 PDT
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(-)
Comment 2 Eric Seidel (no email) 2009-03-31 15:27:28 PDT
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(-)
Comment 3 Eric Seidel (no email) 2009-03-31 15:27:43 PDT
All layout tests pass.
Comment 4 Darin Adler 2009-03-31 15:53:28 PDT
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?
Comment 5 Eric Seidel (no email) 2009-03-31 18:57:20 PDT
Comment on attachment 29136 [details]
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()

Uploading an improved patch in a sec.
Comment 6 Eric Seidel (no email) 2009-03-31 19:13:49 PDT
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(-)
Comment 7 Eric Seidel (no email) 2009-03-31 19:13:52 PDT
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(-)
Comment 8 Eric Seidel (no email) 2009-03-31 19:14:09 PDT
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(-)
Comment 9 Eric Seidel (no email) 2009-03-31 19:14:27 PDT
Comment on attachment 29152 [details]
Move RangeBoundaryPoint off of Position, per Darin's suggestion

Uploaded again by accident.
Comment 10 Darin Adler 2009-03-31 21:37:22 PDT
Why are these other patches in this bug? Given the title, doesn’t make sense.
Comment 11 Eric Seidel (no email) 2009-03-31 23:19:45 PDT
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.
Comment 12 Darin Adler 2009-04-01 13:37:05 PDT
(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.
Comment 13 Eric Seidel (no email) 2009-04-06 02:08:18 PDT
Comment on attachment 29151 [details]
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()

Adding these to other bugs per Darin's suggestion.
Comment 14 Eric Seidel (no email) 2009-04-06 02:08:49 PDT
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.
Comment 15 Eric Seidel (no email) 2009-04-06 02:27:54 PDT
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).
Comment 16 Eric Seidel (no email) 2009-04-07 07:27:08 PDT
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