[LayoutNG] Implement atomic inlines for LayoutNGInline

[LayoutNG] Implement atomic inlines for LayoutNGInline

This patch implements the initial support for atomic inlines in
LayoutNGInline.

Atomic inlines are the first type of objects in LayoutNGInline that
needs be laid out to compute in-flow inline size. NGLineBuilder now
keeps NGLayoutResult for atomic inlines while it builds a line box.

Some tests turned from false-passes to real-failures. Because atomic
inlines were not rendered before this patch, tests using images and
inline-blocks passed by comparing blank result with blank reference.

Some work are not included in this patch and will follow, such as:
* Keep NGBlockNode for atomic inlines in NGLayoutInlineItem.
* Use NGLayoutResult not only for layout but also for fragment
  generation.
* Margin, padding, borders.
* BreakToken if we support breaking atomic inlines[1].
* ...and obviously more.

[1] https://github.com/w3c/csswg-drafts/issues/1111

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

[kojii, 2017-03-13 01:09:30]

Description was changed from

==========
WIP: ngatomic

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

to

==========
[LayoutNG] Implement atomic inlines for LayoutNGInline

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

[kojii, 2017-03-13 01:12:02]

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

[kojii, 2017-03-13 01:12:03]

Emil, could you advise?

Still WIP, but I chose to add LayoutResult to NGLayoutInlineItem. This looks to
work, but when I think about adding BreakToken, it will produce multiple
LayoutResult for an atomic inline, so I started to think this is a wrong
direction.

Any thoughts?

[eae, 2017-03-13 01:17:14]

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

[eae, 2017-03-13 01:17:15]

The overall implementation looks good but the LayoutResult mapping has some
problems as you identified.

Ideally we don't want to store the LayoutResults on the NGLayoutInlineItem at
all. We could either have the builder keep track of it and pass it to the
NGLayoutInlineItem or pass it up along with the fragment.

I know that Christian and Ian have been thinking about this lately, they might
have other ideas.

[ikilpatrick, 2017-03-15 16:39:40]

ikilpatrick@chromium.org changed reviewers:
+ mstensho@opera.com

[ikilpatrick, 2017-03-15 16:39:41]

+morten; need to decide if we are going to allow fragmenting of atomic inlines -
see comment.

https://codereview.chromium.org/2745973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc (right):

https://codereview.chromium.org/2745973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc:320:
LayoutAtomicInlines(*constraint_space);
We need to decide if we are going to support fragmenting atomic inlines, if so
we'll need to do this in the same position as floats.

I.e. we need to determine the position of the atomic inline to determine where
it will fragment before we can perform layout.

This is completely inconsistent between vendors \o/

I.e.
https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E...

Edge: Do fragment and place adjacent text in the first column.
Blink/Webkit: Do fragment and place text in the second column.
FF: Don't fragment and place text in first column.

[kojii, 2017-03-15 17:30:28]

https://codereview.chromium.org/2745973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc (right):

https://codereview.chromium.org/2745973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc:320:
LayoutAtomicInlines(*constraint_space);
It'd be great if we can, though I acknowledge it's not simple to implement.

Edge even tries to break at line box boundaries in inline-block, so that lines
in the inline-block won't cut off. I heard people complains Blink/WebKit to
break at arbitrary point and often cut the lower half of a line when they print.

CSS isn't very clear about this, but if the rules are also applied to inside of
inline-block, Edge looks conforming. Surprised Gecko doesn't do it, I thought it
does.
https://drafts.csswg.org/css2/page.html#allowed-page-breaks

Given Emil's comment, I'm thinking to make this CL can go either way, but we
need to decide when we support BreakToken for inline. I wish us at least to try.

[ikilpatrick, 2017-03-15 17:56:31]

