|
|
Created:
4 years, 2 months ago by Gleb Lanbin Modified:
4 years, 2 months ago 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. |
DescriptionCompute 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 #
Messages
Total messages: 37 (23 generated)
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
glebl@chromium.org changed reviewers: + ikilpatrick@chromium.org
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: linux_chromium_chromeos_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 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.
Just some drive-by comments. https://codereview.chromium.org/2368153003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2368153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:88: // Whether the current constraint space is for the LayoutNG Shouldn't it rather just say "the document root element's children", or something like that? As far I understand, this is merely a spec thing (shouldn't collapse margins with those of the root element), and not something LayoutNG-specific? https://codereview.chromium.org/2368153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:138: bool is_root : 1; Missing trailing underscore.
PTAL https://codereview.chromium.org/2368153003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2368153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:88: // Whether the current constraint space is for the LayoutNG On 2016/09/28 11:49:33, mstensho wrote: > Shouldn't it rather just say "the document root element's children", or > something like that? As far I understand, this is merely a spec thing (shouldn't > collapse margins with those of the root element), and not something > LayoutNG-specific? Done. https://codereview.chromium.org/2368153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:138: bool is_root : 1; On 2016/09/28 11:49:33, mstensho wrote: > Missing trailing underscore. Done.
Update looks nice. However, I don't feel competent to review the actual code changes. I just wanted to nitpick. ;) https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:165: // 2) 1st block in LayoutNG root element's constraint space. Oops, some "LayoutNG" here too...
https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:103: derived_constraint_space->SetIsRoot(); Is this flag for only the root document element? or for the root of the block formatting context?
On 2016/09/28 20:11:20, ikilpatrick wrote: > https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): > > https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:103: > derived_constraint_space->SetIsRoot(); > Is this flag for only the root document element? or for the root of the block > formatting context? done
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/Sour... > > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): > > > > > https://codereview.chromium.org/2368153003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:103: > > derived_constraint_space->SetIsRoot(); > > Is this flag for only the root document element? or for the root of the block > > formatting context? > > done updated based on the offline discussion.
https://codereview.chromium.org/2368153003/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/2368153003/diff/60001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_box.cc (right): https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_box.cc:21: bool isNewBlockFormattingContext(const LayoutBox* layout_box) { I went and read through some of the specs, the best terminology here is: establishesFormattingContext (or createsNewFormattingContext is also ok). this doesn't just check for block formatting context so needs to be more general. https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:90: bool IsNewBfc() const { return is_new_bfc_; } thought we were just going to have a method on NGBox with TODO to move to ComputedStyle?
cbiesinger@chromium.org changed reviewers: + cbiesinger@chromium.org
https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_box.cc (right): https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_box.cc:21: bool isNewBlockFormattingContext(const LayoutBox* layout_box) { nit: Shouldn't this function start with an uppercase character?
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2368153003/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/2368153003/diff/60001/third_party/WebKit/Sour... 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: > can add const back in. Done. https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_box.cc (right): https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_box.cc:21: bool isNewBlockFormattingContext(const LayoutBox* layout_box) { On 2016/09/29 00:41:28, cbiesinger wrote: > nit: Shouldn't this function start with an uppercase character? Done. https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_box.cc:21: bool isNewBlockFormattingContext(const LayoutBox* layout_box) { On 2016/09/29 00:18:11, ikilpatrick wrote: > I went and read through some of the specs, the best terminology here is: > > establishesFormattingContext (or createsNewFormattingContext is also ok). > > this doesn't just check for block formatting context so needs to be more > general. Done. https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2368153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:90: bool IsNewBfc() const { return is_new_bfc_; } On 2016/09/29 00:18:11, ikilpatrick wrote: > thought we were just going to have a method on NGBox with TODO to move to > ComputedStyle? Acknowledged.
Patchset #5 (id:100001) 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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:120001) has been deleted
lgtm
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
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.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/53e875ecbadaba5aa5075a5701473d7a81fba946 Cr-Commit-Position: refs/heads/master@{#421907} |