Bug 89636 – Re-factoring recalcColumn in AutoTableLayout.cpp for readability

• Component: Tables
• Status: RESOLVED FIXED

[Pravin D]

Remove/fix code that is marked for FIXME: in AutoTableLayout.cpp

[Pravin D]

Created attachment 148757 [details]
Clean up Patch

[WebKit Review Bot]

Attachment 148757 [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:8:  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.

[Pravin D]

(In reply to comment #1)
> Created an attachment (id=148757) [details]
> Patch
> 

This is just a proposed patch. Haven't added any testcases as I'm not sure if this cleanup is acceptable or not. 
Will add it in the next patch.

[WebKit Review Bot]

Comment on attachment 148757 [details]
Clean up Patch

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

New failing tests:
fast/table/percent-widths-stretch-vertical.html
tables/mozilla_expected_failures/bugs/bug7113.html
tables/mozilla_expected_failures/core/standards1.html
fast/table/large-width.html
tables/mozilla/bugs/bug2479-2.html
tables/mozilla/bugs/bug215629.html
tables/mozilla_expected_failures/bugs/bug18770.html
fast/table/percent-widths-stretch.html
fast/table/td-width-fifty-percent-regression.html

[WebKit Review Bot]

Created attachment 148781 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Julien Chaffraix]

Comment on attachment 148757 [details]
Clean up Patch

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

> Source/WebCore/ChangeLog:3
> +        Cleanup the code under "FIXME:" in AutoTableLayout.cpp

Let's rename that to something more fitting. It really depends on the scope you want to put for this refactoring. As it will only cover recalcColumn, I would be supportive of something splitting the function for readability.

> Source/WebCore/rendering/AutoTableLayout.cpp:-85
> -                    // FIXME: What is this arbitrary value?
> -                    if (cellLogicalWidth.value() > 32760)
> -                        cellLogicalWidth.setValue(32760);

Allan nicely chimed in and explained that this comes from KHTML where the width / height were 16 bits. Other browsers also implement a maximum cell size so unfortunately we need this constant. You could move it to proper constant somewhere in the file.

> Source/WebCore/rendering/AutoTableLayout.cpp:112
> +                        if (cellLogicalWidth.value() > columnLayout.logicalWidth.value())

Obviously right.

> Source/WebCore/rendering/AutoTableLayout.cpp:-228
> -    const float epsilon = 1 / 128.0f;

|epsilon| serves to round percent width when percent nears 0. Removing it will require some tact and I would advise postponing that to another bug. You could also solve the 0% case.

[Pravin D]

Created attachment 149065 [details]
Patch

[Darin Adler]

Comment on attachment 149065 [details]
Patch

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

Sorry, didn’t have time to finish a review, but I did have two comments.

> Source/WebCore/rendering/AutoTableLayout.cpp:35
> +// Value chosen for legacy reasons.
> +#define TABLE_CELL_MAX_WIDTH 32760

There’s no reason to use a macro here instead of a C++ constant.

The comment here is super-mysterious. “legacy reasons”? What specific reasons? It would be better to not comment at all if we don’t know why this is a good limit.

> Source/WebCore/rendering/AutoTableLayout.cpp:71
> -                bool cellHasContent = cell && !current.inColSpan && (cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding());
> +                bool shouldSkipCell = !cell || current.inColSpan;
> +                bool cellHasContent = !shouldSkipCell && (cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding());
>                  if (cellHasContent)
>                      columnLayout.emptyCellsOnly = false;
>                      
> -                if (current.inColSpan || !cell)
> +                if (shouldSkipCell)
>                      continue;

A better way to refactor here would be to move the if statement before cellHasContent. Then would not need to add a shouldSkipCell boolean, and we could just remove the first part of the cellHasContent expression entirely.

[Pravin D]

(In reply to comment #8)
> (From update of attachment 149065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149065&action=review
> 
> Sorry, didn’t have time to finish a review, but I did have two comments.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:35
> > +// Value chosen for legacy reasons.
> > +#define TABLE_CELL_MAX_WIDTH 32760
> 
> There’s no reason to use a macro here instead of a C++ constant.
> 
> The comment here is super-mysterious. “legacy reasons”? What specific reasons? It would be better to not comment at all if we don’t know why this is a good limit.
> 
I'll try to come up with a better comment on this.

> > Source/WebCore/rendering/AutoTableLayout.cpp:71
> > -                bool cellHasContent = cell && !current.inColSpan && (cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding());
> > +                bool shouldSkipCell = !cell || current.inColSpan;
> > +                bool cellHasContent = !shouldSkipCell && (cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding());
> >                  if (cellHasContent)
> >                      columnLayout.emptyCellsOnly = false;
> >                      
> > -                if (current.inColSpan || !cell)
> > +                if (shouldSkipCell)
> >                      continue;
> 
> A better way to refactor here would be to move the if statement before cellHasContent. Then would not need to add a shouldSkipCell boolean, and we could just remove the first part of the cellHasContent expression entirely.
> 
Shall impl in the next patch.
Thanks for the review comments. Waiting for the cr-linux bulb to go green. Will upload another with the above suggested changes...

[Pravin D]

Created attachment 149266 [details]
Patch

[Julien Chaffraix]

Comment on attachment 149266 [details]
Patch

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

> Source/WebCore/rendering/AutoTableLayout.cpp:70
> +                bool isStartColForCell = !effCol || section->primaryCellAt(i, effCol - 1) != cell;
> +                if (isStartColForCell) {

I really wonder if this is needed. current.inColSpan is checked above which means that cells spanning at your current column that don't originate from the column are already ignored.

> Source/WebCore/rendering/AutoTableLayout.cpp:87
> +                    // implement a similar max cell size we retain this constant for compatibility with other browsers.

Note that other browsers use a smaller constant (in the thousand range, not the 30,000). This means that we could think of shrinking it or at least standardize on a smaller value.

> Source/WebCore/rendering/AutoTableLayout.cpp:88
> +                    const int cellMaxWidth = 32760;

We don't have a real convention for constants, which is unfortunate. I would advise making it more obvious that it's not a variable. Some suggestions (as stated this is a nit as we didn't agree on the best way to do that):

const int cCellMaxWidth;
const int cellMaxWidthConstant;

> Source/WebCore/rendering/AutoTableLayout.cpp:90
> +                                        

The white spaces should be removed.

[Pravin D]

Created attachment 149579 [details]
Patch

[Pravin D]

(In reply to comment #11)
> (From update of attachment 149266 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149266&action=review
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:70
> > +                bool isStartColForCell = !effCol || section->primaryCellAt(i, effCol - 1) != cell;
> > +                if (isStartColForCell) {
> 
> I really wonder if this is needed. current.inColSpan is checked above which means that cells spanning at your current column that don't originate from the column are already ignored.
> 
You are correct. This is a redundant check. Removed this check in the new patch.

> > Source/WebCore/rendering/AutoTableLayout.cpp:87
> > +                    // implement a similar max cell size we retain this constant for compatibility with other browsers.
> 
> Note that other browsers use a smaller constant (in the thousand range, not the 30,000). This means that we could think of shrinking it or at least standardize on a smaller value.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:88
> > +                    const int cellMaxWidth = 32760;
> 
I had one suggest for this actually, before it got removed from FixedTableLayout.cpp .. They used to use 15000 as upper limit on table max width. Maybe some digging into other browser should shed some light on this...

> We don't have a real convention for constants, which is unfortunate. I would advise making it more obvious that it's not a variable. Some suggestions (as stated this is a nit as we didn't agree on the best way to do that):
> 
> const int cCellMaxWidth;
> const int cellMaxWidthConstant;
> 
changed the name to cCellMaxWidth . 

> > Source/WebCore/rendering/AutoTableLayout.cpp:90
> > +                                        
> 
> The white spaces should be removed.
done.

[Julien Chaffraix]

Comment on attachment 149579 [details]
Patch

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

> Source/WebCore/rendering/AutoTableLayout.cpp:84
> +                    // This constant comes from KTML where the widths and heights were 16 bits. We need an upper limit on the max cell width to match
> +                    // the behavior of other browsers.

Nit: The 'We need an upper limit' seems weird, how about:

// All browsers implement a size limit on the cell's width. Our limit is based on KHTML's representation that used 16 bits widths. FIXME: Other browsers have lower limits.

> Source/WebCore/rendering/AutoTableLayout.cpp:85
> +                    // FIXME: However other browsers use a smaller value for the max cell width. Set a suitable smaller value for the constant.

Note that if we go down that path, we should standardize on a cell size with other browsers.

> Source/WebCore/rendering/AutoTableLayout.cpp:123
> +                } else {
> +                    // This spanning cell originates from this column. Insert the cell into the spanning cells lists.

Removing the |else| condition doesn't sound right here. AFAICT it's possible to have several cells at the same position due to a combination of rowspan and colspan (also we share the same column count across sections which means we can artificially break colspans). This check may not be 100% correct but it somewhat ensures that we don't pick several cells for the same position which is important.

Moving the min / max logical widths computation above is OK but I don't think removing the check is.

[Pravin D]

Created attachment 151300 [details]
Patch

[Pravin D]

(In reply to comment #14)
> (From update of attachment 149579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149579&action=review
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:84
> > +                    // This constant comes from KTML where the widths and heights were 16 bits. We need an upper limit on the max cell width to match
> > +                    // the behavior of other browsers.
> 
> Nit: The 'We need an upper limit' seems weird, how about:
> 
> // All browsers implement a size limit on the cell's width. Our limit is based on KHTML's representation that used 16 bits widths. FIXME: Other browsers have lower limits.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:85
> > +                    // FIXME: However other browsers use a smaller value for the max cell width. Set a suitable smaller value for the constant.
> 
> Note that if we go down that path, we should standardize on a cell size with other browsers.
> 
Included both the comment and FIXME.

> > Source/WebCore/rendering/AutoTableLayout.cpp:123
> > +                } else {
> > +                    // This spanning cell originates from this column. Insert the cell into the spanning cells lists.
> 
> Removing the |else| condition doesn't sound right here. AFAICT it's possible to have several cells at the same position due to a combination of rowspan and colspan (also we share the same column count across sections which means we can artificially break colspans). This check may not be 100% correct but it somewhat ensures that we don't pick several cells for the same position which is important.
> 
Preserved the complete |else if| condition.

[Julien Chaffraix]

Comment on attachment 151300 [details]
Patch

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

> Source/WebCore/rendering/AutoTableLayout.cpp:87
> +                    //        If we are to set a lower limit for the constant cCellMaxWidth 
> +                    //        we should first standardize on a cell size  with other browsers.

It's fine to drop this part. It was more a FYI.

> Source/WebCore/rendering/AutoTableLayout.cpp:-113
> -                        // FIXME: Need to understand this case and whether it makes sense to compare values
> -                        // which are not necessarily of the same type.

Sorry, I missed that: this comment still applies: we are comparing cellLogicalWidth and columnLayout.logicalWidth values below. Nothing guarantees that the 2 Lengths are of the same type.

[Pravin D]

Created attachment 151495 [details]
Patch

[Pravin D]

(In reply to comment #17)
> (From update of attachment 151300 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151300&action=review
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:87
> > +                    //        If we are to set a lower limit for the constant cCellMaxWidth 
> > +                    //        we should first standardize on a cell size  with other browsers.
> 
> It's fine to drop this part. It was more a FYI.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:-113
> > -                        // FIXME: Need to understand this case and whether it makes sense to compare values
> > -                        // which are not necessarily of the same type.
> 
> Sorry, I missed that: this comment still applies: we are comparing cellLogicalWidth and columnLayout.logicalWidth values below. Nothing guarantees that the 2 Lengths are of the same type.
> 

Uploaded a new patch with the suggested changes...

[WebKit Review Bot]

Comment on attachment 151495 [details]
Patch

Rejecting attachment 151495 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 11988 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
47>At revision 11988.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13204077

[Pravin D]

Created attachment 151513 [details]
Patch

[Julien Chaffraix]

Created attachment 151531 [details]
Patch for landing

[Julien Chaffraix]

The last patch has a fix for a nit in the ChangeLog per discussion with Pravin.

[Pravin D]

(In reply to comment #23)
> The last patch has a fix for a nit in the ChangeLog per discussion with Pravin.
> 

Thanks for ur time and the review :)

[WebKit Review Bot]

Comment on attachment 151531 [details]
Patch for landing

Clearing flags on attachment: 151531

Committed r122282: <http://trac.webkit.org/changeset/122282>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.