Summary: | [css-grid] Move Grid into GridSizingData | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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, mcatanzaro, 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-09 08:35:55 PST
Created attachment 296653 [details]
Patch
/Volumes/Data/EWS/WebKit/Source/WebCore/rendering/RenderGrid.cpp:1972:13: error: use of undeclared identifier 'grid' ASSERT(!grid.needsItemsPlacement()); Created attachment 296902 [details]
Patch
Fixed a typo
Comment on attachment 296902 [details] Patch Attachment 296902 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2703814 Number of test failures exceeded the failure limit. Created attachment 296906 [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 296912 [details]
Patch
Fixed a failing ASSERT
Comment on attachment 296912 [details] Patch Attachment 296912 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2705028 Number of test failures exceeded the failure limit. Created attachment 296915 [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 297000 [details]
Patch
Temporarily commented some ASSERTs that are not valid yet
Comment on attachment 297000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297000&action=review The change looks fine to me. > Source/WebCore/rendering/RenderGrid.cpp:1973 > + // ASSERT(!m_grid.needsItemsPlacement()); Wouldn't be clearer to assert whether "items placement" is completed ? instead of this negative clause. > Source/WebCore/rendering/RenderGrid.cpp:2279 > + Grid& grid = sizingData.grid(); const ? > Source/WebCore/rendering/RenderGrid.cpp:2796 > + ASSERT(!m_grid.needsItemsPlacement()); ditto. Why we don't need to comment this assert out ? > Source/WebCore/rendering/RenderGrid.cpp:2807 > + // ASSERT(!m_grid.needsItemsPlacement()); Ditto. Comment on attachment 297000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297000&action=review >> Source/WebCore/rendering/RenderGrid.cpp:1973 >> + // ASSERT(!m_grid.needsItemsPlacement()); > > Wouldn't be clearer to assert whether "items placement" is completed ? instead of this negative clause. Do you suggest to add another method? We can do it but it'd be redundant IMHO. I added this because it looks familiar (it's like needsLayout()). >> Source/WebCore/rendering/RenderGrid.cpp:2279 >> + Grid& grid = sizingData.grid(); > > const ? ok >> Source/WebCore/rendering/RenderGrid.cpp:2796 >> + ASSERT(!m_grid.needsItemsPlacement()); > > ditto. > Why we don't need to comment this assert out ? Because all the callers of this function are executed during layout before clearing the grid. That's the key, we're sure we have completed the items placement and we're also sure that we're performing a layout. >> Source/WebCore/rendering/RenderGrid.cpp:2807 >> + // ASSERT(!m_grid.needsItemsPlacement()); > > Ditto. In this case the ASSERT is not always true because we clear the grid after the layout. As the comment says, once the m_grid becomes persistent then the ASSERT will become valid. Committed r210197: <http://trac.webkit.org/changeset/210197> Please be sure to use the webkit-patch script to land changes, so you don't forget to update the dates in the changelog. ;) |