| Summary: | Use the new is<>() / downcast<>() for Text Nodes | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
| Component: | DOM | Assignee: | Chris Dumez <cdumez> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | benjamin, buildbot, commit-queue, darin, kling, rniwa | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 137056 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Chris Dumez
2014-09-27 23:16:32 PDT
Created attachment 238804 [details]
Patch
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.
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 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
Created attachment 238808 [details]
Patch
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.
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. (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. 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. 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. Comment on attachment 238808 [details] Patch Clearing flags on attachment: 238808 Committed r174068: <http://trac.webkit.org/changeset/174068> All reviewed patches have been landed. Closing bug. |