Refactor the code distributing width to columns in auto layout tables.

Refactor the code distributing width to columns in auto layout tables.

Come up with a function that we can use for distributing width across different
types of columns. In a subsequent CL we will use this function for distributing
width to columns with empty cells.

BUG=476370
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195309

[rhogan, 2015-04-28 13:33:48]

robhogan@gmail.com changed reviewers:
+ mstensho@opera.com

[rhogan, 2015-04-28 13:44:08]

https://codereview.chromium.org/1111443003/diff/40001/Source/core/layout/Tabl...
File Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/1111443003/diff/40001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:607: }
This End to Start order for this left over width isn't specified anywhere but
seems to be the the way IE and FF do it too.

https://codereview.chromium.org/1111443003/diff/40001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:654: return;
This is a change. We only bail early for auto and percent currently if we run
out of width to allocate. And we only bail early if we run out available when
examining percent columns.

I suspect we should do:

If (available <= 0 || total <=0)
    return;

instead.

[mstensho (USE GERRIT), 2015-04-28 21:53:13]

https://codereview.chromium.org/1111443003/diff/40001/Source/core/layout/Tabl...
File Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/1111443003/diff/40001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:607: }
On 2015/04/28 13:44:08, rhogan wrote:
> This End to Start order for this left over width isn't specified anywhere but
> seems to be the the way IE and FF do it too.

OK. Worth a comment?

https://codereview.chromium.org/1111443003/diff/40001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:654: return;
On 2015/04/28 13:44:08, rhogan wrote:
> This is a change. We only bail early for auto and percent currently if we run
> out of width to allocate. And we only bail early if we run out available when
> examining percent columns.
> 
> I suspect we should do:
> 
> If (available <= 0 || total <=0)
>     return;
> 
> instead.

But is it safe to change the bailing policy at all? Would that affect column
widths in some cases?

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
File Source/core/layout/TableLayoutAlgorithmAuto.cpp (left):

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:593: // now satisfy variable
I think I'd like to keep these comments.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
File Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:605: unsigned total = nEffCols -
numAutoEmptyCellsOnly;
Could be signed int now.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:627: 
Extraneous blank line.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:633: Length& logicalWidth =
m_layoutStruct[i].effectiveLogicalWidth;
Please make it a const reference.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:641: factor = 1;
It's already 1 at this point.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:643: factor =
logicalWidth.percent();
Losing the fraction part of the value here, which would be a regression. Try
placing a column with 1% next to one with 1.5%. I guess factor needs to be
either float or LayoutUnit.

For even more fun, try a column with 0.1% next to one with 0.5%. Division by
zero - SIGFPE. :) That's thanks to |total| being int. Used to be float for
percentages.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:653: if (available <=0  || total
<= 0)
"<=0  ||" -> "<= 0 ||"

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
File Source/core/layout/TableLayoutAlgorithmAuto.h (right):

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.h:34: enum LimitToCells {
Maybe "CellsToProcess" instead?

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.h:42: LeftOverWidth
Shouldn't that be "LeftoverWidth"? "Leftover" is a dictionary word.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.h:66: void
distributeWidthToColumns(const LengthType&, int& available, int total,
LimitToCells = AllCells, DistributionMode = ExtraWidth, DistributionDirection =
StartToEnd);
Just wondering if it should be called distributeExtraWidthToColumns() instead?
This is what takes place after all desired widths have been satisfied and we
still have more space to distribute.

Also, I think the default values are somewhat non-intuitive, and I think it
would help readability at the call sites to just drop them.

[rhogan, 2015-05-04 18:10:12]

On 2015/04/28 at 21:53:13, mstensho wrote:
> 
> But is it safe to change the bailing policy at all? Would that affect column
widths in some cases?
Yes, I think it is safe. See my comment in the code.

>
https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
> Source/core/layout/TableLayoutAlgorithmAuto.cpp:605: unsigned total = nEffCols
- numAutoEmptyCellsOnly;
> Could be signed int now.
I kept it unsigned. Now it will cast to a float.

>
https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
> Source/core/layout/TableLayoutAlgorithmAuto.cpp:643: factor =
logicalWidth.percent();
> Losing the fraction part of the value here, which would be a regression. Try
placing a column with 1% next to one with 1.5%. I guess factor needs to be
either float or LayoutUnit.
> 
> For even more fun, try a column with 0.1% next to one with 0.5%. Division by
zero - SIGFPE. :) That's thanks to |total| being int. Used to be float for
percentages.

Yes indeedy. Added tests for this and changed factor and total to floats.

