Simplify whitespace layout object creation.

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights
  * reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
@@ skipping 234 matching lines @@
 
   if (parent.isTable() || parent.isTableRow() || parent.isTableSection() ||
       parent.isLayoutTableCol() || parent.isFrameSet() ||
       parent.isFlexibleBox() || parent.isLayoutGrid() || parent.isSVGRoot() ||
       parent.isSVGContainer() || parent.isSVGImage() || parent.isSVGShape()) {
     return false;
   }
   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());
 
   if (!parent.canHaveChildren())
     return false;
 
   if (isEditingText())
     return true;
 
@@ skipping 13 matching lines @@
   if (style.whiteSpace() == EWhiteSpace::kPreWrap && parent.isSVG())
     return false;
 
   // pre/pre-wrap/pre-line always make layoutObjects.
   if (style.preserveNewline())
     return true;
 
   // 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>
+          *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.

+
+  while (prev && prev->isFloatingOrOutOfFlowPositioned() &&
+         --maxSiblingsToVisit) {
+    prev = prev->previousSibling();
+  }
+
+  // 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;
 
-  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;
+  // So it is after a <br>
+  if (prev && prev->isBR())
+    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;
+  // 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.  Don't even
-      // make a layout object for this text.
-      return !firstChildNode;
-    }
-  }
-  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) {
   Node* parentOrShadowHostNode = text->parentOrShadowHostNode();
   DCHECK(parentOrShadowHostNode);
   return parentOrShadowHostNode->isSVGElement() &&
          !isSVGForeignObjectElement(*parentOrShadowHostNode);
 }
 
 LayoutText* Text::createTextLayoutObject(const ComputedStyle& style) {
@@ skipping 128 matching lines @@
 
 Text* Text::cloneWithData(const String& data) {
   return create(document(), data);
 }
 
 DEFINE_TRACE(Text) {
   CharacterData::trace(visitor);
 }
 
 }  // namespace blink