Make NGInlineItemsBuilder construct whitespace-collapsed offset mapping

third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h

Index: third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h
diff --git a/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h b/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h
index 9f295706ea0b3e1cdb2b3afe22068a496810b9bc..0617fc3b0d7278955ceae17e19fafaf7f8358b64 100644
--- a/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h
+++ b/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h
@@ -8,6 +8,7 @@
 #include "core/CoreExport.h"
 #include "core/layout/ng/inline/ng_inline_node.h"
 #include "platform/wtf/Allocator.h"
+#include "platform/wtf/Optional.h"
 #include "platform/wtf/Vector.h"
 #include "platform/wtf/text/StringBuilder.h"
 #include "platform/wtf/text/WTFString.h"
@@ -27,11 +28,19 @@ class NGInlineItem;
 // By calling EnterInline/ExitInline, it inserts bidirectional control
 // characters as defined in:
 // https://drafts.csswg.org/css-writing-modes-3/#bidi-control-codes-injection-table
+//
+// Optionally, NGInlineItemsBuilder takes parameter |removed_indexes| to return

[kojii, 2017/06/19 05:56:33]

How about naming |removed_indexes| as |collapsed_indexes|? "Removed" sounds like
different from "collapsed".

[Xiaocheng, 2017/06/19 17:53:01]

Sound good. Done.

+// how spaces are collapsed. For each string appended to NGInlineItemsBuilder,
+// a Vector<unsigned> is added to |removed_indexes|, storing the indexes of all
+// removed spaces in sorted order. See the unit tests for examples.
 class CORE_EXPORT NGInlineItemsBuilder {
   STACK_ALLOCATED();
 
  public:
-  explicit NGInlineItemsBuilder(Vector<NGInlineItem>* items) : items_(items) {}
+  explicit NGInlineItemsBuilder(Vector<NGInlineItem>* items)
+      : items_(items), removed_indexes_(nullptr) {}
+  explicit NGInlineItemsBuilder(Vector<NGInlineItem>* items,
+                                Vector<Vector<unsigned>>* removed_indexes);
   ~NGInlineItemsBuilder();
 
   String ToString();
@@ -78,6 +87,8 @@ class CORE_EXPORT NGInlineItemsBuilder {
 
  private:
   Vector<NGInlineItem>* items_;
+  Vector<Vector<unsigned>>* removed_indexes_;

[kojii, 2017/06/19 05:56:33]

ditto.

[Xiaocheng, 2017/06/19 17:53:01]

Done.

+  Optional<Vector<unsigned>> input_strings_lengths_;

[Xiaocheng, 2017/06/17 00:35:06]

Appending a string may result in the removal of a trailing space in a previous
string, so we need to remember lengths of previous strings to find out the index
of the removed trailing space.

[yosin_UTC9, 2017/06/19 03:23:11]

Since NGInlineItemsBuild allocated on stack, == only one instance, we don't need
to use Optional<T>.
sizeof(std::vector<T>) is 24 byte (3 pointers in 64-bit), a comment can explain
this is used only for offset mapping calculation.

It is better to introduce new class for collecting offset mapping, e.g.
OffsetCollector, to abstract collecting operations and templatization.

BTW, for me, it is hard to guess what |removed_indexes_| is.

[Xiaocheng, 2017/06/19 05:15:51]

Will do.
Could you explain more about this?

This patch doesn't introduce class for collecting the offset mapping. It only
makes the builder return how spaces are collapsed. A follow-up patch will
introduce the collector.

So I don't think templatization fits here.
Will add comments.

[yosin_UTC9, 2017/06/19 05:55:13]

It is functional abstraction of each then-clause and else-clause.
I imagine SAX API for XML parsing.
However, I'm not sure we can do so.

This comes from:

      // LayoutBR does not set preserve_newline, but should be preserved.
      if (!i && end == 1 && layout_object && layout_object->IsBR()) {
        bool trailing_space_removed = AppendForcedBreak(style, layout_object);
        if (trailing_space_removed && removed_indexes_)
          AddLastTrailingSpaceToRemovedIndexes(i);
        return;
      }
Templatization is the requirement. We should not make NGInlineItemBuilder slow
for layout.
It is OK to checking mode, e.g. Layout or Offset, for prototyping, but final
code should not.

Returning |bool| may be OK, I assume compiler will remove |return| statement for
unused returning value.

In another look, it seems we don't need to return |bool| for
|AppendForcedBreak()|.
We can do it in |AppendForceBreak()|, not sure we have call sites of
|AppendForceBreak()| don't need to
update |removed_indexe_|.

It seems |RemoveTrailingCollapsibleNewlineIfNeeded()| too.
In other words, move map construction into them.

   StringBuilder text_;
 
   typedef struct OnExitNode {
@@ -104,7 +115,8 @@ class CORE_EXPORT NGInlineItemsBuilder {
                                     const ComputedStyle*,
                                     LayoutObject*);
 
-  void AppendForcedBreak(const ComputedStyle*, LayoutObject*);
+  // Returns true if a previously appended collapsible space is removed.
+  bool AppendForcedBreak(const ComputedStyle*, LayoutObject*);
 
   // Because newlines may be removed depends on following characters, newlines
   // at the end of input string is not added to |text_| but instead
@@ -112,15 +124,19 @@ class CORE_EXPORT NGInlineItemsBuilder {
   // This function determines whether to add the newline or ignore.
   void ProcessPendingNewline(const String&, const ComputedStyle*);
 
-  // Removes the collapsible space at the end of |text_| if exists.
-  void RemoveTrailingCollapsibleSpaceIfExists(unsigned*);
+  // Removes the collapsible space at the end of |text_| if exists. Returns true
+  // if such a space is removed.
+  bool RemoveTrailingCollapsibleSpaceIfExists(unsigned*);
   void RemoveTrailingCollapsibleSpace(unsigned*);
 
-  void RemoveTrailingCollapsibleNewlineIfNeeded(unsigned*,
+  // Returns true if a collapsible newline at the end of |text_| is removed.
+  bool RemoveTrailingCollapsibleNewlineIfNeeded(unsigned*,
                                                 const String&,
                                                 unsigned,
                                                 const ComputedStyle*);
 
+  void AddLastTrailingSpaceToRemovedIndexes(unsigned);
+
   void Enter(LayoutObject*, UChar);
   void Exit(LayoutObject*);
 };