[LayoutNG] Out-of-flow positioned objects in inline

[LayoutNG] Out-of-flow positioned objects in inline

This patch implements out-of-flow positioned objects in inline.

Since out-of-flow positioned objects requires static positions, which is
not available until we build line boxes, they are added to NGInlineNode
as Unicode Object Replacement Characters.

When NGLineBuilder creates line boxes, they are added to the box
fragment builder that wraps all line boxes.

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

[kojii, 2017-03-06 17:06:18]

Description was changed from

==========
ngabs

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

to

==========
[LayoutNG] Out-of-flow positioned objects in inline

This patch implements out-of-flow positioned objects in inline.

Since out-of-flow positioned objects requires static positions, which is
not available until we build line boxes, they are added to NGInlineNode
as Unicode Object Replacement Characters.

When NGLineBuilder creates line boxes, they are added to the box
fragment builder that wraps all line boxes.

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

[kojii, 2017-03-06 17:45:13]

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

[kojii, 2017-03-06 17:45:13]

Aleks, Ian, Emil, does this look in good direction?

I haven't understood all the code yet, BfcOffset() and NGOutOfFlowLayoutPart are
not very clear to me, so I guess there are things I missed. Your advice
appreciated.

[kojii, 2017-03-06 17:45:56]

/cc gleb, I think float also needs position, so it should be done in
NGLineBuilder, not in NGInlineNode::CollectInlines().

[ikilpatrick, 2017-03-06 18:09:11]

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:533:
RefPtr<NGLayoutResult> child_result = line_builder.CreateFragments();
It'd be good if we could move the line_builder logic to an "algorithm" type
class. E.g. the above code should really just be:

RefPtr<NGLayoutResult> line_result = inline_child->Layout(space, break_token);

Where:
 -  break_token holds enough information for generating the next line.
 - line_result is the line fragment.

This will mean that we can correctly fragment across multi-cols when needed.

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:353:
&container_builder_)
So this will probably have to occur inside NGBlockLayoutAlgorithm, e.g. consider
this:
https://dbaron.org/css/test/2016/abspos-in-inline

As a containing block may span multiple lines, you need to wait until you've
laid out all of the fragments for a containing block before you can layout the
abs-pos.

Just leaving them as pre-layout OutOfFlowDescendants is good enough for now I
think.

atotic@ does this match your understanding?

[kojii, 2017-03-06 18:54:43]

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:533:
RefPtr<NGLayoutResult> child_result = line_builder.CreateFragments();
On 2017/03/06 at 18:09:11, ikilpatrick wrote:
> It'd be good if we could move the line_builder logic to an "algorithm" type
class. E.g. the above code should really just be:
> 
> RefPtr<NGLayoutResult> line_result = inline_child->Layout(space, break_token);
> 
> Where:
>  -  break_token holds enough information for generating the next line.
>  - line_result is the line fragment.
> 
> This will mean that we can correctly fragment across multi-cols when needed.

For the signature, yeah, I'm thinking of it. I guess we'll need to revise APIs a
few times more as we work on oof/floats/atomic, but I think I'm trying to move
to that direction. For this CL, I have too many unknowns in oof/floats that I'd
like to do it in separate CL.

One thing maybe we don't have the same understanding yet; currently
|child_result| is like an anonymous block that contains all lines from the
NGInlineNode, not a line box that contains single line. If we reach to
page/column breaks, or the end of the fixed-height block, we need to break (not
done yet).

IIUC you're saying to return on every single line. Should I move to that model
when I work on break_token?

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:353:
&container_builder_)
On 2017/03/06 at 18:09:11, ikilpatrick wrote:
> So this will probably have to occur inside NGBlockLayoutAlgorithm, e.g.
consider this:
> https://dbaron.org/css/test/2016/abspos-in-inline
> 
> As a containing block may span multiple lines, you need to wait until you've
laid out all of the fragments for a containing block before you can layout the
abs-pos.

As above, this is after we laid out all lines. So is this the right place then?

For dbaron's example, you're right that I found AddOutOfFlowChildCandidate() has
offset, but not size, so I haven't figure out how I could let the objects know
right/bottom yet.

> Just leaving them as pre-layout OutOfFlowDescendants is good enough for now I
think.

Sorry, I can't understand this.

> atotic@ does this match your understanding?

