Bug 24966 - Move RangeBoundaryPoint off of Position
Summary: Move RangeBoundaryPoint off of Position
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 24763
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-31 15:17 PDT by Eric Seidel (no email)
Modified: 2009-04-07 07:27 PDT (History)
4 users (show)

See Also:


Attachments
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition() (4.68 KB, patch)
2009-03-31 15:27 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Move RangeBoundaryPoint off of Position, per Darin's suggestion (11.24 KB, patch)
2009-03-31 15:27 PDT, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition() (4.87 KB, patch)
2009-03-31 19:13 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Move RangeBoundaryPoint off of Position, per Darin's suggestion (11.24 KB, patch)
2009-03-31 19:13 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Make Position::m_offset private and fix callers to use either deprecatedEditingOffset() or offsetInContainer() (80.42 KB, patch)
2009-03-31 19:14 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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