On 2017/03/15 17:30:28, kojii wrote:
>
https://codereview.chromium.org/2745973002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc (right):
> 
>
https://codereview.chromium.org/2745973002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc:320:
> LayoutAtomicInlines(*constraint_space);
> It'd be great if we can, though I acknowledge it's not simple to implement.
> 
> Edge even tries to break at line box boundaries in inline-block, so that lines
> in the inline-block won't cut off. I heard people complains Blink/WebKit to
> break at arbitrary point and often cut the lower half of a line when they
print.
> 
> CSS isn't very clear about this, but if the rules are also applied to inside
of
> inline-block, Edge looks conforming. Surprised Gecko doesn't do it, I thought
it
> does.
> https://drafts.csswg.org/css2/page.html#allowed-page-breaks
> 
> Given Emil's comment, I'm thinking to make this CL can go either way, but we
> need to decide when we support BreakToken for inline. I wish us at least to
try.

I'd like us to support BreakTokens for inline sooner rather than later; mainly
because of these arch. issues that it'll bring up. I can try looking at it if
you like?

[kojii, 2017-03-15 18:29:04]

On 2017/03/15 at 17:56:31, ikilpatrick wrote:
> 
> I'd like us to support BreakTokens for inline sooner rather than later; mainly
because of these arch. issues that it'll bring up. I can try looking at it if
you like?

Great if you can help.

Atomic inline is blocking many tests, I'd like to get initial sense how this
would be like. I think working on this should also clarify the requirements of
inline break tokens, as we already did one ;)

I'm thinking then to get linebox fragment and API re-unifying to make the
interface to use break tokens, but since you must have much clearer view on how
it will be, you drafting it should be faster to get there.

[kojii, 2017-03-15 18:37:07]

Patchset #6 (id:100001) has been deleted

[kojii, 2017-03-15 18:44:48]

PTAL. PS6 moved NGLayoutResult to NGLineBuilder.

This supports break tokens better, since we layout atomic inlines only up to
needed for current line, or next if it doesn't fit.

TestExpectations changes are to find out which tests pass with this CL. I'll
cleanup after I examined the test results.

[kojii, 2017-03-15 18:54:41]

linebox fragment is likely to conflict with this, I'll think about API re-unify
first.

[kojii, 2017-03-16 08:57:10]

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

[commit-bot: I haz the power, 2017-03-16 08:57:35]

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

[mstensho (USE GERRIT), 2017-03-16 09:20:15]

Good news! Atomic inlines should never be fragmented. :)

https://codereview.chromium.org/2745973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc (right):

https://codereview.chromium.org/2745973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc:320:
LayoutAtomicInlines(*constraint_space);
On 2017/03/15 17:30:28, kojii wrote:
> It'd be great if we can, though I acknowledge it's not simple to implement.
> 
> Edge even tries to break at line box boundaries in inline-block, so that lines
> in the inline-block won't cut off. I heard people complains Blink/WebKit to
> break at arbitrary point and often cut the lower half of a line when they
print.
> 
> CSS isn't very clear about this, but if the rules are also applied to inside
of
> inline-block, Edge looks conforming. Surprised Gecko doesn't do it, I thought
it
> does.
> https://drafts.csswg.org/css2/page.html#allowed-page-breaks
> 
> Given Emil's comment, I'm thinking to make this CL can go either way, but we
> need to decide when we support BreakToken for inline. I wish us at least to
try.

Lines are monolithic, which means that we shouldn't fragment inside them. So we
should never fragment inline-block objects, or any other kind of atomic inline,
for that matter.

See what https://drafts.csswg.org/css-break/#possible-breaks has to say about
monolithic content, and the conclusion: "Since line boxes contain no possible
break points, inline-block and inline-table boxes (and other inline-level
display types that establish a new formatting context) may also be considered
monolithic."

[kojii, 2017-03-16 09:43:17]

Thank you for the pointer to the newer spec, Morten. The text make sense to me.

So it says inline-block "may" be monolithic, while images are monolithic. When 3
browsers break today and the spec is "may", I wish, even not possible on the
first release, us to prepare to allow breaks. Is this forcing us too high bar?

[mstensho (USE GERRIT), 2017-03-16 10:01:16]

On 2017/03/16 09:43:17, kojii wrote:
> Thank you for the pointer to the newer spec, Morten. The text make sense to
me.
> 
> So it says inline-block "may" be monolithic, while images are monolithic. When
3
> browsers break today and the spec is "may", I wish, even not possible on the
> first release, us to prepare to allow breaks. Is this forcing us too high bar?