[kojii, 2017-03-06 19:03:10]

On 2017/03/06 at 18:54:43, kojii wrote:
> For dbaron's example, you're right that I found AddOutOfFlowChildCandidate()
has offset, but not size, so I haven't figure out how I could let the objects
know right/bottom yet.

Please disregard this, I misunderstood between containing block and static
position.

...where can I set the containing block? Do I need to call something other than
AddOutOfFlowChildCandidate()?

[cbiesinger, 2017-03-06 20:43:35]

cbiesinger@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[cbiesinger, 2017-03-06 20:43:36]

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:352:
NGOutOfFlowLayoutPart(ConstraintSpace(), *inline_box_->BlockStyle(),
Why don't you just leave them as unpositioned OOF objects on the fragment? When
the block layout algorithm then calls AddChild, it will collect the OOF objects
and later position them. Does that not work? Would that produce wrong results?

You can maybe do something similar with unpositioned floats, see code in
NGBlockLayoutAlgorithm::FinishChildLayout

[atotic, 2017-03-06 21:46:10]

atotic@chromium.org changed reviewers:
- cbiesinger@chromium.org

[atotic, 2017-03-06 21:46:11]

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:230: new
NGBlockNode(layout_object),
layout_object can be inline. Should be something like:

if (layout_object->isInline())
  node = new NGInlineNode(next_sibling, &Style());
else
  node = new NGBlockNode(next_sibling);

There are 2 other instances where similar code is run: 
NGBlockNode::FirstChild(), NGBlockNode::NextSibling()

Maybe create a utility routine NodeFromLayoutObject?

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:353:
&container_builder_)
On 2017/03/06 at 18:09:11, ikilpatrick wrote:
> So this will probably have to occur inside NGBlockLayoutAlgorithm, e.g.
consider this:
> https://dbaron.org/css/test/2016/abspos-in-inline

Sweet test page!

> As a containing block may span multiple lines, you need to wait until you've
laid out all of the fragments for a containing block before you can layout the
abs-pos.
> 
> Just leaving them as pre-layout OutOfFlowDescendants is good enough for now I
think.
> 
> atotic@ does this match your understanding?

I think NGOutOfFlowLayoutPart has all the information it needs for positioning
right here.

That information is position of all fragments generated by layout of an
InlineNode.

NGOutOfFlowLayout does not genereate the correct containing block for inline
elements. I need to look into this, I was waiting for inline to stabilize so I
can test. The spec looks omnious:
"Note, in some cases when a line wraps it may seem as if the left and right
positions are swapped"

Kojii, can you ping me when inline layout is in state where I can start testing
abspos inside inlines?

[kojii, 2017-03-07 01:22:24]

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:230: new
NGBlockNode(layout_object),
On 2017/03/06 at 21:46:11, atotic wrote:
> layout_object can be inline. Should be something like:
> 
> if (layout_object->isInline())
>   node = new NGInlineNode(next_sibling, &Style());
> else
>   node = new NGBlockNode(next_sibling);
> 
> There are 2 other instances where similar code is run: 
NGBlockNode::FirstChild(), NGBlockNode::NextSibling()
> 
> Maybe create a utility routine NodeFromLayoutObject?

Isn't positioned objects blockified? I.e., even if their display is inline, it
should behave as block container, no?

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:352:
NGOutOfFlowLayoutPart(ConstraintSpace(), *inline_box_->BlockStyle(),
On 2017/03/06 at 20:43:36, cbiesinger wrote:
> Why don't you just leave them as unpositioned OOF objects on the fragment?
When the block layout algorithm then calls AddChild, it will collect the OOF
objects and later position them. Does that not work? Would that produce wrong
results?
> 
> You can maybe do something similar with unpositioned floats, see code in
NGBlockLayoutAlgorithm::FinishChildLayout

Do you mean not to call this but leave in the candidates list? They were not
copied to parent fragments as far as I tested. Maybe that's a bug?

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:353:
&container_builder_)
On 2017/03/06 at 21:46:11, atotic wrote:
> On 2017/03/06 at 18:09:11, ikilpatrick wrote:
> > So this will probably have to occur inside NGBlockLayoutAlgorithm, e.g.
consider this:
> > https://dbaron.org/css/test/2016/abspos-in-inline
> 
> Sweet test page!
> 
> > As a containing block may span multiple lines, you need to wait until you've
laid out all of the fragments for a containing block before you can layout the
abs-pos.
> > 
> > Just leaving them as pre-layout OutOfFlowDescendants is good enough for now
I think.
> > 
> > atotic@ does this match your understanding?
> 
> I think NGOutOfFlowLayoutPart has all the information it needs for positioning
right here.
> 
> That information is position of all fragments generated by layout of an
InlineNode.
> 
> NGOutOfFlowLayout does not genereate the correct containing block for inline
elements. I need to look into this, I was waiting for inline to stabilize so I
can test. The spec looks omnious:
> "Note, in some cases when a line wraps it may seem as if the left and right
positions are swapped"
> 
> Kojii, can you ping me when inline layout is in state where I can start
testing abspos inside inlines?

