Avoid creating consecutive whitespace renderers.

third_party/WebKit/Source/core/layout/LayoutTextTest.cpp

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/layout/LayoutText.h"
 
 #include "core/layout/LayoutTestHelper.h"
 #include "core/layout/line/InlineTextBox.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ skipping 67 matching lines @@
 
 TEST_F(LayoutTextTest, WidthLengthBeyondLength)
 {
     setBasicBody("x");
     // Width may vary by platform and we just want to make sure it's something roughly reasonable.
     float width = getBasicText()->width(0u, 2u, LayoutUnit(), LTR, false);
     ASSERT_GE(width, 4.f);
     ASSERT_LE(width, 20.f);
 }
 
+TEST_F(LayoutTextTest, ConsecutiveWhitespaceRenderers)
+{
+    setBodyInnerHTML("<span></span> <span id='span'></span>");
+
+    Element* span = document().getElementById("span");
+    Node* whitespace1 = span->previousSibling();
+    Node* whitespace2 = Text::create(document(), " ");
+    document().body()->insertBefore(whitespace2, span, ASSERT_NO_EXCEPTION);
+    document().view()->updateAllLifecyclePhases();
+
+    EXPECT_TRUE(whitespace1->layoutObject());
+    EXPECT_FALSE(whitespace2->layoutObject());
+}

[mstensho (USE GERRIT), 2016/09/28 11:19:09]

How about a test similar to this one, except that you insert the new whitespace
node BEFORE the existing whitespace node?

The expectation then would be that the old whitespace node would lose its
LayoutObject and have a LayoutObject created for the new node instead.

But you just explained to me f2f that it won't work like that, and that both
whitespace nodes would end up with a LayoutObject, because
Text::textLayoutObjectIsNeeded() only searches backwards. I don't know if
creating different layout trees with the same DOM, depending on the order things
are inserted in could be a potential problem, but it sounds reasonable to add a
test for it, with an explanation.

We're already inconsistent in textLayoutObjectIsNeeded(), anyway, because we
limit the search to 50 nodes, with a comment saying that it shouldn't be a
problem.

+
+TEST_F(LayoutTextTest, ConsecutiveWhitespaceRenderersDisplayNone)
+{
+    setBodyInnerHTML("<span></span> <span style='display:none'></span> <span></span>");
+
+    // First <span>
+    Node* child = document().body()->firstChild();
+    ASSERT_TRUE(child);
+
+    // First whitespace node
+    child = child->nextSibling();
+    ASSERT_TRUE(child);
+    EXPECT_TRUE(child->isTextNode());
+    EXPECT_TRUE(child->layoutObject());
+
+    // <span display:none>
+    child = child->nextSibling();
+    ASSERT_TRUE(child);
+    EXPECT_FALSE(child->layoutObject());
+
+    // Second whitespace node
+    child = child->nextSibling();
+    ASSERT_TRUE(child);
+    EXPECT_TRUE(child->isTextNode());
+    EXPECT_FALSE(child->layoutObject());
+}
+
+TEST_F(LayoutTextTest, ConsecutiveWhitespaceRenderersComment)
+{
+    setBodyInnerHTML("<span></span> <!-- --> <span></span>");
+
+    // First <span>
+    Node* child = document().body()->firstChild();
+    ASSERT_TRUE(child);
+
+    // First whitespace node
+    child = child->nextSibling();
+    ASSERT_TRUE(child);
+    EXPECT_TRUE(child->isTextNode());
+    EXPECT_TRUE(child->layoutObject());
+
+    // <span display:none>

[rune, 2016/09/27 13:54:14]

This should be the comment <!-- -->

[rune, 2016/09/28 12:13:10]

Fixed.

+    child = child->nextSibling();
+    ASSERT_TRUE(child);
+    EXPECT_FALSE(child->layoutObject());
+
+    // Second whitespace node
+    child = child->nextSibling();
+    ASSERT_TRUE(child);
+    EXPECT_TRUE(child->isTextNode());
+    EXPECT_FALSE(child->layoutObject());
+}
+
 } // namespace blink