The way I read this text, is that atomic inlines will *always* be monolithic,
since they sit on a line, and a line in itself is defined as monolithic.
Therefore, it follows that anything on the line is monolithic (or rather
belonging to a (monolithic) line). Maybe they could rephrase the text to that
effect. It's meaningless to consider breakpoints on an element (e.g.
inline-block) inside something monolithic (line).

Note that, while it appears that Blink may break inside inlines, Blink doesn't
consider breakpoints inside lines and their inlines. However, if it's impossible
to place the complete line inside of one fragmentainer (because the line is
taller than one column, for instance), we'll brutally slice it (just like for
images), without considering the content on the inside. More about this here:
https://drafts.csswg.org/css-break/#unforced-breaks

"In such cases, the UA may also fragment the contents of monolithic elements by
slicing the element’s graphical representation"

[commit-bot: I haz the power, 2017-03-16 10:52:53]

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

[commit-bot: I haz the power, 2017-03-16 10:52:54]

Dry run: Try jobs failed on following builders:
  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-16 11:32:46]

On 2017/03/16 at 10:01:16, mstensho wrote:
> On 2017/03/16 09:43:17, kojii wrote:
> > Thank you for the pointer to the newer spec, Morten. The text make sense to
me.
> > 
> > So it says inline-block "may" be monolithic, while images are monolithic.
When 3
> > browsers break today and the spec is "may", I wish, even not possible on the
> > first release, us to prepare to allow breaks. Is this forcing us too high
bar?
> 
> The way I read this text, is that atomic inlines will *always* be monolithic,
since they sit on a line, and a line in itself is defined as monolithic.
Therefore, it follows that anything on the line is monolithic (or rather
belonging to a (monolithic) line). Maybe they could rephrase the text to that
effect. It's meaningless to consider breakpoints on an element (e.g.
inline-block) inside something monolithic (line).

Ok, sounds like text appears ambiguous if you read so. AFAIR, it's not the
intention.

The basic idea is "a line is monolithic" and "boxes can break between lines".
This looks reasonable, but is contradicting to each other when lines can nest
(i.e., inline-block.) Someone pointed out this contradiction to CSS2, so the
last sentence was added to solve the special case as "may", whichever can win.

That's what I remember, but maybe wrong, we can ask clarification (and better
wording).

> Note that, while it appears that Blink may break inside inlines, Blink doesn't
consider breakpoints inside lines and their inlines. However, if it's impossible
to place the complete line inside of one fragmentainer (because the line is
taller than one column, for instance), we'll brutally slice it (just like for
images), without considering the content on the inside. More about this here:
https://drafts.csswg.org/css-break/#unforced-breaks

Yes, that's exactly the point I heard users complained to Blink/WebKit, since
Trident/Edge breaks only between lineboxes. It's logically not possible to do it
all the cases, but they do quite nicely in common cases.

> "In such cases, the UA may also fragment the contents of monolithic elements
by slicing the element’s graphical representation"

This applies to "monolithic elements", and "inline block may be monolithic", so
this may or may not apply to inline block. This applies to images though. And
"slicing" means to break, so that the below half of images should appear on the
next page. I'm not sure people expects it for multicol, but I believe people
expects it when printing.

[kojii, 2017-03-16 14:05:24]

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

[commit-bot: I haz the power, 2017-03-16 14:05:33]

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-16 14:09:10]

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

[kojii, 2017-03-16 14:09:18]

Patchset #9 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-03-16 14:09:24]

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-16 14:50:37]

Filed an issue for clarification:
https://github.com/w3c/csswg-drafts/issues/1111
Please feel free to join there, or keep discussion here or on e-mail and I can
relay if it's preferred.

[kojii, 2017-03-16 15:27:20]

Description was changed from

==========
[LayoutNG] Implement atomic inlines for LayoutNGInline

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

to

==========
[LayoutNG] Implement atomic inlines for LayoutNGInline

This patch implements the initial support for atomic inlines in
LayoutNGInline.

Atomic inlines are the first type of objects in LayoutNGInline that
needs be laid out to compute in-flow inline size. NGLineBuilder now
keeps NGLayoutResult for atomic inlines while it builds a line box.

