Don't add artifical 1 pixel width to empty tables

Don't add artifical 1 pixel width to empty tables

Don't use a dummy width of 1 pixel for empty columns when calculating the width of
tables. Instead let them be zero width so that empty tables get a zero width.

We still need a dummy value for empty columns when tracking the allocation of width
to columns so do retain that.

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

[rhogan, 2015-03-23 20:16:55]

robhogan@gmail.com changed reviewers:
+ dsinclair@chromium.org

[dsinclair, 2015-03-24 00:03:48]

https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
File Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:546: totalFixed +=
std::max<int>(1, m_layoutStruct[i].effectiveMaxLogicalWidth);
We do this a bunch of times, would it be better to change how we store
effectiveMaxLogicalWidth to so it does the std::max call implicitly? Or, do we
need to use it in cases where we don't want the 1 minimum?

[dsinclair, 2015-03-24 00:03:55]

dsinclair@chromium.org changed reviewers:
+ jchaffraix@chromium.org

[mstensho (USE GERRIT), 2015-03-24 08:36:36]

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

[mstensho (USE GERRIT), 2015-03-24 08:36:37]

Should add a layout test?

https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
File Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:546: totalFixed +=
std::max<int>(1, m_layoutStruct[i].effectiveMaxLogicalWidth);
On 2015/03/24 00:03:48, dsinclair wrote:
> We do this a bunch of times, would it be better to change how we store
> effectiveMaxLogicalWidth to so it does the std::max call implicitly? Or, do we
> need to use it in cases where we don't want the 1 minimum?

I don't think I understand why it's still necessary to clamp to 1 like this.
Sounds like a bug / shortcoming in the column width distribution algorithm.

[rhogan, 2015-03-24 20:06:12]

https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
File Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:546: totalFixed +=
std::max<int>(1, m_layoutStruct[i].effectiveMaxLogicalWidth);
On 2015/03/24 at 08:36:37, mstensho wrote:
> On 2015/03/24 00:03:48, dsinclair wrote:
> > We do this a bunch of times, would it be better to change how we store
> > effectiveMaxLogicalWidth to so it does the std::max call implicitly? Or, do
we
> > need to use it in cases where we don't want the 1 minimum?
> 
> I don't think I understand why it's still necessary to clamp to 1 like this.
Sounds like a bug / shortcoming in the column width distribution algorithm.

It uses the clamped value when deciding what proportion of the table's width the
column represents and also when figuring out how much width is left to
distribute.

I think I can represent it better per dsinclair's suggestion.

[mstensho (USE GERRIT), 2015-03-25 10:10:52]

https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
File Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:546: totalFixed +=
std::max<int>(1, m_layoutStruct[i].effectiveMaxLogicalWidth);
On 2015/03/24 20:06:11, rhogan wrote:
> On 2015/03/24 at 08:36:37, mstensho wrote:
> > On 2015/03/24 00:03:48, dsinclair wrote:
> > > We do this a bunch of times, would it be better to change how we store
> > > effectiveMaxLogicalWidth to so it does the std::max call implicitly? Or,
do
> we
> > > need to use it in cases where we don't want the 1 minimum?
> > 
> > I don't think I understand why it's still necessary to clamp to 1 like this.
> Sounds like a bug / shortcoming in the column width distribution algorithm.
> 
> It uses the clamped value when deciding what proportion of the table's width
the
> column represents and also when figuring out how much width is left to
> distribute.

I think that it's the width distribution algorithm that needs to be fixed, so
that it can cope with 0 max width. Zero-width columns should only receive a
portion of extra space if there are no better candidates (columns with
percentages, columns with fixed width, columns with non-zero max width). Blink
seems to lack this step in the width distribution algorithm.

<p>There should be no red.</p>
<table cellspacing="0" cellpadding="0" style="width:100%;">
    <tr>
        <td style="background:red;"></td>
        <td>cell2</td>
    </tr>
</table>

