Bug 24760 – Rename Position::container to Position::m_anchorNode and fix callsites

• Component: HTML Editing
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Clean up Position.h

I would like to make Position objects aware of the meaning of their container and m_offset values.  This will require removing direct construction of Positions objects.

Eventually positions will have the ability to specify [node, after], [node, before] in addition to [node, childOffset].  Positions already have this ability, but only for content which editing "ignores" (editingIgnoresContent(node) returns true).  When editingIgnoresContent returns true, then Positions [node, 0] and [node, 1] are interpreted as [node, before] and [node, after] by some parts of the editing code, but not all.  you don't actually know what type of position you're dealing with up-front, only in certain places will [node, 0 (before)] be converted into [node->parentNode(), node->nodeIndex()].

I would like to fix positions to hide the details of m_offset and container (which is not always really the "container" for the position, and add new accessors to get that kind of information in a consistent way for all positions.

Anyway, the first steps for this are to clean up Position objects to not make m_offset and container public.  I'll attach patches to do this.

[Eric Seidel (no email)]

Created attachment 28855 [details]
Remove Position::clear() and make VisibleSelection.h not include VisiblePosition.h

 WebCore/ChangeLog                                  |   57 ++++++++++++++++++++
 WebCore/dom/Position.cpp                           |    6 +-
 WebCore/dom/Position.h                             |   12 ++--
 WebCore/dom/RangeBoundaryPoint.h                   |    2 +-
 WebCore/editing/ApplyStyleCommand.cpp              |    1 +
 WebCore/editing/CompositeEditCommand.cpp           |    1 +
 WebCore/editing/DeleteButtonController.cpp         |    1 +
 WebCore/editing/DeleteSelectionCommand.cpp         |   15 +++---
 WebCore/editing/FormatBlockCommand.cpp             |    1 +
 WebCore/editing/IndentOutdentCommand.cpp           |    1 +
 WebCore/editing/InsertListCommand.cpp              |    1 +
 .../editing/InsertParagraphSeparatorCommand.cpp    |    1 +
 WebCore/editing/InsertTextCommand.cpp              |    1 +
 WebCore/editing/ReplaceSelectionCommand.cpp        |    1 +
 WebCore/editing/SelectionController.cpp            |    1 +
 WebCore/editing/TextIterator.cpp                   |    1 -
 WebCore/editing/VisiblePosition.h                  |    9 ++--
 WebCore/editing/VisibleSelection.cpp               |   10 ++++
 WebCore/editing/VisibleSelection.h                 |    8 ++--
 WebCore/editing/markup.cpp                         |    1 +
 WebCore/page/AccessibilityObject.h                 |    4 ++
 WebCore/page/AccessibilityRenderObject.h           |    1 +
 WebCore/page/DOMSelection.cpp                      |    1 +
 WebCore/page/DragController.cpp                    |    1 +
 WebCore/page/Frame.cpp                             |    9 ++--
 WebCore/page/mac/FrameMac.mm                       |    1 +
 WebCore/rendering/RenderBlock.cpp                  |    1 +
 WebCore/rendering/RenderBox.cpp                    |    1 +
 WebCore/rendering/RenderTextControl.cpp            |    1 +
 WebCore/rendering/TextControlInnerElements.cpp     |    1 +
 WebCore/svg/SVGTextContentElement.cpp              |    1 +
 31 files changed, 122 insertions(+), 31 deletions(-)

[Eric Seidel (no email)]

Created attachment 28856 [details]
Rename Position::container to m_container and make it private

 WebCore/ChangeLog                |   36 ++++++++++++++++++++++++++++++++++++
 WebCore/dom/Position.cpp         |    4 ++--
 WebCore/dom/Position.h           |   22 ++++++++++++++--------
 WebCore/dom/Range.cpp            |    4 ++--
 WebCore/dom/RangeBoundaryPoint.h |   24 ++++++++++--------------
 5 files changed, 64 insertions(+), 26 deletions(-)

[Darin Adler]

Comment on attachment 28855 [details]
Remove Position::clear() and make VisibleSelection.h not include VisiblePosition.h

> +
> +    // Returns node() or the closest element accestor
> +    Element* element() const;
>      Element* documentElement() const;

The paragraphing here makes it look like this comment is for both element() and documentElement(). The comment also seems to be unclear about what this will return when node() is not an element, but there is no closest element ancestor. The answer is "0", but the comment makes it sound like we'd return node(), which isn't possible since it's not an Element*.

Why are you removing clear()? I like clear(), and all the functions where you’re removing it seem, well, less clear after the change. What does removing it have to do with the other changes you’re planning?

r=me, but really?

[Eric Seidel (no email)]

(In reply to comment #3)
> Why are you removing clear()? I like clear(), and all the functions where
> you’re removing it seem, well, less clear after the change. What does removing
> it have to do with the other changes you’re planning?
>
> r=me, but really?

Maybe I should toss this one then.

My coding experience has lead me to dislike clear()/reset()/init() functions.  So easy to add to a class and forget to update all the "clear the state" functions.

In this case, I think I was more interested in making Position immutable.  You create a (lightweight) Position object once, and then you never edit it.

This is the third time I've started a "fix Position" patch (I'm certainly not complaining), this time I was trying to do all the little things first, before getting to the meat.  But maybe the little thing of killing clear() is not important.  I think you and I should have a heart-to-heart about clear() and why you like it and I don't over IRC some time.  Maybe there is some CS design book/principle which would make me like the clear() model over the "reset using a copy constructor and default initializer" model.

The main goal at the end of this rewrite, is to get rid of the [node, 0] can mean two different things depending on what editingIgnoresContent(node) returns.

[Eric Seidel (no email)]

(In reply to comment #4)
> ...  I think you and I should have a heart-to-heart about clear() and
> why you like it and I don't over IRC some time.

My point here was that you have a lot more coding experience than I do.  I don't tend to like to write patches which Darin Adler fundamentally disagrees with. ;)  (Especially when they're little cleanup ones like this.)  So I'd be interested in your insight as to why you prefer .clear() for these classes.

It's also possible that if we all understood where I was going with Position we'd all agree. :)

