[webkit-reviews] review granted: [Bug 70570] Remove RenderTableSection::m_gridRows : [Attachment 111889] Proposed removal.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 70570: Remove RenderTableSection::m_gridRows
https://bugs.webkit.org/show_bug.cgi?id=70570

Attachment 111889: Proposed removal.
https://bugs.webkit.org/attachment.cgi?id=111889&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=111889&action=review


> Source/WebCore/rendering/RenderTableSection.cpp:180
> +    if (numRows > (int)m_grid.size()) {

This should be a C++ static_cast, not a C cast.

> Source/WebCore/rendering/RenderTableSection.cpp:183
> +	   size_t maxSize = numeric_limits<size_t>::max() / sizeof(RowStruct);
> +	   if (static_cast<size_t>(numRows) > maxSize)
> +	       return false;

If we plan to return false when a value would overflow what a Vector can hold,
the computation about the limits of what fits in a Vector should not be done
here, because it involves assumptions about how Vector is implemented. Vector
might have a smaller limit than the one here. Instead, we should add a tryGrow
function to Vector.

On the other hand, maybe we want a much smaller limit, because I am concerned
this lets us create enormous data structures that fit in virtual memory, but
put the engine into a very bad state.

> Source/WebCore/rendering/RenderTableSection.cpp:1132
> +    // Although it is possible for our row count to shrink (due to
removeChild being called),
> +    // it is more common for the count to stay the same. Let's just
reallocate the old
> +    // capacity upfront to avoid re-expanding it one row at a time.
> +    m_grid.reserveCapacity(capacity);

It seems unfortunate to throw away and reallocate here. We should figure out a
way to get clearGrid to lower the size and leave the already allocated memory
alone, rather than re-reserving capacity. A good way to do that would be to
factor clearGrid out into two pieces so you can use the deletion part and then
do some other way of emptying the vector that does not throw away the excess
capacity, rather than clear().