Bug 165250 – [css-grid] Pass Grid as argument to items' placement methods

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

[Sergio Villar Senin]

[css-grid] Pass Grid as argument to items' placement methods

[Sergio Villar Senin]

Created attachment 295835 [details]
Patch

[Sergio Villar Senin]

Created attachment 295845 [details]
Patch

[Build Bot]

Comment on attachment 295845 [details]
Patch

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

New failing tests:
imported/blink/fast/css-grid-layout/grid-item-before-anonymous-child-crash.html
fast/css-grid-layout/grid-columns-rows-get-set-multiple.html
fast/css-grid-layout/grid-only-abspos-item-computed-style-crash.html
fast/css-grid-layout/grid-columns-rows-get-set.html
fast/css-grid-layout/named-grid-line-get-set.html
fast/css-grid-layout/grid-auto-columns-rows-get-set.html
fast/css-grid-layout/mark-as-infinitely-growable.html
fast/css-grid-layout/grid-template-shorthand-get-set.html
fast/css-grid-layout/grid-shorthand-get-set.html

[Build Bot]

Created attachment 295850 [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 295855 [details]
Patch

[Sergio Villar Senin]

Ping reviewers

[Darin Adler]

Comment on attachment 295855 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:156
> +    m_needsItemsPlacement = needsItemsPlacement;
> +
> +    if (needsItemsPlacement)
> +        clear();

Do we need to call clear if m_needsItemsPlacement is already true? I assume it does no harm doing an extra clear if so.

> Source/WebCore/rendering/RenderGrid.cpp:1735
> +    for (RenderBox* child = grid.orderIterator().first(); child; child = grid.orderIterator().next()) {

I suggest using auto* here since we would want to use the most specific type that the order iterator can give us, not specifically RenderBox*, although I am sure the two are one and the same at this time.

> Source/WebCore/rendering/RenderGrid.cpp:1778
> +    for (RenderBox* child = grid.orderIterator().first(); child; child = grid.orderIterator().next()) {

Ditto.

[Sergio Villar Senin]

Comment on attachment 295855 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/rendering/RenderGrid.cpp:156
>> +        clear();
> 
> Do we need to call clear if m_needsItemsPlacement is already true? I assume it does no harm doing an extra clear if so.

Right, there is no harm doing that extra clear.

[Sergio Villar Senin]

Committed r209601: <http://trac.webkit.org/changeset/209601>