It might make sense to replace the use of -1 with WTF::nullopt in grid item
Created attachment 421441 [details] Patch
Comment on attachment 421441 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421441&action=review I bealive there are other functions using -1 as indefinite; I think this patch should address all of them. See GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChild for instance. > Source/WebCore/ChangeLog:7 > + We should explain for each change why we need nullopt instead; those functions return an Optional type and we consider nullopt as indefinite, as we did before with the -1 value. It's also need to mention why this change doesn't need new tests, or changes on the already existing ones. In this case, since we don't expect a behavior change, we don't need to provide tests.
https://bugs.webkit.org/show_bug.cgi?id=221439 has addressed and fixed issue mentioned here. Maybe we can close this one?
<rdar://problem/74997975>
Created attachment 422556 [details] Patch
Created attachment 422559 [details] Patch
Comment on attachment 422559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422559&action=review Definitely a step in the right direction. There are some changes that need to be done though > Source/WebCore/ChangeLog:6 > + This change replaces -1 with nullopt in grid to indicate indefinte for funtions that nit: indefinite > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:912 > + overrideSize = WTF::nullopt; Instead of doing this, change estimatedGridAreaBreadthForChild() so that it returns an Optional<LayoutUnit> and make it return nullopt instead of -1_lu > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1064 > + overridingSize = WTF::nullopt; You can do it with a ternary operator: auto overridingSize = direction() == childInlineDirection ? makeOptional(0_lu) : WTF::nullopt; In that case maybe you don't even need the overridingSize variable and pass the value directly as argument to setOverriding... > Source/WebCore/rendering/RenderGrid.cpp:928 > } If we change estimatedGridAreaBreadthForChild() as I suggested above then we don't need this change.
Created attachment 422597 [details] Patch
(In reply to Sergio Villar Senin from comment #7) > Comment on attachment 422559 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=422559&action=review > > Definitely a step in the right direction. There are some changes that need > to be done though > > > Source/WebCore/ChangeLog:6 > > + This change replaces -1 with nullopt in grid to indicate indefinte for funtions that > > nit: indefinite > Corrected. > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:912 > > + overrideSize = WTF::nullopt; > > Instead of doing this, change estimatedGridAreaBreadthForChild() so that it > returns an Optional<LayoutUnit> and make it return nullopt instead of -1_lu > I tried this approach. The main issue is that LayoutUnit GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChild(const RenderBox& child, GridTrackSizingDirection direction) is called by LayoutSize GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChild(const RenderBox& child) const. LayoutSize doesn't like Optional<LayoutUnit>, especially when it's WTF::nullopt. I'm not sure how feasible this approach is. Any suggestions? > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1064 > > + overridingSize = WTF::nullopt; > > You can do it with a ternary operator: > auto overridingSize = direction() == childInlineDirection ? > makeOptional(0_lu) : WTF::nullopt; > > In that case maybe you don't even need the overridingSize variable and pass > the value directly as argument to setOverriding... > Updated. Thanks very much for the suggestion! > > Source/WebCore/rendering/RenderGrid.cpp:928 > > } > > If we change estimatedGridAreaBreadthForChild() as I suggested above then we > don't need this change.
Comment on attachment 422559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422559&action=review >>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:912 >>> + overrideSize = WTF::nullopt; >> >> Instead of doing this, change estimatedGridAreaBreadthForChild() so that it returns an Optional<LayoutUnit> and make it return nullopt instead of -1_lu > > I tried this approach. The main issue is that LayoutUnit GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChild(const RenderBox& child, GridTrackSizingDirection direction) is called by LayoutSize GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChild(const RenderBox& child) const. LayoutSize doesn't like Optional<LayoutUnit>, especially when it's WTF::nullopt. I'm not sure how feasible this approach is. Any suggestions? Umm, the only solution I see now is that we remove the estimatedGridAreaBreadthForChild(child), and the caller takes the responsibility of building the LayoutSize instance, based on the results of the function; this should also be the responsible of handling the nullopt. I'm not sure how big would be the change, though.
Created attachment 422684 [details] Patch
Comment on attachment 422684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422684&action=review > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:554 > +Optional<LayoutUnit> GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChildForColumns(const RenderBox& child) const Perhaps we don't need this function; we can just calling the generic function with the appropriated argument when needed. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:558 > +Optional<LayoutUnit> GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChildForRows(const RenderBox& child) const Ditto > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:839 > + return minLogicalSizeForChild(child, childMinSize, gridAreaSize.valueOr(0_lu)) + baselineShim; I'm not sure about this valurOr(0); it might be the root cause of the layout tests failures we have in the baseline alignment related tests. I think this might be wrong when dealing with orthogonal grid items, so that we need nullopt here to detect indefinite size.
Created attachment 422930 [details] Patch
Comment on attachment 422930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422930&action=review Seems like you have to update the failed expectations of a test that is failing. Looking great, I'm happy that we get rid of the -1's. It's a pitty that we cannot use LayoutSize and have to use width and height separatedely but it isn't a big deal. > Source/WebCore/ChangeLog:7 > + returns an Optional type. This change has exposed an issue when resolving repalced elemnt Nit: replaced element > Source/WebCore/ChangeLog:11 > + Extra line here. BTW this paragraph should go bellow the "Reviewed by" line
Created attachment 423023 [details] Patch
(In reply to Javier Fernandez from comment #12) > Comment on attachment 422684 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=422684&action=review > > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:554 > > +Optional<LayoutUnit> GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChildForColumns(const RenderBox& child) const > > Perhaps we don't need this function; we can just calling the generic > function with the appropriated argument when needed. > > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:558 > > +Optional<LayoutUnit> GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChildForRows(const RenderBox& child) const > > Ditto > > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:839 > > + return minLogicalSizeForChild(child, childMinSize, gridAreaSize.valueOr(0_lu)) + baselineShim; > > I'm not sure about this valurOr(0); it might be the root cause of the layout > tests failures we have in the baseline alignment related tests. > I think this might be wrong when dealing with orthogonal grid items, so that > we need nullopt here to detect indefinite size. Updated. Change the type to Optional.
(In reply to Sergio Villar Senin from comment #14) > Comment on attachment 422930 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=422930&action=review > > Seems like you have to update the failed expectations of a test that is > failing. > Updated. This test is fixing at https://bugs.webkit.org/show_bug.cgi?id=191627. > Looking great, I'm happy that we get rid of the -1's. It's a pitty that we > cannot use LayoutSize and have to use width and height separatedely but it > isn't a big deal. > > > Source/WebCore/ChangeLog:7 > > + returns an Optional type. This change has exposed an issue when resolving repalced elemnt > > Nit: replaced element > > > Source/WebCore/ChangeLog:11 > > + > > Extra line here. BTW this paragraph should go bellow the "Reviewed by" line Updated. Thank you!
Created attachment 423033 [details] Patch
Comment on attachment 423033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423033&action=review r=me > Source/WebCore/ChangeLog:8 > + This change replaces -1 with nullopt in grid to indicate indefinite for funtions that s/funtions/functions > Source/WebCore/ChangeLog:9 > + returns an Optional type. This change has exposed an issue when resolving replaced elemnt s/elemnt/element > Source/WebCore/rendering/RenderReplaced.cpp:357 > if (hasAutoHeightOrContainingBlockWithAutoHeight()) Nit: Maybe we can change this as well, something like this: return !hasAutoHeightOrContainingBlockWithAutoHeight()
Created attachment 423222 [details] Patch
(In reply to Javier Fernandez from comment #19) > Comment on attachment 423033 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423033&action=review > > r=me > > > Source/WebCore/ChangeLog:8 > > + This change replaces -1 with nullopt in grid to indicate indefinite for funtions that > > s/funtions/functions > > > Source/WebCore/ChangeLog:9 > > + returns an Optional type. This change has exposed an issue when resolving replaced elemnt > > s/elemnt/element > > > Source/WebCore/rendering/RenderReplaced.cpp:357 > > if (hasAutoHeightOrContainingBlockWithAutoHeight()) > > Nit: Maybe we can change this as well, something like this: > > return !hasAutoHeightOrContainingBlockWithAutoHeight() Updated. Thanks!
Committed r274477: <https://commits.webkit.org/r274477> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423222 [details].