Make NGInlineItemsBuilder construct whitespace-collapsed offset mapping

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

 // 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.
 
 #ifndef NGInlineItemsBuilder_h
 #define NGInlineItemsBuilder_h
 
 #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"
 
 namespace blink {
 
 class ComputedStyle;
 class LayoutObject;
 class NGInlineItem;
 
 // NGInlineItemsBuilder builds a string and a list of NGInlineItem from inlines.
 //
 // When appending, spaces are collapsed according to CSS Text, The white space
 // processing rules
 // https://drafts.csswg.org/css-text-3/#white-space-rules
 //
 // 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();
 
   void SetIsSVGText(bool value) { is_svgtext_ = value; }
 
   // Returns whether the items contain any Bidi controls.
   bool HasBidiControls() const { return has_bidi_controls_; }
 
   // Append a string.
@@ skipping 26 matching lines @@
   // Append a Bidi control character, for LTR or RTL depends on the style.
   void AppendBidiControl(const ComputedStyle*, UChar ltr, UChar rtl);
 
   void EnterBlock(const ComputedStyle*);
   void ExitBlock();
   void EnterInline(LayoutObject*);
   void ExitInline(LayoutObject*);
 
  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 {
     LayoutObject* node;
     UChar character;
   } OnExitNode;
   Vector<OnExitNode> exits_;
 
   enum class CollapsibleSpace { kNone, kSpace, kNewline };
 
   CollapsibleSpace last_collapsible_space_ = CollapsibleSpace::kSpace;
   bool is_svgtext_ = false;
   bool has_bidi_controls_ = false;
 
   void AppendWithWhiteSpaceCollapsing(const String&,
                                       unsigned start,
                                       unsigned end,
                                       const ComputedStyle*,
                                       LayoutObject*);
   void AppendWithoutWhiteSpaceCollapsing(const String&,
                                          const ComputedStyle*,
                                          LayoutObject*);
   void AppendWithPreservingNewlines(const String&,
                                     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
   // |has_pending_newline_| flag is set.
   // 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*);
 };
 
 }  // namespace blink
 
 #endif  // NGInlineItemsBuilder_h