[LayoutNG] Remove the ng_layout_coordinator and temporary LayoutSync method. (issue 2649583002 by ikilpatrick@chromium.org)

1 view
Skip to first unread message

ikilp...@chromium.org

unread,
Jan 20, 2017, 1:06:35 PM1/20/17
to cbies...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, dgrog...@chromium.org, atotic+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org
Reviewers: cbiesinger
CL: https://codereview.chromium.org/2649583002/

Description:
[LayoutNG] Remove the ng_layout_coordinator and temporary LayoutSync method.

Algorithms now just call the Layout() method on the NGInputNode which is
responsible for selecting an algorithm to produce a fragment.

BUG=635619

Affected files (+57, -243 lines):
M third_party/WebKit/Source/core/layout/BUILD.gn
M third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
M third_party/WebKit/Source/core/layout/ng/ng_block_node.h
M third_party/WebKit/Source/core/layout/ng/ng_block_node.cc
M third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.h
M third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc
M third_party/WebKit/Source/core/layout/ng/ng_inline_node.h
M third_party/WebKit/Source/core/layout/ng/ng_inline_node.cc
M third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.h
D third_party/WebKit/Source/core/layout/ng/ng_layout_coordinator.h
D third_party/WebKit/Source/core/layout/ng/ng_layout_coordinator.cc
M third_party/WebKit/Source/core/layout/ng/ng_layout_input_node.h
M third_party/WebKit/Source/core/layout/ng/ng_legacy_block_layout_algorithm.h
M third_party/WebKit/Source/core/layout/ng/ng_legacy_block_layout_algorithm.cc
M third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc
M third_party/WebKit/Source/core/layout/ng/ng_text_layout_algorithm.h
M third_party/WebKit/Source/core/layout/ng/ng_text_layout_algorithm.cc
M third_party/WebKit/Source/web/tests/NGInlineLayoutTest.cpp


cbies...@chromium.org

unread,
Jan 20, 2017, 4:21:57 PM1/20/17
to ikilp...@chromium.org, chromium...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, dgrog...@chromium.org, atotic+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org
lgtm thanks!


https://codereview.chromium.org/2649583002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
File
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(left):

https://codereview.chromium.org/2649583002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#oldcode299
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:299:
current_child_->UpdateLayoutBox(toNGPhysicalBoxFragment(child_fragment),
Oh I see... this was only needed because we did not call box::Layout...
wow, glad we got rid of that.

https://codereview.chromium.org/2649583002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_node.cc
File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right):

https://codereview.chromium.org/2649583002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_node.cc#newcode97
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:97:
NGPhysicalFragment* physical_fragment = this->Layout(constraint_space);
Why the "this->"?

https://codereview.chromium.org/2649583002/

ikilp...@chromium.org

unread,
Jan 20, 2017, 6:19:57 PM1/20/17
to cbies...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, dgrog...@chromium.org, atotic+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org

https://codereview.chromium.org/2649583002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
File
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(left):

https://codereview.chromium.org/2649583002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#oldcode299
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:299:
current_child_->UpdateLayoutBox(toNGPhysicalBoxFragment(child_fragment),
On 2017/01/20 21:21:56, cbiesinger wrote:
> Oh I see... this was only needed because we did not call
box::Layout... wow,
> glad we got rid of that.

Yeah going through the boxes now might make things easier.


https://codereview.chromium.org/2649583002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_node.cc
File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right):

https://codereview.chromium.org/2649583002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_node.cc#newcode97
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:97:
NGPhysicalFragment* physical_fragment = this->Layout(constraint_space);
On 2017/01/20 21:21:56, cbiesinger wrote:
> Why the "this->"?

Because I am a doofus :), done.

https://codereview.chromium.org/2649583002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 20, 2017, 6:20:45 PM1/20/17
to ikilp...@chromium.org, cbies...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, dgrog...@chromium.org, atotic+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 20, 2017, 9:09:46 PM1/20/17
to ikilp...@chromium.org, cbies...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, dgrog...@chromium.org, atotic+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org
Failed to apply patch for
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:329
error: third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:
patch does not apply

Patch:
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
Index: third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
diff --git
a/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
b/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
index
238c3b0dc9b0e67b3b35ff7d137341c747ca0ecb..045d35587316e7401d7946b5f2af492f3e2d92ba
100644
--- a/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
+++ b/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
@@ -219,10 +219,7 @@ bool NGBlockLayoutAlgorithm::ComputeMinAndMaxContentSizes(
return true;
}

-NGLayoutStatus NGBlockLayoutAlgorithm::Layout(
- NGPhysicalFragment* child_fragment,
- NGPhysicalFragment** fragment_out,
- NGLayoutAlgorithm** algorithm_out) {
+NGPhysicalFragment* NGBlockLayoutAlgorithm::Layout() {
WTF::Optional<MinAndMaxContentSizes> sizes;
if (NeedMinAndMaxContentSizes(ConstraintSpace(), Style())) {
// TODO(ikilpatrick): Change ComputeMinAndMaxContentSizes to return
@@ -291,13 +288,8 @@ NGLayoutStatus NGBlockLayoutAlgorithm::Layout(
SpaceAvailableForCurrentChild() > LayoutUnit());
space_for_current_child_ = CreateConstraintSpaceForCurrentChild();

- NGFragment* fragment;
- current_child_->LayoutSync(space_for_current_child_, &fragment);
- NGPhysicalFragment* child_fragment = fragment->PhysicalFragment();
-
- // TODO(layout_ng): Seems like a giant hack to call this here.
- current_child_->UpdateLayoutBox(toNGPhysicalBoxFragment(child_fragment),
- space_for_current_child_);
+ NGPhysicalFragment* child_fragment =
+ current_child_->Layout(space_for_current_child_);

FinishCurrentChildLayout(new NGBoxFragment(
ConstraintSpace().WritingMode(), ConstraintSpace().Direction(),
@@ -329,8 +321,7 @@ NGLayoutStatus NGBlockLayoutAlgorithm::Layout(
if (ConstraintSpace().HasBlockFragmentation())
FinalizeForFragmentation();

- *fragment_out = builder_->ToBoxFragment();
- return kNewFragment;
+ return builder_->ToBoxFragment();
}

void NGBlockLayoutAlgorithm::FinishCurrentChildLayout(NGFragment* fragment) {


https://codereview.chromium.org/2649583002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 21, 2017, 12:40:27 AM1/21/17
to ikilp...@chromium.org, cbies...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, dgrog...@chromium.org, atotic+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org
CQ is trying da patch. Follow status at

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 21, 2017, 2:18:01 AM1/21/17
to ikilp...@chromium.org, cbies...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, dgrog...@chromium.org, atotic+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org
Try jobs failed on following builders:
win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/368389)

https://codereview.chromium.org/2649583002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 21, 2017, 12:59:06 PM1/21/17
to ikilp...@chromium.org, cbies...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, dgrog...@chromium.org, atotic+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org
CQ is trying da patch. Follow status at

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 21, 2017, 3:05:26 PM1/21/17
to ikilp...@chromium.org, cbies...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, dgrog...@chromium.org, atotic+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org
Reply all
Reply to author
Forward
0 new messages