Some tests turned from false-passes to real-failures. Because atomic
inlines were not rendered before this patch, tests using images or
inline-blocks pass by comparing blank result with blank reference.

Some work are not included in this patch and will follow, such as:
* Keep NGBlockNode for atomic inlines in NGLayoutInlineItem.
* Use NGLayoutResult not only for layout but also for fragment
  generation.
* Margin, padding, borders.
* BreakToken if we support breaking atomic inlines[1].

[1] https://github.com/w3c/csswg-drafts/issues/1111

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

[kojii, 2017-03-16 15:28:05]

Description was changed from

==========
[LayoutNG] Implement atomic inlines for LayoutNGInline

This patch implements the initial support for atomic inlines in
LayoutNGInline.

Atomic inlines are the first type of objects in LayoutNGInline that
needs be laid out to compute in-flow inline size. NGLineBuilder now
keeps NGLayoutResult for atomic inlines while it builds a line box.

Some tests turned from false-passes to real-failures. Because atomic
inlines were not rendered before this patch, tests using images or
inline-blocks pass by comparing blank result with blank reference.

Some work are not included in this patch and will follow, such as:
* Keep NGBlockNode for atomic inlines in NGLayoutInlineItem.
* Use NGLayoutResult not only for layout but also for fragment
  generation.
* Margin, padding, borders.
* BreakToken if we support breaking atomic inlines[1].

[1] https://github.com/w3c/csswg-drafts/issues/1111

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

to

==========
[LayoutNG] Implement atomic inlines for LayoutNGInline

This patch implements the initial support for atomic inlines in
LayoutNGInline.

Atomic inlines are the first type of objects in LayoutNGInline that
needs be laid out to compute in-flow inline size. NGLineBuilder now
keeps NGLayoutResult for atomic inlines while it builds a line box.

Some tests turned from false-passes to real-failures. Because atomic
inlines were not rendered before this patch, tests using images and
inline-blocks passed by comparing blank result with blank reference.

Some work are not included in this patch and will follow, such as:
* Keep NGBlockNode for atomic inlines in NGLayoutInlineItem.
* Use NGLayoutResult not only for layout but also for fragment
  generation.
* Margin, padding, borders.
* BreakToken if we support breaking atomic inlines[1].

[1] https://github.com/w3c/csswg-drafts/issues/1111

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

[kojii, 2017-03-16 15:28:45]

Description was changed from

==========
[LayoutNG] Implement atomic inlines for LayoutNGInline

This patch implements the initial support for atomic inlines in
LayoutNGInline.

Atomic inlines are the first type of objects in LayoutNGInline that
needs be laid out to compute in-flow inline size. NGLineBuilder now
keeps NGLayoutResult for atomic inlines while it builds a line box.

Some tests turned from false-passes to real-failures. Because atomic
inlines were not rendered before this patch, tests using images and
inline-blocks passed by comparing blank result with blank reference.

Some work are not included in this patch and will follow, such as:
* Keep NGBlockNode for atomic inlines in NGLayoutInlineItem.
* Use NGLayoutResult not only for layout but also for fragment
  generation.
* Margin, padding, borders.
* BreakToken if we support breaking atomic inlines[1].

[1] https://github.com/w3c/csswg-drafts/issues/1111

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

to

==========
[LayoutNG] Implement atomic inlines for LayoutNGInline

This patch implements the initial support for atomic inlines in
LayoutNGInline.

Atomic inlines are the first type of objects in LayoutNGInline that
needs be laid out to compute in-flow inline size. NGLineBuilder now
keeps NGLayoutResult for atomic inlines while it builds a line box.

Some tests turned from false-passes to real-failures. Because atomic
inlines were not rendered before this patch, tests using images and
inline-blocks passed by comparing blank result with blank reference.

Some work are not included in this patch and will follow, such as:
* Keep NGBlockNode for atomic inlines in NGLayoutInlineItem.
* Use NGLayoutResult not only for layout but also for fragment
  generation.
* Margin, padding, borders.
* BreakToken if we support breaking atomic inlines[1].
* ...and obviously more.

[1] https://github.com/w3c/csswg-drafts/issues/1111

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

[commit-bot: I haz the power, 2017-03-16 15:43:50]

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

