Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(9)

Issue 23694023: Remove an extra relayout for non-percent logical height grid items (Closed)

Created:
6 years, 3 months ago by Julien - ping for review
Modified:
6 years, 3 months ago
Reviewers:
ojan, esprehn, eseidel
CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, eseidel
Visibility:
Public.

Description

Remove an extra relayout for non-percent logical height grid items The only reason for a grid item to force a relayout after a grid area's logical height change is if it has a percentage height. In all other cases, this would cause an unneeded relayout. Note that changing the logical width still forces a relayout as the situation is not that simple. This improves auto-grid-lots-of-data.html from 94 runs/s to 226 runs/s as we avoid a relayout in layoutGridItems for every grid items. BUG=273238 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157633

Patch Set 1 #

Total comments: 2

Patch Set 2 : Updated patch: Uses RenderBox::hasRelativeLogicalHeight and includes testing for min-height/max-heiā€¦ #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -1 line) Patch
A LayoutTests/fast/css-grid-layout/grid-item-with-percent-min-max-height-dynamic.html View 1 1 chunk +64 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-item-with-percent-min-max-height-dynamic-expected.txt View 1 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderGrid.cpp View 1 1 chunk +1 line, -1 line 1 comment Download

Messages

Total messages: 8 (0 generated)
Julien - ping for review
6 years, 3 months ago (2013-09-06 22:19:52 UTC) #1
eseidel
I'm sure ojan is a better reviewer for grid than I am, but lgtm.
6 years, 3 months ago (2013-09-06 22:21:04 UTC) #2
ojan
https://codereview.chromium.org/23694023/diff/1/Source/core/rendering/RenderGrid.cpp File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/23694023/diff/1/Source/core/rendering/RenderGrid.cpp#newcode853 Source/core/rendering/RenderGrid.cpp:853: return logicalHeight.isPercent(); Don't you also need to check maxHeight/minHeight? ...
6 years, 3 months ago (2013-09-07 17:13:20 UTC) #3
Julien - ping for review
https://codereview.chromium.org/23694023/diff/1/Source/core/rendering/RenderGrid.cpp File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/23694023/diff/1/Source/core/rendering/RenderGrid.cpp#newcode853 Source/core/rendering/RenderGrid.cpp:853: return logicalHeight.isPercent(); On 2013/09/07 17:13:20, ojan wrote: > Don't ...
6 years, 3 months ago (2013-09-09 18:04:42 UTC) #4
Julien - ping for review
https://codereview.chromium.org/23694023/diff/10001/Source/core/rendering/RenderGrid.cpp File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/23694023/diff/10001/Source/core/rendering/RenderGrid.cpp#newcode873 Source/core/rendering/RenderGrid.cpp:873: if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (child->hasRelativeLogicalHeight() && oldOverrideContainingBlockContentLogicalHeight != ...
6 years, 3 months ago (2013-09-11 00:06:05 UTC) #5
ojan
lgtm
6 years, 3 months ago (2013-09-11 21:52:02 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/23694023/10001
6 years, 3 months ago (2013-09-11 22:03:50 UTC) #7
commit-bot: I haz the power
6 years, 3 months ago (2013-09-11 23:26:08 UTC) #8
Message was sent while issue was closed.
Change committed as 157633

Powered by Google App Engine
This is Rietveld 408576698