[Darin Adler]

(In reply to comment #4)
> In this case, I think I was more interested in making Position immutable.  You
> create a (lightweight) Position object once, and then you never edit it.

OK. But if you can assign to a Position then it’s not immutable.

[Darin Adler]

(In reply to comment #4)
> The main goal at the end of this rewrite, is to get rid of the [node, 0] can
> mean two different things depending on what editingIgnoresContent(node)
> returns.

I think it’s a great idea to create a way to represent before and after positions without having to compute a node index. This does make a fundamental change in Position, because Position now has capabilities that a plain old container/index pair does not.

My biggest concern is that if you do this, you programmers will have a difficult choice between using PassRefPtr<Node> or Node* with no reference count churn, or Position with greater capabilities and a stronger model, but mandatory reference count churn.

[Darin Adler]

Comment on attachment 28856 [details]
Rename Position::container to m_container and make it private

> -    RenderBlock* container = renderer->containingBlock();
> +    RenderBlock* m_container = renderer->containingBlock();
>      RenderObject* next = renderer;
> -    while ((next = next->nextInPreOrder(container))) {
> +    while ((next = next->nextInPreOrder(m_container))) {

Oops! Please don't make this change.

> +    
> +    // This constructor should be private
> +    Position(PassRefPtr<Node> c, int o)
> +        : m_container(c)
> +        , m_offset(o)
> +    {}

We put these on separate subsequent lines. I suggest naming the arguments container and offset.

> -    Node* node() const { return container.get(); }
> +    Node* node() const { return m_container.get(); }

I kinda hate this function name. Because, what node? I guess that's the core of what you're going to be fixing.

> +    RefPtr<Node> m_container;
> +public:
> +    int m_offset; // FIXME: This should be made private.

I suggest you add the offset() function in this patch even if you don't adopt it yet. Or don’t put that FIXME in.

> -    m_position.container = container;
> -    m_position.m_offset = offset;
> +    m_position = Position(container, offset);

This is more expensive than the old implementation. It churns the reference count of container one extra time. You should consider having a function that mutates the Position instead of assignment since it can be more efficient.

> -    m_position.container = child->parentNode();
>      m_childBefore = child->previousSibling();
> -    m_position.m_offset = m_childBefore ? invalidOffset : 0;
> +    m_position = Position(child->parentNode(), m_childBefore ? invalidOffset : 0);

Ditto.

>  inline void RangeBoundaryPoint::setToStart(PassRefPtr<Node> container)
>  {
>      ASSERT(container);
> -    m_position.container = container;
> -    m_position.m_offset = 0;
> +    m_position = firstDeepEditingPositionForNode(container);
>      m_childBefore = 0;
>  }

It looks to me like you’re sneaking in a behavior change here by calling firstDeepEditingPositionForNode. How about putting that in a separate patch and documenting what bug it fixes?

>  inline void RangeBoundaryPoint::setToEnd(PassRefPtr<Node> container)
>  {
>      ASSERT(container);
> -    m_position.container = container;
> -    if (m_position.container->offsetInCharacters()) {
> -        m_position.m_offset = m_position.container->maxCharacterOffset();
> +    if (container->offsetInCharacters()) {
> +        m_position = Position(container, container->maxCharacterOffset());
>          m_childBefore = 0;
>      } else {
> -        m_childBefore = m_position.container->lastChild();
> -        m_position.m_offset = m_childBefore ? invalidOffset : 0;
> +        m_childBefore = container->lastChild();
> +        m_position = Position(container, m_childBefore ? invalidOffset : 0);
>      }
>  }

Due to the use of invalidOffset in RangeBoundaryPoint, Position can’t really start treating things in an abstract way until it takes over the “childBefore” optimization. I’m also concerned that in the case of RangeBoundaryPoint, we have special requirements. It’s not OK to automatically translate a Position between multiple forms. Only time will tell, but this seems an easy area to get wrong.

review- for now because of some of the mistakes above.

[Eric Seidel (no email)]

(In reply to comment #7)
> My biggest concern is that if you do this, you programmers will have a
> difficult choice between using PassRefPtr<Node> or Node* with no reference
> count churn, or Position with greater capabilities and a stronger model, but
> mandatory reference count churn.

I don't understand.  Position objects currently ref the node pointer.  I intend to keep it that way.

Are you referring to the loss of setOffset() in an immutable Position model, and the need then to copy (and thus ref-churn) in order to change the Position?

[Eric Seidel (no email)]

(In reply to comment #9)
> I don't understand.

I wrote that comment before seeing your review comments.  I now understand.

The refchurn you identify only occurs when newNode == oldNode.  But I'm starting to think there is no strong need for Positions to be immutable.

[Darin Adler]

(In reply to comment #9)
> (In reply to comment #7)
> > My biggest concern is that if you do this, you programmers will have a
> > difficult choice between using PassRefPtr<Node> or Node* with no reference
> > count churn, or Position with greater capabilities and a stronger model, but
> > mandatory reference count churn.
> 
> I don't understand.  Position objects currently ref the node pointer.  I intend
> to keep it that way.

Position is a convenience today and in theory can get the same behavior more efficiently by having your own RefPtr/offset pair and use PassRefPtr as you like. There’s no PassPosition.

But the more that Position becomes an abstraction that does fancier things than a RefPtr/offset pair, the more the lack of a PassPosition becomes a problem. Maybe we need a PassPosition.

[Eric Seidel (no email)]

(In reply to comment #11)
> Position is a convenience today and in theory can get the same behavior more
> efficiently by having your own RefPtr/offset pair and use PassRefPtr as you
> like. There’s no PassPosition.
> 
> But the more that Position becomes an abstraction that does fancier things than
> a RefPtr/offset pair, the more the lack of a PassPosition becomes a problem.
> Maybe we need a PassPosition.

I like the idea of a PassPosition.  I'm just slightly concerned that it makes more difficult the possible future desire to have an AfterPosition, BeforePosition, OffsetPosition types/subclasses to make it clear at the callsite how the position should be anchored.  (For example for RangeBoundaryPoint member storage.)

The current plan is to use creation functions for all position uses.  Which I can change to use a new PassPosition class.  Things like createOffsetPosition(node, offset) createAfterPosition(node) createBeforePosition(node) and a createEditingOffsetPosition(node, offset) (which matches the current behavior of Position() + the various later canonicalization functions in that it creates a position which is either an offset, or before or after, depending on if the node is ignored by editing or not).   Again, basically we're making Position store what type it is, and moving the type determination to construction time instead of some later time when the right combination of canonicalization functions are called.

I'm not sure we'll really want to have PassAfterPosition, PassBeforePosition, PassOffsetPosition as well (seems like a lot of classes to express such a simple idea) so maybe PassPosition will just be the one generic pass class.

[Darin Adler]

(In reply to comment #12)
> I like the idea of a PassPosition.  I'm just slightly concerned that it makes
> more difficult the possible future desire to have an AfterPosition,
> BeforePosition, OffsetPosition types/subclasses to make it clear at the
> callsite how the position should be anchored.

Do these types need to be subclasses? Can’t they just be states of a Position class?

If you have subclasses I think then you have to use pointers because that’s how polymorphism works.

[Eric Seidel (no email)]

(In reply to comment #13)
> Do these types need to be subclasses? Can’t they just be states of a Position
> class?

They can be states.  That's how I implemented them when I wrote them (patch not posted).  I was was writing them though, there seemed a few places where it would be nice (from a code cleanliness perspective) for the caller to be able to indicate (in a way the compiler could check) that a position was anchored in some specific way.

> If you have subclasses I think then you have to use pointers because that’s how
> polymorphism works.

True.  We'd have to start returning pointers if we wanted to go down that path.

For now I'll stick with a single mutable (at least the offset) Position class (which can be anchored in multiple ways), and a single PassPosition class which is used for passing around Position objects instead of returning Position (and probably in some cases instead of passing const Position&).

[Eric Seidel (no email)]

Created attachment 28870 [details]
Stab at PassPosition, does not compile

 JavaScriptCore/wtf/PassRefPtr.h                    |    4 +-
 WebCore/WebCore.xcodeproj/project.pbxproj          |    4 ++
 WebCore/dom/Position.cpp                           |   36 ++++++++++++++-----
 WebCore/dom/Position.h                             |   25 ++++++++-----
 WebCore/dom/PositionIterator.cpp                   |    1 +
 WebCore/dom/Range.cpp                              |    1 +
 WebCore/editing/ApplyStyleCommand.cpp              |    1 +
 WebCore/editing/BreakBlockquoteCommand.cpp         |    3 +-
 WebCore/editing/CompositeEditCommand.cpp           |    1 +
 WebCore/editing/DeleteSelectionCommand.cpp         |    1 +
 WebCore/editing/Editor.cpp                         |    1 +
 WebCore/editing/FormatBlockCommand.cpp             |    6 ++-
 WebCore/editing/IndentOutdentCommand.cpp           |    1 +
 WebCore/editing/InsertLineBreakCommand.cpp         |    1 +
 WebCore/editing/InsertListCommand.cpp              |    6 ++-
 .../editing/InsertParagraphSeparatorCommand.cpp    |    9 +++--
 WebCore/editing/InsertTextCommand.cpp              |    9 +++--
 WebCore/editing/ReplaceSelectionCommand.cpp        |    1 +
 WebCore/editing/SelectionController.cpp            |    1 +
 WebCore/editing/TypingCommand.cpp                  |    1 +
 WebCore/editing/VisiblePosition.cpp                |    1 +
 WebCore/editing/VisibleSelection.cpp               |    1 +
 WebCore/editing/htmlediting.cpp                    |    1 +
 WebCore/editing/markup.cpp                         |    1 +
 WebCore/editing/visible_units.cpp                  |    1 +
 WebCore/page/AccessibilityObject.cpp               |    1 +
 WebCore/page/AccessibilityRenderObject.cpp         |    1 +
 WebCore/page/Frame.cpp                             |    1 +
 WebCore/rendering/RenderBox.cpp                    |    1 +
 29 files changed, 87 insertions(+), 35 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 28870 [details]
Stab at PassPosition, does not compile

I'm not sure PassPosition is the right approach.  I'm not even sure if Position-based refcount is currently a problem.  Adding PassPosition but not adding a PassVisiblePosition would just shift the problem to VisiblePosition.

[Darin Adler]

I think the best way to do the PassPosition thing more simply is to make PassPosition only usable for passing. There’s no need to be able to use it directly. Unlike PassRefPtr which you can use directly.

[Eric Seidel (no email)]

(In reply to comment #17)
> I think the best way to do the PassPosition thing more simply is to make
> PassPosition only usable for passing. There’s no need to be able to use it
> directly. Unlike PassRefPtr which you can use directly.

There are callers who currently do if (myPosition.downstream().isNotNull) (which returned a Position now PassPosition) which will need to change to store the downstream in a local first before accessing.

Likewise, callers who expect to be able to transparently convert from a returned Position object to a VisiblePosition, which now would need to explicitly make a Position (or more likely, VisiblePosition will just need to be taught about PassPosition)

Those were the two largest errors I encountered (which is why you see PassPosition having methods like isNotNull()).

[Darin Adler]

OK.

[Eric Seidel (no email)]

Created attachment 28876 [details]
Second stab at making Position::container private

 WebCore/ChangeLog                |   39 +++++++++++++++++++++++++++++++++++
 WebCore/dom/Position.h           |   42 +++++++++++++++++++++++++++++--------
 WebCore/dom/Range.cpp            |    4 +-
 WebCore/dom/RangeBoundaryPoint.h |   31 ++++++++++++++-------------
 4 files changed, 90 insertions(+), 26 deletions(-)

[Darin Adler]

Comment on attachment 28876 [details]
Second stab at making Position::container private

>  inline Node* RangeBoundaryPoint::container() const
>  {
> -    return m_position.container.get();
> +    // FIXME: node() is not necessarily the container node!
> +    return m_position.node();
>  }

This FIXME is wrong. The way RangeBoundaryPoint uses Position, everything is perfectly find, and this is indeed the container. It's the uses of Position in editing code that have the problem you mention in the definition of node. The Range class works perfectly here and there's no problem, so I think it's really misleading to have a FIXME here.

r=me though

[Eric Seidel (no email)]

The Position::container rename patch was landed as:

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/dom/Position.h
	M	WebCore/dom/Range.cpp
	M	WebCore/dom/RangeBoundaryPoint.h
Committed r41933


I'll do the rest in separate bugs.