[webkit-reviews] review granted: [Bug 66972] Factor out the code to get the first non-null RenderTableSection in RenderTable : [Attachment 105280] Version 2: Taking Darin's comments.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 66972: Factor out the code to get the first non-null RenderTableSection in
RenderTable
https://bugs.webkit.org/show_bug.cgi?id=66972

Attachment 105280: Version 2: Taking Darin's comments.
https://bugs.webkit.org/attachment.cgi?id=105280&action=review

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


> Source/WebCore/accessibility/AccessibilityTable.cpp:302
> +    // FIXME: Looks like this should use topSection() as it could skip some
cases.

That comment is good, but seems a bit cryptic. Could you be a little more
specific?

> Source/WebCore/accessibility/AccessibilityTable.cpp:470
> +    // FIXME: Looks like this should use topSection() as it could skip some
cases.

Ditto.

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:116
> +    // FIXME: Looks like this should use topSection() as it could miss some
cases.

Ditto.

> Source/WebCore/rendering/RenderTable.cpp:810
> +	   const RenderTableSection* topNonEmptySection =
this->topNonEmptySection();
> +	   if (topNonEmptySection) {

It would be nicer to define this inside the if.

> Source/WebCore/rendering/RenderTable.cpp:865
> +	   const RenderTableSection* topNonEmptySection =
this->topNonEmptySection();
> +	   if (topNonEmptySection) {

It would be nicer to define this inside the if.

> Source/WebCore/rendering/RenderTable.cpp:921
> +    RenderTableSection* topSection = this->topSection();
>      if (topSection) {

It would be nicer to define this inside the if.