Superb, I'll try to land at some point and appreciate you to take this then.

There was a disc discussion to clarify this further, I'll find the link later.

[ikilpatrick, 2017-03-07 01:24:03]

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:533:
RefPtr<NGLayoutResult> child_result = line_builder.CreateFragments();
On 2017/03/06 18:54:43, kojii wrote:
> On 2017/03/06 at 18:09:11, ikilpatrick wrote:
> > It'd be good if we could move the line_builder logic to an "algorithm" type
> class. E.g. the above code should really just be:
> > 
> > RefPtr<NGLayoutResult> line_result = inline_child->Layout(space,
break_token);
> > 
> > Where:
> >  -  break_token holds enough information for generating the next line.
> >  - line_result is the line fragment.
> > 
> > This will mean that we can correctly fragment across multi-cols when needed.
> 
> For the signature, yeah, I'm thinking of it. I guess we'll need to revise APIs
a
> few times more as we work on oof/floats/atomic, but I think I'm trying to move
> to that direction. For this CL, I have too many unknowns in oof/floats that
I'd
> like to do it in separate CL.

Yeah, sgtm, sorry this was just a "we should move in this direction" not a "do
this" comment :).

> One thing maybe we don't have the same understanding yet; currently
> |child_result| is like an anonymous block that contains all lines from the
> NGInlineNode, not a line box that contains single line. If we reach to
> page/column breaks, or the end of the fixed-height block, we need to break
(not
> done yet).

I was imagining that it'd be just a single line? But we should chat about that.
We'll need to have a mode for custom layout which returns one line at a time.

> 
> IIUC you're saying to return on every single line. Should I move to that model
> when I work on break_token?

Up to you, we should talk about the pros/cons of this approach. (I'm unclear as
to what they'll be yet).

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:353:
&container_builder_)
On 2017/03/06 18:54:43, kojii wrote:
> On 2017/03/06 at 18:09:11, ikilpatrick wrote:
> > So this will probably have to occur inside NGBlockLayoutAlgorithm, e.g.
> consider this:
> > https://dbaron.org/css/test/2016/abspos-in-inline
> > 
> > As a containing block may span multiple lines, you need to wait until you've
> laid out all of the fragments for a containing block before you can layout the
> abs-pos.
> 
> As above, this is after we laid out all lines. So is this the right place
then?

Yup this is right place, we should discuss out of band having an algorithm
return line by line. :)

> For dbaron's example, you're right that I found AddOutOfFlowChildCandidate()
has
> offset, but not size, so I haven't figure out how I could let the objects know
> right/bottom yet.
> 
> > Just leaving them as pre-layout OutOfFlowDescendants is good enough for now
I
> think.
> 
> Sorry, I can't understand this.

As per cbiesinger@'s comment above, if you've already called
AddOutOfFlowChildCandidate for the child, it'll just bubble up to the parent
NGBlockLayoutAlgorithm for positioning. 

> > atotic@ does this match your understanding?

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:353:
&container_builder_)
On 2017/03/06 21:46:11, atotic wrote:
> On 2017/03/06 at 18:09:11, ikilpatrick wrote:
> > So this will probably have to occur inside NGBlockLayoutAlgorithm, e.g.
> consider this:
> > https://dbaron.org/css/test/2016/abspos-in-inline
> 
> Sweet test page!
> 
> > As a containing block may span multiple lines, you need to wait until you've
> laid out all of the fragments for a containing block before you can layout the
> abs-pos.
> > 
> > Just leaving them as pre-layout OutOfFlowDescendants is good enough for now
I
> think.
> > 
> > atotic@ does this match your understanding?
> 
> I think NGOutOfFlowLayoutPart has all the information it needs for positioning
> right here.

