Summary: | [css-grid] Pass Grid as argument to items' placement methods | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||||||||
Component: | New Bugs | Assignee: | 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
Sergio Villar Senin
2016-12-01 04:18:30 PST
Created attachment 295835 [details]
Patch
Created attachment 295845 [details]
Patch
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 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
Created attachment 295855 [details]
Patch
Ping reviewers 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 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. Committed r209601: <http://trac.webkit.org/changeset/209601> |