|
|
Created:
4 years, 3 months ago by Gleb Lanbin Modified:
4 years, 3 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. |
DescriptionIgnore zero-height fragments during margin collapsing
1) Ignore zero-height fragments during margin collapsing step.
See https://www.w3.org/TR/CSS2/box.html#collapsing-margins
2) Change the order of expected/actual for all EXPECT_EQ in ng_block_layout_algorithm_test
3) Add more test expectations for the collapsing margin logic.
BUG=635619
Committed: https://crrev.com/67a801a052b6bec0bdf62145d190f9bd2e6e9a36
Cr-Commit-Position: refs/heads/master@{#419982}
Patch Set 1 #
Total comments: 17
Patch Set 2 : renamed children_fragment -> fragment, added spaces before parentheses #
Total comments: 2
Patch Set 3 : moved UpdateMarginStrut to NGBlockLayoutAlgorithm #Patch Set 4 : Set writing mode and fixed size on style_. #
Messages
Total messages: 38 (26 generated)
Patchset #1 (id:1) has been deleted
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Ignore zero-height fragments during margin collapsing 1) Ignore zero-height fragments during margin collapsing step. See https://www.w3.org/TR/CSS2/box.html#collapsing-margins 2) Change the order of expected/actual for all EXPECT_EQ in ng_block_layout_algorithm_test 3) Add more test expectations for the collapsing margin logic. BUG=635619 ========== to ========== Ignore zero-height fragments during margin collapsing 1) Ignore zero-height fragments during margin collapsing step. See https://www.w3.org/TR/CSS2/box.html#collapsing-margins 2) Change the order of expected/actual for all EXPECT_EQ in ng_block_layout_algorithm_test 3) Add more test expectations for the collapsing margin logic. BUG=635619 ==========
glebl@chromium.org changed reviewers: + cbiesinger@chromium.org, ikilpatrick@chromium.org
A very quick pass while jet-lagged. https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:116: const NGFragment& children_fragment) { child_fragment? https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:117: // Zero-height boxes are ignored. ... are ignored and do not participate in margin collapsing? https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h (right): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:55: // @param fragment Children's fragment. Child fragment or Current child fragment? https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:34: NGFragmentBuilder& UpdateMarginStrut(const NGMarginStrut& from); Is there another way that we can do this without splitting the logic between the two classes?
https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (left): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:155: if (current_child_ != first_child_) Why do you no longer need this check? https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:118: bool is_zero_height_box = !children_fragment.BlockSize() && margins.IsEmpty(); We may eventually need to check for "does not establish a new block formatting context" here https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:120: return LayoutUnit(0); We usually just use LayoutUnit() for zero, it's a bit more efficient https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:32: // the block-start once(on the first non-zero height child fragment) and keeps Please add a space before ( here and on the next line
PTAL https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (left): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:155: if (current_child_ != first_child_) On 2016/09/19 17:05:30, cbiesinger wrote: > Why do you no longer need this check? I mistakenly thought that we only need to calculate the margin block start between adjoining blocks. While working on the "writing-mode fix" I noticed a problem with TopOffset, fixed it in this patch, added more test expectations to verify TopOffset as well. https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:116: const NGFragment& children_fragment) { On 2016/09/19 17:04:19, ikilpatrick wrote: > child_fragment? Done. https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:117: // Zero-height boxes are ignored. On 2016/09/19 17:04:19, ikilpatrick wrote: > ... are ignored and do not participate in margin collapsing? Done. https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:118: bool is_zero_height_box = !children_fragment.BlockSize() && margins.IsEmpty(); On 2016/09/19 17:05:30, cbiesinger wrote: > We may eventually need to check for "does not establish a new block formatting > context" here Done. https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:120: return LayoutUnit(0); On 2016/09/19 17:05:30, cbiesinger wrote: > We usually just use LayoutUnit() for zero, it's a bit more efficient Done. https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h (right): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:55: // @param fragment Children's fragment. On 2016/09/19 17:04:19, ikilpatrick wrote: > Child fragment or Current child fragment? Done. https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:32: // the block-start once(on the first non-zero height child fragment) and keeps On 2016/09/19 17:05:30, cbiesinger wrote: > Please add a space before ( here and on the next line Done. https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:34: NGFragmentBuilder& UpdateMarginStrut(const NGMarginStrut& from); On 2016/09/19 17:04:19, ikilpatrick wrote: > Is there another way that we can do this without splitting the logic between the > two classes? that was my concern as well. I decided to keep it in this way in favor of clear NGFragmentBuilder's API and readability of CollapseMargins method. We need to find the first/last non-zero height children to update their parent's margin strut. So we still need to use a some sort of flag to know whether the parent's margin strut block-start has been already updated. We already have this flag in NGFragmentBuilder and I don't like to overload the implementation with IF statements.
OK, seems fine to me but I'd like to leave the final approval to Ian because he knows margin collapsing better.
mstensho@opera.com changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/2350603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_units.cc (right): https://codereview.chromium.org/2350603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_units.cc:69: return std::tie(other.inline_start, other.inline_end, other.block_start, Wouldn't comparing each of the four members individually be faster than using std::tie?
lgtm https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:34: NGFragmentBuilder& UpdateMarginStrut(const NGMarginStrut& from); On 2016/09/19 17:39:52, glebl wrote: > On 2016/09/19 17:04:19, ikilpatrick wrote: > > Is there another way that we can do this without splitting the logic between > the > > two classes? > > that was my concern as well. I decided to keep it in this way in favor of clear > NGFragmentBuilder's API and readability of CollapseMargins method. > > We need to find the first/last non-zero height children to update their parent's > margin strut. So we still need to use a some sort of flag to know whether the > parent's margin strut block-start has been already updated. > > We already have this flag in NGFragmentBuilder and I don't like to overload the > implementation with IF statements. I have a preference to leave this in a separate function inside BlockFlowLayout (i.e. move UpdateMarginStrut to that class and introduce the flag). Simply from a "leave the builder as simple as possible" POV. I'm just concerned that additional complexity for some reason will end up here eventually.
On 2016/09/20 11:00:29, ikilpatrick wrote: > lgtm > > https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): > > https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:34: > NGFragmentBuilder& UpdateMarginStrut(const NGMarginStrut& from); > On 2016/09/19 17:39:52, glebl wrote: > > On 2016/09/19 17:04:19, ikilpatrick wrote: > > > Is there another way that we can do this without splitting the logic between > > the > > > two classes? > > > > that was my concern as well. I decided to keep it in this way in favor of > clear > > NGFragmentBuilder's API and readability of CollapseMargins method. > > > > We need to find the first/last non-zero height children to update their > parent's > > margin strut. So we still need to use a some sort of flag to know whether the > > parent's margin strut block-start has been already updated. > > > > We already have this flag in NGFragmentBuilder and I don't like to overload > the > > implementation with IF statements. > > I have a preference to leave this in a separate function inside BlockFlowLayout > (i.e. move UpdateMarginStrut to that class and introduce the flag). > > Simply from a "leave the builder as simple as possible" POV. I'm just concerned > that additional complexity for some reason will end up here eventually. done
thanks for the review https://codereview.chromium.org/2350603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_units.cc (right): https://codereview.chromium.org/2350603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_units.cc:69: return std::tie(other.inline_start, other.inline_end, other.block_start, On 2016/09/20 09:59:50, mstensho wrote: > Wouldn't comparing each of the four members individually be faster than using > std::tie? I didn't test std:tie performance but std::tie is approved to use in Chromium code https://chromium-cpp.appspot.com/ and it's already used in other parts of Chromium.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:80001) has been deleted
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...
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 glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2350603002/#ps120001 (title: "Set writing mode and fixed size on style_.")
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 ========== Ignore zero-height fragments during margin collapsing 1) Ignore zero-height fragments during margin collapsing step. See https://www.w3.org/TR/CSS2/box.html#collapsing-margins 2) Change the order of expected/actual for all EXPECT_EQ in ng_block_layout_algorithm_test 3) Add more test expectations for the collapsing margin logic. BUG=635619 ========== to ========== Ignore zero-height fragments during margin collapsing 1) Ignore zero-height fragments during margin collapsing step. See https://www.w3.org/TR/CSS2/box.html#collapsing-margins 2) Change the order of expected/actual for all EXPECT_EQ in ng_block_layout_algorithm_test 3) Add more test expectations for the collapsing margin logic. BUG=635619 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Ignore zero-height fragments during margin collapsing 1) Ignore zero-height fragments during margin collapsing step. See https://www.w3.org/TR/CSS2/box.html#collapsing-margins 2) Change the order of expected/actual for all EXPECT_EQ in ng_block_layout_algorithm_test 3) Add more test expectations for the collapsing margin logic. BUG=635619 ========== to ========== Ignore zero-height fragments during margin collapsing 1) Ignore zero-height fragments during margin collapsing step. See https://www.w3.org/TR/CSS2/box.html#collapsing-margins 2) Change the order of expected/actual for all EXPECT_EQ in ng_block_layout_algorithm_test 3) Add more test expectations for the collapsing margin logic. BUG=635619 Committed: https://crrev.com/67a801a052b6bec0bdf62145d190f9bd2e6e9a36 Cr-Commit-Position: refs/heads/master@{#419982} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/67a801a052b6bec0bdf62145d190f9bd2e6e9a36 Cr-Commit-Position: refs/heads/master@{#419982} |