This is essentially crbug.com/406273 and I think that's what should be fixed.
Then crbug.com/424338 (what you're aiming to fix) should automatically go away
too.

[rhogan, 2015-04-06 17:54:38]

On 2015/03/25 at 10:10:52, mstensho wrote:
>
https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
> File Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):
> 
>
https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
> Source/core/layout/TableLayoutAlgorithmAuto.cpp:546: totalFixed +=
std::max<int>(1, m_layoutStruct[i].effectiveMaxLogicalWidth);
> On 2015/03/24 20:06:11, rhogan wrote:
> > On 2015/03/24 at 08:36:37, mstensho wrote:
> > > On 2015/03/24 00:03:48, dsinclair wrote:
> > > > We do this a bunch of times, would it be better to change how we store
> > > > effectiveMaxLogicalWidth to so it does the std::max call implicitly? Or,
do
> > we
> > > > need to use it in cases where we don't want the 1 minimum?
> > > 
> > > I don't think I understand why it's still necessary to clamp to 1 like
this.
> > Sounds like a bug / shortcoming in the column width distribution algorithm.
> > 
> > It uses the clamped value when deciding what proportion of the table's width
the
> > column represents and also when figuring out how much width is left to
> > distribute.
> 
> I think that it's the width distribution algorithm that needs to be fixed, so
that it can cope with 0 max width. Zero-width columns should only receive a
portion of extra space if there are no better candidates (columns with
percentages, columns with fixed width, columns with non-zero max width). Blink
seems to lack this step in the width distribution algorithm.
> 
> <p>There should be no red.</p>
> <table cellspacing="0" cellpadding="0" style="width:100%;">
>     <tr>
>         <td style="background:red;"></td>
>         <td>cell2</td>
>     </tr>
> </table>
> 
> This is essentially crbug.com/406273 and I think that's what should be fixed.
Then crbug.com/424338 (what you're aiming to fix) should automatically go away
too.

Interestingly your testcase above only fails because we special-case columns
with a background as non-empty. That seems wrong and when I remove it I can make
the rendering of LayoutTests/fast/table/empty-cells.html match Presto. Each of
FF, IE, Presto and WebKit/Blink render that page (which is a good test of extra
width distribution on tables with empty columns) differently but I think I'm
most comfortable with your proposal that for zero-width columns we try
distributing leftover width to each of the different types of columns with
zero-width ones as the last. The only step we're currently missing is the last
one.

The one oddity with this approach is that we can and do (like Presto) end up
distributing width to the same column twice. So in the example below, the 50%
column ends up 100% wide.

 <table cellspacing="0" cellpadding="0" style="width:100%;">
     <tr>
         <td style="background:red"></td>
         <td style="width:50%;">cell2</td>
     </tr>
 </table>

We only avoid doing this currently because of our wrong-headed special casing of
cells with background. You comfortable with what I'm suggesting?

[mstensho (USE GERRIT), 2015-04-13 22:02:47]

On 2015/04/06 17:54:38, rhogan wrote:
> On 2015/03/25 at 10:10:52, mstensho wrote:
> >
>
https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
> > File Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):
> > 
> >
>
https://codereview.chromium.org/988443003/diff/80001/Source/core/layout/Table...
> > Source/core/layout/TableLayoutAlgorithmAuto.cpp:546: totalFixed +=
> std::max<int>(1, m_layoutStruct[i].effectiveMaxLogicalWidth);
> > On 2015/03/24 20:06:11, rhogan wrote:
> > > On 2015/03/24 at 08:36:37, mstensho wrote:
> > > > On 2015/03/24 00:03:48, dsinclair wrote:
> > > > > We do this a bunch of times, would it be better to change how we store
> > > > > effectiveMaxLogicalWidth to so it does the std::max call implicitly?
Or,
> do
> > > we
> > > > > need to use it in cases where we don't want the 1 minimum?
> > > > 
> > > > I don't think I understand why it's still necessary to clamp to 1 like
> this.
> > > Sounds like a bug / shortcoming in the column width distribution
algorithm.
> > > 
> > > It uses the clamped value when deciding what proportion of the table's
width
> the
> > > column represents and also when figuring out how much width is left to
> > > distribute.
> > 
> > I think that it's the width distribution algorithm that needs to be fixed,
so
> that it can cope with 0 max width. Zero-width columns should only receive a
> portion of extra space if there are no better candidates (columns with
> percentages, columns with fixed width, columns with non-zero max width). Blink
> seems to lack this step in the width distribution algorithm.
> > 
> > <p>There should be no red.</p>
> > <table cellspacing="0" cellpadding="0" style="width:100%;">
> >     <tr>
> >         <td style="background:red;"></td>
> >         <td>cell2</td>
> >     </tr>
> > </table>
> > 
> > This is essentially crbug.com/406273 and I think that's what should be
fixed.
> Then crbug.com/424338 (what you're aiming to fix) should automatically go away
> too.
> 
> Interestingly your testcase above only fails because we special-case columns
> with a background as non-empty. That seems wrong

Indeed. Completely bogus, that.

> and when I remove it I can make
> the rendering of LayoutTests/fast/table/empty-cells.html match Presto. Each of
> FF, IE, Presto and WebKit/Blink render that page (which is a good test of
extra
> width distribution on tables with empty columns) differently but I think I'm
> most comfortable with your proposal that for zero-width columns we try
> distributing leftover width to each of the different types of columns with
> zero-width ones as the last. The only step we're currently missing is the last
> one.
> 
> The one oddity with this approach is that we can and do (like Presto) end up
> distributing width to the same column twice. So in the example below, the 50%
> column ends up 100% wide.
> 
>  <table cellspacing="0" cellpadding="0" style="width:100%;">
>      <tr>
>          <td style="background:red"></td>
>          <td style="width:50%;">cell2</td>
>      </tr>
>  </table>

Is this behavior really Presto-specific? I'm on Linux now, so the only other
interesting engine I can test is Gecko, which does indeed show 50% red. In my
mind, percentage width columns are better recipients for extra space than
zero-width cells. When I implemented this for Presto, I used IE for inspiration
most of the time. IE is after all de facto always doing the correct thing for
table layout. :)

