Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(714)

Issue 2350603002: Ignore zero-height fragments during margin collapsing (Closed)

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.

Description

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}

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_. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+167 lines, -84 lines) Patch
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h View 1 2 2 chunks +12 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc View 1 2 3 4 chunks +34 lines, -23 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc View 1 2 3 8 chunks +106 lines, -45 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h View 1 2 2 chunks +0 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc View 1 2 2 chunks +1 line, -7 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_units.h View 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_units.cc View 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 38 (26 generated)
Gleb Lanbin
4 years, 3 months ago (2016-09-19 15:46:24 UTC) #8
ikilpatrick
A very quick pass while jet-lagged. https://codereview.chromium.org/2350603002/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/2350603002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#newcode116 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:116: const NGFragment& children_fragment) ...
4 years, 3 months ago (2016-09-19 17:04:19 UTC) #9
cbiesinger
https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (left): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#oldcode155 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:155: if (current_child_ != first_child_) Why do you no longer ...
4 years, 3 months ago (2016-09-19 17:05:30 UTC) #10
Gleb Lanbin
PTAL https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (left): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#oldcode155 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 ...
4 years, 3 months ago (2016-09-19 17:39:52 UTC) #11
cbiesinger
OK, seems fine to me but I'd like to leave the final approval to Ian ...
4 years, 3 months ago (2016-09-19 20:30:34 UTC) #12
mstensho (USE GERRIT)
https://codereview.chromium.org/2350603002/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_units.cc File third_party/WebKit/Source/core/layout/ng/ng_units.cc (right): https://codereview.chromium.org/2350603002/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_units.cc#newcode69 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 ...
4 years, 3 months ago (2016-09-20 09:59:50 UTC) #14
ikilpatrick
lgtm https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode34 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 ...
4 years, 3 months ago (2016-09-20 11:00:29 UTC) #15
Gleb Lanbin
On 2016/09/20 11:00:29, ikilpatrick wrote: > lgtm > > https://codereview.chromium.org/2350603002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h > File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): > ...
4 years, 3 months ago (2016-09-20 19:18:09 UTC) #16
Gleb Lanbin
thanks for the review https://codereview.chromium.org/2350603002/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_units.cc File third_party/WebKit/Source/core/layout/ng/ng_units.cc (right): https://codereview.chromium.org/2350603002/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_units.cc#newcode69 third_party/WebKit/Source/core/layout/ng/ng_units.cc:69: return std::tie(other.inline_start, other.inline_end, other.block_start, On ...
4 years, 3 months ago (2016-09-20 19:18:19 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2350603002/120001
4 years, 3 months ago (2016-09-21 05:37:42 UTC) #34
commit-bot: I haz the power
Committed patchset #4 (id:120001)
4 years, 3 months ago (2016-09-21 05:43:41 UTC) #36
commit-bot: I haz the power
4 years, 3 months ago (2016-09-21 05:45:38 UTC) #38
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/67a801a052b6bec0bdf62145d190f9bd2e6e9a36
Cr-Commit-Position: refs/heads/master@{#419982}

Powered by Google App Engine
This is Rietveld 408576698