[webkit-reviews] review denied: [Bug 52919] Stop instantiating legacy editing Positions in VisiblePosition : [Attachment 81552] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Levi Weintraub
<leviw at chromium.org>'s request for review:
Bug 52919: Stop instantiating legacy editing Positions in VisiblePosition
https://bugs.webkit.org/show_bug.cgi?id=52919

Attachment 81552: Patch
https://bugs.webkit.org/attachment.cgi?id=81552&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81552&action=review

The patch looks better but I think we need one more iteration. r- for various
nit and concerns.

> Source/WebCore/dom/Position.cpp:517
> +    // FIXME: PositionIterator should respect Before and After positions.
> +    PositionIterator lastVisible = m_anchorType == PositionIsAfterAnchor ?
Position(m_anchorNode, caretMaxOffset(m_anchorNode.get())) : *this;

This still instantiates legacy editing position but I guess your FIXME implies
that as well?

> Source/WebCore/dom/Position.cpp:639
> +    // FIXME: PositionIterator should respect Before and After positions.
> +    PositionIterator lastVisible = m_anchorType == PositionIsAfterAnchor ?
Position(m_anchorNode, caretMaxOffset(m_anchorNode.get())) : *this;

Ditto.

> Source/WebCore/editing/VisiblePosition.cpp:510
> +    Node* node = pos.containerNode();
> +    if (!node || !node->isTextNode() || pos.anchorType() ==
Position::PositionIsAfterAnchor)
>	   return 0;

You should assert that pos.anchorType() is either PositionIsBeforeAnchor or
PositionIsOffsetInAnchor after this early exit in the case more types are
added.

> Source/WebCore/editing/VisiblePosition.cpp:647
> +    Position p = visiblePosition.deepEquivalent();
> +
> +    if (!p.containerNode()->isDescendantOf(node))

Why can't we do
visiblePosition.deepEquivalent().containerNode()->isDescendantOf(node) instead?


> Source/WebCore/editing/VisiblePosition.cpp:661
> +    Position p = visiblePosition.deepEquivalent();
> +
> +    if (!p.containerNode()->isDescendantOf(node))

Ditto.

> Source/WebCore/editing/visible_units.cpp:386
> -    VisiblePosition visPos = VisiblePosition(startNode, startOffset,
DOWNSTREAM);
> +    VisiblePosition visPos = VisiblePosition(Position(startNode,
startOffset, Position::PositionIsOffsetInAnchor), DOWNSTREAM);

Why is it safe to create a position inside startNode here?  i.e. what
guarantees that startNode isn't br, img, etc...?

> Source/WebCore/editing/visible_units.cpp:433
>      if (endNode->hasTagName(brTag)) {

Could you fix this manual check against brTag or file a followup bug?

> Source/WebCore/editing/visible_units.cpp:787
> -			   return VisiblePosition(n, i + 1, DOWNSTREAM);
> +			   return VisiblePosition(Position(n, i + 1, type),
DOWNSTREAM);

I think we should just pass Position::PositionIsOffsetInAnchor as the type
instead of "type" so that the correctness is self-evident.

> Source/WebCore/editing/visible_units.cpp:851
> -			   return VisiblePosition(n, i, DOWNSTREAM);
> +			   return VisiblePosition(Position(n, i, type),
DOWNSTREAM);

Ditto.

> Source/WebCore/editing/visible_units.cpp:1137
> +    VisiblePosition visPos = logicalStartNode->isTextNode() ?
VisiblePosition(Position(logicalStartNode, logicalStartBox->caretMinOffset(),
Position::PositionIsOffsetInAnchor), DOWNSTREAM)
> +							       :
VisiblePosition(positionBeforeNode(logicalStartNode), DOWNSTREAM);

It's not obvious to me why logicalStartNode can always have a position inside.

> Source/WebCore/editing/visible_units.cpp:1172
>      if (logicalEndNode->hasTagName(brTag))

Same comment about calling editingIgnoreContents instead of manually checking
against brTag.

> Source/WebCore/page/DOMSelection.cpp:359
> -    m_frame->selection()->setExtent(VisiblePosition(node, offset,
DOWNSTREAM));
> +    m_frame->selection()->setExtent(VisiblePosition(Position(node, offset,
Position::PositionIsOffsetInAnchor), DOWNSTREAM));

I still don't think it's correct to create a Position with a random node. 
Maybe we should throw an exception?  Auto-correcting it to the parent-anchored
equivalent position is fine as well.