Bug 165654 – [css-grid] Move Grid into GridSizingData

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

[Sergio Villar Senin]

[css-grid] Move Grid into GridSizingData

[Sergio Villar Senin]

Created attachment 296653 [details]
Patch

[Darin Adler]

/Volumes/Data/EWS/WebKit/Source/WebCore/rendering/RenderGrid.cpp:1972:13: error: use of undeclared identifier 'grid'
    ASSERT(!grid.needsItemsPlacement());

[Sergio Villar Senin]

Created attachment 296902 [details]
Patch

Fixed a typo

[Build Bot]

Comment on attachment 296902 [details]
Patch

Attachment 296902 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2703814

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 296906 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Sergio Villar Senin]

Created attachment 296912 [details]
Patch

Fixed a failing ASSERT

[Build Bot]

Comment on attachment 296912 [details]
Patch

Attachment 296912 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2705028

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 296915 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Sergio Villar Senin]

Created attachment 297000 [details]
Patch

Temporarily commented some ASSERTs that are not valid yet

[Javier Fernandez]

Comment on attachment 297000 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297000&action=review

The change looks fine to me.

> Source/WebCore/rendering/RenderGrid.cpp:1973
> +    // ASSERT(!m_grid.needsItemsPlacement());

Wouldn't be clearer to assert whether "items placement" is completed ? instead of this negative clause.

> Source/WebCore/rendering/RenderGrid.cpp:2279
> +    Grid& grid = sizingData.grid();

const ?

> Source/WebCore/rendering/RenderGrid.cpp:2796
> +    ASSERT(!m_grid.needsItemsPlacement());

ditto.
Why we don't need to comment this assert out ?

> Source/WebCore/rendering/RenderGrid.cpp:2807
> +    // ASSERT(!m_grid.needsItemsPlacement());

Ditto.

[Sergio Villar Senin]

Comment on attachment 297000 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297000&action=review

>> Source/WebCore/rendering/RenderGrid.cpp:1973
>> +    // ASSERT(!m_grid.needsItemsPlacement());
> 
> Wouldn't be clearer to assert whether "items placement" is completed ? instead of this negative clause.

Do you suggest to add another method? We can do it but it'd be redundant IMHO. I added this because it looks familiar (it's like needsLayout()).

>> Source/WebCore/rendering/RenderGrid.cpp:2279
>> +    Grid& grid = sizingData.grid();
> 
> const ?

ok

>> Source/WebCore/rendering/RenderGrid.cpp:2796
>> +    ASSERT(!m_grid.needsItemsPlacement());
> 
> ditto.
> Why we don't need to comment this assert out ?

Because all the callers of this function are executed during layout before clearing the grid. That's the key, we're sure we have completed the items placement and we're also sure that we're performing a layout.

>> Source/WebCore/rendering/RenderGrid.cpp:2807
>> +    // ASSERT(!m_grid.needsItemsPlacement());
> 
> Ditto.

In this case the ASSERT is not always true because we clear the grid after the layout. As the comment says, once the m_grid becomes persistent then the ASSERT will become valid.

[Sergio Villar Senin]

Committed r210197: <http://trac.webkit.org/changeset/210197>

[Michael Catanzaro]

Please be sure to use the webkit-patch script to land changes, so you don't forget to update the dates in the changelog. ;)