Bug 162150 – [css-grid] Remove the x2 computation of row sizes with indefinite heights

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[css-grid] Remove the x2 computation of row sizes with indefinite heights

[Sergio Villar Senin]

Created attachment 289228 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Javier Fernandez]

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.

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

Created attachment 289343 [details]
Patch

[Javier Fernandez]

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 ?

[Build Bot]

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

[Build Bot]

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

[Sergio Villar Senin]

Created attachment 289359 [details]
Patch with improved reftest

[Javier Fernandez]

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.

[Sergio Villar Senin]

Created attachment 289450 [details]
Patch

I totally overlooked your comment. Here you are the FIXME removal

[Darin Adler]

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.

[Sergio Villar Senin]

Committed r206253: <http://trac.webkit.org/changeset/206253>