Bug 89751 – Refactor RenderTable to use the section's iteration functions.

• Component: New Bugs
• Status: RESOLVED FIXED

[Arpita Bahuguna]

In RenderTable::outerBorderAfter() we have code for obtaining the bottom-most or the last RenderTableSection.

The same is now available in the function RenderTable::bottomSection().

Thus bottomSection() method should be called in outerBorderAfter() for getting the bottom-section.

[Arpita Bahuguna]

Created attachment 148983 [details]
Patch

[WebKit Review Bot]

Attachment 148983 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:11:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Arpita Bahuguna]

Created attachment 148985 [details]
Patch

[Julien Chaffraix]

Comment on attachment 148985 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Refactoring RenderTable.cpp for code re-usability

This is a pretty bad name for a bug as it describes in no-way what you are doing.

I would rather rename it to something like (see below why):
* Change RenderTable sections' iterations to use the helper functions
* Unify RenderTable sections' iterations

> Source/WebCore/rendering/RenderTable.cpp:1000
> +    if (RenderTableSection* section = bottomSection()) {

This is definitely a good change. However if we are to use the helper more, I would just go ahead and kill a lot the following anti-pattern as part of this change:

for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {
    if (child->isTableSection()) {
        ....
    }
}

[Arpita Bahuguna]

Created attachment 149505 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #4)

Thanks for the review Julien. Have incorporated the changes as suggested by you.

> (From update of attachment 148985 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148985&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Refactoring RenderTable.cpp for code re-usability
> 
> This is a pretty bad name for a bug as it describes in no-way what you are doing.
> 
> I would rather rename it to something like (see below why):
> * Change RenderTable sections' iterations to use the helper functions
> * Unify RenderTable sections' iterations
> 
> > Source/WebCore/rendering/RenderTable.cpp:1000
> > +    if (RenderTableSection* section = bottomSection()) {
> 
> This is definitely a good change. However if we are to use the helper more, I would just go ahead and kill a lot the following anti-pattern as part of this change:
> 
> for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {
>     if (child->isTableSection()) {
>         ....
>     }
> }

This change can not be done for iterations in recalcSections() and appendColumn() since m_needsSectionRecalc would be set and thus any call to topSection() would fail in ASSERT(!needsSectionRecalc());

[Arpita Bahuguna]

(In reply to comment #6)
> (In reply to comment #4)
> 
> Thanks for the review Julien. Have incorporated the changes as suggested by you.
> 
> > (From update of attachment 148985 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=148985&action=review
> > 
> > > Source/WebCore/ChangeLog:3
> > > +        Refactoring RenderTable.cpp for code re-usability
> > 
> > This is a pretty bad name for a bug as it describes in no-way what you are doing.
> > 
> > I would rather rename it to something like (see below why):
> > * Change RenderTable sections' iterations to use the helper functions
> > * Unify RenderTable sections' iterations
> > 
> > > Source/WebCore/rendering/RenderTable.cpp:1000
> > > +    if (RenderTableSection* section = bottomSection()) {
> > 
> > This is definitely a good change. However if we are to use the helper more, I would just go ahead and kill a lot the following anti-pattern as part of this change:
> > 
> > for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {
> >     if (child->isTableSection()) {
> >         ....
> >     }
> > }
> 
> This change can not be done for iterations in recalcSections() and appendColumn() since m_needsSectionRecalc would be set and thus any call to topSection() would fail in ASSERT(!needsSectionRecalc());

Also, am not really sure whether we should change the iteration in RenderTable::splitColumn().
It currently doesn't fail any of the existing testcases but there could probably be a scenario, similar to appendColumn(), wherein needsSectionRecalc could be set, in which case it would be inappropriate to iterate over RenderTableSections.

[Julien Chaffraix]

Comment on attachment 149505 [details]
Patch

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

r=me without the splitColumns part.

> Source/WebCore/ChangeLog:3
> +        Change RenderTable sections' iterations for removing anti-patterns and using helper functions.

A better name: Refactor RenderTable to use the section's iteration functions

> Source/WebCore/ChangeLog:11
> +        No new tests required for this change.

Usually you justify why. Here it is because you don't expect any change in behavior.

> Source/WebCore/rendering/RenderTable.cpp:683
> +    for (RenderTableSection* section = topSection(); section; section = sectionBelow(section)) {

You are right nothing guarantees us not to need a section recalc here.

[Arpita Bahuguna]

Created attachment 152165 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #8)
> (From update of attachment 149505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149505&action=review
> 
> r=me without the splitColumns part.
> 
Thanks for your time and the review Julien. Have uploaded another patch with all the specified changes.

[WebKit Review Bot]

Comment on attachment 152165 [details]
Patch

Attachment 152165 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13245027

New failing tests:
http/tests/w3c/webperf/approved/navigation-timing/html5/test_performance_attributes_exist_in_object.html

[WebKit Review Bot]

Created attachment 152241 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Julien Chaffraix]

Comment on attachment 152165 [details]
Patch

The EWS is unrelated, it looks like some rebaseline is needed for the failing test.

[WebKit Review Bot]

Comment on attachment 152165 [details]
Patch

Clearing flags on attachment: 152165

Committed r122636: <http://trac.webkit.org/changeset/122636>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Arpita Bahuguna]

Thanks for the review Julien..