Bug 66972 – Factor out the code to get the first non-null RenderTableSection in RenderTable

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Julien Chaffraix]

Currently there is a lot of duplication in the table code when it comes to finding the first non-null section on the table. To make the code more readable, I propose to move the share this code.

While at it, I changed the signature of sectionAbove / sectionBelow to be more readable as it was easy to misunderstand what the boolean was about.

Patch forthcoming.

[Julien Chaffraix]

(In reply to comment #0)
> Currently there is a lot of duplication in the table code when it comes to finding the first non-null section on the table. To make the code more readable, I propose to move the share this code.

I meant moving the code to some shared location. :-)

[Julien Chaffraix]

Created attachment 105235 [details]
Proposed refactoring: add 2 methods and migrate call sites to use them. Added some FIXMEs to cover the accessibility code.

[Darin Adler]

Comment on attachment 105235 [details]
Proposed refactoring: add 2 methods and migrate call sites to use them. Added some FIXMEs to cover the accessibility code.

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.

[Julien Chaffraix]

Comment on attachment 105235 [details]
Proposed refactoring: add 2 methods and migrate call sites to use them. Added some FIXMEs to cover the accessibility code.

View in context: https://bugs.webkit.org/attachment.cgi?id=105235&action=review

Thanks for the review, the other points will be taken care of.

>> 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.

Technically "method" is an Object-Oriented Programming concept that could apply to C++ as well as any object-oriented language (unless I missed something when I learned to OOP). It's the second time you point this out and technically I don't see the problem with this difference. Could you clarify why you think this is likely a problem? (note that I don't mind the switch, just would like to learn what is the rationale about that for the next time)

>> 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.

Good point, I wondered about the meaningfulness of using spaciality in the name. However the rest of the code follows the same pattern (borders or padding for example) even if top would not be the top due to some flipping.

>> 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.

Make sense to move it to the WebCore namespace.

We normally do not expect people to actually use them as boolean - which would be a point for keeping it this way - but I will side with you as it is dangerous to leave it as-is.

[Darin Adler]

(In reply to comment #4)
> Could you clarify why you think this is likely a problem?

Probably not in a way that would convince you.

I believe method is the wrong term, even though you are comfortable using it. Function is the the one used by C++ programmers and specifications in my experience.

I don’t want to use both terms in our code. I suggest we consistently call these functions.

[Julien Chaffraix]

(In reply to comment #5)
> (In reply to comment #4)
> > Could you clarify why you think this is likely a problem?
> 
> Probably not in a way that would convince you.
> 
> I believe method is the wrong term, even though you are comfortable using it. Function is the the one used by C++ programmers and specifications in my experience.

It makes sense even if we disagree on the core. This will be changed as requested.

> I don’t want to use both terms in our code. I suggest we consistently call these functions.

Agreed!

[Julien Chaffraix]

Created attachment 105280 [details]
Version 2: Taking Darin's comments.

[Julien Chaffraix]

Darin, review ping.

[Darin Adler]

Comment on attachment 105280 [details]
Version 2: Taking Darin's comments.

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.

[Julien Chaffraix]

Comment on attachment 105280 [details]
Version 2: Taking Darin's comments.

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?

How about:

// FIXME: This line should use RenderTable::topSection() as it would skip an table with just a tfoot.

[Darin Adler]

(In reply to comment #10)
> // FIXME: This line should use RenderTable::topSection() as it would skip an table with just a tfoot.

I’d say it like this:

    // FIXME: This will skip a table with just a tfoot. Should fix by using RenderTable::topSection.

[Julien Chaffraix]

(In reply to comment #11)
> (In reply to comment #10)
> > // FIXME: This line should use RenderTable::topSection() as it would skip an table with just a tfoot.
> 
> I’d say it like this:
> 
>     // FIXME: This will skip a table with just a tfoot. Should fix by using RenderTable::topSection.

Fair enough. Thanks Darin.

[Julien Chaffraix]

Created attachment 106639 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 106639 [details]
Patch for landing

Clearing flags on attachment: 106639

Committed r94776: <http://trac.webkit.org/changeset/94776>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.