[webkit-reviews] review denied: [Bug 109880] Stack allocated the grid data in RenderGrid : [Attachment 188463] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Ojan Vafai
<ojan at chromium.org>'s request for review:
Bug 109880: Stack allocated the grid data in RenderGrid
https://bugs.webkit.org/show_bug.cgi?id=109880

Attachment 188463: Patch
https://bugs.webkit.org/attachment.cgi?id=188463&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188463&action=review


As a whole, I don't think this is an improvement (though I do understand the
goal).

If we want to cache the grid representation between layouts, we will have to
revert this change. Also computing the grid falls into logical const-ness IMO.
Enforcing logical constness would only require to add a mutable statement to
m_grid or (probably better) a const_cast to call placeItemsOnGrid vs patching a
lot of RenderGrid's methods to enforce physical const-ness. Because of the
above, I would rather see us the former instead of the latter (unless I missed
something).

> Source/WebCore/rendering/RenderGrid.cpp:471
> +    ASSERT(grid.isEmpty());

This ASSERT becomes pretty useless. It was to catch people not calling clear()
on the grid at the end of layout / computePreferredLogicalWidths.

> Source/WebCore/rendering/RenderGrid.h:53
> +    typedef Vector<Vector<Vector<RenderBox*, 1> > > GridData;

I am not super fan of the naming. GridData is a pretty generic and already
overloaded term (e.g StyleGridData).

I would just stick with Grid as this is really what it is. If you want
something less generic, GridRepresentation could work (btw, this is the typedef
you said was not super useful, guess it was :))