Simplify whitespace layout object creation.

third_party/WebKit/Source/core/dom/Text.cpp

Index: third_party/WebKit/Source/core/dom/Text.cpp
diff --git a/third_party/WebKit/Source/core/dom/Text.cpp b/third_party/WebKit/Source/core/dom/Text.cpp
index f69b8552c899c41d59a2d26db83f4be95856723f..64fe3facbde92a2e1f90e29b37e99368c8d48cfc 100644
--- a/third_party/WebKit/Source/core/dom/Text.cpp
+++ b/third_party/WebKit/Source/core/dom/Text.cpp
@@ -252,6 +252,11 @@ static inline bool canHaveWhitespaceChildren(const LayoutObject& parent) {
   return true;
 }
 
+static inline bool emptyOrEndsWithWhitespace(const WTF::String& string) {
+  unsigned len = string.length();
+  return !len || isASCIISpace(string[len - 1]);
+}
+
 bool Text::textLayoutObjectIsNeeded(const ComputedStyle& style,
                                     const LayoutObject& parent) const {
   DCHECK(!document().childNeedsDistributionRecalc());
@@ -285,47 +290,41 @@ bool Text::textLayoutObjectIsNeeded(const ComputedStyle& style,
   // Avoiding creation of a layoutObject for the text node is a non-essential
   // memory optimization.  So to avoid blowing up on very wide DOMs, we limit
   // the number of siblings to visit.
-  unsigned maxSiblingsToVisit = 50;
-
+  uint32_t maxSiblingsToVisit = 50;

[rune, 2017/02/27 10:38:06]

Looks unnecessary to have a size-specific (32-bit) type for this code.

+  bool ranOutOfSiblings = false;
   const LayoutObject* prev =
       LayoutTreeBuilderTraversal::previousSiblingLayoutObject(
-          *this, maxSiblingsToVisit);
-  if (prev && prev->isBR())  // <span><br/> <br/></span>
-    return false;
+          *this, maxSiblingsToVisit, &ranOutOfSiblings);

[rune, 2017/02/27 10:38:06]

Can't you send maxSiblingsToVisit as a non-const reference instead of adding
ranOutOfSiblings?

Both before and after this CL you can visit nearly a 100 siblings if
previousSiblingLayoutObject visits 49 and you have 50 out-of-flow boxes before
that.

[emilio, 2017/02/27 12:09:32]

That's right. I can, but that'd be somewhat annoying for other callers that
don't want to limit it. I can pass it by pointer though, I guess.

 
-  if (parent.isLayoutInline()) {
-    // <span><div/> <div/></span>
-    if (prev && !prev->isInline() && !prev->isOutOfFlowPositioned())
-      return false;
-  } else {
-    if (parent.isLayoutBlock() && !parent.childrenInline() &&
-        (!prev || !prev->isInline()))
-      return false;
-
-    LayoutObject* first = parent.slowFirstChild();
-    for (; first && first->isFloatingOrOutOfFlowPositioned() &&
-           maxSiblingsToVisit;
-         first = first->nextSibling(), --maxSiblingsToVisit) {
-    }
-    if (!first || first == layoutObject() ||
-        LayoutTreeBuilderTraversal::nextSiblingLayoutObject(
-            *this, maxSiblingsToVisit) == first) {
-      // If we're adding children to this flow our previous siblings are not in
-      // the layout tree yet so we cannot know if we will be the first child in
-      // the line and collapse away. We have to assume we need a layout object.
-      Node* firstChildNode =
-          parent.node() ? LayoutTreeBuilderTraversal::firstChild(*parent.node())
-                        : nullptr;
-      if (first && first == layoutObject() && firstChildNode &&
-          !firstChildNode->layoutObject())
-        return true;
-
-      // Whitespace at the start of a block just goes away.  Don't even
-      // make a layout object for this text.
-      return !firstChildNode;
-    }
+  while (prev && prev->isFloatingOrOutOfFlowPositioned() &&
+         --maxSiblingsToVisit) {
+    prev = prev->previousSibling();
   }
-  return true;
+
+  // If we give up due to too many siblings, just create a layout object.
+  if (ranOutOfSiblings || !maxSiblingsToVisit)
+    return true;
+
+  // Collapse whitespace away if we find previous whitespace.
+  if (prev && prev->isText() &&
+      emptyOrEndsWithWhitespace(toLayoutText(prev)->text())) {

[rune, 2017/02/27 10:38:06]

Is checking the previous layout object for white-space all that changes test
expectations here?

[emilio, 2017/02/27 12:09:32]

Not totally sure about this, but it's likely. Actually, let me submit a CL with
this change, and another with the LayoutTreeBuilderTraversal limit fixes
separately before trying to land this.

+    return false;
+  }
+
+  // Whitespace after a non-inline is useless.
+  if (prev && !prev->isInline())
+    return false;
+
+  // So it is after a <br>
+  if (prev && prev->isBR())
+    return false;
+
+  // It may matter if we're in an inline and we haven't proven otherwise though.
+  if (parent.isLayoutInline())
+    return true;
+
+  // Whitespace at the start of a block just goes away.
+  return !!prev;

[rune, 2017/02/27 10:38:06]

We still re-attach right-to-left? That means we'll always return false first for
white-space nodes while attaching inline siblings inside a block, but it will be
rectified correctly by reattachWhitespaceSiblings() after attaching the previous
inline?

It seems to me this code is changing the behavior for that case, at least.

[emilio, 2017/02/27 12:09:32]

Yes, I believe that's right. I don't think it matters a lot as long as
reattachWhitespaceSiblings does the right thing, though?

 }
 
 static bool isSVGText(Text* text) {