[webkit-reviews] review denied: [Bug 64546] Redrawing dirty parts of a large table is very slow : [Attachment 106870] Changes for review comments

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 14 11:34:19 PDT 2011


Dave Hyatt <hyatt at apple.com> has denied Konstantin Shcheglov
<scheglov at google.com>'s request for review:
Bug 64546: Redrawing dirty parts of a large table is very slow
https://bugs.webkit.org/show_bug.cgi?id=64546

Attachment 106870: Changes for review comments
https://bugs.webkit.org/attachment.cgi?id=106870&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106870&action=review


You should use styleDidChange to do invalidation and not styleWillChange. The
big advantage of doing so is you could call the base class first and it would
mark for layout in the case where the border change results in needing a
layout. This would allow you to refine your check so that you only do it if the
table and cell don't need relayout. So for example here's what
RenderTableSection::styleDidChange might look like:

void RenderTableSection::styleDidChange(StyleDifference diff, const
RenderStyle* oldStyle)
 
    RenderBox::styleWillChange(diff, oldStyle);
     if (!selfNeedsLayout() && !normalChildNeedsLayout() && oldStyle &&
oldStyle->border() != style()->border()) {
	RenderTable* tableRenderer = table();
	if (!tableRenderer->selfNeedsLayout() &&
!tableRenderer->normalChildNeedsLayout())
	    tableRenderer->invalidateCollapsedBorder();
}

This would save you from having to check for borders in a lot of cases, e.g.,
when the border width of the cell changes, since the cell will already be
marked as needing layout, and so you don't have to worry about it.

> Source/WebCore/ChangeLog:10
> +	   Invalidate cache when cell, row, row group, col, col group or table
border it changed.

Typo. "it" should be "is"

> Source/WebCore/rendering/RenderTable.h:237
> +    virtual void recalcCollapsedBorders();

This should be private and non-virtual. Move it.

> Source/WebCore/rendering/RenderTable.h:270
> +    bool m_collapsedBordersValid;

Move this after m_currentBorder and make it part of the bitfield with
m_hasColElements


More information about the webkit-reviews mailing list