[webkit-reviews] review denied: [Bug 52919] Stop instantiating legacy editing Positions in VisiblePosition : [Attachment 81315] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Feb 4 17:20:51 PST 2011
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 81315: Patch
https://bugs.webkit.org/attachment.cgi?id=81315&action=review
------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81315&action=review
Thanks for tackling this!
> Source/WebCore/accessibility/AXObjectCache.cpp:576
> - VisiblePosition visiblePos = VisiblePosition(textMarkerData.node,
textMarkerData.offset, textMarkerData.affinity);
> + VisiblePosition visiblePos =
VisiblePosition(Position(textMarkerData.node, textMarkerData.offset,
Position::PositionIsOffsetInAnchor), textMarkerData.affinity);
Are you sure textMarkerData.node can always contain a position inside? i.e. is
there a guarantee that node can't be br, img, etc...?
> Source/WebCore/accessibility/AccessibilityObject.cpp:339
> - return VisiblePosition(startRenderer->node(), 0, VP_DEFAULT_AFFINITY);
> + return
VisiblePosition(firstPositionInOrBeforeNode(startRenderer->node()),
VP_DEFAULT_AFFINITY);
I think you can just return firstPositionInOrBeforeNode(startRenderer->node()
here.
> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2493
> - VisiblePosition startPosition = VisiblePosition(node, 0,
DOWNSTREAM);
> + VisiblePosition startPosition = VisiblePosition(Position(node,
Position::PositionIsBeforeAnchor), DOWNSTREAM);
You should be calling positionBeforeNode(node) instead.
> Source/WebCore/dom/Position.cpp:516
> - PositionIterator lastVisible = *this;
> + PositionIterator lastVisible = m_anchorType == PositionIsAfterAnchor ?
Position(m_anchorNode, caretMaxOffset(m_anchorNode.get())) : *this;
You don't consider the possibility of m_anchorType == PositionIsBeforeAnchor?
Also, can't you call parentAnchoredEquivalent? Maybe we need a variant of
parentAnchoredEquivalent that uses caretMaxOffset/caretMinOffset?
> Source/WebCore/dom/Range.cpp:1573
> + VisiblePosition visiblePosition(Position(m_start.container(),
m_start.offset(), Position::PositionIsOffsetInAnchor), VP_DEFAULT_AFFINITY);
I think you can just do:
VisiblePosition visiblePosition = Position(m_start.container(),
m_start.offset(), Position::PositionIsOffsetInAnchor)
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:887
> - originalVisPosBeforeEndBR = VisiblePosition(endBR, 0,
DOWNSTREAM).previous();
> + originalVisPosBeforeEndBR = VisiblePosition(Position(endBR,
Position::PositionIsBeforeAnchor), DOWNSTREAM).previous();
You should be calling positionBeforeNode(endBR) instead.
> Source/WebCore/editing/SelectionController.cpp:1319
> - VisiblePosition beforeOwnerElement(VisiblePosition(ownerElementParent,
ownerElementNodeIndex, SEL_DEFAULT_AFFINITY));
> - VisiblePosition afterOwnerElement(VisiblePosition(ownerElementParent,
ownerElementNodeIndex + 1, VP_UPSTREAM_IF_POSSIBLE));
> + VisiblePosition
beforeOwnerElement(VisiblePosition(Position(ownerElementParent,
ownerElementNodeIndex, Position::PositionIsOffsetInAnchor),
SEL_DEFAULT_AFFINITY));
> + VisiblePosition
afterOwnerElement(VisiblePosition(Position(ownerElementParent,
ownerElementNodeIndex + 1, Position::PositionIsOffsetInAnchor),
VP_UPSTREAM_IF_POSSIBLE));
We should just get rid of SEL_DEFAULT_AFFINITY.
> Source/WebCore/editing/TextIterator.cpp:881
> + VisiblePosition startPos = VisiblePosition(Position(m_startContainer,
m_startOffset, Position::PositionIsOffsetInAnchor), DOWNSTREAM);
Ditto about m_startContainer can be br, img, etc...
> Source/WebCore/editing/TextIterator.cpp:882
> + VisiblePosition currPos = VisiblePosition(Position(m_node,
Position::PositionIsBeforeAnchor), DOWNSTREAM);
Should be calling positionBeforeNode(m_node).
> Source/WebCore/editing/VisiblePosition.cpp:512
> + Text* textNode = static_cast<Text*>(pos.containerNode());
> + unsigned offset = pos.offsetInContainerNode();
offsetInContainerNode will hit an assertion if pos can ever be before/after
anchor node. You should probably check the anchor type above and bail out if
the position was before/after a node. But maybe we need to take care of a
position before a text node? Regardless, this change can't be right as is so
r-.
> Source/WebCore/editing/VisiblePosition.cpp:618
> - r->setStart(p.node(), p.deprecatedEditingOffset(), code);
> + r->setStart(p.containerNode(), p.offsetInContainerNode(), code);
Ditto about assertion in offsetInContainerNode.
> Source/WebCore/editing/VisiblePosition.cpp:628
> - r->setEnd(p.node(), p.deprecatedEditingOffset(), code);
> + r->setEnd(p.containerNode(), p.offsetInContainerNode(), code);
Ditto.
> Source/WebCore/editing/VisiblePosition.cpp:647
> + if (!p.node()->isDescendantOf(node))
Should this be calling containerNode? Or will that break something?
> Source/WebCore/editing/VisiblePosition.cpp:661
> + if (!p.node()->isDescendantOf(node))
Ditto about node()
> Source/WebCore/editing/visible_units.cpp:-434
> + Position pos;
> if (endNode->hasTagName(brTag)) {
> - endOffset = 0;
I've got the feeling that this should really be checking editingIgnoresContent
instead of just br.
> Source/WebCore/editing/visible_units.cpp:434
> + pos = Position(endNode, Position::PositionIsBeforeAnchor);
Should read: pos = positionBeforeNode(endNode)
> Source/WebCore/editing/visible_units.cpp:440
> + pos = Position(endNode, endOffset,
Position::PositionIsOffsetInAnchor);
If you make the change about to use editingIgnoresContent then this position
will be safe but otherwise, there is a potential that endNode is img, etc...
> Source/WebCore/editing/visible_units.cpp:580
> + return VisiblePosition(Position(rootElement, 0,
Position::PositionIsOffsetInAnchor), DOWNSTREAM);
Should call firstPositionInNode(rootElement)
> Source/WebCore/editing/visible_units.cpp:685
> - return VisiblePosition(rootElement, rootElement ?
rootElement->childNodeCount() : 0, DOWNSTREAM);
> + return VisiblePosition(Position(rootElement, rootElement ?
rootElement->childNodeCount() : 0, Position::PositionIsOffsetInAnchor),
DOWNSTREAM);
Ditto.
> Source/WebCore/editing/visible_units.cpp:803
> +
> + return VisiblePosition(Position(node, type), DOWNSTREAM);
Can we ASSERT(!offset) here?
> Source/WebCore/editing/visible_units.cpp:941
> - return VisiblePosition(startBlock, startBlock->childNodeCount(),
VP_DEFAULT_AFFINITY);
> + return VisiblePosition(Position(startBlock,
startBlock->childNodeCount(), Position::PositionIsOffsetInAnchor),
VP_DEFAULT_AFFINITY);
Should call lastPositionInNode(startBlock)
> Source/WebCore/editing/visible_units.cpp:966
> - return VisiblePosition(node->document()->documentElement(), 0,
DOWNSTREAM);
> + return VisiblePosition(Position(node->document()->documentElement(), 0,
Position::PositionIsOffsetInAnchor), DOWNSTREAM);
Should call firstPositionInNode(node->document()->documentElement()).
> Source/WebCore/editing/visible_units.cpp:980
> - return VisiblePosition(doc, doc->childNodeCount(), DOWNSTREAM);
> + return VisiblePosition(Position(doc, doc->childNodeCount(),
Position::PositionIsOffsetInAnchor), DOWNSTREAM);
lastPositionInNode again.
> Source/WebCore/editing/visible_units.cpp:1137
> + :
VisiblePosition(Position(logicalStartNode, Position::PositionIsBeforeAnchor),
DOWNSTREAM);
Should call positionBeforeNode(logicalStartNode).
> Source/WebCore/editing/visible_units.cpp:1173
> + pos = Position(logicalEndNode, Position::PositionIsBeforeAnchor);
positionBeforeNode.
> Source/WebCore/editing/visible_units.cpp:1181
> + pos = Position(logicalEndNode, Position::PositionIsAfterAnchor);
positionAfterNode.
> Source/WebCore/page/DOMSelection.cpp:214
> - m_frame->selection()->moveTo(VisiblePosition(node, offset, DOWNSTREAM));
> + m_frame->selection()->moveTo(VisiblePosition(Position(node, offset,
Position::PositionIsOffsetInAnchor), DOWNSTREAM));
Is it always safe to create a position inside node?
> Source/WebCore/page/DOMSelection.cpp:268
> - VisiblePosition visibleBase = VisiblePosition(baseNode, baseOffset,
DOWNSTREAM);
> - VisiblePosition visibleExtent = VisiblePosition(extentNode,
extentOffset, DOWNSTREAM);
> + VisiblePosition visibleBase = VisiblePosition(Position(baseNode,
baseOffset, Position::PositionIsOffsetInAnchor), DOWNSTREAM);
> + VisiblePosition visibleExtent = VisiblePosition(Position(extentNode,
extentOffset, Position::PositionIsOffsetInAnchor), DOWNSTREAM);
Ditto.
> Source/WebCore/page/DOMSelection.cpp:285
> - m_frame->selection()->moveTo(VisiblePosition(node, offset, DOWNSTREAM));
> + m_frame->selection()->moveTo(VisiblePosition(Position(node, offset,
Position::PositionIsOffsetInAnchor), DOWNSTREAM));
Ditto.
> Source/WebCore/page/DOMSelection.cpp:356
> - m_frame->selection()->setExtent(VisiblePosition(node, offset,
DOWNSTREAM));
> + m_frame->selection()->setExtent(VisiblePosition(Position(node, offset,
Position::PositionIsOffsetInAnchor), DOWNSTREAM));
Ditto.
> Source/WebCore/rendering/RenderObject.cpp:2657
> - return VisiblePosition(node, offset, affinity);
> + return VisiblePosition(Position(node, offset), affinity);
This is creating a legacy editing position. Is there a reason we can't get rid
of this?
> Source/WebCore/svg/SVGTextContentElement.cpp:148
> + VisiblePosition start(Position(const_cast<SVGTextContentElement*>(this),
0, Position::PositionIsOffsetInAnchor), SEL_DEFAULT_AFFINITY);
firstPositionInNode
More information about the webkit-reviews
mailing list