If we produce line boxes, we'll need to do this one level above. kojii@ we
should switch to this mode sooner rather than later if we go down this
direciton. :)

> That information is position of all fragments generated by layout of an
> InlineNode.
> 
> NGOutOfFlowLayout does not genereate the correct containing block for inline
> elements. I need to look into this, I was waiting for inline to stabilize so I
> can test. The spec looks omnious:
> "Note, in some cases when a line wraps it may seem as if the left and right
> positions are swapped"
> 
> Kojii, can you ping me when inline layout is in state where I can start
testing
> abspos inside inlines?
> 

We'll probably need to pass in a proper container size and inspect all of the
fragments to produce it.

[cbiesinger, 2017-03-07 01:27:29]

cbiesinger@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[cbiesinger, 2017-03-07 01:27:30]

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:352:
NGOutOfFlowLayoutPart(ConstraintSpace(), *inline_box_->BlockStyle(),
On 2017/03/07 01:22:24, kojii wrote:
> On 2017/03/06 at 20:43:36, cbiesinger wrote:
> > Why don't you just leave them as unpositioned OOF objects on the fragment?
> When the block layout algorithm then calls AddChild, it will collect the OOF
> objects and later position them. Does that not work? Would that produce wrong
> results?
> > 
> > You can maybe do something similar with unpositioned floats, see code in
> NGBlockLayoutAlgorithm::FinishChildLayout
> 
> Do you mean not to call this but leave in the candidates list? They were not
> copied to parent fragments as far as I tested. Maybe that's a bug?

Yes, that's what I meant; so far as I can tell this should work. Does the code
in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng...
not work?

[kojii, 2017-03-07 02:09:51]

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:352:
NGOutOfFlowLayoutPart(ConstraintSpace(), *inline_box_->BlockStyle(),
On 2017/03/07 at 01:27:30, cbiesinger wrote:
> On 2017/03/07 01:22:24, kojii wrote:
> > On 2017/03/06 at 20:43:36, cbiesinger wrote:
> > > Why don't you just leave them as unpositioned OOF objects on the fragment?
> > When the block layout algorithm then calls AddChild, it will collect the OOF
> > objects and later position them. Does that not work? Would that produce
wrong
> > results?
> > > 
> > > You can maybe do something similar with unpositioned floats, see code in
> > NGBlockLayoutAlgorithm::FinishChildLayout
> > 
> > Do you mean not to call this but leave in the candidates list? They were not
> > copied to parent fragments as far as I tested. Maybe that's a bug?
> 
> Yes, that's what I meant; so far as I can tell this should work. Does the code
in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng...
not work?

No, it copies from |OutOfFlowPositions()|, but boxes in
|out_of_flow_descendant_candidates_| are left untouched. Someone needs to call
|GetAndClearOutOfFlowDescendantCandidates| and |AddOutOfFlowDescendant|, and
|NGOutOfFlowLayoutPart::Run| looked like the one.

Oh, or should I call |AddOutOfFlowDescendant| directly instead of
|AddOutOfFlowChildCandidate|?

[kojii, 2017-03-07 02:11:13]

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:533:
RefPtr<NGLayoutResult> child_result = line_builder.CreateFragments();
On 2017/03/07 at 01:24:03, ikilpatrick wrote:
> Yeah, sgtm, sorry this was just a "we should move in this direction" not a "do
this" comment :).

Great to know we're in sync!

> > IIUC you're saying to return on every single line. Should I move to that
model
> > when I work on break_token?
> 
> Up to you, we should talk about the pros/cons of this approach. (I'm unclear
as to what they'll be yet).

I'll write up doc to discuss on, thank you for your attention on this.

[kojii, 2017-03-07 02:13:17]

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc (right):

https://codereview.chromium.org/2735703005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:230: new
NGBlockNode(layout_object),
atotic@:
> Isn't positioned objects blockified? I.e., even if their display is inline, it
should behave as block container, no?

Found the spec saying:
> Absolute positioning or floating an element blockifies the box’s display type.
[CSS2]
https://drafts.csswg.org/css-display/#transformations

[kojii, 2017-03-07 02:20:12]

atotic@:

The CSS WG issue here:
[css-2][css-position-3] Absolutely positioned boxes in inline relatives: not
interoperable
https://github.com/w3c/csswg-drafts/issues/609

A resolution was made, but edit wasn't done yet, nor our question unanswered
yet.

[kojii, 2017-03-07 04:43:46]

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

[commit-bot: I haz the power, 2017-03-07 04:44:38]

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-07 04:49:00]

PTAL.

Since |NGFragmentBuilder.ToBoxFragment()| discards candidates, I had to
|NGOutOfFlowLayoutPart.Run()| to move to |OutOfFlowDescendants|.

IIUC "unpositioned" means to keep them as candidates, correct? So in the new
patch, |NGLayoutBlockAlgorithm| passes |builder_| to |NGLneBulider|, which adds
oof candidates to the |builder_| instead of its own builder. This way we can
keep them unpositioned. Did I get this right?

Very simple test case works good, I'll see how tests go and some more cases.

[commit-bot: I haz the power, 2017-03-07 04:53:05]

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

[commit-bot: I haz the power, 2017-03-07 04:53:06]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-07 04:58:42]

Patchset #4 (id:60001) has been deleted

[atotic, 2017-03-07 19:25:06]

> No, it copies from |OutOfFlowPositions()|, but boxes in
|out_of_flow_descendant_candidates_| are left untouched. Someone needs to call
|GetAndClearOutOfFlowDescendantCandidates| and |AddOutOfFlowDescendant|, and
|NGOutOfFlowLayoutPart::Run| looked like the one.
> 
> Oh, or should I call |AddOutOfFlowDescendant| directly instead of
|AddOutOfFlowChildCandidate|?

Yes. If you are skipping descendant positioning, just add them directly.

[kojii, 2017-03-08 02:25:55]

On 2017/03/07 at 19:25:06, atotic wrote:
> > No, it copies from |OutOfFlowPositions()|, but boxes in
|out_of_flow_descendant_candidates_| are left untouched. Someone needs to call
|GetAndClearOutOfFlowDescendantCandidates| and |AddOutOfFlowDescendant|, and
|NGOutOfFlowLayoutPart::Run| looked like the one.
> > 
> > Oh, or should I call |AddOutOfFlowDescendant| directly instead of
|AddOutOfFlowChildCandidate|?
> 
> Yes. If you are skipping descendant positioning, just add them directly.

Reading the code more, so the difference between |AddOutOfFlowDescendant|
directly instead of |AddOutOfFlowChildCandidate| is whether the block is the
containing block, or we're not sure about it, correct?

If so, the line box (or the wrapper of the line boxes) isn't a containing block
that the use of |Candidate| looks correct to me, and that we should add to the
|NGBlockLayoutAlgorithm::builder_| as I did in PS4.

Is my understanding correct?

[kojii, 2017-03-08 18:09:28]

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

[commit-bot: I haz the power, 2017-03-08 18:10:07]

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-08 18:12:41]

