Bug 63384

Created attachment 98601 [details] 63384

Comment on attachment 98601 [details] 63384 Attachment 98601 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8934867

I've got rid of all calls to old constructor. After this patch there will be 30 lines in WebCore that calls the old constructor: Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2061: frame->selection()->setSelection(VisibleSelection(Position(node, range.start, Position::PositionIsOffsetInAnchor), Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2062: Position(node, range.start + range.length, Position::PositionIsOffsetInAnchor), DOWNSTREAM)); Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2496: return VisiblePosition(Position(it.range()->endContainer(ec), it.range()->endOffset(ec), Position::PositionIsOffsetInAnchor), UPSTREAM); Source/WebCore/dom/Position.cpp:191: return Position(m_anchorNode.get(), 0, PositionIsOffsetInAnchor); Source/WebCore/dom/Position.cpp:200: return Position(containerNode(), computeOffsetInContainerNode(), PositionIsOffsetInAnchor); Source/WebCore/dom/Position.h:240: return Position(node->nonShadowBoundaryParentNode(), node->nodeIndex(), Position::PositionIsOffsetInAnchor); Source/WebCore/dom/Position.h:246: return Position(node->nonShadowBoundaryParentNode(), node->nodeIndex() + 1, Position::PositionIsOffsetInAnchor); Source/WebCore/dom/Position.h:271: return Position(anchorNode, 0, Position::PositionIsOffsetInAnchor); Source/WebCore/dom/Position.h:278: return Position(anchorNode, lastOffsetInNode(anchorNode), Position::PositionIsOffsetInAnchor); Source/WebCore/dom/Range.cpp:1589: VisiblePosition visiblePosition = Position(m_start.container(), m_start.offset(), Position::PositionIsOffsetInAnchor); Source/WebCore/editing/ApplyStyleCommand.cpp:1264: updateStartEnd(Position(startNode, startOffsetAdjustment, Position::PositionIsOffsetInAnchor), Source/WebCore/editing/ApplyStyleCommand.cpp:1265: Position(end.deprecatedNode(), end.deprecatedEditingOffset() + endOffsetAdjustment, Position::PositionIsOffsetInAnchor)); Source/WebCore/editing/ApplyStyleCommand.cpp:1302: updateStartEnd(shouldUpdateStart ? Position(nextElement, start.offsetInContainerNode(), Position::PositionIsOffsetInAnchor) : start, Source/WebCore/editing/ApplyStyleCommand.cpp:1303: Position(nextElement, endOffset, Position::PositionIsOffsetInAnchor)); Source/WebCore/editing/FrameSelection.cpp:1408: VisiblePosition beforeOwnerElement(VisiblePosition(Position(ownerElementParent, ownerElementNodeIndex, Position::PositionIsOffsetInAnchor))); Source/WebCore/editing/FrameSelection.cpp:1409: VisiblePosition afterOwnerElement(VisiblePosition(Position(ownerElementParent, ownerElementNodeIndex + 1, Position::PositionIsOffsetInAnchor), VP_UPSTREAM_IF_POSSIBLE)); Source/WebCore/editing/TextIterator.cpp:871: VisiblePosition startPos = VisiblePosition(Position(m_startContainer, m_startOffset, Position::PositionIsOffsetInAnchor), DOWNSTREAM); Source/WebCore/editing/TypingCommand.cpp:620: extent = Position(extent.containerNode(), extent.computeOffsetInContainerNode() + extraCharacters, Position::PositionIsOffsetInAnchor); Source/WebCore/editing/visible_units.cpp:1083: VisiblePosition visPos = logicalStartNode->isTextNode() ? VisiblePosition(Position(logicalStartNode, logicalStartBox->caretMinOffset(), Position::PositionIsOffsetInAnchor), DOWNSTREAM) Source/WebCore/editing/visible_units.cpp:1130: pos = Position(logicalEndNode, endOffset, Position::PositionIsOffsetInAnchor); Source/WebCore/editing/visible_units.cpp:1191: wordBreak = Position(box->renderer()->node(), box->caretMaxOffset(), Position::PositionIsOffsetInAnchor); Source/WebCore/editing/visible_units.cpp:1234: return Position(node, box->caretMaxOffset(), Position::PositionIsOffsetInAnchor); Source/WebCore/editing/visible_units.cpp:1238: return Position(previousLeaf->renderer()->node(), previousLeaf->caretMaxOffset(), Position::PositionIsOffsetInAnchor); Source/WebCore/editing/visible_units.cpp:1245: return Position(lastRTLLeaf->renderer()->node(), lastRTLLeaf->caretMinOffset(), Position::PositionIsOffsetInAnchor); Source/WebCore/editing/visible_units.cpp:1248: return Position(node, box->caretMinOffset(), Position::PositionIsOffsetInAnchor); Source/WebCore/editing/visible_units.cpp:1259: return Position(node, box->caretMaxOffset(), Position::PositionIsOffsetInAnchor); Source/WebCore/editing/visible_units.cpp:1263: return Position(nextLeaf->renderer()->node(), nextLeaf->caretMaxOffset(), Position::PositionIsOffsetInAnchor); Source/WebCore/editing/visible_units.cpp:1270: return Position(firstLTRLeaf->renderer()->node(), firstLTRLeaf->caretMinOffset(), Position::PositionIsOffsetInAnchor); Source/WebCore/editing/visible_units.cpp:1273: return Position(node, box->caretMinOffset(), Position::PositionIsOffsetInAnchor); Source/WebCore/editing/visible_units.cpp:1323: VisiblePosition wordBreak = hasSeenWordBreakInThisBox ? previousWordBreak : Position(box->renderer()->node(), box->caretMinOffset(), Position::PositionIsOffsetInAnchor);

