Bug 63384

Summary: Stop instantiating Position with PositionIsOffsetInAnchor in various files
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, enrica, eric, leviw, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 63040    
Attachments:
Description Flags
63384
none
fixed build
none
Added containerText
none
Fixed per comments darin: review+

Description Ryosuke Niwa 2011-06-25 12:54:57 PDT
This is a refactoring to resolve the 63040.
Comment 1 Ryosuke Niwa 2011-06-25 13:12:10 PDT
Created attachment 98601 [details]
63384
Comment 2 WebKit Review Bot 2011-06-25 13:16:32 PDT
Comment on attachment 98601 [details]
63384

Attachment 98601 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8934867
Comment 3 Ryosuke Niwa 2011-06-25 13:17:16 PDT
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 4 Early Warning System Bot 2011-06-25 13:19:59 PDT
Comment on attachment 98601 [details]
63384

Attachment 98601 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8931926
Comment 5 WebKit Review Bot 2011-06-25 13:25:29 PDT
Comment on attachment 98601 [details]
63384

Attachment 98601 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8933923
Comment 6 Ryosuke Niwa 2011-06-25 14:00:06 PDT
Created attachment 98604 [details]
fixed build
Comment 7 Hajime Morrita 2011-06-27 01:36:37 PDT
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?
Comment 8 Ryosuke Niwa 2011-06-27 18:26:12 PDT
(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.
Comment 9 Ryosuke Niwa 2011-06-27 19:11:00 PDT
Created attachment 98844 [details]
Added containerText
Comment 10 Ryosuke Niwa 2011-06-28 08:17:43 PDT
Ping reviewers
Comment 11 Darin Adler 2011-06-28 09:49:38 PDT
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 12 Ryosuke Niwa 2011-06-28 10:11:35 PDT
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.
Comment 13 Darin Adler 2011-06-28 10:18:56 PDT
(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 14 Ryosuke Niwa 2011-06-28 11:21:59 PDT
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>.
Comment 15 Ryosuke Niwa 2011-06-28 11:27:59 PDT
I'll re-submit my patch for another review even though darin r+ed it because I've made quite few changes.
Comment 16 Ryosuke Niwa 2011-06-28 11:32:22 PDT
Created attachment 98951 [details]
Fixed per comments
Comment 17 Darin Adler 2011-06-28 12:09:15 PDT
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 18 Ryosuke Niwa 2011-06-28 12:14:12 PDT
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.
Comment 19 Ryosuke Niwa 2011-06-28 13:07:37 PDT
Committed r89952: <http://trac.webkit.org/changeset/89952>