As I tested a few cases, adding to |NGBlockLayoutAlgorithm::builder_| looks more
correct, but if you think I'm missing something and better to skip adding as
candidates, please let me know.

Rebaseline is needed as this change affects trailing whitespaces. There's a
missing piece to handle trailing spaces, so I'll work on it in a separate CL.

[cbiesinger, 2017-03-08 18:40:47]

On 2017/03/08 18:12:41, kojii wrote:
> As I tested a few cases, adding to |NGBlockLayoutAlgorithm::builder_| looks
more
> correct, but if you think I'm missing something and better to skip adding as
> candidates, please let me know.

Yeah I'd be happy with either that or adding it to the inline builder's
descendants array

[commit-bot: I haz the power, 2017-03-08 20:06:02]

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

[commit-bot: I haz the power, 2017-03-08 20:06:03]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-03-09 01:41:15]

atotic@, does this look good to go?

[atotic, 2017-03-09 19:15:57]

On 2017/03/09 at 01:41:15, kojii wrote:
> atotic@, does this look good to go?

Yes, if my understanding of the code is correct:
"abspos children of inline are addeed to inline->container oof_candididates"

[atotic, 2017-03-09 19:19:26]

https://codereview.chromium.org/2735703005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2735703005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:462:
break;
I was reading this code, and 'break' statement looks suspicious. 

Are we sure we want to break here? This means that later children will not get
Layout().

