Bug 155197

Created attachment 279646 [details] Patch

Comment on attachment 279646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279646&action=review Really cool you're finally fixing this issue that have been around for a long time. I've some questions about the patch, as I think I'm not getting it 100% yet. > Source/WebCore/rendering/RenderGrid.cpp:150 > + ASSERT(!m_grid.isEmpty()); So we're not calling this if the m_grid is empty. Are we actually creating any GridIterator when m_grid is empty? If that's the case, shouldn't we just avoid it? What's the point of having a GridIterator if the grid is empty? I'm wondering if we could move the ASSERT to GridIterator constructor directly. > Source/WebCore/rendering/RenderGrid.cpp:335 > + return m_grid.size() ? m_grid[0].size() : GridPositionsResolver::explicitGridColumnCount(style(), m_autoRepeatColumns); In an empty grid, do we really need to get the size of the columns from style? Couldn't be enough just return "0" here? I might be missing something as I'm not getting this change. > Source/WebCore/rendering/RenderGrid.cpp:1529 > + m_gridIsDirty = true; Should we add an ASSERT to check that m_gridIsDirty is FALSE when clearGrid() is called? > Source/WebCore/rendering/RenderGrid.cpp:1773 > +void RenderGrid::populateGridPositionsForDirection(GridSizingData& sizingData, GridTrackSizingDirection direction) Nice refactoring, but I'd do it in a different patch as it's unrelated with the rest of things. > LayoutTests/fast/css-grid-layout/empty-grid-expected.html:4 > + position: absolute; Do you need this line? > LayoutTests/fast/css-grid-layout/empty-grid.html:3 > +.absGrid { The grid is not absolutely positioned, so the class name is misleading. > LayoutTests/fast/css-grid-layout/empty-grid.html:20 > + width: 100%; I'd use "height: 100%;" too to verify the behaviour on the other axis. > LayoutTests/fast/css-grid-layout/empty-grid.html:21 > + background: cyan; Nit: It's more common to use "green" or "lime" for the right result, rather than "cyan".

Comment on attachment 279646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279646&action=review >> Source/WebCore/rendering/RenderGrid.cpp:150 >> + ASSERT(!m_grid.isEmpty()); > > So we're not calling this if the m_grid is empty. > > Are we actually creating any GridIterator when m_grid is empty? > If that's the case, shouldn't we just avoid it? What's the point of having a GridIterator if the grid is empty? > > I'm wondering if we could move the ASSERT to GridIterator constructor directly. We have 2 options, we either check that the grid is empty every time before creating the GridIterator or we create it unconditionally and deal with the empty case. Since having an empty is grid is by far the most uncommon case I decided to optimize the normal case and did not add any extra check. >> Source/WebCore/rendering/RenderGrid.cpp:335 >> + return m_grid.size() ? m_grid[0].size() : GridPositionsResolver::explicitGridColumnCount(style(), m_autoRepeatColumns); > > In an empty grid, do we really need to get the size of the columns from style? > Couldn't be enough just return "0" here? > > I might be missing something as I'm not getting this change. Well that line is basically the main point of this change. The thing is that our grid representation is a Vector of rows. Each row is a Vector of columns, and each column is a Vector of RenderBoxes (the children of the grid). This means that in order to have at least a column we need to artificially create a row. What happens when you have a declaration like this "grid-template-columns: 10px;" ? If we do what you suggest you'll return 0 as the number of columns, but we actually have 1. Note that the else part of the ternary operator will only happen in the event of not having any children and not having a grid-template-rows declaration. >> Source/WebCore/rendering/RenderGrid.cpp:1529 >> + m_gridIsDirty = true; > > Should we add an ASSERT to check that m_gridIsDirty is FALSE when clearGrid() is called? I don't think we need it. We are not doing anything dangerous, and that would prevent us from calling clearGrid() twice, something not really useful but that should not trigger an ASSERT either. >> LayoutTests/fast/css-grid-layout/empty-grid-expected.html:4 >> + position: absolute; > > Do you need this line? Probably not but this way we process it in the out of flow path as in the test. >> LayoutTests/fast/css-grid-layout/empty-grid.html:3 >> +.absGrid { > > The grid is not absolutely positioned, so the class name is misleading. True, should be gridWithAbsItem or something like this. >> LayoutTests/fast/css-grid-layout/empty-grid.html:20 >> + width: 100%; > > I'd use "height: 100%;" too to verify the behaviour on the other axis. To verify which behavior? What we want to verify is how empty grids behave not the sizes of the items. >> LayoutTests/fast/css-grid-layout/empty-grid.html:21 >> + background: cyan; > > Nit: It's more common to use "green" or "lime" for the right result, rather than "cyan". Ack.

Comment on attachment 279646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279646&action=review > Source/WebCore/ChangeLog:20 > + A new bool was added to verify that placeItemsOnGrid() was called as the previous code was > + relying on the fact that there were items in the internal representation which is wrong as > + there might be no items in the grid. Nit: 3 lines and not a single comma or dot. :-) >>> Source/WebCore/rendering/RenderGrid.cpp:150 >>> + ASSERT(!m_grid.isEmpty()); >> >> So we're not calling this if the m_grid is empty. >> >> Are we actually creating any GridIterator when m_grid is empty? >> If that's the case, shouldn't we just avoid it? What's the point of having a GridIterator if the grid is empty? >> >> I'm wondering if we could move the ASSERT to GridIterator constructor directly. > > We have 2 options, we either check that the grid is empty every time before creating the GridIterator or we create it unconditionally and deal with the empty case. Since having an empty is grid is by far the most uncommon case I decided to optimize the normal case and did not add any extra check. If I'm getting it right, GridIterator is only called when m_grid is not empty. So we could move this ASSERT to GridIterator constructor. So in that case we won't need to modify GridIterator, we could keep using the previous code relying on m_grid not being empty. Or just use the new calls to grid.gridColumn|RowCount() as they make the code cleaner than using directly "m_grid.size()". If you do that, probably you need to update isEmptyAreaEnough() too, to use m_maxColumns|Rows. >>> Source/WebCore/rendering/RenderGrid.cpp:335 >>> + return m_grid.size() ? m_grid[0].size() : GridPositionsResolver::explicitGridColumnCount(style(), m_autoRepeatColumns); >> >> In an empty grid, do we really need to get the size of the columns from style? >> Couldn't be enough just return "0" here? >> >> I might be missing something as I'm not getting this change. > > Well that line is basically the main point of this change. The thing is that our grid representation is a Vector of rows. Each row is a Vector of columns, and each column is a Vector of RenderBoxes (the children of the grid). This means that in order to have at least a column we need to artificially create a row. > > What happens when you have a declaration like this "grid-template-columns: 10px;" ? If we do what you suggest you'll return 0 as the number of columns, but we actually have 1. Note that the else part of the ternary operator will only happen in the event of not having any children and not having a grid-template-rows declaration. Ok, I got it now. After populateExplicitGridAndOrderIterator() is run you're sure you have the rows and columns from the style. But if there're not rows, populateExplicitGridAndOrderIterator() won't be able to create the required columns. That's why you only need this in gridColumnCount(). I think it'd be nice to add a pair of tests cases for this: 1) grid-template-columns: 100px; grid-tempalte-rows: none; 2) grid-template-columns: none; grid-tempalte-rows: 100px; I know that the 2) case is not really important for our current implementation, but if the implementation changes in the future, it'd be good to have a test case for this. >>> LayoutTests/fast/css-grid-layout/empty-grid.html:3 >>> +.absGrid { >> >> The grid is not absolutely positioned, so the class name is misleading. > > True, should be gridWithAbsItem or something like this. BTW, in other tests we've a comment like this: /* Ensures that the grid container is the containing block of the absolutely positioned grid children. */ position: relative >>> LayoutTests/fast/css-grid-layout/empty-grid.html:20 >>> + width: 100%; >> >> I'd use "height: 100%;" too to verify the behaviour on the other axis. > > To verify which behavior? What we want to verify is how empty grids behave not the sizes of the items. I meant, if we add "height: 200px" on ".absGrid" and "height: 100%" here the result would be a 200x200 box. Maybe not really important, but we'll be checking that everything is working fine in both axis related with having no rows/columns.

Created attachment 279761 [details] Patch Reworked patch and test. We now need just a few changes to the current code

Comment on attachment 279761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279761&action=review Really nice! The patch LGTM just a few minor comments. > Source/WebCore/ChangeLog:20 > + A new bool was added to verify that placeItemsOnGrid() was called as the previous code was > + relying on the fact that there were items in the internal representation which is wrong as > + there might be no items in the grid. Nit: Still a very long sentence here. :-) > Source/WebCore/rendering/RenderGrid.cpp:613 > + if (!m_gridItemArea.isEmpty()) { What do you think about having a new function like: bool RenderGrid::hasGridItems() const { return !m_gridItemArea.isEmpty(); } Or "RenderGrid::isEmpty()", I'm not sure about the name. Probably it'd be easier to understand what we're checking on those cases. > Source/WebCore/rendering/RenderGrid.cpp:940 > } I guess you can skip the next while too, if there're no items. > LayoutTests/fast/css-grid-layout/empty-grid.html:17 > + grid-column: 1 / 2; You don't need this line.

Comment on attachment 279761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279761&action=review > Source/WebCore/rendering/RenderGrid.cpp:189 > ASSERT(fixedTrackSpan >= 1 && varyingTrackSpan >= 1); These assertions should be two separate assertions.

Created attachment 280077 [details] Patch Minor fixes after review

Comment on attachment 280077 [details] Patch Informal r+.

Comment on attachment 280077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280077&action=review Looks good to me, overall. I fews comments for your conderation. > Source/WebCore/rendering/RenderGrid.cpp:142 > + ASSERT(!m_grid[0].isEmpty()); Quick question: Is m_grid is an empty vector, can m_grid[0] be troublesome? > Source/WebCore/rendering/RenderGrid.cpp:615 > + for (unsigned i = 0; i < flexibleSizedTracksIndex.size(); ++i) { Minor: I believe here you can use the "for (auto trackIndex = )" as you did below (line 630). > Source/WebCore/rendering/RenderGrid.h:201 > + bool m_gridIsDirty { true }; Minor: I believe WTF::Optional is the new cool way of "flippable" class variables like this.

Comment on attachment 280077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280077&action=review >> Source/WebCore/rendering/RenderGrid.cpp:142 >> + ASSERT(!m_grid[0].isEmpty()); > > Quick question: Is m_grid is an empty vector, can m_grid[0] be troublesome? s/Is m_grid/If m_grid/g

Comment on attachment 280077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280077&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:142 >>> + ASSERT(!m_grid[0].isEmpty()); >> >> Quick question: Is m_grid is an empty vector, can m_grid[0] be troublesome? > > s/Is m_grid/If m_grid/g That's the main point of this change. If m_grid is empty none of the GridIterator methods could be called (not even constructed). The caller must enforce that restriction, with all these asserts we just make the preconditions explicit.

Comment on attachment 280077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280077&action=review >> Source/WebCore/rendering/RenderGrid.cpp:615 >> + for (unsigned i = 0; i < flexibleSizedTracksIndex.size(); ++i) { > > Minor: I believe here you can use the "for (auto trackIndex = )" as you did below (line 630). Not if he wants to use "i - 1". > Source/WebCore/rendering/RenderGrid.cpp:617 > + while (RenderBox* gridItem = iterator.nextGridItem()) { I suggest auto* or auto here instead of RenderBox*. > Source/WebCore/rendering/RenderGrid.cpp:618 > + const GridSpan span = cachedGridSpan(*gridItem, direction); I don’t think const adds much here. I would just write auto or auto& or GridSpan. > Source/WebCore/rendering/RenderGrid.cpp:930 > + while (RenderBox* gridItem = iterator.nextGridItem()) { Ditto. > Source/WebCore/rendering/RenderGrid.cpp:932 > + const GridSpan& span = cachedGridSpan(*gridItem, direction); Ditto. >> Source/WebCore/rendering/RenderGrid.h:201 >> + bool m_gridIsDirty { true }; > > Minor: I believe WTF::Optional is the new cool way of "flippable" class variables like this. I agree that it would be neat if we could make something optional rather than having a separate dirty bit. I guess maybe you are suggesting we make m_grid optional? > LayoutTests/fast/css-grid-layout/empty-grid-expected.txt:12 > +PASS > +XXXX > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS The best quality tests should state what they are testing as the go rather than just writing PASS PASS PASS. I won’t insist on that for this test, but it’s something to consider when writing new tests. The checkLayout style of test unfortunately always ends up like this, but that seems like something we could fix.

Comment on attachment 280077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280077&action=review Thanks for the reviews! >> Source/WebCore/rendering/RenderGrid.cpp:618 >> + const GridSpan span = cachedGridSpan(*gridItem, direction); > > I don’t think const adds much here. I would just write auto or auto& or GridSpan. I was not changing that code just moving it inside the if block. Anyway I can do those changes before landing. >> Source/WebCore/rendering/RenderGrid.cpp:932 >> + const GridSpan& span = cachedGridSpan(*gridItem, direction); > > Ditto. And ditto. :) >>> Source/WebCore/rendering/RenderGrid.h:201 >>> + bool m_gridIsDirty { true }; >> >> Minor: I believe WTF::Optional is the new cool way of "flippable" class variables like this. > > I agree that it would be neat if we could make something optional rather than having a separate dirty bit. I guess maybe you are suggesting we make m_grid optional? Not really because it's just signaling that the children (if any) have been processed and added to m_grid. >> LayoutTests/fast/css-grid-layout/empty-grid-expected.txt:12 >> +PASS > > The best quality tests should state what they are testing as the go rather than just writing PASS PASS PASS. I won’t insist on that for this test, but it’s something to consider when writing new tests. The checkLayout style of test unfortunately always ends up like this, but that seems like something we could fix. I agree with you but unfortunately for checkLayout() tests there is nothing much we can do apart from adding some description at the very beginning as I'm doing.

Committed r201510: <http://trac.webkit.org/changeset/201510>

Comment on attachment 280077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280077&action=review >>> LayoutTests/fast/css-grid-layout/empty-grid-expected.txt:12 >>> +PASS >> >> The best quality tests should state what they are testing as the go rather than just writing PASS PASS PASS. I won’t insist on that for this test, but it’s something to consider when writing new tests. The checkLayout style of test unfortunately always ends up like this, but that seems like something we could fix. > > I agree with you but unfortunately for checkLayout() tests there is nothing much we can do apart from adding some description at the very beginning as I'm doing. In the future we should change checkLayout so we don’t make so many cryptic tests. I think we could pass in a string to checkLayout to clarify what is being tested.