> We only avoid doing this currently because of our wrong-headed special casing
of
> cells with background. You comfortable with what I'm suggesting?

I think so. I.e. get rid of the background-means-non-empty hack and add the
final step to the width distribution algorithm, so that columns with zero
desired width get their equal share of extra width, if there are no better
columns to stretch. This is a semi-risky change, though, and I fear that stuff
will break (even if it brings us closer to what IE and the others do).

[rhogan, 2015-05-16 12:04:49]

On 2015/04/13 at 22:02:47, mstensho wrote:

> I think so. I.e. get rid of the background-means-non-empty hack and add the
final step to the width distribution algorithm, so that columns with zero
desired width get their equal share of extra width, if there are no better
columns to stretch. This is a semi-risky change, though, and I fear that stuff
will break (even if it brings us closer to what IE and the others do).

This is ready for you to take a look at now.

[mstensho (USE GERRIT), 2015-05-18 08:45:39]

https://codereview.chromium.org/988443003/diff/140001/LayoutTests/fast/table/...
File LayoutTests/fast/table/large-shrink-wrapped-width.html (right):

https://codereview.chromium.org/988443003/diff/140001/LayoutTests/fast/table/...
LayoutTests/fast/table/large-shrink-wrapped-width.html:30: <!-- The 1 px extra
is from the align=right td. -->
Strange how align=right could do that, but now the extra pixel is gone, so these
comments are invalid and should be removed.

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

https://codereview.chromium.org/988443003/diff/140001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:45: static bool
isEmptyCell(LayoutObject* object)
I don't get this.

<p>Should be 50% lime and 50% cyan.</p>
<table cellspacing="0" cellpadding="0" style="width:100%; height:50px;">
    <tr>
        <td style="background:lime;"></td>
        <td style="background:cyan;"><div></div></td>
    </tr>
</table>

With your changes, the two columns end up in different "spread categories", so
that all extra space is given to the second one that "has content". No other
browser does this. I think the "has content" distinction should go away
completely.

