[webkit-reviews] review granted: [Bug 69569] Remove colSpan / rowSpan caching from RenderTableCell : [Attachment 111138] Change 2: Removed the bogus span changed logic.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 69569: Remove colSpan / rowSpan caching from RenderTableCell
https://bugs.webkit.org/show_bug.cgi?id=69569

Attachment 111138: Change 2: Removed the bogus span changed logic.
https://bugs.webkit.org/attachment.cgi?id=111138&action=review

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


> Source/WebCore/ChangeLog:14
> +	   on PageCycler Alexia-US.

I think it’s Alexa, not Alexia.

> Source/WebCore/html/HTMLTableCellElement.h:29
> +#include "HTMLNames.h"

When I did this recently in some other classes, I avoided putting HTMLNames.h
into the header by putting the assertions into the .cpp file and having only
the NDEBUG version be inlined.

> Source/WebCore/rendering/RenderTableCell.cpp:52
> +    m_hasAssociatedTableCellElement = node && (node->hasTagName(tdTag) ||
node->hasTagName(thTag));

Seems this should be done with initialization, not assignment.

> Source/WebCore/rendering/RenderTableCell.cpp:84
> +    ASSERT(node() && (node()->hasTagName(tdTag) ||
node()->hasTagName(thTag)));

Normally we do not use && in ASSERT. Instead we do two separate assertions.
That allows us to see which has failed.

> Source/WebCore/rendering/RenderTableCell.cpp:87
> +    if (parent() && section())

I’m not sure the null check of parent() is helpful, even though the old code
had it.

> Source/WebCore/rendering/RenderTableCell.h:153
> +    // FIXME: It would be nice to pack those 2 bits into some of the
previous fields.

Should be "these", not "those".