[commit-bot: I haz the power, 2017-03-16 15:43:51]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-03-16 15:45:48]

PTAL. I think PS9 is ready for review, and at this level, whether to allow break
or not doesn't matter. Obviously we need a consensus on it though.

[ikilpatrick, 2017-03-16 15:58:57]

On 2017/03/16 15:45:48, kojii wrote:
> PTAL. I think PS9 is ready for review, and at this level, whether to allow
break
> or not doesn't matter. Obviously we need a consensus on it though.

I talked to fantasai in person yesterday and she said that FF has the more
correct behaviour, I.e. as the inline-block is inside a line-box there should be
no fragmentation.
The spec is worded that way so that UAs have the option to slice if needed, i.e.
in printing mode where slicing is better for example.

But we should go towards not fragmenting in this case...

[kojii, 2017-03-16 16:12:24]

On 2017/03/16 at 15:58:57, ikilpatrick wrote:
> On 2017/03/16 15:45:48, kojii wrote:
> > PTAL. I think PS9 is ready for review, and at this level, whether to allow
break
> > or not doesn't matter. Obviously we need a consensus on it though.
> 
> I talked to fantasai in person yesterday and she said that FF has the more
correct behaviour, I.e. as the inline-block is inside a line-box there should be
no fragmentation.
> The spec is worded that way so that UAs have the option to slice if needed,
i.e. in printing mode where slicing is better for example.
> 
> But we should go towards not fragmenting in this case...

Ok, she preferred that when I talked before too, and that's what Gecko does. But
on the other hand, Edge and WebKit are not willing to change, so, in my
understanding, that's why the spec has "may". Probably a minority, but some
wants it, and there're no reasons for Edge and WebKit to degrade for them.

To me, this is something I can live without, so I'm ok either way. But I feel
mixed if this new generation engine can't support it in its deep architecture
when we changed our mind to want it. I mean, I wish this to be re-visitable in
future.

[ikilpatrick, 2017-03-16 16:15:41]

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc (right):

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc:246:
NGConstraintSpaceBuilder constraint_space_builder(&parent_space);
we should do this in the parents call, ::Layout takes the space it should
layout, not the parents space.

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:145: return
IsHorizontalWritingMode()
instead of this can you stack allocate a NGBoxFragment? I.e.

return NGFragment(constraint_space->WritingMode(),
layout_result->PhysicalFragment()).InlineSize();

I'd just like to keep all the geometry switching in one place at the moment.

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:158: layout_result =
item.Layout(ConstraintSpace());
We'll need to fix this constraint space, this might be in the wrong writing
mode; fine with TODO atm.

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:158: layout_result =
item.Layout(ConstraintSpace());
on this is not a NGLayoutInputNode, hmmm... i found this surprising. Thoughts on
doing the construction of the NGBlockNode here and the layout?

It'd be nice if NGLayoutInlineItem was as "dumb" as possible?

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:159: return
layout_result;
do we need to assign back to the layout_results_ vector?

[ikilpatrick, 2017-03-16 16:17:51]

On 2017/03/16 16:12:24, kojii wrote:
> On 2017/03/16 at 15:58:57, ikilpatrick wrote:
> > On 2017/03/16 15:45:48, kojii wrote:
> > > PTAL. I think PS9 is ready for review, and at this level, whether to allow
> break
> > > or not doesn't matter. Obviously we need a consensus on it though.
> > 
> > I talked to fantasai in person yesterday and she said that FF has the more
> correct behaviour, I.e. as the inline-block is inside a line-box there should
be
> no fragmentation.
> > The spec is worded that way so that UAs have the option to slice if needed,
> i.e. in printing mode where slicing is better for example.
> > 
> > But we should go towards not fragmenting in this case...
> 
> Ok, she preferred that when I talked before too, and that's what Gecko does.
But
> on the other hand, Edge and WebKit are not willing to change, so, in my
> understanding, that's why the spec has "may". Probably a minority, but some
> wants it, and there're no reasons for Edge and WebKit to degrade for them.
> 
> To me, this is something I can live without, so I'm ok either way. But I feel
> mixed if this new generation engine can't support it in its deep architecture
> when we changed our mind to want it. I mean, I wish this to be re-visitable in
> future.

