Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2636353002:
[LayoutNG] Remove the state machine from ng_out_of_flow_layout_part. (Closed)
|
Created:
3 years, 11 months ago by ikilpatrick Modified:
3 years, 11 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, glebl+reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[LayoutNG] Remove the state machine from ng_out_of_flow_layout_part.
This is part of removing the resumable layout code from LayoutNG.
This also fixes a bug where node_space_ wasn't getting reset at the start of Layout().
BUG=635619
Review-Url: https://codereview.chromium.org/2636353002
Cr-Commit-Position: refs/heads/master@{#444602}
Committed: https://chromium.googlesource.com/chromium/src/+/3c1b4a7b6e20faedb8ae1c069796a3651bdda746
Patch Set 1 #Patch Set 2 : [LayoutNG] Remove the state machine from ng_out_of_flow_layout_part. #Patch Set 3 : fix compile #
Total comments: 6
Patch Set 4 : .. #
Total comments: 7
Patch Set 5 : [LayoutNG] Remove the state machine from ng_out_of_flow_layout_part. #Patch Set 6 : rename to IsContainingBlockForAbsoluteChild #Patch Set 7 : rebase. #Messages
Total messages: 45 (31 generated)
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
falalala.
BUG=
==========
to
==========
[LayoutNG] Remove the state machine from ng_out_of_flow_layout_part.
This is part of removing the resumable layout code from LayoutNG.
BUG=635619
==========
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
[LayoutNG] Remove the state machine from ng_out_of_flow_layout_part.
This is part of removing the resumable layout code from LayoutNG.
BUG=635619
==========
to
==========
[LayoutNG] Remove the state machine from ng_out_of_flow_layout_part.
This is part of removing the resumable layout code from LayoutNG.
This also fixes a bug where node_space_ wasn't getting reset at the start of
Layout().
BUG=635619
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ikilpatrick@chromium.org changed reviewers: + atotic@chromium.org, cbiesinger@chromium.org
I added a bunch of comments as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:398: // TODO(atotic) Need to adjust size of overflow rect per spec. I think the code to decide whether NGBlockNode can be positioned by NGOutOfFlowPart should remain a method of NGOutOfFlowPart. The reason is code reuse: - there will be other callers of NGOutOfFlowPart. All of these would have to replicate this logic. - the code to be reused is non-trivial, and might change in the future. https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_node.h (right): https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_node.h:38: Are Sync methods temporary? What are our plans for interruptible code? We've thrown out resumable, but interruptible is still required. https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc (right): https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:37: space_builder.SetPercentageResolutionSize(space_size); SetIsNewFormattingContext() has been removed. Why? This is for my education, I was cargo-culting it, unsure of what NewFormattingContext means.
I feel like this would be easier to review if you split it up into a patch that just removes the state machine and then possibly a second patch to move the code around (though, per atotic's comments, such a patch may not be desirable anyway)
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:398: // TODO(atotic) Need to adjust size of overflow rect per spec. On 2017/01/18 18:48:49, atotic wrote: > I think the code to decide whether NGBlockNode can be positioned by > NGOutOfFlowPart should remain a method of NGOutOfFlowPart. The reason is code > reuse: > - there will be other callers of NGOutOfFlowPart. All of these would have to > replicate this logic. > - the code to be reused is non-trivial, and might change in the future. Right, I've pulled it into a static method inside ng_absolute_utils. It may be easier to pull some of this into the LayoutPart now that we are removing the state machine. I didn't add it back into the LayoutPart yet as it makes it confusing as to what is if-branch is doing. https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_node.h (right): https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_node.h:38: On 2017/01/18 18:48:49, atotic wrote: > Are Sync methods temporary? > > What are our plans for interruptible code? We've thrown out resumable, but > interruptible is still required. Yup the sync methods are temporary, the eventual Layout() method will be: NGFragment* Layout(NGConstraintSpace*, NGBreakToken*); Returning a nullptr from Layout will mean that the parent algorithm should abort. Inside each of the algorithms they'll have a macro which does this effectively: NGLayoutAlgorithm::Layout(/* arguments */) { for (child of children) { fragment = NG_MAYBE_ABORT(child->Layout(child_space, break_token)); } NG_MAYBE_ABORT(fragment) { if (!fragment) return nullptr; if (OutOfTime()) return nullptr; return fragment; } https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc (right): https://codereview.chromium.org/2636353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:37: space_builder.SetPercentageResolutionSize(space_size); On 2017/01/18 18:48:49, atotic wrote: > SetIsNewFormattingContext() has been removed. Why? > > This is for my education, I was cargo-culting it, unsure of what > NewFormattingContext means. Yeah NewFormattingContext means basically this: https://drafts.csswg.org/css2/visuren.html#block-formatting I believe it's mostly true for a parent to be a new formatting context, but not always. (position:relative; i think does this). The ng_length_utils class shouldn't depend on this. :)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, with nits: There is also documentation that contains sample code for using NGOutOfFlowLayoutPart inside ng_fragment_builder.h. Can you fix it up? https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:386: [out_of_flow_candidate_positions_index_++]; out_of_flow_candidate_positions_index_ should be a local variable now, the entire iteration happens here. Looking at the rest of the code, you can move entire "Out of flow setup" from kStatePrepareForChildLayout into LayoutOutOfFlowChild(). I assume you were planning on doing this later, when redoing NGBlockLAyoutAlgorithm.
lgtm as long as Aleks is happy, modulo comments below https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h (right): https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:47: CORE_EXPORT bool IsContainerForAbsoluteChild( Why not name this IsContainingBlockForChild? https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc (left): https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:39: space_builder.SetIsNewFormattingContext(true); But abspos items are new formatting contexts, no? As per the link you quoted: "Floats, absolutely positioned elements, [...] establish new block formatting contexts for their contents" https://drafts.csswg.org/css2/visuren.html#block-formatting
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
aleks: updated comments, thanks for the pointer. christian: I initially just did a part for this class just removing the state machine, but a lot of things didn't make sense, and seemed contained enough to perform in one shot. I'll do block_algorithm in a more peicemeal fashion. https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h (right): https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:47: CORE_EXPORT bool IsContainerForAbsoluteChild( On 2017/01/18 20:27:00, cbiesinger wrote: > Why not name this IsContainingBlockForChild? Done. A better name might also be IsContainingBlockForAbsoluteChild as i think the containing block verbage is also used for floats? https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:386: [out_of_flow_candidate_positions_index_++]; On 2017/01/18 20:02:16, atotic wrote: > out_of_flow_candidate_positions_index_ should be a local variable now, the > entire iteration happens here. > > Looking at the rest of the code, you can move entire "Out of flow setup" from > kStatePrepareForChildLayout into LayoutOutOfFlowChild(). > > I assume you were planning on doing this later, when redoing > NGBlockLAyoutAlgorithm. I'll fix this in a follow up so not to make this patch too large. https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc (left): https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:39: space_builder.SetIsNewFormattingContext(true); On 2017/01/18 20:27:00, cbiesinger wrote: > But abspos items are new formatting contexts, no? As per the link you quoted: > "Floats, absolutely positioned elements, [...] establish new block formatting > contexts for their contents" > https://drafts.csswg.org/css2/visuren.html#block-formatting Hmm... looking at this closer this is both used as a sort of parent _and_ child constraint space in the absolute algorithm. I'll add this back in, but it shouldn't make any difference. It might be easier to pass container_size/percentage_size, writing_mode, direction directly to the absolute calculations to remove this confusion.
https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h (right): https://codereview.chromium.org/2636353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:47: CORE_EXPORT bool IsContainerForAbsoluteChild( On 2017/01/18 21:32:19, ikilpatrick wrote: > On 2017/01/18 20:27:00, cbiesinger wrote: > > Why not name this IsContainingBlockForChild? > > Done. > > A better name might also be IsContainingBlockForAbsoluteChild as i think the > containing block verbage is also used for floats? Well, it's used for any block, but point taken -- that name also sounds good to me. Mostly I wanted to avoid using nonstandard wording here.
cool, renamed.
The CQ bit was checked by ikilpatrick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, atotic@chromium.org Link to the patchset: https://codereview.chromium.org/2636353002/#ps100001 (title: "rename to IsContainingBlockForAbsoluteChild")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@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:4
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
ef259608b6a7629e153cdb1c81ab18ab62122dbc..7fcc5c05f43bef44a1821eecd887d020a5cd166b
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
@@ -4,6 +4,7 @@
#include "core/layout/ng/ng_block_layout_algorithm.h"
+#include "core/layout/ng/ng_absolute_utils.h"
#include "core/layout/ng/ng_box_fragment.h"
#include "core/layout/ng/ng_break_token.h"
#include "core/layout/ng/ng_constraint_space.h"
@@ -374,30 +375,27 @@ void
NGBlockLayoutAlgorithm::FinishCurrentChildLayout(NGFragment* fragment) {
}
bool NGBlockLayoutAlgorithm::LayoutOutOfFlowChild() {
- if (!current_child_) {
- if (out_of_flow_candidates_.isEmpty()) {
- out_of_flow_layout_ = nullptr;
- out_of_flow_candidate_positions_.clear();
- return true;
- }
- current_child_ = out_of_flow_candidates_.first();
- out_of_flow_candidates_.removeFirst();
- NGStaticPosition position = out_of_flow_candidate_positions_
- [out_of_flow_candidate_positions_index_++];
-
- if (!out_of_flow_layout_->StartLayout(current_child_, position)) {
- builder_->AddOutOfFlowDescendant(current_child_, position);
- current_child_ = nullptr;
- return false;
- }
+ if (out_of_flow_candidates_.isEmpty()) {
+ out_of_flow_layout_ = nullptr;
+ out_of_flow_candidate_positions_.clear();
+ return true;
}
- NGFragment* fragment;
- NGLogicalOffset offset;
- if (out_of_flow_layout_->Layout(&fragment, &offset) == kNewFragment) {
+ current_child_ = out_of_flow_candidates_.first();
+ out_of_flow_candidates_.removeFirst();
+ NGStaticPosition static_position = out_of_flow_candidate_positions_
+ [out_of_flow_candidate_positions_index_++];
+
+ if (IsContainingBlockForAbsoluteChild(Style(), *current_child_->Style())) {
+ NGFragment* fragment;
+ NGLogicalOffset offset;
+ out_of_flow_layout_->Layout(*current_child_, static_position, &fragment,
+ &offset);
// TODO(atotic) Need to adjust size of overflow rect per spec.
builder_->AddChild(fragment, offset);
- current_child_ = nullptr;
+ } else {
+ builder_->AddOutOfFlowDescendant(current_child_, static_position);
}
+
return false;
}
The CQ bit was checked by ikilpatrick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, atotic@chromium.org Link to the patchset: https://codereview.chromium.org/2636353002/#ps120001 (title: "rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ikilpatrick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484791917138140,
"parent_rev": "095fbc7f5a33b47c9506cecf8007e809552aa7b1", "commit_rev":
"3c1b4a7b6e20faedb8ae1c069796a3651bdda746"}
Message was sent while issue was closed.
Description was changed from
==========
[LayoutNG] Remove the state machine from ng_out_of_flow_layout_part.
This is part of removing the resumable layout code from LayoutNG.
This also fixes a bug where node_space_ wasn't getting reset at the start of
Layout().
BUG=635619
==========
to
==========
[LayoutNG] Remove the state machine from ng_out_of_flow_layout_part.
This is part of removing the resumable layout code from LayoutNG.
This also fixes a bug where node_space_ wasn't getting reset at the start of
Layout().
BUG=635619
Review-Url: https://codereview.chromium.org/2636353002
Cr-Commit-Position: refs/heads/master@{#444602}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c1b4a7b6e20faedb8ae1c069796...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/3c1b4a7b6e20faedb8ae1c069796... |