>
https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
> Source/core/layout/TableLayoutAlgorithmAuto.h:66: void
distributeWidthToColumns(const LengthType&, int& available, int total,
LimitToCells = AllCells, DistributionMode = ExtraWidth, DistributionDirection =
StartToEnd);
> Just wondering if it should be called distributeExtraWidthToColumns() instead?
This is what takes place after all desired widths have been satisfied and we
still have more space to distribute.
It's also used to distribute the initial width to auto columns (using
InitialWidth) so that wouldn't be a good rename.

> Also, I think the default values are somewhat non-intuitive, and I think it
would help readability at the call sites to just drop them.
Wasn't sure what you meant here.

[mstensho (USE GERRIT), 2015-05-04 20:42:48]

https://codereview.chromium.org/1111443003/diff/40001/Source/core/layout/Tabl...
File Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/1111443003/diff/40001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:607: }
On 2015/04/28 21:53:12, mstensho wrote:
> On 2015/04/28 13:44:08, rhogan wrote:
> > This End to Start order for this left over width isn't specified anywhere
but
> > seems to be the the way IE and FF do it too.
> 
> OK. Worth a comment?

Acknowledged.

https://codereview.chromium.org/1111443003/diff/40001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:654: return;
On 2015/04/28 21:53:12, mstensho wrote:
> On 2015/04/28 13:44:08, rhogan wrote:
> > This is a change. We only bail early for auto and percent currently if we
run
> > out of width to allocate. And we only bail early if we run out available
when
> > examining percent columns.
> > 
> > I suspect we should do:
> > 
> > If (available <= 0 || total <=0)
> >     return;
> > 
> > instead.
> 
> But is it safe to change the bailing policy at all? Would that affect column
> widths in some cases?

OK, you added a comment, so I'll just trust you. :) It looks safe to me too.

Would be rather neat to avoid this oops-I-grew-it-too-much-lets-shrink-it thing
at all, though. Should be completely possible to just get it right (Presto does
it). But that's for a different CL, or maybe even a different life. :)

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
File Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:627: 
On 2015/04/28 21:53:13, mstensho wrote:
> Extraneous blank line.

Acknowledged.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:633: Length& logicalWidth =
m_layoutStruct[i].effectiveLogicalWidth;
On 2015/04/28 21:53:12, mstensho wrote:
> Please make it a const reference.

Acknowledged.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:641: factor = 1;
On 2015/04/28 21:53:12, mstensho wrote:
> It's already 1 at this point.

Acknowledged.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:643: factor =
logicalWidth.percent();
On 2015/04/28 21:53:13, mstensho wrote:
> Losing the fraction part of the value here, which would be a regression. Try
> placing a column with 1% next to one with 1.5%. I guess factor needs to be
> either float or LayoutUnit.
> 
> For even more fun, try a column with 0.1% next to one with 0.5%. Division by
> zero - SIGFPE. :) That's thanks to |total| being int. Used to be float for
> percentages.

Not sure if using floats all over the place is right, either. That could affect
both precision and performance, couldn't it?

This entire method looks like it could benefit from a more template-based
approach. A lot of stuff can be determined at compile-time.

How about something like:

    template<typename T, LengthType, CellsToProcess, DistributionMode,
DistributionDirection>
    void distributeWidthToColumns(int& available, T total);

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:653: if (available <=0  || total
<= 0)
On 2015/04/28 21:53:12, mstensho wrote:
> "<=0  ||" -> "<= 0 ||"

Acknowledged.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
File Source/core/layout/TableLayoutAlgorithmAuto.h (right):

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.h:34: enum LimitToCells {
On 2015/04/28 21:53:13, mstensho wrote:
> Maybe "CellsToProcess" instead?

Acknowledged.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.h:42: LeftOverWidth
On 2015/04/28 21:53:13, mstensho wrote:
> Shouldn't that be "LeftoverWidth"? "Leftover" is a dictionary word.

Acknowledged.

https://codereview.chromium.org/1111443003/diff/60001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.h:66: void
distributeWidthToColumns(const LengthType&, int& available, int total,
LimitToCells = AllCells, DistributionMode = ExtraWidth, DistributionDirection =
StartToEnd);
On 2015/04/28 21:53:13, mstensho wrote:
> Just wondering if it should be called distributeExtraWidthToColumns() instead?
> This is what takes place after all desired widths have been satisfied and we
> still have more space to distribute.

You explained why this would be a bad move. So the current name is fine.

> Also, I think the default values are somewhat non-intuitive, and I think it
> would help readability at the call sites to just drop them.

What I meant to suggest was to remove all default parameter values, like this:

void distributeWidthToColumns(const LengthType&, int& available, int total,
LimitToCells, DistributionMode, DistributionDirection)

... but see my suggestion regarding using templates.

https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:206: crbug.com/476370 [ Mac Win ]
fast/table/fixed-widths-exceed-available.html [ NeedsRebaseline ]
Why this? Aren't your tests going to pass on all platforms?

https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/fast/table/...
File LayoutTests/fast/table/percent-widths-float.html (right):

https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/fast/table/...
LayoutTests/fast/table/percent-widths-float.html:15: <p> crbug.com/476370: Test
cells get appropriate width when we use float percentage values. </p>
"float" as in "fractional", perhaps?

https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/fast/table/...
File LayoutTests/fast/table/percent-widths-total-less-than-zero.html (right):

https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/fast/table/...
LayoutTests/fast/table/percent-widths-total-less-than-zero.html:1: <!DOCTYPE
HTML>
File name is "...-less-than-zero.html".

Should be "...-less-than-one.html" perhaps? :)