https://codereview.chromium.org/988443003/diff/140001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:80: bool cellHasContent =
cell->style()->hasBorder() || cell->style()->hasPadding() ||
!isEmptyCell(cell->firstChild());
This certainly looks like a step in the right direction, but, as I wrote above,
I think the "has content" thing should go away (maybe except for using it to set
columnLayout.emptyCellsOnly; not sure how that one works). Why even bother to
check for border of padding. If such things are present, min logical width
should already be larger than 0, so there should be no need to force it to 1.

https://codereview.chromium.org/988443003/diff/140001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:631:
distributeWidthToColumns<unsigned, Auto, EmptyCells, LeftoverWidth,
EndToStart>(available, total);
Is it possible to over-allocate here? If so, don't you need to add a
corresponding shrink step?

If it's not possible to over-allocate, you should ASSERT on that after having
distributed.

[rhogan, 2015-05-19 19:45:42]

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

https://codereview.chromium.org/988443003/diff/140001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:45: static bool
isEmptyCell(LayoutObject* object)
On 2015/05/18 at 08:45:38, mstensho wrote:
> I don't get this.
> 
> <p>Should be 50% lime and 50% cyan.</p>
> <table cellspacing="0" cellpadding="0" style="width:100%; height:50px;">
>     <tr>
>         <td style="background:lime;"></td>
>         <td style="background:cyan;"><div></div></td>
>     </tr>
> </table>
> 
> With your changes, the two columns end up in different "spread categories", so
that all extra space is given to the second one that "has content". No other
browser does this. I think the "has content" distinction should go away
completely.

The second one should definitely be treated as empty. In fact, I realized while
working through your review that we should be allocating initial width to
columns that have only empty cells. Doing this allows us to match FF on
empty-cells.html and is just saner in general. I think the reason it wasn't done
before is that columns with empty cells were sometimes shorthand for columns
that had no cells of their own and just fleshed out a colspan so it wasn't safe,
without recognizing that's what they were, to go ahead
and allocate width.

https://codereview.chromium.org/988443003/diff/140001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:80: bool cellHasContent =
cell->style()->hasBorder() || cell->style()->hasPadding() ||
!isEmptyCell(cell->firstChild());
On 2015/05/18 at 08:45:38, mstensho wrote:
> This certainly looks like a step in the right direction, but, as I wrote
above, I think the "has content" thing should go away (maybe except for using it
to set columnLayout.emptyCellsOnly; not sure how that one works). Why even
bother to check for border of padding. If such things are present, min logical
width should already be larger than 0, so there should be no need to force it to
1.

