Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2334133002:
[css-grid] Cache definite height detection (Closed)
|
Created:
4 years, 3 months ago by Manuel Rego Modified:
4 years, 3 months ago CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, svillar, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Cache definite height detection
This is a refactoring patch which stores the definite/indefinite height
detection in a new attribute m_hasDefiniteLogicalHeight in LayoutGrid.
That way we just only call LayoutBlock::hasDefiniteLogicalHeight() once,
from LayoutGrid::layoutBlock(). Then in LayoutGrid::gridTrackSize()
we reuse the cached value.
No new tests, no change of behavior.
BUG=624301
Committed: https://crrev.com/e09a66db87cd70ecbd3b045464bf10ef623a03f9
Cr-Commit-Position: refs/heads/master@{#418831}
Patch Set 1 #
Total comments: 8
Patch Set 2 : New version using m_hasDefiniteLogicalHeight class member #
Total comments: 7
Patch Set 3 : New version applying suggested changes #
Messages
Total messages: 26 (11 generated)
jfernandez@igalia.com changed reviewers: + jfernandez@igalia.com
I like the idea of caching the definite/indefinite logical height condition, but I think we should follow a different approach. https://codereview.chromium.org/2334133002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2334133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:242: GridSizingData(size_t gridColumnCount, size_t gridRowCount, bool definiteLogicalHeight) better use 'hasDefiniteLogicalHeigth" as variable name. https://codereview.chromium.org/2334133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:245: , m_definiteLogicalHeight(definiteLogicalHeight) Again, better use the 'has' prefix in the attribute name. https://codereview.chromium.org/2334133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:468: GridSizingData sizingData(gridColumnCount(), gridRowCount(), hasDefiniteLogicalHeight()); Are we suer we want to store this information in the GridSizingData structure ? it seems something that depends more on the grid container box than the temporary sizing structure data. My point is that passing the info as a boolean variable it's a bit strange. Why not just defining a new class attribute so we can access to it independently of the sizingData structure ? https://codereview.chromium.org/2334133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:828: double LayoutGrid::findFlexFactorUnitSize(const Vector<GridTrack>& tracks, const GridSpan& tracksSpan, GridTrackSizingDirection direction, const GridSizingData& sizingData, LayoutUnit leftOverSpace) const an example of why I don't like a lot the approach you followed is that some methods, like this one, receive the sizingData argument just to forward it to the ones actually requiring to know whether the container has a definite logical height or not. https://codereview.chromium.org/2334133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:929: if ((sizingData.sizingOperation == IntrinsicSizeComputation) || (direction == ForRows && !sizingData.hasDefiniteLogicalHeight())) { If I understood it correctly, you have passed 'false' as 'definiteLogicalHeight" argument value when instantiating the SizingData structure for intrinsic size computation. Since being under intrinsic size computation implies that height is indefinite, wouldn't be better to just add an assertion here and avoid the redundant condition ? https://codereview.chromium.org/2334133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1085: bool LayoutGrid::spanningItemCrossesFlexibleSizedTracks(const GridSpan& span, GridTrackSizingDirection direction, const GridSizingData& sizingData) const isn't this another example of a method just forwarding the sizingData structure ?
svillar@igalia.com changed reviewers: + svillar@igalia.com
So it seems that it's indeed very intrusive. Perhaps we should store it in an attribute but we should ensure that we never check it before setting it.
rego@igalia.com changed reviewers: + cbiesinger@chromium.org
Uploaded a new version using a class attribute. https://codereview.chromium.org/2334133002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2334133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:468: GridSizingData sizingData(gridColumnCount(), gridRowCount(), hasDefiniteLogicalHeight()); Ok, I've moved it to an attribute called m_hasDefiniteLogicalHeight. https://codereview.chromium.org/2334133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:929: if ((sizingData.sizingOperation == IntrinsicSizeComputation) || (direction == ForRows && !sizingData.hasDefiniteLogicalHeight())) { On 2016/09/14 10:29:37, jfernandez wrote: > If I understood it correctly, you have passed 'false' as 'definiteLogicalHeight" > argument value when instantiating the SizingData structure for intrinsic size > computation. Since being under intrinsic size computation implies that height is > indefinite, wouldn't be better to just add an assertion here and avoid the > redundant condition ? The possibilities are: * IntrinsicSizeComputation: Both width and height are considered indefinite (the first condition of the if). * TrackSizing: Width is always definite in that step. Height can be definite or indefinite, so we need to check it. So it's not redundant, if we're in IntrinsicSizeComputation phase, the first part of the if is true and we consider the size as indefinite. But we need the other part of the if for the TrackSizing phase.
Description was changed from ========== [css-grid] Refactoring definite height detection This is a refactoring patch which stores the definite/indefinite height detection in GridSizingData. That way we just only call LayoutBlock::hasDefiniteLogicalHeight() once, from LayoutGrid::layoutBlock(). Then in LayoutGrid::gridTrackSize() we reuse the stored value. No new tests, no change of behavior. BUG=624301 ========== to ========== [css-grid] Refactoring definite height detection This is a refactoring patch which stores the definite/indefinite height detection in a new attribute m_hasDefiniteLogicalHeight in LayoutGrid. That way we just only call LayoutBlock::hasDefiniteLogicalHeight() once, from LayoutGrid::layoutBlock(). Then in LayoutGrid::gridTrackSize() we reuse the stored value. No new tests, no change of behavior. BUG=624301 ==========
lgtm https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:456: m_hasDefiniteLogicalHeight = LayoutBlock::hasDefiniteLogicalHeight(); As commented before, we shouldn't need to specify the superclass if we name differently both methods. https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:223: bool hasDefiniteLogicalHeight() const This method has the same name than the one defined in LayoutBox class, but we are not override it; we don't do it because both methods are of a different nature, indeed. I'd rather use a different name for this getter method, to avoid confusions and the need of specifying the class on invocation.
lgtm
lgtm with some changes https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:456: m_hasDefiniteLogicalHeight = LayoutBlock::hasDefiniteLogicalHeight(); On 2016/09/15 07:27:11, jfernandez wrote: > As commented before, we shouldn't need to specify the superclass if we name > differently both methods. I would not initialize it this way, see my comment in the header file. https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:223: bool hasDefiniteLogicalHeight() const On 2016/09/15 07:27:11, jfernandez wrote: > This method has the same name than the one defined in LayoutBox class, but we > are not override it; we don't do it because both methods are of a different > nature, indeed. I'd rather use a different name for this getter method, to avoid > confusions and the need of specifying the class on invocation. Agree, do not use the same name, someone could be tempted to add an override and then we'd be ... you know :) Also let's not add multiline implementations in the class definition, they make it much more difficult to understand. https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:226: return m_hasDefiniteLogicalHeight.value(); For caching like methods I think it's preferred some other form if (!m_hasDefiniteLogicalHeight) m_hasDefiniteLogicalHeight = hasDefiniteLogicalHeight(); return m_hasDefiniteLogicalHeight.value();
BTW shouldn't the title be "Cache definite height detection" instead of "Refactoring" (also "Refactoring of" if we don't want to use cache. https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:226: return m_hasDefiniteLogicalHeight.value(); On 2016/09/15 08:02:22, svillar wrote: > For caching like methods I think it's preferred some other form > > if (!m_hasDefiniteLogicalHeight) > m_hasDefiniteLogicalHeight = hasDefiniteLogicalHeight(); > return m_hasDefiniteLogicalHeight.value(); Beh, forget about this. We need to call it on every layout so what I was suggesting is wrong.
Description was changed from ========== [css-grid] Refactoring definite height detection This is a refactoring patch which stores the definite/indefinite height detection in a new attribute m_hasDefiniteLogicalHeight in LayoutGrid. That way we just only call LayoutBlock::hasDefiniteLogicalHeight() once, from LayoutGrid::layoutBlock(). Then in LayoutGrid::gridTrackSize() we reuse the stored value. No new tests, no change of behavior. BUG=624301 ========== to ========== [css-grid] Cache definite height detection This is a refactoring patch which stores the definite/indefinite height detection in a new attribute m_hasDefiniteLogicalHeight in LayoutGrid. That way we just only call LayoutBlock::hasDefiniteLogicalHeight() once, from LayoutGrid::layoutBlock(). Then in LayoutGrid::gridTrackSize() we reuse the cached value. No new tests, no change of behavior. BUG=624301 ==========
Thanks for the reviews. On 2016/09/15 08:16:00, svillar wrote: > BTW shouldn't the title be "Cache definite height detection" instead of > "Refactoring" (also "Refactoring of" if we don't want to use cache. Yeah, description updated.
https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2334133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:223: bool hasDefiniteLogicalHeight() const On 2016/09/15 08:02:22, svillar wrote: > On 2016/09/15 07:27:11, jfernandez wrote: > > This method has the same name than the one defined in LayoutBox class, but we > > are not override it; we don't do it because both methods are of a different > > nature, indeed. I'd rather use a different name for this getter method, to > avoid > > confusions and the need of specifying the class on invocation. > > Agree, do not use the same name, someone could be tempted to add an override and > then we'd be ... you know :) True, I'll change the name to cachedHasDefiniteLogicalHeight(). > Also let's not add multiline implementations in the class definition, they make > it much more difficult to understand. Done.
The CQ bit was checked by rego@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com, jfernandez@igalia.com Link to the patchset: https://codereview.chromium.org/2334133002/#ps40001 (title: "New version applying suggested changes")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rego@igalia.com
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.
Description was changed from ========== [css-grid] Cache definite height detection This is a refactoring patch which stores the definite/indefinite height detection in a new attribute m_hasDefiniteLogicalHeight in LayoutGrid. That way we just only call LayoutBlock::hasDefiniteLogicalHeight() once, from LayoutGrid::layoutBlock(). Then in LayoutGrid::gridTrackSize() we reuse the cached value. No new tests, no change of behavior. BUG=624301 ========== to ========== [css-grid] Cache definite height detection This is a refactoring patch which stores the definite/indefinite height detection in a new attribute m_hasDefiniteLogicalHeight in LayoutGrid. That way we just only call LayoutBlock::hasDefiniteLogicalHeight() once, from LayoutGrid::layoutBlock(). Then in LayoutGrid::gridTrackSize() we reuse the cached value. No new tests, no change of behavior. BUG=624301 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Cache definite height detection This is a refactoring patch which stores the definite/indefinite height detection in a new attribute m_hasDefiniteLogicalHeight in LayoutGrid. That way we just only call LayoutBlock::hasDefiniteLogicalHeight() once, from LayoutGrid::layoutBlock(). Then in LayoutGrid::gridTrackSize() we reuse the cached value. No new tests, no change of behavior. BUG=624301 ========== to ========== [css-grid] Cache definite height detection This is a refactoring patch which stores the definite/indefinite height detection in a new attribute m_hasDefiniteLogicalHeight in LayoutGrid. That way we just only call LayoutBlock::hasDefiniteLogicalHeight() once, from LayoutGrid::layoutBlock(). Then in LayoutGrid::gridTrackSize() we reuse the cached value. No new tests, no change of behavior. BUG=624301 Committed: https://crrev.com/e09a66db87cd70ecbd3b045464bf10ef623a03f9 Cr-Commit-Position: refs/heads/master@{#418831} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e09a66db87cd70ecbd3b045464bf10ef623a03f9 Cr-Commit-Position: refs/heads/master@{#418831} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
