[webkit-reviews] review granted: [Bug 24763] Position should support neighbor-anchored positions : [Attachment 29275] Pre-computed and store AnchorType and isLegacyEditingPosition on Position during construction

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 24763: Position should support neighbor-anchored positions
https://bugs.webkit.org/show_bug.cgi?id=24763

Attachment 29275: Pre-computed and store AnchorType and isLegacyEditingPosition
on Position during construction
https://bugs.webkit.org/attachment.cgi?id=29275&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -Element *Position::element() const
> +Element* Position::element() const

It seems to me that both element() and node() need to eventually be private
unless it's clearer whether they should be named anchorNode or what.

> +    Position()
> +	   : m_offset(0)
> +	   , m_anchorType(PositionIsOffsetInAnchor)
> +	   , m_isLegacyEditingPosition(true) // FIXME: Switch the null position
to not be in legacy mode
>      {}

Braces should be on separate lines.

>      void clear() { m_anchorNode.clear(); m_offset = 0; }

This should set m_anchorType to PositionIsOffsetInAnchor and set
m_isLegacyEditingPosition to true.

> +    // These should only be used for PositionIsOffsetInAnchor positions,
unless
> +    // the position is a legacy editing position.
> +    void moveToPosition(PassRefPtr<Node> anchorNode, int offset);
> +    void moveToOffset(int offset);

Do the implementations have an assertion to this effect? Also, I think that
moveToPosition seems like a full assignment that could entirely wipe out the
old value of Position. I don't think a Position should necessarily have a
persistent mode that outlasts it value. Obviously, moveToOffset is different in
this respect since it's not a full assignment.

r=me