[LayoutNG] Add NGInlineBreakToken and back of NGInlineLayoutAlgorithm

[LayoutNG] Add NGInlineBreakToken and back of NGInlineLayoutAlgorithm

This patch implements NGInlineBreakToken, a subclass of NGBreakToken for
inline. Unfinished break tokens are attached to lineboxes that do not
fit in the constraint space, and layout can restart from an unfinished
NGInlineBreakToken.

NGLineBuilder now has equivalent interface as NGLayoutAlgorithm, and
that it is renamed to NGInlineLayoutAlgorithm.

NGTextLayoutAlgorithm is now a line breaker and is not
interface-compatible with NGLayoutAlgorithm that it is renamed to
NGLineBreaker.

BUG=636993
Review-Url: https://codereview.chromium.org/2772503004
Cr-Commit-Position: refs/heads/master@{#459788}
Committed: https://chromium.googlesource.com/chromium/src/+/e1bb8449b2eeab40d89574339ac634c6e1f797cd

[kojii, 2017-03-24 08:53:24]

Patchset #2 (id:20001) has been deleted

[kojii, 2017-03-24 20:29:49]

Description was changed from

==========
ngbreaktoken

BUG=
==========

to

==========
[LayoutNG] Add NGInlineBreakToken and back of NGInlineLayoutAlgorithm

This patch implements NGInlineBreakToken, a subclass of NGBreakToken for
inline.

NGLineBuilder now has equivalent interface as NGLayoutAlgorithm, and
that it is renamed to NGInlineLayoutAlgorithm.

NGTextLayoutAlgorithm is now a line breaker and is not
interface-compatible with NGLayoutAlgorithm that it is renamed to
NGLineBreaker.

BUG=636993
==========

[kojii, 2017-03-24 20:32:20]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 20:33:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2017-03-24 20:34:27]

Description was changed from

==========
[LayoutNG] Add NGInlineBreakToken and back of NGInlineLayoutAlgorithm

This patch implements NGInlineBreakToken, a subclass of NGBreakToken for
inline.

NGLineBuilder now has equivalent interface as NGLayoutAlgorithm, and
that it is renamed to NGInlineLayoutAlgorithm.

NGTextLayoutAlgorithm is now a line breaker and is not
interface-compatible with NGLayoutAlgorithm that it is renamed to
NGLineBreaker.

BUG=636993
==========

to

==========
[LayoutNG] Add NGInlineBreakToken and back of NGInlineLayoutAlgorithm

This patch implements NGInlineBreakToken, a subclass of NGBreakToken for
inline. Unfinished break tokens are attached to lineboxes that do not
fit in the constraint space, and layout can restart from an unfinished
NGInlineBreakToken.

NGLineBuilder now has equivalent interface as NGLayoutAlgorithm, and
that it is renamed to NGInlineLayoutAlgorithm.

NGTextLayoutAlgorithm is now a line breaker and is not
interface-compatible with NGLayoutAlgorithm that it is renamed to
NGLineBreaker.

BUG=636993
==========

[kojii, 2017-03-24 20:35:22]

kojii@chromium.org changed reviewers:
+ eae@chromium.org, ikilpatrick@chromium.org

[kojii, 2017-03-24 20:35:23]

PTAL.

[ikilpatrick, 2017-03-24 20:58:05]

This looks great \o/

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right):

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:91: // the last
break token is needed to be taken to parent.
.nit ... is needed to be reported to the parent. ?

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:98: break;
default:
  NOTREACHED()?

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:192: did_break_
= true;
does this need to check if last_inline_break_token_->IsFinished() ?

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc
(right):

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc:549:
return CreateFragments();
can just inline this function now?

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.h
(right):

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.h:55: //
This Empties the current line.
This empties the current line. (no E cap)

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.h:56: void
Initialize(unsigned index, unsigned offset);
can this be private?

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.h:84: //
direction.
do we indent text here?

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.h:102: //
Copy fragment data of all lines to LayoutBlockFlow.
add TODO move to NGInlineNode?

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_breaker.cc (right):

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_breaker.cc:24: void
NGLineBreaker::BreakLines(NGInlineLayoutAlgorithm* algorithm,
this is just a thought for the future, (no need to do anything now) - but I get
the sense we want to invert the NGInlineLayoutAlgorithm & NGLineBreaker
relationship.

E.g. have the algorithm call the linebreaker to do things, and not the other way
around.

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_breaker.h (right):

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_breaker.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2017?

[commit-bot: I haz the power, 2017-03-24 22:08:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 22:08:32]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-03-27 03:12:38]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 03:12:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2017-03-27 03:15:12]

