Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2435803005:
Initial implementation of LayoutNG's block layout algorithm for floats. (Closed)
|
Created:
4 years, 2 months ago by Gleb Lanbin Modified:
4 years, 2 months ago CC:
chromium-reviews, ojan+watch_chromium.org, szager+layoutwatch_chromium.org, glebl+reviews_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial implementation of LayoutNG's block layout algorithm for floats.
This patch adds support of left/right floats in LayoutNG code.
Things that are not supported yet:
- top aligning floats
- clearing
- child blocks in the same formatting formatting context
BUG=635619
TEST=virtual/layout_ng/fast/block/float, NGBlockLayoutAlgorithmTest::PositionFloatFragments
Committed: https://crrev.com/05059b38db14102048bfbe34bafbaf52b9fcc7ba
Cr-Commit-Position: refs/heads/master@{#426982}
Patch Set 1 #
Total comments: 16
Patch Set 2 : move CreateExclusion to a separate function, fix other comments #
Total comments: 28
Patch Set 3 : fix a bunch of comments from cbiesinger@ #
Total comments: 10
Patch Set 4 : fix PositionFragment's doc, added TODO to fix floats with margins and add more test expectations. #Messages
Total messages: 35 (11 generated)
Patchset #1 (id:1) has been deleted
glebl@chromium.org changed reviewers: + cbiesinger@chromium.org, ikilpatrick@chromium.org
This looks nice and clean. Am i correct in thinking that they missing things here are: - top aligning floats, - clearing - child blocks in the same formatting formatting context - ??? https://codereview.chromium.org/2435803005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2435803005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:45: if (opportunity->Size().inline_size > fragment.InlineSize()) May want to say why the block size isn't important. (is there a DCHECK we can place here?) https://codereview.chromium.org/2435803005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:155: CollapseMargins(*constraint_space, child_margins, *fragment); this can get pulled into PositionFragment function? https://codereview.chromium.org/2435803005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:255: NGLogicalOffset NGBlockLayoutAlgorithm::PositionFragment( thanks for splitting this out :) https://codereview.chromium.org/2435803005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:268: return logical_offset; could just do: return NGLogicalOffset(blah, blah); would be better? https://codereview.chromium.org/2435803005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:274: const NGConstraintSpace* opportunity = FindLayoutOpportunityForFragment( this doesn't top edge align yet right? https://codereview.chromium.org/2435803005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:307: constraint_space_for_children_->PhysicalSpace()->AddExclusion(exclusion); we shouldn't access the PhysicalSpace directly. It may also be worth moving the logic to AddExclusion() i.e. AddExclusion(fragment, EFloat::Left, MarginBox); ? not sure, up to you. https://codereview.chromium.org/2435803005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h (right): https://codereview.chromium.org/2435803005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:61: // Calculates position of the regular/block fragment that needs to be placed s/regular\/block/in-flow block-level/ ? clearer? https://codereview.chromium.org/2435803005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:62: // in the parent's constraint space. not really placed in the parents constraint space, more positioned relative to the current fragment we are building.
https://chromiumcodereview.appspot.com/2435803005/diff/20001/third_party/WebK... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://chromiumcodereview.appspot.com/2435803005/diff/20001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:45: if (opportunity->Size().inline_size > fragment.InlineSize()) On 2016/10/20 16:27:09, ikilpatrick wrote: > May want to say why the block size isn't important. > > (is there a DCHECK we can place here?) Done. https://chromiumcodereview.appspot.com/2435803005/diff/20001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:155: CollapseMargins(*constraint_space, child_margins, *fragment); On 2016/10/20 16:27:09, ikilpatrick wrote: > this can get pulled into PositionFragment function? thanks for spotting this. https://chromiumcodereview.appspot.com/2435803005/diff/20001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:255: NGLogicalOffset NGBlockLayoutAlgorithm::PositionFragment( On 2016/10/20 16:27:09, ikilpatrick wrote: > thanks for splitting this out :) Acknowledged. https://chromiumcodereview.appspot.com/2435803005/diff/20001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:268: return logical_offset; On 2016/10/20 16:27:09, ikilpatrick wrote: > could just do: > > return NGLogicalOffset(blah, blah); > > would be better? done, but I want to keep auxiliary variables like inline_offset, block_offset for the readability's sake. https://chromiumcodereview.appspot.com/2435803005/diff/20001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:274: const NGConstraintSpace* opportunity = FindLayoutOpportunityForFragment( On 2016/10/20 16:27:09, ikilpatrick wrote: > this doesn't top edge align yet right? not yet. added a TODO https://chromiumcodereview.appspot.com/2435803005/diff/20001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:307: constraint_space_for_children_->PhysicalSpace()->AddExclusion(exclusion); On 2016/10/20 16:27:09, ikilpatrick wrote: > we shouldn't access the PhysicalSpace directly. > > It may also be worth moving the logic to AddExclusion() i.e. > > AddExclusion(fragment, EFloat::Left, MarginBox); ? > > not sure, up to you. Done. https://chromiumcodereview.appspot.com/2435803005/diff/20001/third_party/WebK... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h (right): https://chromiumcodereview.appspot.com/2435803005/diff/20001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:61: // Calculates position of the regular/block fragment that needs to be placed On 2016/10/20 16:27:09, ikilpatrick wrote: > s/regular\/block/in-flow block-level/ ? clearer? Done. https://chromiumcodereview.appspot.com/2435803005/diff/20001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:62: // in the parent's constraint space. On 2016/10/20 16:27:09, ikilpatrick wrote: > not really placed in the parents constraint space, more positioned relative to > the current fragment we are building. Done.
Description was changed from ========== Initial implementation of LayoutNG's block layout algorithm for floats. BUG=635619 TEST=virtual/layout_ng/fast/block/float, NGBlockLayoutAlgorithmTest::PositionFloatFragments ========== to ========== Initial implementation of LayoutNG's block layout algorithm for floats. This patch adds support of left/right floats in LayoutNG code. Things that are not supported yet: - top aligning floats - clearing - child blocks in the same formatting formatting context BUG=635619 TEST=virtual/layout_ng/fast/block/float, NGBlockLayoutAlgorithmTest::PositionFloatFragments ==========
https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:39: exclusion_bottom += margins.BlockSum(); I'm not super-happy with naming it top instead of block_start_offset or something but maybe that's ok https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:63: auto* opportunity_iter = space->LayoutOpportunities(); I'd really rather you didn't use auto like this, it makes it very hard to tell what the type of this object is, particularly since this doesn't work like a STL iterator so you actually need to know its API. https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:67: opportunity = opportunity_candidate; Why do you have two different variables for opportunity and opportunity_candidate? Aren't they always identical? https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:68: // Checking ppportunity's block size is not necessary as a float cannot be ppp -> opp https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:177: fragment_offset = PositionFloatFragment(*fragment, child_margins); I don't think margin:auto applies to floats -- so, I think you should move the ApplyAutoMargins call into the if, and perhaps into PositionFragment. http://plexode.com/eval3/#s=aekVQXANJVQMbAx1yAXgePQOLGwFTRkUcAYOdFhERUVkcmh89... https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:182: // TODO(layout-ng): Support auto margins I forgot to remove this comment :( Can you delete it while you're touching this? https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:306: LayoutUnit inline_offset = border_and_padding_.inline_start; Hmm, shouldn't we create the NGConstraintSpace with an offset for the border and padding so it can take it into account when iterating? https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:320: inline_offset += right_float_offset; Hm, there's similar code in CreateExclusion. Is there a way to pass in the offset we compute here into CreateExclusion to avoid the duplication? https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:65: NGPhysicalConstraintSpace* MutablePhysicalSpace() const { A const function that returns a non-const member? :( https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... File third_party/WebKit/Source/core/layout/ng/ng_fragment_base.h (right): https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_fragment_base.h:10: #include "core/layout/ng/ng_physical_fragment_base.h" Why replace the forward-decl with an include? https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... File third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.h (right): https://chromiumcodereview.appspot.com/2435803005/diff/40001/third_party/WebK... third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.h:40: NGLayoutOpportunityTreeNode* MutableOpportunityTreeRoot() const { Why do they have to be const?
thanks for the review. PTAL https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:39: exclusion_bottom += margins.BlockSum(); On 2016/10/20 20:41:50, cbiesinger wrote: > I'm not super-happy with naming it top instead of block_start_offset or > something but maybe that's ok ack, lets discuss this at the next layoutNG sync meeting. I will add an action item. I think Emil authored NGExclusion. We can ask him. https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:63: auto* opportunity_iter = space->LayoutOpportunities(); On 2016/10/20 20:41:51, cbiesinger wrote: > I'd really rather you didn't use auto like this, it makes it very hard to tell > what the type of this object is, particularly since this doesn't work like a STL > iterator so you actually need to know its API. Done. https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:67: opportunity = opportunity_candidate; On 2016/10/20 20:41:51, cbiesinger wrote: > Why do you have two different variables for opportunity and > opportunity_candidate? Aren't they always identical? we need to return the 1st opportunity that will fit our fragment or the latest one. I'm planning to change NGLayoutOpportunityIterator to be a standard STL iterator. in this case we can check whether we reached the end. With the current API I don't know how you can implement it in another way. https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:68: // Checking ppportunity's block size is not necessary as a float cannot be On 2016/10/20 20:41:51, cbiesinger wrote: > ppp -> opp Done. https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:177: fragment_offset = PositionFloatFragment(*fragment, child_margins); On 2016/10/20 20:41:51, cbiesinger wrote: > I don't think margin:auto applies to floats -- so, I think you should move the > ApplyAutoMargins call into the if, and perhaps into PositionFragment. > > http://plexode.com/eval3/#s=aekVQXANJVQMbAx1yAXgePQOLGwFTRkUcAYOdFhERUVkcmh89... Done. https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:182: // TODO(layout-ng): Support auto margins On 2016/10/20 20:41:51, cbiesinger wrote: > I forgot to remove this comment :( Can you delete it while you're touching this? Done. https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:306: LayoutUnit inline_offset = border_and_padding_.inline_start; On 2016/10/20 20:41:51, cbiesinger wrote: > Hmm, shouldn't we create the NGConstraintSpace with an offset for the border and > padding so it can take it into account when iterating? added TODO. I asked Ian about that and he said that he is planning to refactor constraint spaces we're using today. https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:320: inline_offset += right_float_offset; On 2016/10/20 20:41:51, cbiesinger wrote: > Hm, there's similar code in CreateExclusion. Is there a way to pass in the > offset we compute here into CreateExclusion to avoid the duplication? Done. https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:65: NGPhysicalConstraintSpace* MutablePhysicalSpace() const { On 2016/10/20 20:41:51, cbiesinger wrote: > A const function that returns a non-const member? :( if we remove const here then we will need to remove const constraint from a lot of places. As the first step I propose to have read-only pointer and mutable pointer that cannot be replaced. https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_base.h (right): https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_fragment_base.h:10: #include "core/layout/ng/ng_physical_fragment_base.h" On 2016/10/20 20:41:51, cbiesinger wrote: > Why replace the forward-decl with an include? I'm not changing this file so I reverted this change. https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.h (right): https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.h:40: NGLayoutOpportunityTreeNode* MutableOpportunityTreeRoot() const { On 2016/10/20 20:41:51, cbiesinger wrote: > Why do they have to be const? Done.
https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:39: exclusion_bottom += margins.BlockSum(); On 2016/10/20 23:30:35, glebl wrote: > On 2016/10/20 20:41:50, cbiesinger wrote: > > I'm not super-happy with naming it top instead of block_start_offset or > > something but maybe that's ok > > ack, lets discuss this at the next layoutNG sync meeting. I will add an action > item. I think Emil authored NGExclusion. We can ask him. Hold on, I misunderstood -- NGExclusion is in physical coordinates. However, you are passing logical coordinates into it. You should convert that? https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:67: opportunity = opportunity_candidate; On 2016/10/20 23:30:35, glebl wrote: > On 2016/10/20 20:41:51, cbiesinger wrote: > > Why do you have two different variables for opportunity and > > opportunity_candidate? Aren't they always identical? > > we need to return the 1st opportunity that will fit our fragment or the latest > one. I'm planning to change NGLayoutOpportunityIterator to be a standard STL > iterator. in this case we can check whether we reached the end. With the current > API I don't know how you can implement it in another way. Under which circumstance will opportunity point to a different object than opportunity_candidate in your code? https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:306: LayoutUnit inline_offset = border_and_padding_.inline_start; On 2016/10/20 23:30:36, glebl wrote: > On 2016/10/20 20:41:51, cbiesinger wrote: > > Hmm, shouldn't we create the NGConstraintSpace with an offset for the border > and > > padding so it can take it into account when iterating? > > added TODO. I asked Ian about that and he said that he is planning to refactor > constraint spaces we're using today. sounds good. https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:65: NGPhysicalConstraintSpace* MutablePhysicalSpace() const { On 2016/10/20 23:30:36, glebl wrote: > On 2016/10/20 20:41:51, cbiesinger wrote: > > A const function that returns a non-const member? :( > > if we remove const here then we will need to remove const constraint from a lot > of places. As the first step I propose to have read-only pointer and mutable > pointer that cannot be replaced. ok.
https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:39: exclusion_bottom += margins.BlockSum(); On 2016/10/20 23:37:53, cbiesinger wrote: > On 2016/10/20 23:30:35, glebl wrote: > > On 2016/10/20 20:41:50, cbiesinger wrote: > > > I'm not super-happy with naming it top instead of block_start_offset or > > > something but maybe that's ok > > > > ack, lets discuss this at the next layoutNG sync meeting. I will add an action > > item. I think Emil authored NGExclusion. We can ask him. > > Hold on, I misunderstood -- NGExclusion is in physical coordinates. However, you > are passing logical coordinates into it. You should convert that? I think the exclusions should be stored in logical coordinates, and we should probably perform a followup patch that changes them to logical. (It should be logical coordinates as exclusions are never going to cross formatting context boundary, i.e. they are always going to exist in the same writing mode).
On 2016/10/20 23:46:11, ikilpatrick wrote: > https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc > (right): > > https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:39: > exclusion_bottom += margins.BlockSum(); > On 2016/10/20 23:37:53, cbiesinger wrote: > > On 2016/10/20 23:30:35, glebl wrote: > > > On 2016/10/20 20:41:50, cbiesinger wrote: > > > > I'm not super-happy with naming it top instead of block_start_offset or > > > > something but maybe that's ok > > > > > > ack, lets discuss this at the next layoutNG sync meeting. I will add an > action > > > item. I think Emil authored NGExclusion. We can ask him. > > > > Hold on, I misunderstood -- NGExclusion is in physical coordinates. However, > you > > are passing logical coordinates into it. You should convert that? > > I think the exclusions should be stored in logical coordinates, and we should > probably perform a followup patch that changes them to logical. > > (It should be logical coordinates as exclusions are never going to cross > formatting context boundary, i.e. they are always going to exist in the same > writing mode). That seems fair enough but we store them on the physical constraint space, with which this will not interact well?
On 2016/10/20 23:47:45, cbiesinger wrote: > On 2016/10/20 23:46:11, ikilpatrick wrote: > > > https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc > > (right): > > > > > https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:39: > > exclusion_bottom += margins.BlockSum(); > > On 2016/10/20 23:37:53, cbiesinger wrote: > > > On 2016/10/20 23:30:35, glebl wrote: > > > > On 2016/10/20 20:41:50, cbiesinger wrote: > > > > > I'm not super-happy with naming it top instead of block_start_offset or > > > > > something but maybe that's ok > > > > > > > > ack, lets discuss this at the next layoutNG sync meeting. I will add an > > action > > > > item. I think Emil authored NGExclusion. We can ask him. > > > > > > Hold on, I misunderstood -- NGExclusion is in physical coordinates. However, > > you > > > are passing logical coordinates into it. You should convert that? > > > > I think the exclusions should be stored in logical coordinates, and we should > > probably perform a followup patch that changes them to logical. > > > > (It should be logical coordinates as exclusions are never going to cross > > formatting context boundary, i.e. they are always going to exist in the same > > writing mode). > > That seems fair enough but we store them on the physical constraint space, with > which this will not interact well? Also -- don't we need a parent's exclusions to compute a child's/descendent's layout opportunities, potentially in a different writing mode?
On 2016/10/20 23:47:45, cbiesinger wrote: > On 2016/10/20 23:46:11, ikilpatrick wrote: > > > https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc > > (right): > > > > > https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:39: > > exclusion_bottom += margins.BlockSum(); > > On 2016/10/20 23:37:53, cbiesinger wrote: > > > On 2016/10/20 23:30:35, glebl wrote: > > > > On 2016/10/20 20:41:50, cbiesinger wrote: > > > > > I'm not super-happy with naming it top instead of block_start_offset or > > > > > something but maybe that's ok > > > > > > > > ack, lets discuss this at the next layoutNG sync meeting. I will add an > > action > > > > item. I think Emil authored NGExclusion. We can ask him. > > > > > > Hold on, I misunderstood -- NGExclusion is in physical coordinates. However, > > you > > > are passing logical coordinates into it. You should convert that? > > > > I think the exclusions should be stored in logical coordinates, and we should > > probably perform a followup patch that changes them to logical. > > > > (It should be logical coordinates as exclusions are never going to cross > > formatting context boundary, i.e. they are always going to exist in the same > > writing mode). > > That seems fair enough but we store them on the physical constraint space, with > which this will not interact well? I think this is fine, (but happy to be wrong!) this is similar to the NGMarginStrut which is in logical coordinates on the physical fragment, (but again is ok, as never accessed from a different writing mode). We never want to store non-physical fragments/spaces in my mind as they should really just be "views" on top of the physical.
On 2016/10/20 23:50:56, cbiesinger wrote: > On 2016/10/20 23:47:45, cbiesinger wrote: > > On 2016/10/20 23:46:11, ikilpatrick wrote: > > > > > > https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:39: > > > exclusion_bottom += margins.BlockSum(); > > > On 2016/10/20 23:37:53, cbiesinger wrote: > > > > On 2016/10/20 23:30:35, glebl wrote: > > > > > On 2016/10/20 20:41:50, cbiesinger wrote: > > > > > > I'm not super-happy with naming it top instead of block_start_offset > or > > > > > > something but maybe that's ok > > > > > > > > > > ack, lets discuss this at the next layoutNG sync meeting. I will add an > > > action > > > > > item. I think Emil authored NGExclusion. We can ask him. > > > > > > > > Hold on, I misunderstood -- NGExclusion is in physical coordinates. > However, > > > you > > > > are passing logical coordinates into it. You should convert that? > > > > > > I think the exclusions should be stored in logical coordinates, and we > should > > > probably perform a followup patch that changes them to logical. > > > > > > (It should be logical coordinates as exclusions are never going to cross > > > formatting context boundary, i.e. they are always going to exist in the same > > > writing mode). > > > > That seems fair enough but we store them on the physical constraint space, > with > > which this will not interact well? > > Also -- don't we need a parent's exclusions to compute a child's/descendent's > layout opportunities, potentially in a different writing mode? A different writing mode always establishes a new formatting context, so doesn't have any parent exclusions (and similarly children in different writing modes can't add exclusions as they are in a new formatting context).
On 2016/10/20 23:54:41, ikilpatrick wrote: > On 2016/10/20 23:50:56, cbiesinger wrote: > > On 2016/10/20 23:47:45, cbiesinger wrote: > > > On 2016/10/20 23:46:11, ikilpatrick wrote: > > > > > > > > > > https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:39: > > > > exclusion_bottom += margins.BlockSum(); > > > > On 2016/10/20 23:37:53, cbiesinger wrote: > > > > > On 2016/10/20 23:30:35, glebl wrote: > > > > > > On 2016/10/20 20:41:50, cbiesinger wrote: > > > > > > > I'm not super-happy with naming it top instead of block_start_offset > > or > > > > > > > something but maybe that's ok > > > > > > > > > > > > ack, lets discuss this at the next layoutNG sync meeting. I will add > an > > > > action > > > > > > item. I think Emil authored NGExclusion. We can ask him. > > > > > > > > > > Hold on, I misunderstood -- NGExclusion is in physical coordinates. > > However, > > > > you > > > > > are passing logical coordinates into it. You should convert that? > > > > > > > > I think the exclusions should be stored in logical coordinates, and we > > should > > > > probably perform a followup patch that changes them to logical. > > > > > > > > (It should be logical coordinates as exclusions are never going to cross > > > > formatting context boundary, i.e. they are always going to exist in the > same > > > > writing mode). > > > > > > That seems fair enough but we store them on the physical constraint space, > > with > > > which this will not interact well? > > > > Also -- don't we need a parent's exclusions to compute a child's/descendent's > > layout opportunities, potentially in a different writing mode? > > A different writing mode always establishes a new formatting context, so doesn't > have any parent exclusions (and similarly children in different writing modes > can't add exclusions as they are in a new formatting context). OK... I think you're right that this will work.
PTAL https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2435803005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:67: opportunity = opportunity_candidate; On 2016/10/20 23:37:53, cbiesinger wrote: > On 2016/10/20 23:30:35, glebl wrote: > > On 2016/10/20 20:41:51, cbiesinger wrote: > > > Why do you have two different variables for opportunity and > > > opportunity_candidate? Aren't they always identical? > > > > we need to return the 1st opportunity that will fit our fragment or the latest > > one. I'm planning to change NGLayoutOpportunityIterator to be a standard STL > > iterator. in this case we can check whether we reached the end. With the > current > > API I don't know how you can implement it in another way. > > Under which circumstance will opportunity point to a different object than > opportunity_candidate in your code? ack. discussed offline.
looks really good. https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:72: return opportunity; Cant' this function just be: NGLayoutOpportunityIterator* opportunity_iter = space->LayoutOpportunities(); while (const NGConstraintSpace* opportunity = opportunity_iter->Next()) { if (...) { return opportunity; } } NOTREACHED(); return nullptr; https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:87: // Adjust to child's margin. The margins actually need to be incorporated into the check if it fits in a layout opportunity. I.e. (above) if (opportunity->Size().inline_size > fragment.InlineSize() + margins.InlineSum()) https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:312: LayoutUnit block_offset = content_size_ + collapsed_margins.block_start; .nit Move these two lines below to just before return as next lines don't rely on calcuation? https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h (right): https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:67: // @return Position of the fragment in the parent's constraint space. .nit Maybe Position of the fragment relative to the parent? It's we aren't really placing in the constraint space, rather in the current fragment we are building. https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:599: TEST_F(NGBlockLayoutAlgorithmTest, PositionFloatFragments) { might be also good to have one test that doesn't take the first opportunity, e.g. <!DOCTYPE html> <style> body { width: 200px; height: 200px; border: solid; } left { float: left; width: 100px; height: 100px; background: green; } right { margin-left: 20px; float: left; width: 100px; height: 100px; background: red; } </style> <left></left> <right></right>
https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:72: return opportunity; On 2016/10/21 16:14:01, ikilpatrick wrote: > Cant' this function just be: > > NGLayoutOpportunityIterator* opportunity_iter = space->LayoutOpportunities(); > while (const NGConstraintSpace* opportunity = opportunity_iter->Next()) { > if (...) { > return opportunity; > } > } > > NOTREACHED(); > return nullptr; discussed offline. opportunity_candidate is a temp variable that I use to check whether we reached the end. Once NGLayoutOpportunityIterator started supporting a standard c++ STL iterator interface we will change it. https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:87: // Adjust to child's margin. On 2016/10/21 16:14:01, ikilpatrick wrote: > The margins actually need to be incorporated into the check if it fits in a > layout opportunity. > > I.e. (above) > if (opportunity->Size().inline_size > fragment.InlineSize() + > margins.InlineSum()) thanks. I added TODO for now. https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:312: LayoutUnit block_offset = content_size_ + collapsed_margins.block_start; On 2016/10/21 16:14:01, ikilpatrick wrote: > .nit Move these two lines below to just before return as next lines don't rely > on calcuation? I'm not changing the order of operations we used in the original version of this code. We use content_size_ to calculate block_offset and then modify it below. https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h (right): https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:67: // @return Position of the fragment in the parent's constraint space. On 2016/10/21 16:14:01, ikilpatrick wrote: > .nit Maybe Position of the fragment relative to the parent? > > It's we aren't really placing in the constraint space, rather in the current > fragment we are building. Done. https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2435803005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:599: TEST_F(NGBlockLayoutAlgorithmTest, PositionFloatFragments) { On 2016/10/21 16:14:01, ikilpatrick wrote: > might be also good to have one test that doesn't take the first opportunity, > e.g. > > > <!DOCTYPE html> > <style> > body { width: 200px; height: 200px; border: solid; } > left { float: left; width: 100px; height: 100px; background: green; } > right { margin-left: 20px; float: left; width: 100px; height: 100px; background: > red; } > </style> > <left></left> > <right></right> added TODO for now. I will need to fix floats with margins first.
The CQ bit was checked by glebl@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...
lgtm
lgtm, though if we do start storing exclusions with logical offsets, we should rename the exclusion_top etc. variables
On 2016/10/21 19:39:27, cbiesinger wrote: > lgtm, though if we do start storing exclusions with logical offsets, we should > rename the exclusion_top etc. variables thanks Christian, noted. It's in my action items list to make a decision on this.
On 2016/10/21 19:39:27, cbiesinger wrote: > lgtm, though if we do start storing exclusions with logical offsets, we should > rename the exclusion_top etc. variables thanks Christian, noted. It's in my action items list to make a decision on this.
The CQ bit was unchecked by glebl@chromium.org
The CQ bit was checked by glebl@chromium.org
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by glebl@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Initial implementation of LayoutNG's block layout algorithm for floats. This patch adds support of left/right floats in LayoutNG code. Things that are not supported yet: - top aligning floats - clearing - child blocks in the same formatting formatting context BUG=635619 TEST=virtual/layout_ng/fast/block/float, NGBlockLayoutAlgorithmTest::PositionFloatFragments ========== to ========== Initial implementation of LayoutNG's block layout algorithm for floats. This patch adds support of left/right floats in LayoutNG code. Things that are not supported yet: - top aligning floats - clearing - child blocks in the same formatting formatting context BUG=635619 TEST=virtual/layout_ng/fast/block/float, NGBlockLayoutAlgorithmTest::PositionFloatFragments ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Initial implementation of LayoutNG's block layout algorithm for floats. This patch adds support of left/right floats in LayoutNG code. Things that are not supported yet: - top aligning floats - clearing - child blocks in the same formatting formatting context BUG=635619 TEST=virtual/layout_ng/fast/block/float, NGBlockLayoutAlgorithmTest::PositionFloatFragments ========== to ========== Initial implementation of LayoutNG's block layout algorithm for floats. This patch adds support of left/right floats in LayoutNG code. Things that are not supported yet: - top aligning floats - clearing - child blocks in the same formatting formatting context BUG=635619 TEST=virtual/layout_ng/fast/block/float, NGBlockLayoutAlgorithmTest::PositionFloatFragments Committed: https://crrev.com/05059b38db14102048bfbe34bafbaf52b9fcc7ba Cr-Commit-Position: refs/heads/master@{#426982} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/05059b38db14102048bfbe34bafbaf52b9fcc7ba Cr-Commit-Position: refs/heads/master@{#426982} |
