[LayoutNG] Pull out of flow candidate loop into out of flow layout part.

[LayoutNG] Pull out of flow candidate loop into out of flow layout part.

Now that we don't have the state machine its easier to reason about
here.

BUG=635619
Review-Url: https://codereview.chromium.org/2646853006
Cr-Commit-Position: refs/heads/master@{#446059}
Committed: https://chromium.googlesource.com/chromium/src/+/f2e4af61a8a9c876d4eaeb82bdb5840af5da6ff7

[ikilpatrick, 2017-01-20 23:48:29]

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

[ikilpatrick, 2017-01-20 23:48:30]

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (left):

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:70: //    
oof_candidates.prepend(child_oof_candidates);
So the code in ng_block_algorithm never did this, is this needed?

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc
(right):

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:62: void
NGOutOfFlowLayoutPart::Run() {
plz halp, better name?

[atotic, 2017-01-23 07:37:56]

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (left):

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:70: //    
oof_candidates.prepend(child_oof_candidates);
On 2017/01/20 at 23:48:30, ikilpatrick wrote:
> So the code in ng_block_algorithm never did this, is this needed?

No. Good catch. This was code that was trying to solve the problem of
fixed position LegacyLayout being positioned inside an oof_descendant.

It turns out this is not a problem because the contaner for LegacyLayout
will always be either a descendant of current element (for position:absolute
legacy)
or #document (for position:fixed legacy). Both of those cases are already
handled.

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc
(right):

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:62: void
NGOutOfFlowLayoutPart::Run() {
On 2017/01/20 at 23:48:30, ikilpatrick wrote:
> plz halp, better name?

I like Run(). Kinda like DoIt(), the all-purpose method name.

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:70: while
(!out_of_flow_candidates.isEmpty()) {
Convert to standard iteration:
for (auto& child : out_of_flow_candidates)

Thankfully, out_of_flow_candidates_ no longer have to keep state.

See rewritten NGBlockLayoutAlgorithm::LayoutOutOfFlowChildren

Run() will also have to return list of unpositioned children so
their positions can be updated.

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:90:
NGFragment* NGOutOfFlowLayoutPart::LayoutChild(NGBlockNode& child,
Strictly, the routine is laying out a "positioned descendant", not a child.

But "positioned descendant" is verbose. LayoutBlock would be clearer,
with comment stating that block is a positioned descendant of container.

Humorous: 
- I called container parent, you renamed it container.
- I called positioned descendant node, you renamed it child.

[ikilpatrick, 2017-01-24 19:11:32]

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (left):

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:70: //    
oof_candidates.prepend(child_oof_candidates);
On 2017/01/23 07:37:56, atotic wrote:
> On 2017/01/20 at 23:48:30, ikilpatrick wrote:
> > So the code in ng_block_algorithm never did this, is this needed?
> 
> No. Good catch. This was code that was trying to solve the problem of
> fixed position LegacyLayout being positioned inside an oof_descendant.
> 
> It turns out this is not a problem because the contaner for LegacyLayout
> will always be either a descendant of current element (for position:absolute
> legacy)
> or #document (for position:fixed legacy). Both of those cases are already
> handled.
> 

Is there any reason for the GetAndClear method now then?

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc
(right):

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:70: while
(!out_of_flow_candidates.isEmpty()) {
On 2017/01/23 07:37:56, atotic wrote:
> Convert to standard iteration:
> for (auto& child : out_of_flow_candidates)
> 
> Thankfully, out_of_flow_candidates_ no longer have to keep state.
> 
Done.
> See rewritten NGBlockLayoutAlgorithm::LayoutOutOfFlowChildren
> 
> Run() will also have to return list of unpositioned children so
> their positions can be updated.

See layout object patch.

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:90:
NGFragment* NGOutOfFlowLayoutPart::LayoutChild(NGBlockNode& child,
On 2017/01/23 07:37:56, atotic wrote:
> Strictly, the routine is laying out a "positioned descendant", not a child.
> 
> But "positioned descendant" is verbose. LayoutBlock would be clearer,
> with comment stating that block is a positioned descendant of container.
> 
> Humorous: 
> - I called container parent, you renamed it container.
> - I called positioned descendant node, you renamed it child.

Renamed:
  child->descendant
  parent->container
for this class.

[atotic, 2017-01-24 20:48:34]

lgtm.

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (left):

https://codereview.chromium.org/2646853006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:70: //    
oof_candidates.prepend(child_oof_candidates);
On 2017/01/24 at 19:11:32, ikilpatrick wrote:
> On 2017/01/23 07:37:56, atotic wrote:
> > On 2017/01/20 at 23:48:30, ikilpatrick wrote:
> > > So the code in ng_block_algorithm never did this, is this needed?
> > 
> > No. Good catch. This was code that was trying to solve the problem of
> > fixed position LegacyLayout being positioned inside an oof_descendant.
> > 
> > It turns out this is not a problem because the contaner for LegacyLayout
> > will always be either a descendant of current element (for position:absolute
> > legacy)
> > or #document (for position:fixed legacy). Both of those cases are already
> > handled.
> > 
> 
> Is there any reason for the GetAndClear method now then?

Miscommunication: I was referring to the last two lines of the original comment:
//     builder->GetAndClearOutOfFlowDescendantCandidates(child_oof_candidates);
//     oof_candidates.prepend(child_oof_candidates);

Here we were getting oof_candidates at the end of every loop iteration.
This turned out to be unnecessary.

The rest of the sample code still stands, and
GetAndClearOutOfFlowDescendantCandidates
is called at the beginning.

[ikilpatrick, 2017-01-25 04:34:13]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2017-01-25 04:34:13]

The patchset sent to the CQ was uploaded after l-g-t-m from atotic@chromium.org
Link to the patchset: https://codereview.chromium.org/2646853006/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-25 04:34:30]

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-01-25 06:30:37]

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

[commit-bot: I haz the power, 2017-01-25 06:30:38]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ikilpatrick, 2017-01-25 17:06:00]

The CQ bit was checked by ikilpatrick@chromium.org

[commit-bot: I haz the power, 2017-01-25 17:06:31]

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-01-25 18:19:00]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485363960475550,
"parent_rev": "bfc27f01533251e2297d3bfbd4291b09f78f196a", "commit_rev":
"f2e4af61a8a9c876d4eaeb82bdb5840af5da6ff7"}

[commit-bot: I haz the power, 2017-01-25 18:20:10]

Description was changed from

==========
[LayoutNG] Pull out of flow candidate loop into out of flow layout part.

Now that we don't have the state machine its easier to reason about
here.

BUG=635619
==========

to

==========
[LayoutNG] Pull out of flow candidate loop into out of flow layout part.

Now that we don't have the state machine its easier to reason about
here.

BUG=635619

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

[commit-bot: I haz the power, 2017-01-25 18:20:11]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f2e4af61a8a9c876d4eaeb82bdb5...