Comment on attachment 98601 [details] 63384 Attachment 98601 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8931926

Comment on attachment 98601 [details] 63384 Attachment 98601 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8933923

Created attachment 98604 [details] fixed build

Comment on attachment 98604 [details] fixed build View in context: https://bugs.webkit.org/attachment.cgi?id=98604&action=review I hope we have TextPosition class which is a subtype of Position, and provides text-based position specific helper methods. It is another story and should be another bug, though. > Source/WebCore/editing/ApplyBlockElementCommand.cpp:197 > + end = Position(static_cast<Text*>(start.containerNode()), end.offsetInContainerNode() - startOffset); It looks we should have containerText() or something and encapsulate static_cast<> into it. By doing so, we can assert nodeType() in that method. raw static_cast is makes my heart weak. Especially the method is named containerNode(). > Source/WebCore/editing/ApplyBlockElementCommand.cpp:250 > + if (containerNode.get() == start.containerNode() && containerNode->previousSibling() && containerNode->previousSibling()->isTextNode()) { It it possible for text node to have non-text sibling?

(In reply to comment #7) > (From update of attachment 98604 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98604&action=review > > I hope we have TextPosition class which is a subtype of Position, and provides text-based position specific helper methods. That'll be tricky because Position is a stack-allocated object (unless you new/malloc yourself). > > Source/WebCore/editing/ApplyBlockElementCommand.cpp:197 > > + end = Position(static_cast<Text*>(start.containerNode()), end.offsetInContainerNode() - startOffset); > > It looks we should have containerText() or something and encapsulate static_cast<> into it. > By doing so, we can assert nodeType() in that method. raw static_cast is makes my heart weak. Especially the method is named containerNode(). That's a good idea. Will add one. > > Source/WebCore/editing/ApplyBlockElementCommand.cpp:250 > > + if (containerNode.get() == start.containerNode() && containerNode->previousSibling() && containerNode->previousSibling()->isTextNode()) { > > It it possible for text node to have non-text sibling? Yes. Scripts can modify it.

Created attachment 98844 [details] Added containerText

Ping reviewers

Comment on attachment 98844 [details] Added containerText View in context: https://bugs.webkit.org/attachment.cgi?id=98844&action=review > Source/WebCore/dom/Position.cpp:95 > + ASSERT(!((anchorType == PositionIsBeforeChildren || anchorType == PositionIsAfterChildren) > + && (m_anchorNode->isTextNode() || editingIgnoresContent(m_anchorNode.get())))); Normally we’d not line up the && with the parenthesis, and just indent it 4 characters. Or we’d leave this all in one big if statement. Or even make a helper so this assertion is easier to read and shorter. > Source/WebCore/dom/Position.cpp:152 > +Text* Position::containerText() const I’m a little confused about the rules for when this function returns 0 and when it’s illegal to call the function. It seems that for PositionIsBeforeChildren and PositionIsAfterChildren it’s illegal to even call the function. Why is that? And why not the same rule for the other anchor types. I’m guessing some of this strangeness might be shared by the containerNode function. I’d like to understand the rationale for when this returns 0 and when it asserts. > Source/WebCore/dom/Position.cpp:155 > + if (!m_anchorNode || !m_anchorNode->isTextNode()) > + return 0; Strange to not check the anchor type when the anchor node happens to not be a text node. It would be more natural to have the isTextNode check be inside the switch statement just before the return statement. > Source/WebCore/editing/ApplyBlockElementCommand.cpp:193 > + splitTextNode(start.containerText(), startOffset); For uses like this it might be good to have a containerText function that asserts it’s non-zero rather than one that returns 0 if it’s not text or a null position or whatever. I think it makes sense to have a function that is only callable when you know the container is a text node. > Source/WebCore/editing/ApplyBlockElementCommand.cpp:194 > + start = firstPositionInNode(start.containerNode()); It seems a little strange to get the container again right after getting it on the previous line. This was less strange when this was a free getter, but now that it does some additional checking it’s a little irritating that it does it twice. > Source/WebCore/editing/ApplyBlockElementCommand.cpp:197 > + end = Position(start.containerText(), end.offsetInContainerNode() - startOffset); Not twice, three times! > Source/WebCore/editing/ApplyBlockElementCommand.cpp:201 > + m_endOfLastParagraph = Position(start.containerText(), m_endOfLastParagraph.offsetInContainerNode() - startOffset); Four times! > Source/WebCore/editing/ApplyBlockElementCommand.cpp:250 > + if (containerNode.get() == start.containerNode() && containerNode->previousSibling() && containerNode->previousSibling()->isTextNode()) { Should not need the get() here. > Source/WebCore/editing/ApplyBlockElementCommand.cpp:254 > + if (containerNode.get() == end.containerNode() && containerNode->previousSibling() && containerNode->previousSibling()->isTextNode()) { Should not need the get() here. > Source/WebCore/editing/ApplyBlockElementCommand.cpp:258 > + if (containerNode.get() == m_endOfLastParagraph.containerNode()) { Should not need the get() here. > Source/WebCore/editing/ApplyStyleCommand.cpp:1130 > + updateStartEnd(firstPositionInNode(start.containerNode()), newEnd); A general pattern here seems to be re-fetching the same node over and over again. Can we maybe use RefPtr<Text> a bit to avoid that. Not sure what the best style is.

Comment on attachment 98844 [details] Added containerText View in context: https://bugs.webkit.org/attachment.cgi?id=98844&action=review >> Source/WebCore/dom/Position.cpp:95 >> + && (m_anchorNode->isTextNode() || editingIgnoresContent(m_anchorNode.get())))); > > Normally we’d not line up the && with the parenthesis, and just indent it 4 characters. Or we’d leave this all in one big if statement. Or even make a helper so this assertion is easier to read and shorter. Oops, I forgot to fix it. Will fix. >> Source/WebCore/dom/Position.cpp:152 >> +Text* Position::containerText() const > > I’m a little confused about the rules for when this function returns 0 and when it’s illegal to call the function. It seems that for PositionIsBeforeChildren and PositionIsAfterChildren it’s illegal to even call the function. Why is that? And why not the same rule for the other anchor types. > > I’m guessing some of this strangeness might be shared by the containerNode function. I’d like to understand the rationale for when this returns 0 and when it asserts. So the idea is that if m_anchorNode is not a text, then we'll return 0 regardless of m_anchorType. Now if the anchor node is a text node, then the anchor type should NOT be PositionIsBeforeChildren or PositionIsAfterChildren because a text node cannot have children. >> Source/WebCore/dom/Position.cpp:155 >> + return 0; > > Strange to not check the anchor type when the anchor node happens to not be a text node. It would be more natural to have the isTextNode check be inside the switch statement just before the return statement. I guess I can switch it around. Do you strongly prefer me to make that change? >> Source/WebCore/editing/ApplyBlockElementCommand.cpp:201 >> + m_endOfLastParagraph = Position(start.containerText(), m_endOfLastParagraph.offsetInContainerNode() - startOffset); > > Four times! I'll add a local variable here. >> Source/WebCore/editing/ApplyStyleCommand.cpp:1130 >> + updateStartEnd(firstPositionInNode(start.containerNode()), newEnd); > > A general pattern here seems to be re-fetching the same node over and over again. Can we maybe use RefPtr<Text> a bit to avoid that. Not sure what the best style is. We could but I'm not sure how much of benefit we'd get out of that.

(In reply to comment #12) > >> Source/WebCore/dom/Position.cpp:152 > >> +Text* Position::containerText() const > > > > I’m a little confused about the rules for when this function returns 0 and when it’s illegal to call the function. It seems that for PositionIsBeforeChildren and PositionIsAfterChildren it’s illegal to even call the function. Why is that? And why not the same rule for the other anchor types. > > > > I’m guessing some of this strangeness might be shared by the containerNode function. I’d like to understand the rationale for when this returns 0 and when it asserts. > > So the idea is that if m_anchorNode is not a text, then we'll return 0 regardless of m_anchorType. Now if the anchor node is a text node, then the anchor type should NOT be PositionIsBeforeChildren or PositionIsAfterChildren because a text node cannot have children. > > >> Source/WebCore/dom/Position.cpp:155 > >> + return 0; > > > > Strange to not check the anchor type when the anchor node happens to not be a text node. It would be more natural to have the isTextNode check be inside the switch statement just before the return statement. > > I guess I can switch it around. Do you strongly prefer me to make that change? I think what you’re saying is that it’s always legal to call containerText on any Position. The assertion will only happen if somehow we have an impossible, badly formed, Position. It’s never the caller’s fault. That seems like a fine design, but was not clear from the code. Maybe a small comment would clear this up, but I guess it’s OK as is. > >> Source/WebCore/editing/ApplyStyleCommand.cpp:1130 > >> + updateStartEnd(firstPositionInNode(start.containerNode()), newEnd); > > > > A general pattern here seems to be re-fetching the same node over and over again. Can we maybe use RefPtr<Text> a bit to avoid that. Not sure what the best style is. > > We could but I'm not sure how much of benefit we'd get out of that. Not sure, I think we’d get some clarity from it. The expressions in these functions are getting so long that some simpler names for things could be a big help.

Comment on attachment 98844 [details] Added containerText View in context: https://bugs.webkit.org/attachment.cgi?id=98844&action=review >>> Source/WebCore/editing/ApplyStyleCommand.cpp:1130 >>> + updateStartEnd(firstPositionInNode(start.containerNode()), newEnd); >> >> A general pattern here seems to be re-fetching the same node over and over again. Can we maybe use RefPtr<Text> a bit to avoid that. Not sure what the best style is. > > We could but I'm not sure how much of benefit we'd get out of that. Oh wait, we're calling containerNode/containerText on end / start so I don't think we can use RefPtr<Text>.

I'll re-submit my patch for another review even though darin r+ed it because I've made quite few changes.

Created attachment 98951 [details] Fixed per comments

Comment on attachment 98951 [details] Fixed per comments View in context: https://bugs.webkit.org/attachment.cgi?id=98951&action=review > Source/WebCore/dom/Position.cpp:156 > + return m_anchorNode && m_anchorNode->isTextNode() ? static_cast<Text*>(m_anchorNode.get()) : 0; Wrong indentation here. > Source/WebCore/editing/ApplyStyleCommand.cpp:1130 > + updateStartEnd(firstPositionInNode(start.containerNode()), newEnd); This could be text instead of start.containerNode(). > Source/WebCore/editing/VisiblePosition.cpp:-557 > - unsigned length = textNode->length(); Why did you get rid of this local variable?

Comment on attachment 98951 [details] Fixed per comments View in context: https://bugs.webkit.org/attachment.cgi?id=98951&action=review >> Source/WebCore/dom/Position.cpp:156 >> + return m_anchorNode && m_anchorNode->isTextNode() ? static_cast<Text*>(m_anchorNode.get()) : 0; > > Wrong indentation here. WIll fix. >> Source/WebCore/editing/ApplyStyleCommand.cpp:1130 >> + updateStartEnd(firstPositionInNode(start.containerNode()), newEnd); > > This could be text instead of start.containerNode(). Okay, will replace Text* by RefPtr<Text>. >> Source/WebCore/editing/VisiblePosition.cpp:-557 >> - unsigned length = textNode->length(); > > Why did you get rid of this local variable? Oops, that was unintentional. Will revert.

Committed r89952: <http://trac.webkit.org/changeset/89952>