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

Issue 2368153003: Compute margin block start for 1st block in LayoutNG root constraint space (Closed)

Created:
4 years, 2 months ago by Gleb Lanbin
Modified:
4 years, 2 months ago
Reviewers:
cbiesinger, ikilpatrick
CC:
chromium-reviews, cbiesinger, 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

Compute margin block start for 1st block in LayoutNG root constraint space It will correctly compute margin block start for the 1st block in LayoutNG's root constraint space. This is because margins of the root element's box do not collapse https://www.w3.org/TR/CSS2/box.html#collapsing-margins BUG=635619 Committed: https://crrev.com/53e875ecbadaba5aa5075a5701473d7a81fba946 Cr-Commit-Position: refs/heads/master@{#421907}

Patch Set 1 #

Total comments: 4

Patch Set 2 : added a missing underscore, removed LayoutNG from comments #

Total comments: 2

Patch Set 3 : IsRoot -> IsBfcRoot #

Patch Set 4 : Get IsNewBfc from layout_box->createsNewFormattingContext() #

Total comments: 8

Patch Set 5 : rename IsNewBfc -> IsNewFormattingContext #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -13 lines) Patch
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc View 1 2 3 4 2 chunks +9 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_box.cc View 1 2 3 4 2 chunks +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h View 1 2 3 4 3 chunks +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc View 1 2 3 4 4 chunks +12 lines, -5 lines 0 comments Download

Messages

Total messages: 37 (23 generated)
Gleb Lanbin
4 years, 2 months ago (2016-09-26 23:24:51 UTC) #3
mstensho (USE GERRIT)
Just some drive-by comments. https://codereview.chromium.org/2368153003/diff/1/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2368153003/diff/1/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h#newcode88 third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:88: // Whether the current constraint ...
4 years, 2 months ago (2016-09-28 11:49:33 UTC) #11
Gleb Lanbin
PTAL https://codereview.chromium.org/2368153003/diff/1/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2368153003/diff/1/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h#newcode88 third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:88: // Whether the current constraint space is for ...
4 years, 2 months ago (2016-09-28 17:26:07 UTC) #12
mstensho (USE GERRIT)
Update looks nice. However, I don't feel competent to review the actual code changes. I ...
4 years, 2 months ago (2016-09-28 18:32:33 UTC) #13
ikilpatrick
https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc#newcode103 third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:103: derived_constraint_space->SetIsRoot(); Is this flag for only the root document ...
4 years, 2 months ago (2016-09-28 20:11:20 UTC) #14
Gleb Lanbin
On 2016/09/28 20:11:20, ikilpatrick wrote: > https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): > > https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc#newcode103 > ...
4 years, 2 months ago (2016-09-28 21:10:17 UTC) #15
Gleb Lanbin
On 2016/09/28 21:10:17, glebl wrote: > On 2016/09/28 20:11:20, ikilpatrick wrote: > > > https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc ...
4 years, 2 months ago (2016-09-28 22:41:01 UTC) #16
ikilpatrick
https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc#newcode22 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:22: NGPhysicalFragment* RunBlockLayoutAlgorithm(NGConstraintSpace* space, can add const back in. https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_box.cc ...
4 years, 2 months ago (2016-09-29 00:18:11 UTC) #17
cbiesinger
https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_box.cc File third_party/WebKit/Source/core/layout/ng/ng_box.cc (right): https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_box.cc#newcode21 third_party/WebKit/Source/core/layout/ng/ng_box.cc:21: bool isNewBlockFormattingContext(const LayoutBox* layout_box) { nit: Shouldn't this function ...
4 years, 2 months ago (2016-09-29 00:41:28 UTC) #19
Gleb Lanbin
https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc#newcode22 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:22: NGPhysicalFragment* RunBlockLayoutAlgorithm(NGConstraintSpace* space, On 2016/09/29 00:18:11, ikilpatrick wrote: > ...
4 years, 2 months ago (2016-09-29 17:11:15 UTC) #21
ikilpatrick
lgtm
4 years, 2 months ago (2016-09-29 17:34:06 UTC) #28
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/2368153003/140001
4 years, 2 months ago (2016-09-29 19:43:29 UTC) #34
commit-bot: I haz the power
Committed patchset #5 (id:140001)
4 years, 2 months ago (2016-09-29 19:48:46 UTC) #35
commit-bot: I haz the power
4 years, 2 months ago (2016-09-29 19:50:25 UTC) #37
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/53e875ecbadaba5aa5075a5701473d7a81fba946
Cr-Commit-Position: refs/heads/master@{#421907}

Powered by Google App Engine
This is Rietveld 408576698