[webkit-reviews] review granted: [Bug 24763] Position should support neighbor-anchored positions : [Attachment 29274] Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()

[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 29274: Add containerNode(), computeOffsetInContainerNode(),
nodeBeforePosition(), nodeAfterPosition()
https://bugs.webkit.org/attachment.cgi?id=29274&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    switch (anchorType()) {
> +    case PositionIsOffsetInAnchor:
> +	   return m_anchorNode.get();

I prefer indenting the cases inside the switch statements.

> +    case PositionIsOffsetInAnchor:
> +    {

Opening brace should be on same line as case.

> +// FIXME: Position should store an AnchorType up-front, instead of lazily
> +// determining it via editingIgnoresContent(anchorNode())
> +// That would require fixing moveToOffset to know how to recompute the
AnchorType
> +// for positions which need historical editing-compatible behavior
> +// (and for explicitly anchored positions to ASSERT_NOT_REACHED())

Or maybe you could just compute the anchor type at construction time for now.

> +    // These are always DOM compliant values.  Editing positions like [img,
0] (aka [img, before])
> +    // will return img->parentNode() and img->nodeIndex() from these
functions.
> +    Node* containerNode() const;
> +    int computeOffsetInContainerNode() const;
> +
> +    // These are convenience methods which are smart about whether the
position is neighbor anchored or parent anchored
> +    Node* nodeBeforePosition() const;
> +    Node* nodeAfterPosition() const;

I'm not sure I understand the rule for when you use "compute" and when you
don't. It seems that nodeBeforePosition and nodeAfterPosition can both be
expensive operations that involve calling childNode, so it seems they should
have compute in their names unless and until you change the class's design.

r=me, but I am concerned about the "compute" rule.