Bug 110633

This is being addressed in https://codereview.chromium.org/196943026/ and should be ported to WebKit.

Actually there're some related changes in Blink that will be ported, all of the linked from the following bug: http://code.google.com/p/chromium/issues/detail?id=353789

Created attachment 233361 [details] Patch

Created attachment 233690 [details] Patch Rebased patch after r170182.

Comment on attachment 233690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233690&action=review Awesome job. Please fix the nits before landing. > Source/WebCore/ChangeLog:44 > + (WebCore::RenderGrid::growGrid): Deleted. Renamed to ensureGridSize(). Nit no need for Deleted. It's more than enough with Renamed. > Source/WebCore/rendering/RenderGrid.cpp:120 > + bool checkEmptyCells(size_t rowSpan, size_t columnSpan) const I don't like the function name, in this case it should be something like hasEnoughRoomForArea() or isEmptyAreaEnoughForItem() for example. Function name should start with isXXX() or hasXXX() but definitely not checkXXX(). > Source/WebCore/rendering/RenderGrid.cpp:124 > + size_t maxColumns = std::min(m_columnIndex + columnSpan, m_grid[0].size()); Better use gridColumnCount() and gridRowCount(). > Source/WebCore/rendering/RenderGrid.cpp:149 > const size_t endOfVaryingTrackIndex = (m_direction == ForColumns) ? m_grid.size() : m_grid[0].size(); I know it was not modified by this patch but use gridColumnCount() and gridRowCount() also here. > Source/WebCore/rendering/RenderGrid.cpp:635 > + const size_t oldRowSize = gridRowCount(); Let's use oldColumnCount and oldRowCount for consistency. > Source/WebCore/rendering/style/GridResolvedPosition.cpp:65 > + Nit: extra line here. > LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:19 > + background-color: maroon; I'm sure it was useful for testing :) but let's remove the background colors as they're not needed by the test. > LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:11 > + background-color: maroon; Same comment here about colors.

(In reply to comment #5) > (From update of attachment 233690 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=233690&action=review > > Awesome job. Please fix the nits before landing. Thanks for the review! > > Source/WebCore/ChangeLog:44 > > + (WebCore::RenderGrid::growGrid): Deleted. Renamed to ensureGridSize(). > > Nit no need for Deleted. It's more than enough with Renamed. Fixed. > > Source/WebCore/rendering/RenderGrid.cpp:120 > > + bool checkEmptyCells(size_t rowSpan, size_t columnSpan) const > > I don't like the function name, in this case it should be something like hasEnoughRoomForArea() or isEmptyAreaEnoughForItem() for example. Function name should start with isXXX() or hasXXX() but definitely not checkXXX(). I've modified it, thanks for the suggestions. > > Source/WebCore/rendering/RenderGrid.cpp:124 > > + size_t maxColumns = std::min(m_columnIndex + columnSpan, m_grid[0].size()); > > Better use gridColumnCount() and gridRowCount(). We cannot use them from GridIterator, as we only have the m_grid but not access to the RenderGrid. Probably it would be nice to explore other options here (like use a friend class). > > Source/WebCore/rendering/RenderGrid.cpp:149 > > const size_t endOfVaryingTrackIndex = (m_direction == ForColumns) ? m_grid.size() : m_grid[0].size(); > > I know it was not modified by this patch but use gridColumnCount() and gridRowCount() also here. Ditto. > > Source/WebCore/rendering/RenderGrid.cpp:635 > > + const size_t oldRowSize = gridRowCount(); > > Let's use oldColumnCount and oldRowCount for consistency. Modified. > > Source/WebCore/rendering/style/GridResolvedPosition.cpp:65 > > + > > Nit: extra line here. Removed. > > LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:19 > > + background-color: maroon; > > I'm sure it was useful for testing :) but let's remove the background colors as they're not needed by the test. As we've discussed I prefer to keep them. When you run the test manually with run-launcher it makes much easier to understand it. In addition, we're already using background colors in lots of places in grid tests. > > LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:11 > > + background-color: maroon; > > Same comment here about colors. Ditto.

Created attachment 233988 [details] Patch for landing

Comment on attachment 233988 [details] Patch for landing Clearing flags on attachment: 233988 Committed r170531: <http://trac.webkit.org/changeset/170531>

All reviewed patches have been landed. Closing bug.