Good point - and in your example above (which I've added as a test) both should
be treated as empty cells. So I've adapted the check to look for minimum
preferred width and failing that, a div element with non-empty children.

https://codereview.chromium.org/988443003/diff/140001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:631:
distributeWidthToColumns<unsigned, Auto, EmptyCells, LeftoverWidth,
EndToStart>(available, total);
On 2015/05/18 at 08:45:39, mstensho wrote:
> Is it possible to over-allocate here? If so, don't you need to add a
corresponding shrink step?
> 
> If it's not possible to over-allocate, you should ASSERT on that after having
distributed.

No, the shrink step is keyed to Length type only - it's agnostic to empty cells.

https://codereview.chromium.org/988443003/diff/180001/LayoutTests/accessibili...
File LayoutTests/accessibility/table-detection.html (left):

https://codereview.chromium.org/988443003/diff/180001/LayoutTests/accessibili...
LayoutTests/accessibility/table-detection.html:88: <td nowrap="1"
class="labelPol_080915_Lehman_Contributions">
The author of this test incorrectly thought that an unclosed <td> was parsed as
invalid. In fact it's parsed as valid and treated as empty. i.e. contributing no
width. Now we've changed this behaviour to match FF and it gets width, so to
preserve the test result we just remove the cell.

[mstensho (USE GERRIT), 2015-05-20 09:34:22]

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

https://codereview.chromium.org/988443003/diff/140001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:629: if (available > 0) {
Why not remove this code block entirely? You now spread over empty cells before
distributing excess space, so I don't see how we would ever get here.

https://codereview.chromium.org/988443003/diff/160001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/988443003/diff/160001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1659: crbug.com/477076
fast/block/margin-collapse/004.html [ NeedsRebaseline ]
These seem unrelated.

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

https://codereview.chromium.org/988443003/diff/160001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:603: if (available > 0 &&
numAutoEmptyCellsOnly) {
A comment in front of this would be nice.

https://codereview.chromium.org/988443003/diff/160001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:608: // Any remaining available
width expands fixed width, percent width, non-empty auto width columns and empty
auto width columns, in that order.
Should you put the following inside a large else block? If there were empty
cells above, there should be no space left to distribute.

https://codereview.chromium.org/988443003/diff/160001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:654: // When allocating width to
columns with nothing but empty cells we avoid columns that exist only to flesh
out a colspan and have no actual cells.
Could you break this comment into two lines?

[rhogan, 2015-06-01 16:51:34]

On 2015/05/20 at 09:34:22, mstensho wrote:
>
https://codereview.chromium.org/988443003/diff/140001/Source/core/layout/Tabl...
> Source/core/layout/TableLayoutAlgorithmAuto.cpp:629: if (available > 0) {
> Why not remove this code block entirely? You now spread over empty cells
before distributing excess space, so I don't see how we would ever get here.

You're right - I've removed it.

>
https://codereview.chromium.org/988443003/diff/160001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:1659: crbug.com/477076
fast/block/margin-collapse/004.html [ NeedsRebaseline ]
> These seem unrelated.
Removed.

>
https://codereview.chromium.org/988443003/diff/160001/Source/core/layout/Tabl...
> Source/core/layout/TableLayoutAlgorithmAuto.cpp:603: if (available > 0 &&
numAutoEmptyCellsOnly) {
> A comment in front of this would be nice.
Done.

> 
>
https://codereview.chromium.org/988443003/diff/160001/Source/core/layout/Tabl...
> Source/core/layout/TableLayoutAlgorithmAuto.cpp:608: // Any remaining
available width expands fixed width, percent width, non-empty auto width columns
and empty auto width columns, in that order.
> Should you put the following inside a large else block? If there were empty
cells above, there should be no space left to distribute.
Didn't do this. We can still have space left over, as the empty cells may be
fleshing out a colspan. See my comment in distributedWidthToColumns().

> 
>
https://codereview.chromium.org/988443003/diff/160001/Source/core/layout/Tabl...
> Source/core/layout/TableLayoutAlgorithmAuto.cpp:654: // When allocating width
to columns with nothing but empty cells we avoid columns that exist only to
flesh out a colspan and have no actual cells.
> Could you break this comment into two lines?

Done.

[mstensho (USE GERRIT), 2015-06-01 18:01:49]

lgtm

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

https://codereview.chromium.org/988443003/diff/200001/Source/core/layout/Tabl...
Source/core/layout/TableLayoutAlgorithmAuto.cpp:29: #include
"core/layout/LayoutText.h"
Don't need to include this.

[rhogan, 2015-06-02 18:19:39]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-06-02 18:19:40]

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/988443003/#ps220001
(title: "Updated")

[commit-bot: I haz the power, 2015-06-02 18:19:56]

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

[commit-bot: I haz the power, 2015-06-02 19:25:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-02 19:25:52]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/64840)

[rhogan, 2015-06-02 20:00:00]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-06-02 20:00:42]

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

[commit-bot: I haz the power, 2015-06-02 22:01:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-02 22:01:10]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[rhogan, 2015-06-03 18:19:51]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-06-03 18:19:52]

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/988443003/#ps240001
(title: "Updated")

[commit-bot: I haz the power, 2015-06-03 18:21:14]

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

[commit-bot: I haz the power, 2015-06-03 19:35:17]

Committed patchset #13 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196425

[rhogan, 2015-06-16 18:21:26]

Hi Morten,