PTAL. All done, with one reply below. Thank you for your prompt review as
always.

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_breaker.cc (right):

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_breaker.cc:24: void
NGLineBreaker::BreakLines(NGInlineLayoutAlgorithm* algorithm,
On 2017/03/24 at 20:58:05, ikilpatrick wrote:
> this is just a thought for the future, (no need to do anything now) - but I
get the sense we want to invert the NGInlineLayoutAlgorithm & NGLineBreaker
relationship.
> 
> E.g. have the algorithm call the linebreaker to do things, and not the other
way around.

Agreed, thank you for sharing this, I thought that too. NGLineBreaker will have
a huge overhaul when shaper-driven line breaker is ready, I'll invert the
relationship at that point.

[commit-bot: I haz the power, 2017-03-27 04:39:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 04:39:31]

Dry run: This issue passed the CQ dry run.

[ikilpatrick, 2017-03-27 09:20:00]

lgtm (sorry should have said this before).

Looks great!

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_breaker.cc (right):

https://codereview.chromium.org/2772503004/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_breaker.cc:24: void
NGLineBreaker::BreakLines(NGInlineLayoutAlgorithm* algorithm,
On 2017/03/27 03:15:12, kojii wrote:
> On 2017/03/24 at 20:58:05, ikilpatrick wrote:
> > this is just a thought for the future, (no need to do anything now) - but I
> get the sense we want to invert the NGInlineLayoutAlgorithm & NGLineBreaker
> relationship.
> > 
> > E.g. have the algorithm call the linebreaker to do things, and not the other
> way around.
> 
> Agreed, thank you for sharing this, I thought that too. NGLineBreaker will
have
> a huge overhaul when shaper-driven line breaker is ready, I'll invert the
> relationship at that point.

Sg, thanks. Yeah it started to look a bit strange.

.nit worth adding TODO/NOTE?

https://codereview.chromium.org/2772503004/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.h
(right):

https://codereview.chromium.org/2772503004/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.h:65: //
direction.
+indent.

[kojii, 2017-03-27 13:18:18]

All done, thank you.

We're likely to tweak the interface design of line breaker more. Another goal is
to keep it pluggable, so that we can add different algorithms, such as
paragraph-level optimized line breaker. Thank you for your advice in advance!

[kojii, 2017-03-27 13:18:27]

The CQ bit was checked by kojii@chromium.org

[kojii, 2017-03-27 13:18:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2772503004/#ps150001
(title: "ikilpatrik nits")

[commit-bot: I haz the power, 2017-03-27 13:18:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 13:21:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 13:21:23]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kojii, 2017-03-27 13:32:42]

The CQ bit was checked by kojii@chromium.org

[kojii, 2017-03-27 13:32:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2772503004/#ps170001
(title: "Resolved merge conflicts")

[commit-bot: I haz the power, 2017-03-27 13:32:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 15:48:15]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1490621562125130,
"parent_rev": "12e16766adec5bb1e5cdd73d3cd8f5016b59ce18", "commit_rev":
"e1bb8449b2eeab40d89574339ac634c6e1f797cd"}

[commit-bot: I haz the power, 2017-03-27 15:49:10]

Description was changed from

==========
[LayoutNG] Add NGInlineBreakToken and back of NGInlineLayoutAlgorithm

This patch implements NGInlineBreakToken, a subclass of NGBreakToken for
inline. Unfinished break tokens are attached to lineboxes that do not
fit in the constraint space, and layout can restart from an unfinished
NGInlineBreakToken.

NGLineBuilder now has equivalent interface as NGLayoutAlgorithm, and
that it is renamed to NGInlineLayoutAlgorithm.

NGTextLayoutAlgorithm is now a line breaker and is not
interface-compatible with NGLayoutAlgorithm that it is renamed to
NGLineBreaker.

BUG=636993
==========

to

==========
[LayoutNG] Add NGInlineBreakToken and back of NGInlineLayoutAlgorithm

This patch implements NGInlineBreakToken, a subclass of NGBreakToken for
inline. Unfinished break tokens are attached to lineboxes that do not
fit in the constraint space, and layout can restart from an unfinished
NGInlineBreakToken.

NGLineBuilder now has equivalent interface as NGLayoutAlgorithm, and
that it is renamed to NGInlineLayoutAlgorithm.

NGTextLayoutAlgorithm is now a line breaker and is not
interface-compatible with NGLayoutAlgorithm that it is renamed to
NGLineBreaker.

BUG=636993

Review-Url: https://codereview.chromium.org/2772503004
Cr-Commit-Position: refs/heads/master@{#459788}
Committed:
https://chromium.googlesource.com/chromium/src/+/e1bb8449b2eeab40d89574339ac6...
==========

[commit-bot: I haz the power, 2017-03-27 15:49:11]

Committed patchset #9 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/e1bb8449b2eeab40d89574339ac6...