Have NodeTraversal::nextSkippingChildren() take a reference

Source/core/editing/TextIterator.cpp

 /*
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2005 Alexey Proskuryakov.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 729 matching lines @@
 
 static bool shouldEmitNewlineForNode(Node* node, bool emitsOriginalText)
 {
     RenderObject* renderer = node->renderer();
 
     if (renderer ? !renderer->isBR() : !node->hasTagName(brTag))
         return false;
     return emitsOriginalText || !(node->isInShadowTree() && node->shadowHost()->hasTagName(inputTag));
 }
 
-static bool shouldEmitNewlinesBeforeAndAfterNode(Node* node)
+static bool shouldEmitNewlinesBeforeAndAfterNode(Node& node)
 {
     // Block flow (versus inline flow) is represented by having
     // a newline both before and after the element.
-    RenderObject* r = node->renderer();
+    RenderObject* r = node.renderer();
     if (!r) {
-        return (node->hasTagName(blockquoteTag)
-            || node->hasTagName(ddTag)
-            || node->hasTagName(divTag)
-            || node->hasTagName(dlTag)
-            || node->hasTagName(dtTag)
-            || node->hasTagName(h1Tag)
-            || node->hasTagName(h2Tag)
-            || node->hasTagName(h3Tag)
-            || node->hasTagName(h4Tag)
-            || node->hasTagName(h5Tag)
-            || node->hasTagName(h6Tag)
-            || node->hasTagName(hrTag)
-            || node->hasTagName(liTag)
-            || node->hasTagName(listingTag)
-            || node->hasTagName(olTag)
-            || node->hasTagName(pTag)
-            || node->hasTagName(preTag)
-            || node->hasTagName(trTag)
-            || node->hasTagName(ulTag));
+        return (node.hasTagName(blockquoteTag)
+            || node.hasTagName(ddTag)
+            || node.hasTagName(divTag)
+            || node.hasTagName(dlTag)
+            || node.hasTagName(dtTag)
+            || node.hasTagName(h1Tag)
+            || node.hasTagName(h2Tag)
+            || node.hasTagName(h3Tag)
+            || node.hasTagName(h4Tag)
+            || node.hasTagName(h5Tag)
+            || node.hasTagName(h6Tag)
+            || node.hasTagName(hrTag)
+            || node.hasTagName(liTag)
+            || node.hasTagName(listingTag)
+            || node.hasTagName(olTag)
+            || node.hasTagName(pTag)
+            || node.hasTagName(preTag)
+            || node.hasTagName(trTag)
+            || node.hasTagName(ulTag));
     }
 
     // Need to make an exception for table cells, because they are blocks, but we
     // want them tab-delimited rather than having newlines before and after.
-    if (isTableCell(node))
+    if (isTableCell(&node))
         return false;
 
     // Need to make an exception for table row elements, because they are neither
     // "inline" or "RenderBlock", but we want newlines for them.
     if (r->isTableRow()) {
         RenderTable* t = toRenderTableRow(r)->table();
         if (t && !t->isInline())
             return true;
     }
 
     return !r->isInline() && r->isRenderBlock()
         && !r->isFloatingOrOutOfFlowPositioned() && !r->isBody() && !r->isRubyText();
 }
 
-static bool shouldEmitNewlineAfterNode(Node* node)
+static bool shouldEmitNewlineAfterNode(Node& node)
 {
     // FIXME: It should be better but slower to create a VisiblePosition here.
     if (!shouldEmitNewlinesBeforeAndAfterNode(node))
         return false;
     // Check if this is the very last renderer in the document.
     // If so, then we should not emit a newline.
-    while ((node = NodeTraversal::nextSkippingChildren(node))) {
-        if (node->renderer())
+    Node* next = &node;
+    while ((next = NodeTraversal::nextSkippingChildren(*next))) {

[adamk, 2013/11/12 21:07:43]

Nit: can you remove the extra parentheses while you're modifying this line?

[Inactive, 2013/11/12 21:19:16]

I would but I cannot :)

../../third_party/WebKit/Source/core/editing/TextIterator.cpp: In function ‘bool
WebCore::shouldEmitNewlineAfterNode(WebCore::Node&)’:
../../third_party/WebKit/Source/core/editing/TextIterator.cpp:802:60: error:
suggest parentheses around assignment used as truth value [-Werror=parentheses]

I am using gcc 4.7.

[adamk, 2013/11/12 22:21:36]

Sigh. Oh well, lgtm anyway

+        if (next->renderer())
             return true;
     }
     return false;
 }
 
-static bool shouldEmitNewlineBeforeNode(Node* node)
+static bool shouldEmitNewlineBeforeNode(Node& node)
 {
     return shouldEmitNewlinesBeforeAndAfterNode(node);
 }
 
 static bool shouldEmitExtraNewlineForNode(Node* node)
 {
     // When there is a significant collapsed bottom margin, emit an extra
     // newline for a more realistic result. We end up getting the right
     // result even without margin collapsing. For example: <div><p>text</p></div>
     // will work right even if both the <div> and the <p> have bottom margins.
@@ skipping 110 matching lines @@
 {
     // Emit a character to show the positioning of m_node.
 
     // When we haven't been emitting any characters, shouldRepresentNodeOffsetZero() can
     // create VisiblePositions, which is expensive. So, we perform the inexpensive checks
     // on m_node to see if it necessitates emitting a character first and will early return
     // before encountering shouldRepresentNodeOffsetZero()s worse case behavior.
     if (shouldEmitTabBeforeNode(m_node)) {
         if (shouldRepresentNodeOffsetZero())
             emitCharacter('\t', m_node->parentNode(), m_node, 0, 0);
-    } else if (shouldEmitNewlineBeforeNode(m_node)) {
+    } else if (shouldEmitNewlineBeforeNode(*m_node)) {
         if (shouldRepresentNodeOffsetZero())
             emitCharacter('\n', m_node->parentNode(), m_node, 0, 0);
     } else if (shouldEmitSpaceBeforeAndAfterNode(m_node)) {
         if (shouldRepresentNodeOffsetZero())
             emitCharacter(' ', m_node->parentNode(), m_node, 0, 0);
     }
 }
 
 bool TextIterator::handleNonTextNode()
 {
@@ skipping 17 matching lines @@
         return;
 
     // Emit with a position *inside* m_node, after m_node's contents, in
     // case it is a block, because the run should start where the
     // emitted character is positioned visually.
     Node* baseNode = m_node->lastChild() ? m_node->lastChild() : m_node;
     // FIXME: This shouldn't require the m_lastTextNode to be true, but we can't change that without making
     // the logic in _web_attributedStringFromRange match. We'll get that for free when we switch to use
     // TextIterator in _web_attributedStringFromRange.
     // See <rdar://problem/5428427> for an example of how this mismatch will cause problems.
-    if (m_lastTextNode && shouldEmitNewlineAfterNode(m_node)) {
+    if (m_lastTextNode && shouldEmitNewlineAfterNode(*m_node)) {
         // use extra newline to represent margin bottom, as needed
         bool addNewline = shouldEmitExtraNewlineForNode(m_node);
 
         // FIXME: We need to emit a '\n' as we leave an empty block(s) that
         // contain a VisiblePosition when doing selection preservation.
         if (m_lastCharacter != '\n') {
             // insert a newline with a position following this block's contents.
             emitCharacter('\n', baseNode->parentNode(), baseNode, 1, 1);
             // remember whether to later add a newline for the current node
             ASSERT(!m_needsAnotherNewline);
@@ skipping 318 matching lines @@
     // code in moveParagraphs, so we use a comma. Unconditionally emit
     // here because this iterator is only used for boundary finding.
     emitCharacter(',', m_node->parentNode(), index, index + 1);
     return true;
 }
 
 bool SimplifiedBackwardsTextIterator::handleNonTextNode()
 {
     // We can use a linefeed in place of a tab because this simple iterator is only used to
     // find boundaries, not actual content. A linefeed breaks words, sentences, and paragraphs.
-    if (shouldEmitNewlineForNode(m_node, m_emitsOriginalText) || shouldEmitNewlineAfterNode(m_node) || shouldEmitTabBeforeNode(m_node)) {
+    if (shouldEmitNewlineForNode(m_node, m_emitsOriginalText) || shouldEmitNewlineAfterNode(*m_node) || shouldEmitTabBeforeNode(m_node)) {
         unsigned index = m_node->nodeIndex();
         // The start of this emitted range is wrong. Ensuring correctness would require
         // VisiblePositions and so would be slow. previousBoundary expects this.
         emitCharacter('\n', m_node->parentNode(), index + 1, index + 1);
     }
     return true;
 }
 
 void SimplifiedBackwardsTextIterator::exitNode()
 {
-    if (shouldEmitNewlineForNode(m_node, m_emitsOriginalText) || shouldEmitNewlineBeforeNode(m_node) || shouldEmitTabBeforeNode(m_node)) {
+    if (shouldEmitNewlineForNode(m_node, m_emitsOriginalText) || shouldEmitNewlineBeforeNode(*m_node) || shouldEmitTabBeforeNode(m_node)) {
         // The start of this emitted range is wrong. Ensuring correctness would require
         // VisiblePositions and so would be slow. previousBoundary expects this.
         emitCharacter('\n', m_node, 0, 0);
     }
 }
 
 void SimplifiedBackwardsTextIterator::emitCharacter(UChar c, Node* node, int startOffset, int endOffset)
 {
     m_singleCharacterBuffer = c;
     m_positionNode = node;
@@ skipping 987 matching lines @@
         if (!matchLength)
             return collapsedToBoundary(range, !(options & Backwards));
     }
 
     // Then, find the document position of the start and the end of the text.
     CharacterIterator computeRangeIterator(range, TextIteratorEntersTextControls);
     return characterSubrange(computeRangeIterator, matchStart, matchLength);
 }
 
 }