FF's behaviour seems more correct; I think we should be able to revisit this if
we want.

[kojii, 2017-03-16 16:46:56]

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

[commit-bot: I haz the power, 2017-03-16 16:47:19]

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-16 16:48:24]

On 2017/03/16 at 16:17:51, ikilpatrick wrote:
> FF's behaviour seems more correct; I think we should be able to revisit this
if we want.

Ok, I think it depends on use cases, I heard complaints before, but in your
test, I agree. Thanks for understanding to possible revisit in future.

[kojii, 2017-03-16 16:48:45]

Thank you for your prompt review, all done. See inline.

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc (right):

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc:246:
NGConstraintSpaceBuilder constraint_space_builder(&parent_space);
On 2017/03/16 at 16:15:40, ikilpatrick wrote:
> we should do this in the parents call, ::Layout takes the space it should
layout, not the parents space.

Moved this function to NGLineBuilder, please see there.

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:145: return
IsHorizontalWritingMode()
On 2017/03/16 at 16:15:40, ikilpatrick wrote:
> instead of this can you stack allocate a NGBoxFragment? I.e.
> 
> return NGFragment(constraint_space->WritingMode(),
layout_result->PhysicalFragment()).InlineSize();
> 
> I'd just like to keep all the geometry switching in one place at the moment.

Done. I didn't know I can pass parent's writing-mode, not self, great, thanks.

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:158: layout_result =
item.Layout(ConstraintSpace());
On 2017/03/16 at 16:15:41, ikilpatrick wrote:
> on this is not a NGLayoutInputNode, hmmm... i found this surprising. Thoughts
on doing the construction of the NGBlockNode here and the layout?
> 
> It'd be nice if NGLayoutInlineItem was as "dumb" as possible?

Understood, moved the layout code here.

https://codereview.chromium.org/2745973002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:159: return
layout_result;
On 2017/03/16 at 16:15:40, ikilpatrick wrote:
> do we need to assign back to the layout_results_ vector?

|layout_result| here is a reference to the vector.

[kojii, 2017-03-16 16:53:42]

https://codereview.chromium.org/2745973002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2745973002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:431: if
(IsHorizontalWritingMode()) {
ah, wait, I should use NGBoxFragment here too.

[kojii, 2017-03-16 16:59:14]

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

[commit-bot: I haz the power, 2017-03-16 16:59:57]

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-16 17:00:31]

https://codereview.chromium.org/2745973002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2745973002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:431: if
(IsHorizontalWritingMode()) {
On 2017/03/16 at 16:53:41, kojii wrote:
> ah, wait, I should use NGBoxFragment here too.

Done, PTAL.

[ikilpatrick, 2017-03-16 17:53:19]

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:174: layout_result =
node->Layout(constraint_space.get());
just:

return node->Layout(constraint_space.get());

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:286: LayoutUnit top;
can we switch this to block_start at some point?

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:429: LayoutUnit
height = fragment.BlockSize();
as above, nice if this was just block_size.

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:431: // TODO(kojii):
Try to eliminate the wrapping text fragment and use the
+1.

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:449:
line_box_data->max_ascent_and_leading =
might be nice if we had a specific line box builder eventually.

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.h (right):

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.h:101:
RefPtr<NGLayoutResult>& LayoutItem(const NGLayoutInlineItem&);
this should just return a raw ptr if no ownership is being passed?

[ikilpatrick, 2017-03-16 17:55:16]

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc (right):

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc:312:
sizes.max_content += line_builder.InlineSize(item);
this will need to be changed as some point to query the MinMaxContent sizes if
its an atomic inline, maybe todo?

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:160:
RefPtr<NGLayoutResult>& layout_result = layout_results_[index];
need to store result? or remove this?

[kojii, 2017-03-16 18:17:13]

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

[commit-bot: I haz the power, 2017-03-16 18:18:02]

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-16 18:19:29]

All done, thank you for your prompt review again.

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:174: layout_result =
node->Layout(constraint_space.get());
On 2017/03/16 at 17:53:19, ikilpatrick wrote:
> just:
> 
> return node->Layout(constraint_space.get());

layout_result is a ref to vector, so need to assign.

