Use Opportunity Iterator to position text fragments in NGLineBuilder

third_party/WebKit/Source/core/layout/ng/ng_text_layout_algorithm.cc

 // 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/ng/ng_text_layout_algorithm.h"
 
 #include "core/layout/ng/ng_box_fragment.h"
 #include "core/layout/ng/ng_break_token.h"
 #include "core/layout/ng/ng_constraint_space.h"
 #include "core/layout/ng/ng_fragment_builder.h"
 #include "core/layout/ng/ng_inline_node.h"
+#include "core/layout/ng/ng_layout_opportunity_iterator.h"
 #include "core/layout/ng/ng_line_builder.h"
 #include "core/layout/ng/ng_text_fragment.h"
 #include "core/style/ComputedStyle.h"
 #include "platform/text/TextBreakIterator.h"
 
 namespace blink {
 
 NGTextLayoutAlgorithm::NGTextLayoutAlgorithm(
     NGInlineNode* inline_box,
     NGConstraintSpace* constraint_space,
@@ skipping 15 matching lines @@
 
 void NGTextLayoutAlgorithm::LayoutInline(NGLineBuilder* line_builder) {
   // TODO(kojii): Make this tickable. Each line is easy. Needs more thoughts
   // for each fragment in a line. Bidi reordering is probably atomic.
   // TODO(kojii): oof is not well-thought yet. The bottom static position may be
   // in the next line, https://github.com/w3c/csswg-drafts/issues/609
   const String& text_content = inline_box_->Text();
   DCHECK(!text_content.isEmpty());
   // TODO(kojii): Give the locale to LazyLineBreakIterator.
   LazyLineBreakIterator line_break_iterator(text_content);
+
   unsigned current_offset = 0;
   line_builder->SetStart(0, current_offset);
+  line_builder->FindNextLayoutOpportunity();

[kojii, 2017/03/09 03:01:54]

It looks more natural to call this at the end of |SetStart|.

[Gleb Lanbin, 2017/03/09 22:26:45]

done. Although I think it adds an additional side effect to SetStart and
CreateLineUpToLastBreakOpportunity and violates the single responsibility
principle.

may be we can to change the line builder API a bit
1) rename SetStart -> Initialize
2) Move the logic starting from line 91-107 to line_builder->Finalize or
line_builder->CreateLine or line_builder->BuildLine
Feel free to ignore this if you already have a solution for this. 

[kojii, 2017/03/10 03:12:43]

Yeah, I was thinking of this, eae@ misunderstood this before, and I'm seeing a
need of "SetStart without initialize". But let's do this later, we'll probably
have large refactoring + renaming sometime soon, and currently eae@ has a WIP to
re-write this line breaker.
Finalize isn't probably appropriate here though, since the call flow would be:
  SetStart w/Initialize
  CreateLine
  (may need to call SetStart without initialize)
  CreateLine
  CreateLine
  CreateFragments <= so maybe this is finalize
but this may change as we support BreakToken.

Thank you for the nice advice anyway.

   const unsigned end_offset = text_content.length();
   while (current_offset < end_offset) {
     // Find the next break opportunity.
     int tmp_next_breakable_offset = -1;
     line_break_iterator.isBreakable(current_offset + 1,
                                     tmp_next_breakable_offset);
     current_offset =
         tmp_next_breakable_offset >= 0 ? tmp_next_breakable_offset : end_offset;
     DCHECK_LE(current_offset, end_offset);
 
     // Advance the break opportunity to the end of hangable characters; e.g.,
     // spaces.
     // Unlike the ICU line breaker, LazyLineBreakIterator breaks before
     // breakable spaces, and expect the line breaker to handle spaces
     // differently. This logic computes in the ICU way; break after spaces, and
     // handle spaces as hangable characters.
     unsigned start_of_hangables = current_offset;
     while (current_offset < end_offset &&
            IsHangable(text_content[current_offset]))
       current_offset++;
 
     // Set the end to the next break opportunity.
     line_builder->SetEnd(current_offset);
 
     // If there are more available spaces, mark the break opportunity and fetch
     // more text.
     if (line_builder->CanFitOnLine()) {

[ikilpatrick, 2017/03/09 22:00:51]

we also need logic to check if the height of the linebox can fit within the
current opportunity, where does that fit in? I worry that we'll need to do
line-by-line layout to have the right layering here?
@kojii thoughts?

[Gleb Lanbin, 2017/03/09 22:26:45]

good point. added TODO for now

[kojii, 2017/03/10 03:12:43]

TODO sounds good for now. I think this will be a collaboration of NGLineBuilder
and line breaker.

There are a few other cases where we need to move a line down after it was
built:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/css3...

In these cases, we build a line assuming it will fit, but find out it doesn't as
we build it or after we built it, and move it down. It may need to build the
line again using new logical top, because available width will change.

       line_builder->SetBreakOpportunity();
       continue;
     }
 
     // Compute hangable characters if exists.
     if (current_offset != start_of_hangables) {
       line_builder->SetStartOfHangables(start_of_hangables);
       // If text before hangables can fit, include it in the current line.
       if (line_builder->CanFitOnLine())
         line_builder->SetBreakOpportunity();
     }
 
     if (!line_builder->HasBreakOpportunity()) {
       // The first word (break opportunity) did not fit on the line.
       // Create a line including items that don't fit, allowing them to
       // overflow.
       line_builder->CreateLine();
     } else {
       line_builder->CreateLineUpToLastBreakOpportunity();
 
       // Items after the last break opportunity were sent to the next line.
       // Set the break opportunity, or create a line if the word doesn't fit.
       if (line_builder->HasItems()) {
         if (!line_builder->CanFitOnLine())
           line_builder->CreateLine();
         else
           line_builder->SetBreakOpportunity();
       }
     }
+    line_builder->FindNextLayoutOpportunity();

[kojii, 2017/03/09 03:01:54]

and at the end of |CreateLineUpToLastBreakOpportunity|.

[Gleb Lanbin, 2017/03/09 22:26:45]

Done.

   }
 
   // If inline children ended with items left in the line builder, create a line
   // for them.
   if (line_builder->HasItems())
     line_builder->CreateLine();
 }
 
 }  // namespace blink