[rhogan, 2015-05-12 08:22:06]

On 2015/05/04 at 20:42:48, mstensho wrote:
> Not sure if using floats all over the place is right, either. That could
affect both precision and performance, couldn't it?
> 
> This entire method looks like it could benefit from a more template-based
approach. A lot of stuff can be determined at compile-time.
> 
> How about something like:
> 
>     template<typename T, LengthType, CellsToProcess, DistributionMode,
DistributionDirection>
>     void distributeWidthToColumns(int& available, T total);

Done.

>
https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:206: crbug.com/476370 [ Mac Win ]
fast/table/fixed-widths-exceed-available.html [ NeedsRebaseline ]
> Why this? Aren't your tests going to pass on all platforms?
Sub-pixel differences mean the values are slightly different for Mac and
Windows.

>
https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/fast/table/...
> LayoutTests/fast/table/percent-widths-total-less-than-zero.html:1: <!DOCTYPE
HTML>
> File name is "...-less-than-zero.html".
> 
> Should be "...-less-than-one.html" perhaps? :)

Done.

[mstensho (USE GERRIT), 2015-05-12 08:50:28]

https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:206: crbug.com/476370 [ Mac Win ]
fast/table/fixed-widths-exceed-available.html [ NeedsRebaseline ]
On 2015/05/04 20:42:48, mstensho wrote:
> Why this? Aren't your tests going to pass on all platforms?

So this means that there'll be FAIL in the expectation files on Windows and Mac?
Doesn't sound good.

https://codereview.chromium.org/1111443003/diff/100001/Source/core/layout/Tab...
File Source/core/layout/TableLayoutAlgorithmAuto.h (right):

https://codereview.chromium.org/1111443003/diff/100001/Source/core/layout/Tab...
Source/core/layout/TableLayoutAlgorithmAuto.h:66: template<typename Total> void
distributeWidthToColumns(const LengthType&, int& available, Total,
CellsToProcess, DistributionMode, DistributionDirection);
I was suggesting that you moved LengthType, CellsToProcess, DistributionMode and
DistributionDirection to the template side as well.

[rhogan, 2015-05-12 18:30:26]

On 2015/05/12 at 08:50:28, mstensho wrote:
>
https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/TestExpecta...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/1111443003/diff/80001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:206: crbug.com/476370 [ Mac Win ]
fast/table/fixed-widths-exceed-available.html [ NeedsRebaseline ]
> On 2015/05/04 20:42:48, mstensho wrote:
> > Why this? Aren't your tests going to pass on all platforms?
> 
> So this means that there'll be FAIL in the expectation files on Windows and
Mac? Doesn't sound good.

OK. I think they will actually pass - it's a rounding difference and that's
common code I think.

> 
>
https://codereview.chromium.org/1111443003/diff/100001/Source/core/layout/Tab...
> File Source/core/layout/TableLayoutAlgorithmAuto.h (right):
> 
>
https://codereview.chromium.org/1111443003/diff/100001/Source/core/layout/Tab...
> Source/core/layout/TableLayoutAlgorithmAuto.h:66: template<typename Total>
void distributeWidthToColumns(const LengthType&, int& available, Total,
CellsToProcess, DistributionMode, DistributionDirection);
> I was suggesting that you moved LengthType, CellsToProcess, DistributionMode
and DistributionDirection to the template side as well.

OK - had another go at this, wasn't clear on how it would work initially as I'm
not that conversant with templates.

[mstensho (USE GERRIT), 2015-05-12 20:06:35]

lgtm

[leviw_travelin_and_unemployed, 2015-05-13 02:13:53]

leviw@chromium.org changed reviewers:
+ leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-05-13 02:13:56]

RS LGTM, thanks, Morten :)

[rhogan, 2015-05-13 15:18:42]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-05-13 15:18:42]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1111443003/#ps140001
(title: "Updated")

[commit-bot: I haz the power, 2015-05-13 15:19:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1111443003/140001

[commit-bot: I haz the power, 2015-05-13 15:22:33]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195309