I guess using ref is confusing, change it to a pointer.

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:286: LayoutUnit top;
On 2017/03/16 at 17:53:19, ikilpatrick wrote:
> can we switch this to block_start at some point?

Done. Not too important but for line-relative logical properties, "line top" is
a defined term, but I understand block_start is easier to understand for other
majority, and it's equivalent except for vertical-lr which we handle by
flipping.

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:429: LayoutUnit
height = fragment.BlockSize();
On 2017/03/16 at 17:53:19, ikilpatrick wrote:
> as above, nice if this was just block_size.

Done. Again, "line height" is a defined term, but for atomic inline case, you're
right that it's equivalent to block size.

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:431: // TODO(kojii):
Try to eliminate the wrapping text fragment and use the
On 2017/03/16 at 17:53:19, ikilpatrick wrote:
> +1.

;)

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:449:
line_box_data->max_ascent_and_leading =
On 2017/03/16 at 17:53:19, ikilpatrick wrote:
> might be nice if we had a specific line box builder eventually.

Thank you, I'll keep it in mind. Was thinking just to add type and
ToLineBoxFragment(), but you're right, FragmentBuilder might be overkill for
line boxes. Thank you for sharing your thoughts on this.

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.h (right):

https://codereview.chromium.org/2745973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.h:101:
RefPtr<NGLayoutResult>& LayoutItem(const NGLayoutInlineItem&);
On 2017/03/16 at 17:53:19, ikilpatrick wrote:
> this should just return a raw ptr if no ownership is being passed?

Done.

[ikilpatrick, 2017-03-16 18:26:16]

lgtm w/ nits. Thanks.

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc (right):

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc:312:
sizes.max_content += line_builder.InlineSize(item);
add todo for atomic inlines.

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:174: return
layout_result->get();
oh you may need to perform a std::move?

i.e.

return std::move(node->Layout(constraint_space.get()));

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:305:
.SetBlockSize(height)
.nit, block_size, but fine with cleaning up later.

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:430: // |fragment|
directly. Currently |CopyFragmentDataToLayoutBlockFlow|
it'd also be nice if we had all the information in the fragments, and could
perform the copy inside NGInlineNode eventually.

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.h (right):

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.h:91: // compute atomic
inlines by laying out them.
.nit
"by layout them out."
or
"by performing layout."

[kojii, 2017-03-16 19:15:31]

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

[commit-bot: I haz the power, 2017-03-16 19:16:20]

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-16 19:16:33]

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc (right):

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc:312:
sizes.max_content += line_builder.InlineSize(item);
On 2017/03/16 at 18:26:15, ikilpatrick wrote:
> add todo for atomic inlines.

atomic inline is computed by this change, by calling line_builder.InlineSize(),
so no more todo...or do I miss what you meant?

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:174: return
layout_result->get();
On 2017/03/16 at 18:26:16, ikilpatrick wrote:
> oh you may need to perform a std::move?
> 
> i.e.
> 
> return std::move(node->Layout(constraint_space.get()));

raw pointers should not need std::move(), nor copy elision, nor NRVO, no?

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:305:
.SetBlockSize(height)
On 2017/03/16 at 18:26:15, ikilpatrick wrote:
> .nit, block_size, but fine with cleaning up later.

This one is really a line height, not a block size, just passed to
SetBlockSize()...maybe I should create a TextFragmentBuilder with line-relative
terminologies?

Changed to |line_height| to clarify that this is not physical height but logical
line height.

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:430: // |fragment|
directly. Currently |CopyFragmentDataToLayoutBlockFlow|
On 2017/03/16 at 18:26:16, ikilpatrick wrote:
> it'd also be nice if we had all the information in the fragments, and could
perform the copy inside NGInlineNode eventually.

Yes, that's the plan. All information -- line_top, text_top, text_bottom, and
line_bottom. Legacy also has selection_top, selection_bottom, I need to read a
bit more code on selection_{top|bottom} for what it does differently from
text_{top|bottom}. The plan is to have them in LineBoxFragment, so that paint
can use them without relying on LayoutObject tree.

We also need baseline position, but this is needed for box, since all boxes have
synthesized baseline position. This is in my queue.

