[css-grid] Remove the x2 computation of row sizes with indefinite heights
Created attachment 289228 [details] Patch
Comment on attachment 289228 [details] Patch Attachment 289228 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2106089 New failing tests: fast/css-grid-layout/nested-grid.html
Created attachment 289233 [details] Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 289228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289228&action=review > Source/WebCore/rendering/RenderGrid.cpp:479 > + bool hasDefiniteLogicalHeight = hasOverrideLogicalContentHeight() || computeContentLogicalHeight(MainOrPreferredSize, style().logicalHeight(), Nullopt); I guess the hasOverrideLogicalContentHeight() is intended for the "nested grid" case, right ? In the case of the grid, as item, being stretched. I wonder whether computeContentLogicalheight should handle that case by itself, though.
Comment on attachment 289228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289228&action=review >> Source/WebCore/rendering/RenderGrid.cpp:479 >> + bool hasDefiniteLogicalHeight = hasOverrideLogicalContentHeight() || computeContentLogicalHeight(MainOrPreferredSize, style().logicalHeight(), Nullopt); > > I guess the hasOverrideLogicalContentHeight() is intended for the "nested grid" case, right ? In the case of the grid, as item, being stretched. I wonder whether computeContentLogicalheight should handle that case by itself, though. As mentioned in the above FIXME it should be really handled by hasDefiniteLogicalHeight(). The problem is that that method needs to be reworked as it fails in many cases, like with positioned, replaced or objects with override sizes like grid items.
(In reply to comment #2) > Comment on attachment 289228 [details] > Patch > > Attachment 289228 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: http://webkit-queues.webkit.org/results/2106089 > > New failing tests: > fast/css-grid-layout/nested-grid.html So there is a 0.02% difference between both results, I guess that should be considered equal. Anyway I've just realized that I was adding a reftest and that the contents do not fit inside the viewport without having a vertical scrollbar. This means that the results on the bottom won't be ever considered. I'll rework the test so it fits.
Created attachment 289343 [details] Patch
Comment on attachment 289343 [details] Patch The changes look good to me. Since we have fixed the bug that caused some test cases to FAIL, shouldn't we remove the FIXME present in the html test ?
Comment on attachment 289343 [details] Patch Attachment 289343 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2112603 New failing tests: fast/css-grid-layout/nested-grid.html
Created attachment 289350 [details] Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 289359 [details] Patch with improved reftest
Comment on attachment 289359 [details] Patch with improved reftest I still fail to see the removal of the FIXME in the percent-track-breadths-regarding-container-size.html test.
Created attachment 289450 [details] Patch I totally overlooked your comment. Here you are the FIXME removal
Comment on attachment 289450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289450&action=review > Source/WebCore/rendering/RenderGrid.cpp:478 > + // FIXME: We should use RenderBlock::hasDefiniteLogicalHeight() but it does not work for positioned stuff > + // FIXME: consider caching the hasDefiniteLogicalHeight value throughout the layout Most WebKit comments are supposed to be sentence style, with capital letters at the start and periods at the end.
Committed r206253: <http://trac.webkit.org/changeset/206253>