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..1bdfa7712e0bbb6c5677b145912ea449224cbe67 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
@@ -27,11 +27,21 @@ 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 |collapsed_indexes| to
+// return how spaces are collapsed. For each string appended to
+// NGInlineItemsBuilder, a Vector<unsigned> will be added to
+// |collapsed_indexes|, storing the indexes of all collapsed 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), collapsed_indexes_(nullptr) {}
+  explicit NGInlineItemsBuilder(Vector<NGInlineItem>* items,
+                                Vector<Vector<unsigned>>* collapsed_indexes)
+      : items_(items), collapsed_indexes_(collapsed_indexes) {}
   ~NGInlineItemsBuilder();
 
   String ToString();
@@ -92,6 +102,13 @@ class CORE_EXPORT NGInlineItemsBuilder {
   bool is_svgtext_ = false;
   bool has_bidi_controls_ = false;
 
+  // When provided, the indexes of collapsed spaces in each appended string will
+  // be eventually added to |collapsed_indexes_|.
+  Vector<Vector<unsigned>>* collapsed_indexes_;

[yosin_UTC9, 2017/06/21 04:22:41]

Could you introduce OffsetMappingBuilder class to hold |collapsed_indexes_| and
|input_strings_length_| and
member functions to update them in another file?
We would like to bold editing related part in core/layout for ease of
maintenance.

e.g.
  if (collapsed_indexes_) {
    input_strings_lengths_.push_back(string.length());
    collapsed_indexes_->emplace_back();
  }
=>
  if (offset_builder_)
    offset_builder_->Append(string);


  if (collapsed_indeexs_)
    collapsed_indexes_->back().push_back(i);
=>
   if (offset_builder_)
    offset_builder_->Append(i);

These names are just example. 
Please use appropriate name for member functions.

[kojii, 2017/06/21 09:47:13]

I take this opposite. Since this is tightly coupled with this class, separating
out will make us harder to change this class. Higher-level classes/logic can
live in editing.

Agree on better to have a struct/class of a vector, rather than two vector, but
I haven't understood why we need a vector of vector for each append. Isn't what
we need just a map of character indexes?

If possible, I'd like to avoid editing code depends on how we call Append(), but
you must have some needs for this data, can you explain?

[Xiaocheng, 2017/06/21 18:15:06]

I agree with yosin@ that it's a better pattern if we take an
OffsetMappingBuilder as the optional parameter instead of |collapsed_indexes|.
With the builder, we can return the collapsing information in a more flexible
way, and allows the builder to utilize the information more easily. I'll update
the patch to see how it works.
The current design is: for each string appended, we need a vector to store the
indexes of the collapsed spaces. So we need a vector of vector for all the
strings together. An offset mapping builder will take this vector of vector and
build the mapping.
I'll see if the dependence can be reduced in the updated patch.

+  // Length of each appended string. Used only for computing the indexes of
+  // collapsed spaces when |collapsed_indexes_| is provided.
+  Vector<unsigned> input_strings_lengths_;
+
   void AppendWithWhiteSpaceCollapsing(const String&,
                                       unsigned start,
                                       unsigned end,
@@ -104,7 +121,7 @@ class CORE_EXPORT NGInlineItemsBuilder {
                                     const ComputedStyle*,
                                     LayoutObject*);
 
-  void AppendForcedBreak(const ComputedStyle*, LayoutObject*);
+  void AppendForcedBreak(const ComputedStyle*, LayoutObject*, unsigned);
 
   // Because newlines may be removed depends on following characters, newlines
   // at the end of input string is not added to |text_| but instead
@@ -113,14 +130,16 @@ class CORE_EXPORT NGInlineItemsBuilder {
   void ProcessPendingNewline(const String&, const ComputedStyle*);
 
   // Removes the collapsible space at the end of |text_| if exists.
-  void RemoveTrailingCollapsibleSpaceIfExists(unsigned*);
-  void RemoveTrailingCollapsibleSpace(unsigned*);
+  void RemoveTrailingCollapsibleSpaceIfExists(unsigned*, unsigned);
+  void RemoveTrailingCollapsibleSpace(unsigned*, unsigned);
 
   void RemoveTrailingCollapsibleNewlineIfNeeded(unsigned*,
                                                 const String&,
                                                 unsigned,
                                                 const ComputedStyle*);
 
+  void AddLastTrailingSpaceToRemovedIndexes(unsigned);
+
   void Enter(LayoutObject*, UChar);
   void Exit(LayoutObject*);
 };