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+

Sergio Villar Senin
Reported 2016-12-01 04:18:30 PST
[css-grid] Pass Grid as argument to items' placement methods
Attachments
Patch (28.48 KB, patch)
2016-12-01 04:28 PST, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
Patch (28.96 KB, patch)
2016-12-01 06:25 PST, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
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+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 2016-12-01 04:28:11 PST
Created attachment 295835 [details] Patch
Sergio Villar Senin
Comment 2 2016-12-01 06:25:11 PST
Created attachment 295845 [details] Patch
Build Bot
Comment 3 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
Build Bot
Comment 4 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
Sergio Villar Senin
Comment 5 2016-12-01 09:42:20 PST
Created attachment 295855 [details] Patch
Sergio Villar Senin
Comment 6 2016-12-08 00:56:47 PST
Ping reviewers
Darin Adler
Comment 7 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.
Sergio Villar Senin
Comment 8 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.
Sergio Villar Senin
Comment 9 2016-12-09 02:32:59 PST
Committed r209601: <http://trac.webkit.org/changeset/209601>
Note You need to log in before you can comment on or make changes to this bug.