[webkit-reviews] review denied: [Bug 73349] compareBorders() is called too often during painting : [Attachment 117039] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 73349: compareBorders() is called too often during painting
https://bugs.webkit.org/show_bug.cgi?id=73349

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

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


r- for the unneeded size increase for each cell which is a definite NO.

> Source/WebCore/rendering/RenderTable.h:206
> +    }

Nit: Would be better on a single line.

> Source/WebCore/rendering/RenderTableCell.cpp:901
> +    // The parent table is being laid out, so update our collapsed border
cache.

Looks like you could add this ASSERT here:

ASSERT(!needsLayout());

> Source/WebCore/rendering/RenderTableCell.cpp:939
> +    }

It would be better to share this code with the previous function. That would
provide a single point of failure + likely enhance our checks (like being able
to add more ASSERTs).

> Source/WebCore/rendering/RenderTableCell.h:119
> +    void collectBorderValues(RenderTable::CollapsedBorderValues&);

I would prefer to keep the const here and match logical constness definition.
You will need to add the mutable property on your collapsed*Borders.

> Source/WebCore/rendering/RenderTableCell.h:187
> +    CollapsedBorderValue m_collapsedBottomBorder;

No! You are adding 4 bytes to each table cell. Tables are already using a lot
of memory and there is no way we are going to add more to it. Border collapse
are not likely to be the common case (it's not the initial value for
border-collapse) and you are increasing *every* table regardless of that. I
would better see the cached borders attached to the section / table itself or
in a rare field that gets allocated only if the table needs it (hack the
styleDidChange / styleWillChange to properly allocate / deallocate the object).