Created attachment 188212 [details] Proposed grid: Vector of Vector of Vector of RenderBoxes.

Comment on attachment 188212 [details] Proposed grid: Vector of Vector of Vector of RenderBoxes. View in context: https://bugs.webkit.org/attachment.cgi?id=188212&action=review > Source/WebCore/rendering/RenderGrid.cpp:376 > + // FIXME: We should have a column / row iterator. I'd prefer we do this as part of this patch. I actually think it'd make the diff a lot smaller since the structure of the code wouldn't be changing as much. For example, you could keep this code sharing the same code inside the loop for the column and row cases. > Source/WebCore/rendering/RenderGrid.cpp:464 > + size_t columnTrack = resolveGridPosition(child->style()->gridItemColumn()); > + size_t rowTrack = resolveGridPosition(child->style()->gridItemRow()); Nit: about calling these columnIndex and rowIndex since they're size_t's? > Source/WebCore/rendering/RenderGrid.h:85 > + Vector<Vector<Vector<RenderBox*, 1> > > m_grid; How do you feel about using typedefs to make this more readable? typedef Vector<RenderBox*> GridAreaItems typedef Vector<GridAreaItems> GridColumns typedef Vector<GridColumns> GridRows GridRows m_grid; Then in the calling code you'd have things like: const GridAreaItems children = m_grid[trackIndex][i]; I'm on the fence as to whether this is actually more readable. :)

Comment on attachment 188212 [details] Proposed grid: Vector of Vector of Vector of RenderBoxes. View in context: https://bugs.webkit.org/attachment.cgi?id=188212&action=review >> Source/WebCore/rendering/RenderGrid.cpp:376 >> + // FIXME: We should have a column / row iterator. > > I'd prefer we do this as part of this patch. I actually think it'd make the diff a lot smaller since the structure of the code wouldn't be changing as much. For example, you could keep this code sharing the same code inside the loop for the column and row cases. Fine by me, I prefer atomic changes as they are easier to review (also usually smaller). >> Source/WebCore/rendering/RenderGrid.h:85 >> + Vector<Vector<Vector<RenderBox*, 1> > > m_grid; > > How do you feel about using typedefs to make this more readable? > > typedef Vector<RenderBox*> GridAreaItems > typedef Vector<GridAreaItems> GridColumns > typedef Vector<GridColumns> GridRows > > GridRows m_grid; > > Then in the calling code you'd have things like: > const GridAreaItems children = m_grid[trackIndex][i]; > > I'm on the fence as to whether this is actually more readable. :) If you introduce the iterator, only 2 places in the code will have to deal with m_grid directly: placeItemsOnGrid and the iterator. That said, I think that all the typedefs won't give us much as they suppose - and in a way enforce - a certain representation of the grid / iterator. Also GridAreaItems doesn't convey that it's a Vector, which is probably a requirement for any representation of the grid. The following typedef would be enough IMO: typedef Grid Vector<Vector<Vector<RenderBox*> > >; Grid m_grid;

Comment on attachment 188212 [details] Proposed grid: Vector of Vector of Vector of RenderBoxes. View in context: https://bugs.webkit.org/attachment.cgi?id=188212&action=review >> Source/WebCore/rendering/RenderGrid.cpp:464 >> + size_t rowTrack = resolveGridPosition(child->style()->gridItemRow()); > > Nit: about calling these columnIndex and rowIndex since they're size_t's? The rest of the code uses columnTrack / rowTrack. I would rather be consistent as both are equivalent (it's really which columnTrack / rowTrack ** your grid item belongs to) ** For now, we only span one column / row track. Once we lift this constraint, the definition will probably evolve to be the start track.

Created attachment 188370 [details] Updated fix: Now with GridIterator.

Comment on attachment 188370 [details] Updated fix: Now with GridIterator. View in context: https://bugs.webkit.org/attachment.cgi?id=188370&action=review > Source/WebCore/rendering/RenderGrid.cpp:92 > + for (; m_childIndex < children.size(); ++m_childIndex) I think this can just be: if (m_childIndex < children.size()) return ...; > Source/WebCore/rendering/RenderGrid.h:87 > + typedef Vector<Vector<Vector<RenderBox*, 1> > > Grid; > + Grid m_grid; Not sure this typedef adds much value. What I had in mind before was that the typedefs would help make it clear whether the first index is into the columns or the rows. But, I'm fine with saying it was too confusing/complicated.

Comment on attachment 188370 [details] Updated fix: Now with GridIterator. View in context: https://bugs.webkit.org/attachment.cgi?id=188370&action=review >> Source/WebCore/rendering/RenderGrid.cpp:92 >> + for (; m_childIndex < children.size(); ++m_childIndex) > > I think this can just be: > if (m_childIndex < children.size()) > return ...; Good catch! >> Source/WebCore/rendering/RenderGrid.h:87 >> + Grid m_grid; > > Not sure this typedef adds much value. What I had in mind before was that the typedefs would help make it clear whether the first index is into the columns or the rows. But, I'm fine with saying it was too confusing/complicated. I found the GridRows a bit confusing (but saw where you came from to explain which direction we use). I would also argue that putting such a specific typedef would constrain our design (if we ever wanted to change how the grid is represented). Let's just drop it as it's really used in 3 places.

Created attachment 188386 [details] Patch for landing

Comment on attachment 188386 [details] Patch for landing Hit wp too fast.

Created attachment 188389 [details] Patch for landing

Comment on attachment 188389 [details] Patch for landing Clearing flags on attachment: 188389 Committed r142898: <http://trac.webkit.org/changeset/142898>

All reviewed patches have been landed. Closing bug.