Bug 109714 - [CSS Grid Layout] Add an internal 2D grid representation to RenderGrid
Summary: [CSS Grid Layout] Add an internal 2D grid representation to RenderGrid
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 103316
  Show dependency treegraph
 
Reported: 2013-02-13 10:42 PST by Julien Chaffraix
Modified: 2013-02-14 12:17 PST (History)
6 users (show)

See Also:


Attachments
Proposed grid: Vector of Vector of Vector of RenderBoxes. (10.41 KB, patch)
2013-02-13 16:17 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Updated fix: Now with GridIterator. (10.98 KB, patch)
2013-02-14 09:28 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (11.02 KB, patch)
2013-02-14 11:13 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (11.00 KB, patch)
2013-02-14 11:33 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2013-02-13 10:42:40 PST
In order to add grid-auto-flow support, we will need to represent our 2D grid so that we can place the grid items into the empty grid areas.

It would also make our algorithm a lot more efficient as we could easily answer which grid items are in a specific row or column.

Another upside of that would be that we won't have to resolve our grid position over and over as we are currently doing.
Comment 1 Julien Chaffraix 2013-02-13 16:17:42 PST
Created attachment 188212 [details]
Proposed grid: Vector of Vector of Vector of RenderBoxes.
Comment 2 Ojan Vafai 2013-02-13 19:34:08 PST
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 3 Julien Chaffraix 2013-02-14 07:49:37 PST
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 4 Julien Chaffraix 2013-02-14 07:59:46 PST
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.
Comment 5 Julien Chaffraix 2013-02-14 09:28:52 PST
Created attachment 188370 [details]
Updated fix: Now with GridIterator.
Comment 6 Ojan Vafai 2013-02-14 10:16:27 PST
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 7 Julien Chaffraix 2013-02-14 11:09:11 PST
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.
Comment 8 Julien Chaffraix 2013-02-14 11:13:07 PST
Created attachment 188386 [details]
Patch for landing
Comment 9 Julien Chaffraix 2013-02-14 11:28:36 PST
Comment on attachment 188386 [details]
Patch for landing

Hit wp too fast.
Comment 10 Julien Chaffraix 2013-02-14 11:33:51 PST
Created attachment 188389 [details]
Patch for landing
Comment 11 WebKit Review Bot 2013-02-14 12:17:09 PST
Comment on attachment 188389 [details]
Patch for landing

Clearing flags on attachment: 188389

Committed r142898: <http://trac.webkit.org/changeset/142898>
Comment 12 WebKit Review Bot 2013-02-14 12:17:13 PST
All reviewed patches have been landed.  Closing bug.