https://codereview.chromium.org/2735703005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:522: void
NGBlockLayoutAlgorithm::LayoutInlineChildren(
Would LayoutInlineChild() now be a better name?

[kojii, 2017-03-10 04:28:35]

https://codereview.chromium.org/2735703005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2735703005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:462:
break;
On 2017/03/09 at 19:19:26, atotic wrote:
> I was reading this code, and 'break' statement looks suspicious. 
> 
> Are we sure we want to break here? This means that later children will not get
Layout().

You're right, I think someone changed when we support NGBlockChildIterator, but
it's ok for now because |NGInlineNode::NextSibling()| always return nullptr.
When that was fixed, this also needs to be fixed.

Added TODO.

https://codereview.chromium.org/2735703005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:522: void
NGBlockLayoutAlgorithm::LayoutInlineChildren(
On 2017/03/09 at 19:19:26, atotic wrote:
> Would LayoutInlineChild() now be a better name?

Done.

[kojii, 2017-03-10 04:28:42]

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

[kojii, 2017-03-10 04:28:57]

On 2017/03/09 at 19:15:57, atotic wrote:
> On 2017/03/09 at 01:41:15, kojii wrote:
> > atotic@, does this look good to go?
> 
> Yes, if my understanding of the code is correct:
> "abspos children of inline are addeed to inline->container oof_candididates"

Great, thank you for the confirmation.

[kojii, 2017-03-10 04:29:09]

PTAL.

[commit-bot: I haz the power, 2017-03-10 04:29:12]

Dry run: 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-10 05:54:40]

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

[commit-bot: I haz the power, 2017-03-10 05:54:41]

Dry run: This issue passed the CQ dry run.

[atotic, 2017-03-10 19:35:10]

On 2017/03/10 at 04:29:09, kojii wrote:
> PTAL.

lgtm

[kojii, 2017-03-11 13:21:09]

The CQ bit was checked by kojii@chromium.org

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

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-11 13:21:24]

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

[commit-bot: I haz the power, 2017-03-11 13:21:25]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[ikilpatrick, 2017-03-11 15:13:59]

lgtm

[ikilpatrick, 2017-03-11 15:14:18]

The CQ bit was checked by ikilpatrick@chromium.org

[commit-bot: I haz the power, 2017-03-11 15:14:28]

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-11 15:16:43]

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

[commit-bot: I haz the power, 2017-03-11 15:16:45]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[kojii, 2017-03-11 15:30:56]

Thank you Ian, you wake up so early on Saturday! I'll rebase.

[kojii, 2017-03-11 16:08:42]

The CQ bit was checked by kojii@chromium.org

[kojii, 2017-03-11 16:08:42]

The patchset sent to the CQ was uploaded after l-g-t-m from atotic@chromium.org,
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2735703005/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-11 16:08:55]

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-11 17:44:49]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489248522200040,
"parent_rev": "24189c14d8852ac75295b6ec72f7930def0c3c06", "commit_rev":
"4dc3db2b7fbdc48b6a3f052d725d85b5355d8cd2"}

[commit-bot: I haz the power, 2017-03-11 17:45:53]

Description was changed from

==========
[LayoutNG] Out-of-flow positioned objects in inline

This patch implements out-of-flow positioned objects in inline.

Since out-of-flow positioned objects requires static positions, which is
not available until we build line boxes, they are added to NGInlineNode
as Unicode Object Replacement Characters.

When NGLineBuilder creates line boxes, they are added to the box
fragment builder that wraps all line boxes.

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

to

==========
[LayoutNG] Out-of-flow positioned objects in inline

This patch implements out-of-flow positioned objects in inline.

Since out-of-flow positioned objects requires static positions, which is
not available until we build line boxes, they are added to NGInlineNode
as Unicode Object Replacement Characters.

When NGLineBuilder creates line boxes, they are added to the box
fragment builder that wraps all line boxes.

BUG=636993

Review-Url: https://codereview.chromium.org/2735703005
Cr-Commit-Position: refs/heads/master@{#456280}
Committed:
https://chromium.googlesource.com/chromium/src/+/4dc3db2b7fbdc48b6a3f052d725d...
==========

[commit-bot: I haz the power, 2017-03-11 17:45:55]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/4dc3db2b7fbdc48b6a3f052d725d...