[webkit-reviews] review denied: [Bug 66972] Factor out the code to get the first non-null RenderTableSection in RenderTable : [Attachment 105235] Proposed refactoring: add 2 methods and migrate call sites to use them. Added some FIXMEs to cover the accessibility code.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 105235: Proposed refactoring: add 2 methods and migrate call sites
to use them. Added some FIXMEs to cover the accessibility code.
https://bugs.webkit.org/attachment.cgi?id=105235&action=review

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


Since this is only refactoring, I’m going to say review- even though my
comments are editorial and stylistic rather than substantive.

> Source/WebCore/rendering/RenderTable.h:147
> +    // This method can return 0.

The word “method” is not a C++ term and should not be used to refer to member
functions. You should just say “function”.

I also think that if we have this comment, it should have more information. I’d
suggest:

    // Returns 0 if the table has no sections.

> Source/WebCore/rendering/RenderTable.h:148
> +    RenderTableSection* firstNonNullSection() const

The name for this should probably be firstSection(). I don’t think having
“non-null” in the name is helpful.

It’s annoying that the functions that iterate forward and backward are called
“above” and “below” but the one that starts is called “first”. I think it
should be first/next/previous or top/above/below.

Even if we want this inlined, I think it would be easier to read the class if
the function body was outside the class definition. It can be later in the
header, after the class definition, with an explicit inline keyword.

> Source/WebCore/rendering/RenderTable.h:154
> +	   if (header())
> +	       return header();
> +	   if (firstBody())
> +	       return firstBody();
> +	   return footer();

I think it would be clearer to use the data members directly rather than
calling functions.

> Source/WebCore/rendering/RenderTable.h:157
> +    // This method can return 0.

I’d suggest:

    // Returns 0 if the table has no non-empty sections.

> Source/WebCore/rendering/RenderTable.h:158
> +    RenderTableSection* firstNonNullSectionWithRows() const;

Elsewhere in this patch, we call sections without rows “empty sections”. I
think the name for this should be firstNonEmptySection().

> Source/WebCore/rendering/RenderTable.h:200
> +    enum SkipEmptySectionsValue { SkipEmptySections, DoNotSkipEmptySections
};

Normally I prefer to put these kinds of enums at the namespace level even if
they are used in only one class. Otherwise, the class qualifier can make call
sites look unpleasant. And the risk of conflict without our own namespace is
low.

Normally it’s better to have the false value first, then the true value. That
way even if someone uses enum as a boolean we get the correct behavior.