Bug 137188 – Use the new is<>() / downcast<>() for Text Nodes

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Use the new is<>() / downcast<>() for Text Nodes instead of isText() / toText().

[Chris Dumez]

Created attachment 238804 [details]
Patch

[WebKit Commit Bot]

Attachment 238804 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Position.cpp:953:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 238804 [details]
Patch

Attachment 238804 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6208895211012096

New failing tests:
fast/dom/Node/normalize-with-cdata.html

[Build Bot]

Created attachment 238806 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Chris Dumez]

Created attachment 238808 [details]
Patch

[WebKit Commit Bot]

Attachment 238808 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Position.cpp:953:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 238808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238808&action=review

I got sick of commenting on all the many lines that add new null checks. I don’t think we should add those, although the compiler might optimize many of them out.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1643
> +    if (node && is<Text>(node))

Should be *node, or get rid of the "node &&" part.

> Source/WebCore/css/SelectorChecker.cpp:563
> +                    if (is<Text>(node)) {

*node

> Source/WebCore/dom/CharacterData.cpp:91
> +    ASSERT(!renderer() || is<Text>(this));
> +    if (is<Text>(this))

*this

> Source/WebCore/dom/CharacterData.cpp:198
> +    ASSERT(!renderer() || is<Text>(this));
> +    if (is<Text>(this))

*this

> Source/WebCore/dom/Node.cpp:492
> -        RefPtr<Text> text = toText(node.get());
> +        RefPtr<Text> text = downcast<Text>(node.get());

Should be *node and in the long run probably should be Ref instead of RefPtr.

Code above this is really slow. I wonder if we really need nodeType instead of is<Element> and is<Text>.

> Source/WebCore/dom/Node.cpp:504
>              if (nextSibling->nodeType() != TEXT_NODE)

This is really slow. I wonder if we really need nodeType here instead of is<Text>.

> Source/WebCore/dom/Node.cpp:506
> +            RefPtr<Text> nextText = downcast<Text>(nextSibling);

*nextSibling

> Source/WebCore/dom/Position.cpp:173
> +        return m_anchorNode && is<Text>(*m_anchorNode) ? downcast<Text>(m_anchorNode.get()) : nullptr;

Could just write:

    is<Text>(m_anchorNode) ?

> Source/WebCore/dom/Position.cpp:179
> +        ASSERT(!m_anchorNode || !is<Text>(*m_anchorNode));

Could just write:

    ASSERT(!is<Text>(m_anchorNode));

> Source/WebCore/dom/Position.cpp:953
> +    if (isNull() || !is<Text>(deprecatedNode()))

*deprecatedNode, or get rid of the isNull check.

> Source/WebCore/dom/Position.cpp:1065
> +    if (prev != *this && inSameEnclosingBlockFlowElement(deprecatedNode(), prev.deprecatedNode()) && is<Text>(prev.deprecatedNode())) {

*prev.deprecatedNode unless we want to add a null check for someone reason

> Source/WebCore/dom/Range.cpp:981
> +    bool startIsText = is<Text>(m_start.container());

Should be *m_start.container()

> Source/WebCore/dom/Range.cpp:2235
> +        } else if (is<Text>(node)) {

*node

> Source/WebCore/dom/Text.cpp:97
> +        if (!is<Text>(node))

*node

> Source/WebCore/dom/Text.cpp:108
> +        if (!is<Text>(node))

*node

> Source/WebCore/dom/TextNodeTraversal.cpp:50
> +    if (is<Text>(root))

*root

> Source/WebCore/dom/TextNodeTraversal.h:69
> +    while (node && !is<Text>(node))

*node

> Source/WebCore/dom/TextNodeTraversal.h:80
> +    while (node && !is<Text>(node))

*node

> Source/WebCore/dom/TextNodeTraversal.h:91
> +    while (node && !is<Text>(node))

*node

> Source/WebCore/dom/TextNodeTraversal.h:102
> +    while (node && !is<Text>(node))

*node

> Source/WebCore/dom/TextNodeTraversal.h:112
> +    while (node && !is<Text>(node))

*node

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:167
> +    if (!textNode || !is<Text>(textNode) || offset < 0 || offset >= textNode->maxCharacterOffset())

*textNode

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:270
> +    if (text == start.containerNode() && text->previousSibling() && is<Text>(text->previousSibling())) {

remove the text->previousSibling() &&

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:274
> +    if (text == end.containerNode() && text->previousSibling() && is<Text>(text->previousSibling())) {

remove the text->previousSibling() &&

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:281
> +            if (is<Text>(text->previousSibling())

Put a * here unless we are adding a null check on purpose

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:283
> +                && static_cast<unsigned>(m_endOfLastParagraph.offsetInContainerNode()) <= downcast<Text>(text->previousSibling())->length())
> +                m_endOfLastParagraph = Position(downcast<Text>(text->previousSibling()), m_endOfLastParagraph.offsetInContainerNode());

These are going to do null checks

> Source/WebCore/editing/ApplyStyleCommand.cpp:1204
> +    ASSERT(is<Text>(end.containerNode()));

Added a null check here

> Source/WebCore/editing/ApplyStyleCommand.cpp:1211
> +    if (!prevNode || !is<Text>(prevNode))

No need for !prevNode || here.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1214
> +    Position newStart = shouldUpdateStart ? Position(downcast<Text>(prevNode), start.offsetInContainerNode()) : start;

This does a null check.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1243
> +    if (!firstTextNode || !is<Text>(firstTextNode))

No need for !firstTextNode || here.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1246
> +    Position newStart = shouldUpdateStart ? Position(downcast<Text>(firstTextNode), start.offsetInContainerNode()) : start;

This does a null check.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1532
> +        if (!next || !is<Text>(next))
>              continue;

No need for !next || here.

> Source/WebCore/editing/BreakBlockquoteCommand.cpp:109
> +    if (is<Text>(startNode)) {

*startNode

> Source/WebCore/editing/CompositeEditCommand.cpp:379
> +    else if (is<Text>(refChild) && caretMaxOffset(refChild) > offset) {

*refChild

> Source/WebCore/editing/CompositeEditCommand.cpp:380
> +        splitTextNode(downcast<Text>(refChild), offset);

This does a null check

> Source/WebCore/editing/CompositeEditCommand.cpp:679
> +    if (position.anchorType() != Position::PositionIsOffsetInAnchor || !node || !is<Text>(node))

Double null check here

> Source/WebCore/editing/CompositeEditCommand.cpp:751
> +    if (!node || !is<Text>(node))

Double null check here.

[Chris Dumez]

(In reply to comment #7)
> (From update of attachment 238808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238808&action=review
> 
> I got sick of commenting on all the many lines that add new null checks. I don’t think we should add those, although the compiler might optimize many of them out.

They only add new assertions, not new null checks. See my comment on the other patch.

[Chris Dumez]

Comment on attachment 238808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238808&action=review

>> Source/WebCore/dom/Node.cpp:492
>> +        RefPtr<Text> text = downcast<Text>(node.get());
> 
> Should be *node and in the long run probably should be Ref instead of RefPtr.
> 
> Code above this is really slow. I wonder if we really need nodeType instead of is<Element> and is<Text>.

For the record, I tried using is<Text>() instead of nodeType() == TEXT_NODE here but this caused the following layout test to fail:
fast/dom/Node/normalize-with-cdata.html

This causes a web-exposed behavior change because is<Text>() matches CDataSection Nodes, while nodeType() == TEXT_NODE doesn't.

[Darin Adler]

Comment on attachment 238808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238808&action=review

>>> Source/WebCore/dom/Node.cpp:492
>>> +        RefPtr<Text> text = downcast<Text>(node.get());
>> 
>> Should be *node and in the long run probably should be Ref instead of RefPtr.
>> 
>> Code above this is really slow. I wonder if we really need nodeType instead of is<Element> and is<Text>.
> 
> For the record, I tried using is<Text>() instead of nodeType() == TEXT_NODE here but this caused the following layout test to fail:
> fast/dom/Node/normalize-with-cdata.html
> 
> This causes a web-exposed behavior change because is<Text>() matches CDataSection Nodes, while nodeType() == TEXT_NODE doesn't.

I had an inkling that might be so, which is why I worded this as a question.

[WebKit Commit Bot]

Comment on attachment 238808 [details]
Patch

Clearing flags on attachment: 238808

Committed r174068: <http://trac.webkit.org/changeset/174068>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.