[LayoutNG] Remove unintuitive side effects in NGInlineItemsBuilder::ToString

NGInlineItemsBuilder::ToString() removes trailing spaces at
the end of the block. This is unintuitive from the function
name, and caused CL:1322184 to be reverted.

This patch moves this to ExitBlock(), and make ToString()
clean, without side effects.

Ran webkit_unit_tests locally with DCHECK disabled.

Bug: 636993
Change-Id: I03f814afc272b9be0c274391cde076aecb6dbc91
Reviewed-on: https://chromium-review.googlesource.com/c/1329804
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607579}

third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.cc

@@ -22,11 +22,6 @@ NGInlineItemsBuilderTemplate<
 
 template <typename OffsetMappingBuilder>
 String NGInlineItemsBuilderTemplate<OffsetMappingBuilder>::ToString() {
-  // Segment Break Transformation Rules[1] defines to keep trailing new lines,
-  // but it will be removed in Phase II[2]. We prefer not to add trailing new
-  // lines and collapsible spaces in Phase I.
-  RemoveTrailingCollapsibleSpaceIfExists();
-
   return text_.ToString();
 }
 
@@ -952,6 +947,11 @@ void NGInlineItemsBuilderTemplate<OffsetMappingBuilder>::EnterInline(
 template <typename OffsetMappingBuilder>
 void NGInlineItemsBuilderTemplate<OffsetMappingBuilder>::ExitBlock() {
   Exit(nullptr);
+
+  // Segment Break Transformation Rules[1] defines to keep trailing new lines,
+  // but it will be removed in Phase II[2]. We prefer not to add trailing new
+  // lines and collapsible spaces in Phase I.
+  RemoveTrailingCollapsibleSpaceIfExists();
 }
 
 template <typename OffsetMappingBuilder>

third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder_test.cc

@@ -57,6 +57,7 @@ class NGInlineItemsBuilderTest : public NGLayoutTest {
       }
       builder.Append(input.text, input.layout_text->Style(), input.layout_text);
     }
+    builder.ExitBlock();
     text_ = builder.ToString();
     ValidateItems();
     CheckReuseItemsProducesSameResult(inputs);
@@ -111,6 +112,7 @@ class NGInlineItemsBuilderTest : public NGLayoutTest {
       }
     }
 
+    reuse_builder.ExitBlock();
     String reuse_text = reuse_builder.ToString();
     EXPECT_EQ(text_, reuse_text);
   }

third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.cc

@@ -308,14 +308,12 @@ void NGInlineNode::ComputeOffsetMapping(LayoutBlockFlow* layout_block_flow,
   const bool update_layout = false;
   CollectInlinesInternal(layout_block_flow, &builder, nullptr, update_layout);
 
-  // TODO(kojii): We need to call ToString() regardless the |text| is needed or
-  // not, because it includes finalizing steps. It should be separated, because
-  // it's not intuitive to require a call of ToString().
-  String text = builder.ToString();
+  // We need the text for non-NG object. Otherwise |data| already has the text
+  // from the pre-layout phase, check they match.
   if (data->text_content.IsNull())
-    data->text_content = text;
+    data->text_content = builder.ToString();
   else
-    DCHECK_EQ(data->text_content, text);
+    DCHECK_EQ(data->text_content, builder.ToString());
 
   // TODO(xiaochengh): This doesn't compute offset mapping correctly when
   // text-transform CSS property changes text length.