Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2257223002:
Allow using overrideContainingBlockContentLogical{Width,Height} outside of grid (Closed)
|
Created:
4 years, 4 months ago by cbiesinger Modified:
4 years, 3 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow using overrideContainingBlockContentLogical{Width,Height} outside of grid
I am using it for the LayoutNG transition; there does not seem to be a need
to limit this to grid items as nothing else currently sets this.
This change also makes LayoutNG use this new function.
R=eae@chromium.org,ikilpatrick@chromium.org,glebl@chromium.org
BUG=635619
Committed: https://crrev.com/febef5e01067d1ff21b2f7af4b9c6b0e15023e84
Cr-Commit-Position: refs/heads/master@{#413445}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 24 (9 generated)
The CQ bit was checked by cbiesinger@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...
Rego, please review the LayoutBox change to make sure that it really is an unnecessary check Emil/Ian/Gleb, one of you please review the layout-ng change
cbiesinger@chromium.org changed reviewers: + rego@igalia.com
Actually add Rego like I meant to
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM for ng
cbiesinger@chromium.org changed reviewers: + jfernandez@igalia.com, svillar@igalia.com
Adding some more grid people for this review
The change LGTM, but I'd rather let @rego take a look before landing. https://codereview.chromium.org/2257223002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left): https://codereview.chromium.org/2257223002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:3019: } else if (isGridItem() && hasOverrideContainingBlockLogicalHeight()) { I think it's correct, from the layout logic point of view. It may have an impact on performance, though, since we will check the HashMap for every object. Other than that I think the change is correct.
On 2016/08/19 21:14:24, jfernandez wrote: > The change LGTM, but I'd rather let @rego take a look before landing. > > https://codereview.chromium.org/2257223002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left): > > https://codereview.chromium.org/2257223002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:3019: } else if > (isGridItem() && hasOverrideContainingBlockLogicalHeight()) { > I think it's correct, from the layout logic point of view. It may have an impact > on performance, though, since we will check the HashMap for every object. Other > than that I think the change is correct. Thanks. I'll note that most of the places where we call this function already do not check isGridItem(). I could look into moving the override sizes into rare data though.
The change in LayoutBox LGTM too. https://codereview.chromium.org/2257223002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left): https://codereview.chromium.org/2257223002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:3019: } else if (isGridItem() && hasOverrideContainingBlockLogicalHeight()) { The isGridItem() was added in: https://codereview.chromium.org/2039223002 But it's not really needed, and we don't use it in other places either.
The CQ bit was checked by cbiesinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/22 10:31:09, Manuel Rego wrote: > The change in LayoutBox LGTM too. > > https://codereview.chromium.org/2257223002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left): > > https://codereview.chromium.org/2257223002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:3019: } else if > (isGridItem() && hasOverrideContainingBlockLogicalHeight()) { > The isGridItem() was added in: > https://codereview.chromium.org/2039223002 > > But it's not really needed, and we don't use it in other places either. Ah thanks. I see that I asked for the isGridItem check because that used to check for the containing block logical height. Makes sense now! Thanks.
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from
==========
Allow using overrideContainingBlockContentLogical{Width,Height} outside of grid
I am using it for the LayoutNG transition; there does not seem to be a need
to limit this to grid items as nothing else currently sets this.
This change also makes LayoutNG use this new function.
R=eae@chromium.org,ikilpatrick@chromium.org,glebl@chromium.org
BUG=635619
==========
to
==========
Allow using overrideContainingBlockContentLogical{Width,Height} outside of grid
I am using it for the LayoutNG transition; there does not seem to be a need
to limit this to grid items as nothing else currently sets this.
This change also makes LayoutNG use this new function.
R=eae@chromium.org,ikilpatrick@chromium.org,glebl@chromium.org
BUG=635619
Committed: https://crrev.com/febef5e01067d1ff21b2f7af4b9c6b0e15023e84
Cr-Commit-Position: refs/heads/master@{#413445}
==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/febef5e01067d1ff21b2f7af4b9c6b0e15023e84 Cr-Commit-Position: refs/heads/master@{#413445}
Message was sent while issue was closed.
ojan@chromium.org changed reviewers: + ojan@chromium.org
Message was sent while issue was closed.
Is this a temporary thing for the transition period before all layout is ng-ified? If so, it probably deserves a comment to that effect. If not, we might want to have a discussion about whether override sizes are needed in ng.
Message was sent while issue was closed.
nm...the code is pretty clear that this is for the transition period. Sorry for the noise.
Message was sent while issue was closed.
On 2016/08/23 21:59:53, ojan wrote: > Is this a temporary thing for the transition period before all layout is > ng-ified? If so, it probably deserves a comment to that effect. > > If not, we might want to have a discussion about whether override sizes are > needed in ng. Even when not relevant for this issue, I'd be interested on participating in such discussion, probably in the layout-dev mailing list, about override sizes. Specially regarding alternative approaches to implement the grid cell concept.
Message was sent while issue was closed.
On 2016/08/23 22:19:41, jfernandez wrote: > On 2016/08/23 21:59:53, ojan wrote: > > Is this a temporary thing for the transition period before all layout is > > ng-ified? If so, it probably deserves a comment to that effect. > > > > If not, we might want to have a discussion about whether override sizes are > > needed in ng. > > Even when not relevant for this issue, I'd be interested on participating in > such > discussion, probably in the layout-dev mailing list, about override sizes. > Specially > regarding alternative approaches to implement the grid cell concept. Happy to discuss, let me know (or email layout-dev). Emil has previously sent the LayoutNG design doc to layout-dev: https://docs.google.com/document/d/1uxbDh4uONFQOiGuiumlJBLGgO4KDWB8ZEkp7Rd47f... The sizing section discusses override sizes. In LayoutNG, container sizes are passed into the child layout by the parent layout, and so override CB sizes work naturally by passing in a different size for the constraint space. Regular override sizes work by setting a flag on the constraint space to indicate that the container size is really a fixed size. That's our current thinking anyway. See also comments and code in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
