Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/318931) chromium_presubmit on ...
3 years, 8 months ago
(2017-04-13 19:58:01 UTC)
#7
On 2017/04/14 22:46:34, Gleb Lanbin wrote: > On 2017/04/14 10:42:28, ikilpatrick wrote: > > I'll ...
3 years, 8 months ago
(2017-04-17 08:17:16 UTC)
#10
On 2017/04/14 22:46:34, Gleb Lanbin wrote:
> On 2017/04/14 10:42:28, ikilpatrick wrote:
> > I'll look at this tomorrow morning.
>
> Also I tried a bit more complicated example
>
https://docs.google.com/document/d/1xiwqdIHm-2NdYOEHXMjUOrXSfCPf9fE093bWeS4Ya....
> I think LayoutNG lays out that example very close to how it should be, i.e.
> without any overlapping etc.
So in that example change contain: paint, to overflow: hidden, edge/FF may not
support that yet.
ikilpatrick
https://codereview.chromium.org/2816933003/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 (right): https://codereview.chromium.org/2816933003/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#newcode139 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:139: NGLogicalOffset NGBlockLayoutAlgorithm::PositionNewFc( might be worth writing up here which ...
3 years, 8 months ago
(2017-04-17 08:17:25 UTC)
#11
3 years, 8 months ago
(2017-04-17 21:50:10 UTC)
#13
PTAL
https://codereview.chromium.org/2816933003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):
https://codereview.chromium.org/2816933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:139:
NGLogicalOffset NGBlockLayoutAlgorithm::PositionNewFc(
On 2017/04/17 08:17:24, ikilpatrick wrote:
> .nit, so i like having these types of methods close to where they are used,
> thoughts on moving them below ::FinishChildLayout?
Done.
https://codereview.chromium.org/2816933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:139:
NGLogicalOffset NGBlockLayoutAlgorithm::PositionNewFc(
On 2017/04/17 08:17:24, ikilpatrick wrote:
> might be worth writing up here which case this covers, (and why it's gross).
Done.
https://codereview.chromium.org/2816933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:141: const
NGConstraintSpace* child_space) {
On 2017/04/17 08:17:24, ikilpatrick wrote:
> you aren't modifying child_space right? so this should be a ref?
Done.
https://codereview.chromium.org/2816933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:154:
&tmp_builder);
On 2017/04/17 08:17:24, ikilpatrick wrote:
> is there an easy way we can avoid tmp_builder here?
Done.
https://codereview.chromium.org/2816933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:169: // 4.
Child's BFC block's offset is known here.
On 2017/04/17 08:17:24, ikilpatrick wrote:
> 4. The child's BFC block offset is known here.
>
> ?
Done.
https://codereview.chromium.org/2816933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:393: if
(!IsNewFormattingContextForBlockLevelChild(Style(), *child))
On 2017/04/17 08:17:24, ikilpatrick wrote:
> you don't strictly need to do this right?
the decision about whether we should append margins for a new FC block happens
in PositionNewFc method.
if (opportunity.offset.block_offset == curr_bfc_offset_.block_offset)
curr_margin_strut_.Append(curr_child_margins_.block_start);
https://codereview.chromium.org/2816933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:422: //
Determine the fragment's position in the parent space.
On 2017/04/17 08:17:24, ikilpatrick wrote:
> Yay! thanks for pulling these out.
Acknowledged.
https://codereview.chromium.org/2816933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:465: // No
need to postpone the positioning if we know the correct offset.
On 2017/04/17 08:17:24, ikilpatrick wrote:
> can you add a newline above this line? (just having trouble with separation
> here).
Done.
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2816933003/40001
3 years, 8 months ago
(2017-04-17 21:50:14 UTC)
#14
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/159025)
3 years, 8 months ago
(2017-04-17 22:15:08 UTC)
#16
lgtm https://codereview.chromium.org/2816933003/diff/40001/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 (right): https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#newcode325 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:325: // Non empty border/padding, new FC use cases ...
3 years, 8 months ago
(2017-04-18 13:19:56 UTC)
#17
https://codereview.chromium.org/2816933003/diff/40001/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 (right): https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#newcode325 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:325: // Non empty border/padding, new FC use cases are ...
3 years, 8 months ago
(2017-04-18 17:30:20 UTC)
#18
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:325: //
Non empty border/padding, new FC use cases are handled inside of the
On 2017/04/18 13:19:55, ikilpatrick wrote:
> .nit Non empty border/padding, and new FC use cases....
>
> (oxford comma)
Done.
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:418:
opportunity = FindLayoutOpportunityForFragment(
On 2017/04/18 13:19:55, ikilpatrick wrote:
> will this always be just:
>
> LayoutUnit block_offset_delta = curr_margin_strut_.Sum(); // (bad name).
> curr_bfc_offset_.block_offset += block_offset_delta;
> curr_margin_strut_ = {};
> // ^ (step 3).
>
> opportunity.offset.block_offset += block_offset_delta;
> // ^ (step 5)
> ?
we need to call FindLayoutOpportunityForFragment one more time because we might
have intruding floats that we need to take into account after we determined the
position of parent's fragment.
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_floats_utils.cc (right):
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floats_utils.cc:32: // Finds a
layout opportunity for the fragment.
On 2017/04/18 13:19:55, ikilpatrick wrote:
> update comments?
I just deleted this comment. We don't need it anymore because
1) it's a private method
2) it's very clear what it does from its name.
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.h (right):
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.h:70: void
PositionPendingFloats(LayoutUnit origin_block_offset) {
On 2017/04/18 13:19:56, ikilpatrick wrote:
> if there is another way to do this i'd prefer that, as this is very block-flow
> specific... i.e. no other layout algorithms will use this.
>
> Could do the same thing as MaybeUpdateFragmentBfcOffset for now?
>
> Or might be time to do this block/inline specific sub-class.
yes, I was thinking about that as well. Here are 4 options:
1) Make NGInlineLayoutAlgorithm to inherit NGBlockLayoutAlgorithm. That's
because NGInlineLayoutAlgorithm produces a block fragment as as result.
2) (SELECTED) Leave PositionPendingFloats in NGLayoutAlgorithm base class for
now but have an annotation that it should be only used by
NG{Block|Inline}Algorithms. Later we can decide what to do with them
3) Make PositionPendingFloats static. But that will make PositionPendingFloats
usage not very convenient.
4) Create an interim base class for Block/Inline algorithms.
Gleb Lanbin
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
3 years, 8 months ago
(2017-04-18 17:30:25 UTC)
#19
Dry run: 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_rel_ng/builds/433405)
3 years, 8 months ago
(2017-04-18 19:24:28 UTC)
#22
3 years, 8 months ago
(2017-04-18 21:04:11 UTC)
#26
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:418:
opportunity = FindLayoutOpportunityForFragment(
On 2017/04/18 17:30:20, Gleb Lanbin wrote:
> On 2017/04/18 13:19:55, ikilpatrick wrote:
> > will this always be just:
> >
> > LayoutUnit block_offset_delta = curr_margin_strut_.Sum(); // (bad name).
> > curr_bfc_offset_.block_offset += block_offset_delta;
> > curr_margin_strut_ = {};
> > // ^ (step 3).
> >
> > opportunity.offset.block_offset += block_offset_delta;
> > // ^ (step 5)
> > ?
>
> we need to call FindLayoutOpportunityForFragment one more time because we
might
> have intruding floats that we need to take into account after we determined
the
> position of parent's fragment.
thanks, yup.
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.h (right):
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.h:70: void
PositionPendingFloats(LayoutUnit origin_block_offset) {
On 2017/04/18 17:30:20, Gleb Lanbin wrote:
> On 2017/04/18 13:19:56, ikilpatrick wrote:
> > if there is another way to do this i'd prefer that, as this is very
block-flow
> > specific... i.e. no other layout algorithms will use this.
> >
> > Could do the same thing as MaybeUpdateFragmentBfcOffset for now?
> >
> > Or might be time to do this block/inline specific sub-class.
>
> yes, I was thinking about that as well. Here are 4 options:
>
> 1) Make NGInlineLayoutAlgorithm to inherit NGBlockLayoutAlgorithm. That's
> because NGInlineLayoutAlgorithm produces a block fragment as as result.
>
> 2) (SELECTED) Leave PositionPendingFloats in NGLayoutAlgorithm base class for
> now but have an annotation that it should be only used by
> NG{Block|Inline}Algorithms. Later we can decide what to do with them
>
> 3) Make PositionPendingFloats static. But that will make PositionPendingFloats
> usage not very convenient.
>
> 4) Create an interim base class for Block/Inline algorithms.
My preferred option here would be (4) in a follow up patch. We'll need to be
very principled about only simple methods in the base class.
3 years, 8 months ago
(2017-04-18 22:07:20 UTC)
#27
On 2017/04/18 21:04:11, ikilpatrick wrote:
>
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
> (right):
>
>
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:418:
> opportunity = FindLayoutOpportunityForFragment(
> On 2017/04/18 17:30:20, Gleb Lanbin wrote:
> > On 2017/04/18 13:19:55, ikilpatrick wrote:
> > > will this always be just:
> > >
> > > LayoutUnit block_offset_delta = curr_margin_strut_.Sum(); // (bad name).
> > > curr_bfc_offset_.block_offset += block_offset_delta;
> > > curr_margin_strut_ = {};
> > > // ^ (step 3).
> > >
> > > opportunity.offset.block_offset += block_offset_delta;
> > > // ^ (step 5)
> > > ?
> >
> > we need to call FindLayoutOpportunityForFragment one more time because we
> might
> > have intruding floats that we need to take into account after we determined
> the
> > position of parent's fragment.
>
> thanks, yup.
>
>
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.h (right):
>
>
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.h:70: void
> PositionPendingFloats(LayoutUnit origin_block_offset) {
> On 2017/04/18 17:30:20, Gleb Lanbin wrote:
> > On 2017/04/18 13:19:56, ikilpatrick wrote:
> > > if there is another way to do this i'd prefer that, as this is very
> block-flow
> > > specific... i.e. no other layout algorithms will use this.
> > >
> > > Could do the same thing as MaybeUpdateFragmentBfcOffset for now?
> > >
> > > Or might be time to do this block/inline specific sub-class.
> >
> > yes, I was thinking about that as well. Here are 4 options:
> >
> > 1) Make NGInlineLayoutAlgorithm to inherit NGBlockLayoutAlgorithm. That's
> > because NGInlineLayoutAlgorithm produces a block fragment as as result.
> >
> > 2) (SELECTED) Leave PositionPendingFloats in NGLayoutAlgorithm base class
for
> > now but have an annotation that it should be only used by
> > NG{Block|Inline}Algorithms. Later we can decide what to do with them
> >
> > 3) Make PositionPendingFloats static. But that will make
PositionPendingFloats
> > usage not very convenient.
> >
> > 4) Create an interim base class for Block/Inline algorithms.
>
> My preferred option here would be (4) in a follow up patch. We'll need to be
> very principled about only simple methods in the base class.
ok, lets choose a name for that class then. NGBaseBlockLayoutAlgorithm ?
Gleb Lanbin
On 2017/04/18 22:07:20, Gleb Lanbin wrote: > On 2017/04/18 21:04:11, ikilpatrick wrote: > > > ...
3 years, 8 months ago
(2017-04-19 16:25:16 UTC)
#28
On 2017/04/18 22:07:20, Gleb Lanbin wrote:
> On 2017/04/18 21:04:11, ikilpatrick wrote:
> >
>
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:418:
> > opportunity = FindLayoutOpportunityForFragment(
> > On 2017/04/18 17:30:20, Gleb Lanbin wrote:
> > > On 2017/04/18 13:19:55, ikilpatrick wrote:
> > > > will this always be just:
> > > >
> > > > LayoutUnit block_offset_delta = curr_margin_strut_.Sum(); // (bad name).
> > > > curr_bfc_offset_.block_offset += block_offset_delta;
> > > > curr_margin_strut_ = {};
> > > > // ^ (step 3).
> > > >
> > > > opportunity.offset.block_offset += block_offset_delta;
> > > > // ^ (step 5)
> > > > ?
> > >
> > > we need to call FindLayoutOpportunityForFragment one more time because we
> > might
> > > have intruding floats that we need to take into account after we
determined
> > the
> > > position of parent's fragment.
> >
> > thanks, yup.
> >
> >
>
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.h (right):
> >
> >
>
https://codereview.chromium.org/2816933003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.h:70: void
> > PositionPendingFloats(LayoutUnit origin_block_offset) {
> > On 2017/04/18 17:30:20, Gleb Lanbin wrote:
> > > On 2017/04/18 13:19:56, ikilpatrick wrote:
> > > > if there is another way to do this i'd prefer that, as this is very
> > block-flow
> > > > specific... i.e. no other layout algorithms will use this.
> > > >
> > > > Could do the same thing as MaybeUpdateFragmentBfcOffset for now?
> > > >
> > > > Or might be time to do this block/inline specific sub-class.
> > >
> > > yes, I was thinking about that as well. Here are 4 options:
> > >
> > > 1) Make NGInlineLayoutAlgorithm to inherit NGBlockLayoutAlgorithm. That's
> > > because NGInlineLayoutAlgorithm produces a block fragment as as result.
> > >
> > > 2) (SELECTED) Leave PositionPendingFloats in NGLayoutAlgorithm base class
> for
> > > now but have an annotation that it should be only used by
> > > NG{Block|Inline}Algorithms. Later we can decide what to do with them
> > >
> > > 3) Make PositionPendingFloats static. But that will make
> PositionPendingFloats
> > > usage not very convenient.
> > >
> > > 4) Create an interim base class for Block/Inline algorithms.
> >
> > My preferred option here would be (4) in a follow up patch. We'll need to be
> > very principled about only simple methods in the base class.
>
> ok, lets choose a name for that class then. NGBaseBlockLayoutAlgorithm ?
made PositionPendingFloats to be a static method, same as
MaybeUpdateFragmentBfcOffset.
Gleb Lanbin
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
3 years, 8 months ago
(2017-04-19 19:57:04 UTC)
#29
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/251494) android_clang_dbg_recipe on ...
3 years, 8 months ago
(2017-04-19 20:03:38 UTC)
#32
Dry run: 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/426100)
3 years, 8 months ago
(2017-04-19 22:03:55 UTC)
#36
CQ is committing da patch. Bot data: {"patchset_id": 150001, "attempt_start_ts": 1492662863844820, "parent_rev": "2ecea20b01bdccc4f80893e5e14955bad1e9a554", "commit_rev": "c873e8003e60b30ff8bd65fd345b70faf77cd700"}
3 years, 8 months ago
(2017-04-20 04:39:04 UTC)
#44
CQ is committing da patch.
Bot data: {"patchset_id": 150001, "attempt_start_ts": 1492662863844820,
"parent_rev": "2ecea20b01bdccc4f80893e5e14955bad1e9a554", "commit_rev":
"c873e8003e60b30ff8bd65fd345b70faf77cd700"}
commit-bot: I haz the power
Description was changed from ========== Use Layout Opportunity Iterator to position new FC blocks. This ...
3 years, 8 months ago
(2017-04-20 04:39:38 UTC)
#45
Message was sent while issue was closed.
Description was changed from
==========
Use Layout Opportunity Iterator to position new FC blocks.
This patch fixes the use case when we need to use Layout Opportunity
Iterator to position a block that establishes a new formatting context.
BUG=635619
TEST=NGBlockLayoutAlgorithmTest.NewFcBlockWithAdjoiningFloatCollapsesMargins
==========
to
==========
Use Layout Opportunity Iterator to position new FC blocks.
This patch fixes the use case when we need to use Layout Opportunity
Iterator to position a block that establishes a new formatting context.
BUG=635619
TEST=NGBlockLayoutAlgorithmTest.NewFcBlockWithAdjoiningFloatCollapsesMargins
Review-Url: https://codereview.chromium.org/2816933003
Cr-Commit-Position: refs/heads/master@{#465899}
Committed:
https://chromium.googlesource.com/chromium/src/+/c873e8003e60b30ff8bd65fd345b...
==========
commit-bot: I haz the power
Committed patchset #8 (id:150001) as https://chromium.googlesource.com/chromium/src/+/c873e8003e60b30ff8bd65fd345b70faf77cd700
3 years, 8 months ago
(2017-04-20 04:39:39 UTC)
#46
Issue 2816933003: Use Layout Opportunity Iterator to position new FC blocks.
(Closed)
Created 3 years, 8 months ago by Gleb Lanbin
Modified 3 years, 8 months ago
Reviewers: ikilpatrick, cbiesinger
Base URL:
Comments: 28