With all of them in fragments, I'll eliminate keeping LineBoxData and Fragment
to match later in CopyFragmentDataToLayoutBlockFlow().

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.h (right):

https://codereview.chromium.org/2745973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.h:91: // compute atomic
inlines by laying out them.
On 2017/03/16 at 18:26:16, ikilpatrick wrote:
> .nit
> "by layout them out."
> or
> "by performing layout."

Done, thank you!

[commit-bot: I haz the power, 2017-03-16 22:06:51]

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

[commit-bot: I haz the power, 2017-03-16 22:06:52]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-03-17 01:41:37]

The CQ bit was checked by kojii@chromium.org

[kojii, 2017-03-17 01:41:37]

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/2745973002/#ps280001
(title: "ikilpatrick nits")

[commit-bot: I haz the power, 2017-03-17 01:42:09]

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

[kojii, 2017-03-17 01:44:37]

The CQ bit was unchecked by kojii@chromium.org

[kojii, 2017-03-17 02:01:42]

The CQ bit was checked by kojii@chromium.org

[kojii, 2017-03-17 02:01:43]

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/2745973002/#ps300001
(title: "Resolved merge conflicts")

[commit-bot: I haz the power, 2017-03-17 02:02:19]

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-17 03:33:33]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1489716102885980,
"parent_rev": "32c86f9955aa028ea55a4538c61c67e0dbf6294e", "commit_rev":
"45bfc6a336b60497cb564ebfac61f8858664e86a"}

[commit-bot: I haz the power, 2017-03-17 03:34:38]

Description was changed from

==========
[LayoutNG] Implement atomic inlines for LayoutNGInline

This patch implements the initial support for atomic inlines in
LayoutNGInline.

Atomic inlines are the first type of objects in LayoutNGInline that
needs be laid out to compute in-flow inline size. NGLineBuilder now
keeps NGLayoutResult for atomic inlines while it builds a line box.

Some tests turned from false-passes to real-failures. Because atomic
inlines were not rendered before this patch, tests using images and
inline-blocks passed by comparing blank result with blank reference.

Some work are not included in this patch and will follow, such as:
* Keep NGBlockNode for atomic inlines in NGLayoutInlineItem.
* Use NGLayoutResult not only for layout but also for fragment
  generation.
* Margin, padding, borders.
* BreakToken if we support breaking atomic inlines[1].
* ...and obviously more.

[1] https://github.com/w3c/csswg-drafts/issues/1111

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

to

==========
[LayoutNG] Implement atomic inlines for LayoutNGInline

This patch implements the initial support for atomic inlines in
LayoutNGInline.

Atomic inlines are the first type of objects in LayoutNGInline that
needs be laid out to compute in-flow inline size. NGLineBuilder now
keeps NGLayoutResult for atomic inlines while it builds a line box.

Some tests turned from false-passes to real-failures. Because atomic
inlines were not rendered before this patch, tests using images and
inline-blocks passed by comparing blank result with blank reference.

Some work are not included in this patch and will follow, such as:
* Keep NGBlockNode for atomic inlines in NGLayoutInlineItem.
* Use NGLayoutResult not only for layout but also for fragment
  generation.
* Margin, padding, borders.
* BreakToken if we support breaking atomic inlines[1].
* ...and obviously more.

[1] https://github.com/w3c/csswg-drafts/issues/1111

BUG=636993

Review-Url: https://codereview.chromium.org/2745973002
Cr-Commit-Position: refs/heads/master@{#457673}
Committed:
https://chromium.googlesource.com/chromium/src/+/45bfc6a336b60497cb564ebfac61...
==========

[commit-bot: I haz the power, 2017-03-17 03:34:40]

Committed patchset #14 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/45bfc6a336b60497cb564ebfac61...

[kojii, 2017-03-22 08:43:10]

FYI, fantasai comment on the issue:

I think we made this a MAY case so that an implementation could fragment an
inline block, if it wanted to--didn't want to forbid it, since that can be a
better result than what the spec requirs. The alternatives are to clip the line
box it's in or to graphically slice said line box and all its contents.

https://github.com/w3c/csswg-drafts/issues/1111#issuecomment-287677936