Bug 165250 - [css-grid] Pass Grid as argument to items' placement methods
Summary: [css-grid] Pass Grid as argument to items' placement methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks: 165007
  Show dependency treegraph
 
Reported: 2016-12-01 04:18 PST by Sergio Villar Senin
Modified: 2016-12-09 02:32 PST (History)
10 users (show)

See Also:


Attachments
Patch (28.48 KB, patch)
2016-12-01 04:28 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (28.96 KB, patch)
2016-12-01 06:25 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-yosemite (2.40 MB, application/zip)
2016-12-01 07:41 PST, Build Bot
no flags Details
Patch (28.32 KB, patch)
2016-12-01 09:42 PST, Sergio Villar Senin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>