Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2287533002:
[css-grid] Remove a duplicated auto repeat computation for intrinsic sizes (Closed)
|
Created:
4 years, 3 months ago by svillar Modified:
4 years, 1 month ago CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Remove a duplicated auto repeat computation for intrinsic sizes
After crrev.com/414446 we can safely remove the temporary fix
(crrev.com/405529) we had to avoid crashes and recursive calls when
computing the auto repeat tracks count for grids with intrinsic sizes.
This change unveiled a couple of bugs in the computation of auto repeat
tracks for intrinsic sizes. The first one is that we were not considering the
existence of definite minimum sizes. In those cases, instead of just returning 1
repetition, we should return the minimum number of repetitions that fulfill the
minimum size requirement.
The second bug was that grids were not properly recomputing the number of
auto repeat tracks when the min|max-content contributions of grid items
changed (whenever the grid container had an intrinsic width).
One Mozilla test had to be updated too because it was wrong.
BUG=621517, 633474
Committed: https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6
Cr-Commit-Position: refs/heads/master@{#425958}
Patch Set 1 #Patch Set 2 : Patch #
Total comments: 9
Patch Set 3 : Use const_cast #Patch Set 4 : Rebased patch #
Messages
Total messages: 33 (11 generated)
svillar@igalia.com changed reviewers: + cbiesinger@chromium.org, jfernandez@igalia.com, rego@igalia.com
> master@{#414446}
I would suggest entering those as crrev.com/414446 so they're easier to map to a
commit
But I have a question about the change in general. Doesn't intrinsic size
computation still change the object state?
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
calls placeItemsOnGrid, which changes a number of variables and sets
m_gridIsDirty to false. Why is that not an issue anymore?
Description was changed from
==========
[css-grid] Remove a duplicated auto repeat computation for intrinsic sizes
After master@{#414446} we can safely remove the temporary fix
(master@{#405529}) we had to avoid crashes and recursive calls when
computing the auto repeat tracks count for grids with intrinsic sizes.
This change unveiled a bug in the computation of auto repeat tracks for
intrinsic sizes as we were not considering the existence of definite minimum
sizes. In those cases, instead of just returning 1 repetition, we should
return the minimum number of repetitions that fulfill the minimum size
requirement.
==========
to
==========
[css-grid] Remove a duplicated auto repeat computation for intrinsic sizes
After crrev.com/414446 we can safely remove the temporary fix
(crrev.com/405529) we had to avoid crashes and recursive calls when
computing the auto repeat tracks count for grids with intrinsic sizes.
This change unveiled a bug in the computation of auto repeat tracks for
intrinsic sizes as we were not considering the existence of definite minimum
sizes. In those cases, instead of just returning 1 repetition, we should
return the minimum number of repetitions that fulfill the minimum size
requirement.
==========
On 2016/08/26 20:59:14, cbiesinger wrote:
> > master@{#414446}
>
> I would suggest entering those as crrev.com/414446 so they're easier to map to
a
> commit
Done
> But I have a question about the change in general. Doesn't intrinsic size
> computation still change the object state?
>
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
> calls placeItemsOnGrid, which changes a number of variables and sets
> m_gridIsDirty to false. Why is that not an issue anymore?
Good question. I think the problem in this case were inaccurate comments (and a
previous incomplete analysis). I've checked the commit message of
crrev.com/405529 and it's partially wrong because it talks about adding mutable
attributes which was not the case (that patch was reworked a couple of times but
I forgot to update the commit message).
This patch should be really part of crrev.com/414446 which is the one fixing the
underlying issue. The intrinsic size computation changing the internal state of
the object was not the actual issue but a (bad) side effect. The actual problem
was that we were making recursive calls during the intrinsic size computation,
we were just hitting the asserts before overflowing the stack.
The patch LGTM but let's wait for @cbiesinger feedback. BTW, I think you need to add something in the description regarding the Mozilla test you're modifying. Also, it might be useful to link to the related bugs for future reference: BUG=621517,633474
Description was changed from ========== [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes After crrev.com/414446 we can safely remove the temporary fix (crrev.com/405529) we had to avoid crashes and recursive calls when computing the auto repeat tracks count for grids with intrinsic sizes. This change unveiled a bug in the computation of auto repeat tracks for intrinsic sizes as we were not considering the existence of definite minimum sizes. In those cases, instead of just returning 1 repetition, we should return the minimum number of repetitions that fulfill the minimum size requirement. ========== to ========== [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes After crrev.com/414446 we can safely remove the temporary fix (crrev.com/405529) we had to avoid crashes and recursive calls when computing the auto repeat tracks count for grids with intrinsic sizes. This change unveiled a bug in the computation of auto repeat tracks for intrinsic sizes as we were not considering the existence of definite minimum sizes. In those cases, instead of just returning 1 repetition, we should return the minimum number of repetitions that fulfill the minimum size requirement. One Mozilla test had to be updated too because it was wrong. BUG=621517,633474 ==========
Thanks for the explanation, but I'm still unclear why it is safe to remove the dirtyGrid call. Don't we have a potentially different grid during actual layout than during intrinsic size computations? Certainly, available width could be different which affects computeAutoRepeatTracksCount.
On 2016/08/29 20:16:36, cbiesinger wrote: > Thanks for the explanation, but I'm still unclear why it is safe to remove the > dirtyGrid call. Don't we have a potentially different grid during actual layout > than during intrinsic size computations? Certainly, available width could be > different which affects computeAutoRepeatTracksCount. Oh you're right. I've forgotten that the place items step is not performed if the grid is not dirty. I'll double check the current tests because it's weird that none of them is failing...
Description was changed from ========== [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes After crrev.com/414446 we can safely remove the temporary fix (crrev.com/405529) we had to avoid crashes and recursive calls when computing the auto repeat tracks count for grids with intrinsic sizes. This change unveiled a bug in the computation of auto repeat tracks for intrinsic sizes as we were not considering the existence of definite minimum sizes. In those cases, instead of just returning 1 repetition, we should return the minimum number of repetitions that fulfill the minimum size requirement. One Mozilla test had to be updated too because it was wrong. BUG=621517,633474 ========== to ========== [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes After crrev.com/414446 we can safely remove the temporary fix (crrev.com/405529) we had to avoid crashes and recursive calls when computing the auto repeat tracks count for grids with intrinsic sizes. This change unveiled a couple of bugs in the computation of auto repeat tracks for intrinsic sizes. The first one is that we were not considering the existence of definite minimum sizes. In those cases, instead of just returning 1 repetition, we should return the minimum number of repetitions that fulfill the minimum size requirement. The second bug was that grids were not properly recomputing the number of auto repeat tracks when the min|max-content contributions of grid items changed (whenever the grid container had an intrinsic width). One Mozilla test had to be updated too because it was wrong. BUG=621517,633474 ==========
ping reviewers
Sorry for the delay, I hadn't realized that you updated a new version https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (left): https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1559: m_autoRepeatColumns = styleRef().gridAutoRepeatColumns().size(); (...as compared to this one) https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:602: const_cast<LayoutGrid*>(this)->placeItemsOnGrid(); You still have a const_cast here. Instead of making dirtyGrid and updateAutoRepeatTracksAndSetDirtyIfNeeded const functions even though they really aren't, why not just use a const_cast for both of these function calls? https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1443: size_t newAutoRepeatColumns = computeAutoRepeatTracksCount(ForColumns, sizingOperation); Why are you computing this differently now, if sizingOperation==Intrinsic?
Hope I have solved your doubts https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:602: const_cast<LayoutGrid*>(this)->placeItemsOnGrid(); On 2016/10/05 22:22:13, cbiesinger wrote: > You still have a const_cast here. Instead of making dirtyGrid and > updateAutoRepeatTracksAndSetDirtyIfNeeded const functions even though they > really aren't, why not just use a const_cast for both of these function calls? I personally dislike those const_cast a lot, actually I'm planning to remove the placeItemsOnGrid() one. I think those two functions you mention are semantically const even though technically they modify the object. The data we are clearing/modifying in those methods is cached data used to improve the performance of the layout, that's why I made them all mutable, it makes sense to modify them in some const methods. https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1443: size_t newAutoRepeatColumns = computeAutoRepeatTracksCount(ForColumns, sizingOperation); On 2016/10/05 22:22:13, cbiesinger wrote: > Why are you computing this differently now, if sizingOperation==Intrinsic? Actually I'm going back to the original implementation, we made it different to fix a recursive call to layout. The computeAutoRepeatTracksCount() already handles the case of computing repetitions during intrinsic size computation (i.e. computing sizes with indefinite sizes) so we can use it directly. For most of the cases both implementations are equivalent, but the new code (which is the original one as mentioned) properly handles some situations that were not working correctly right now (like for example indefinite sizes with an specified minimum size).
Now that @svillar clarified @cbiesinger questions, the patch LGTM.
Sorry but I've doubts about this patch. https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:61: void dirtyGrid() const; I still don't get why we want to make this method const. It seems really confusing to me, as dirtyGrid() is actually changing the grid. Why insertItemIntoGrid() is not const then? It only modifies m_grid which is mutable now. https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:230: Vector<LayoutUnit> m_rowPositions; Why some attributes are mutables and other don't? What's the reasoning behind? For example m_rowPositions is also a value calculated during layout, like the rest of attributes in the class. Why do we just mark a few as "mutable" and not all?
https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:61: void dirtyGrid() const; On 2016/10/06 12:31:30, Manuel Rego wrote: > I still don't get why we want to make this method const. Because we want to use it in the intrinsic size computation method which is const > It seems really confusing to me, as dirtyGrid() is actually changing the grid. It is not semantically. It only touches cached data, i.e. attributes that are not mandatory for the layout object but that ease the implementation/make it faster. The problem is perhaps the name. I wanted to rename it to clearCachedData() but I left it for another patch. https://codereview.chromium.org/2287533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:230: Vector<LayoutUnit> m_rowPositions; On 2016/10/06 12:31:30, Manuel Rego wrote: > Why some attributes are mutables and other don't? > What's the reasoning behind? Why do you want to make them all mutable or none? What's the rationale of that? > For example m_rowPositions is also a value calculated during layout, > like the rest of attributes in the class. > Why do we just mark a few as "mutable" and not all? This totally depends on what you consider is part of the state of the object and what is not. For me there are some attributes that are really cached data (like m_gridItemsOverflowingGridArea), i.e. attributes that are not mandatory but that help. Some others are subject to further discussion I agree. This is the minimal subset that allows us to have a const dirtyGrid().
Yep, most of the attributes on LayoutGrid could be considered cached ones. I don't have any good alternative to this patch, I don't like the mutable attributes, but I agree that the const_cast is awful too, so I'll let other people review it.
Just uploaded a new version with a const_cast which seems to be the preferred option of both Chris and Manuel
I also don't love the const_cast but this change seems less intrusive to me. LGTM.
lgtm
The CQ bit was checked by svillar@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from jfernandez@igalia.com Link to the patchset: https://codereview.chromium.org/2287533002/#ps40001 (title: "Use const_cast")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by svillar@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, jfernandez@igalia.com, rego@igalia.com Link to the patchset: https://codereview.chromium.org/2287533002/#ps60001 (title: "Rebased patch")
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] Remove a duplicated auto repeat computation for intrinsic sizes After crrev.com/414446 we can safely remove the temporary fix (crrev.com/405529) we had to avoid crashes and recursive calls when computing the auto repeat tracks count for grids with intrinsic sizes. This change unveiled a couple of bugs in the computation of auto repeat tracks for intrinsic sizes. The first one is that we were not considering the existence of definite minimum sizes. In those cases, instead of just returning 1 repetition, we should return the minimum number of repetitions that fulfill the minimum size requirement. The second bug was that grids were not properly recomputing the number of auto repeat tracks when the min|max-content contributions of grid items changed (whenever the grid container had an intrinsic width). One Mozilla test had to be updated too because it was wrong. BUG=621517,633474 ========== to ========== [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes After crrev.com/414446 we can safely remove the temporary fix (crrev.com/405529) we had to avoid crashes and recursive calls when computing the auto repeat tracks count for grids with intrinsic sizes. This change unveiled a couple of bugs in the computation of auto repeat tracks for intrinsic sizes. The first one is that we were not considering the existence of definite minimum sizes. In those cases, instead of just returning 1 repetition, we should return the minimum number of repetitions that fulfill the minimum size requirement. The second bug was that grids were not properly recomputing the number of auto repeat tracks when the min|max-content contributions of grid items changed (whenever the grid container had an intrinsic width). One Mozilla test had to be updated too because it was wrong. BUG=621517,633474 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes After crrev.com/414446 we can safely remove the temporary fix (crrev.com/405529) we had to avoid crashes and recursive calls when computing the auto repeat tracks count for grids with intrinsic sizes. This change unveiled a couple of bugs in the computation of auto repeat tracks for intrinsic sizes. The first one is that we were not considering the existence of definite minimum sizes. In those cases, instead of just returning 1 repetition, we should return the minimum number of repetitions that fulfill the minimum size requirement. The second bug was that grids were not properly recomputing the number of auto repeat tracks when the min|max-content contributions of grid items changed (whenever the grid container had an intrinsic width). One Mozilla test had to be updated too because it was wrong. BUG=621517,633474 ========== to ========== [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes After crrev.com/414446 we can safely remove the temporary fix (crrev.com/405529) we had to avoid crashes and recursive calls when computing the auto repeat tracks count for grids with intrinsic sizes. This change unveiled a couple of bugs in the computation of auto repeat tracks for intrinsic sizes. The first one is that we were not considering the existence of definite minimum sizes. In those cases, instead of just returning 1 repetition, we should return the minimum number of repetitions that fulfill the minimum size requirement. The second bug was that grids were not properly recomputing the number of auto repeat tracks when the min|max-content contributions of grid items changed (whenever the grid container had an intrinsic width). One Mozilla test had to be updated too because it was wrong. BUG=621517,633474 Committed: https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6 Cr-Commit-Position: refs/heads/master@{#425958} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6 Cr-Commit-Position: refs/heads/master@{#425958}
Message was sent while issue was closed.
We're going to revert this on https://codereview.chromium.org/2510393002/ Due to https://bugs.chromium.org/p/chromium/issues/detail?id=662016 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