This was rolled out due to crbug.com/497759 and crbug.com/497073 - both of which
are the same bug and are covered by the new test
LayoutTests/fast/table/empty-cells-and-percentage-width-content.html.

I now use maxPreferredWidth() instead of minPreferredWidth() to decide if a cell
has content that will give it a width.

So ready for another go!

[mstensho (USE GERRIT), 2015-06-16 18:44:39]

lgtm.

One thing I'd like to ask from you for future CLs: could you please separate
master rebases and actual changes into separate patches? It's much easier to
review that way. :)

https://codereview.chromium.org/988443003/diff/260001/LayoutTests/fast/table/...
File LayoutTests/fast/table/empty-cells-and-percentage-width-content.html
(right):

https://codereview.chromium.org/988443003/diff/260001/LayoutTests/fast/table/...
LayoutTests/fast/table/empty-cells-and-percentage-width-content.html:21:
checkLayout('td', document.getElementById('console'));
The first TD in this document has no expectations, so I think you should either
not select it, or add an expectation.

[rhogan, 2015-06-16 18:50:33]

On 2015/06/16 at 18:44:39, mstensho wrote:
> lgtm.
> 
> One thing I'd like to ask from you for future CLs: could you please separate
master rebases and actual changes into separate patches? It's much easier to
review that way. :)

You mean put the rebaselined results in a separate patch? If so, sure - but I'm
surprised it's easier as I assumed you would want to cast your eye over the new
results while reviewing the code change.

> 
>
https://codereview.chromium.org/988443003/diff/260001/LayoutTests/fast/table/...
> File LayoutTests/fast/table/empty-cells-and-percentage-width-content.html
(right):
> 
>
https://codereview.chromium.org/988443003/diff/260001/LayoutTests/fast/table/...
> LayoutTests/fast/table/empty-cells-and-percentage-width-content.html:21:
checkLayout('td', document.getElementById('console'));
> The first TD in this document has no expectations, so I think you should
either not select it, or add an expectation.

[mstensho (USE GERRIT), 2015-06-16 18:57:32]

On 2015/06/16 18:50:33, rhogan wrote:
> On 2015/06/16 at 18:44:39, mstensho wrote:
> > lgtm.
> > 
> > One thing I'd like to ask from you for future CLs: could you please separate
> master rebases and actual changes into separate patches? It's much easier to
> review that way. :)
> 
> You mean put the rebaselined results in a separate patch? If so, sure - but
I'm
> surprised it's easier as I assumed you would want to cast your eye over the
new
> results while reviewing the code change.

Oh, no, not that.

Your latest patch contains both changes you made to fix the bug, AND stuff from
upstream (changing from "0" to "nullptr", for instance, in
TableLayoutAlgorithmAuto.cpp). That's what I'm talking about. I'd like those
things in two separate patch sets.

[rhogan, 2015-06-21 09:47:57]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-06-21 09:47:58]

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/988443003/#ps340001
(title: "Updated")

[commit-bot: I haz the power, 2015-06-21 09:48:03]

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

[commit-bot: I haz the power, 2015-06-21 09:55:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-21 09:55:13]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[rhogan, 2015-06-21 09:58:14]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-06-21 09:58:15]

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/988443003/#ps360001
(title: "Updated")

[commit-bot: I haz the power, 2015-06-21 09:58:27]

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

[commit-bot: I haz the power, 2015-06-21 10:00:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-21 10:00:23]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59957)

[rhogan, 2015-06-21 15:37:31]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-06-21 15:37:31]

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/988443003/#ps400001
(title: "Updated")

[commit-bot: I haz the power, 2015-06-21 15:37:35]

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

[commit-bot: I haz the power, 2015-06-21 17:08:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-21 17:08:54]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59962)

[rhogan, 2015-06-21 18:34:15]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-06-21 18:34:23]

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

[commit-bot: I haz the power, 2015-06-21 20:05:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-21 20:05:38]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59969)

[rhogan, 2015-06-21 21:12:41]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-06-21 21:12:47]

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

[commit-bot: I haz the power, 2015-06-21 22:40:58]

Committed patchset #21 (id:400001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197544