Bug 165250

Summary: [css-grid] Pass Grid as argument to items' placement methods
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, glenn, jfernandez, kondapallykalyan, rego, saam, svillar, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 165007    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Patch darin: review+

Description Sergio Villar Senin 2016-12-01 04:18:30 PST
[css-grid] Pass Grid as argument to items' placement methods
Comment 1 Sergio Villar Senin 2016-12-01 04:28:11 PST
Created attachment 295835 [details]
Patch
Comment 2 Sergio Villar Senin 2016-12-01 06:25:11 PST
Created attachment 295845 [details]
Patch
Comment 3 Build Bot 2016-12-01 07:41:46 PST
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
Comment 4 Build Bot 2016-12-01 07:41:49 PST
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
Comment 5 Sergio Villar Senin 2016-12-01 09:42:20 PST
Created attachment 295855 [details]
Patch
Comment 6 Sergio Villar Senin 2016-12-08 00:56:47 PST
Ping reviewers
Comment 7 Darin Adler 2016-12-08 09:10:31 PST
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.
Comment 8 Sergio Villar Senin 2016-12-09 02:24:11 PST
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.
Comment 9 Sergio Villar Senin 2016-12-09 02:32:59 PST
Committed r209601: <http://trac.webkit.org/changeset/209601>