REGRESSION(81625): Tables are not rendered correctly

[Nico Weber]

See attached test case (passes if everything is green, fails if theres a red stripe to the right of the green)

Regressed in http://trac.webkit.org/changeset/81625

This breaks e.g. gmail, see http://crbug.com/78114

[Nico Weber]

Created attachment 90598 [details]
repro

[Rik Cabanier]

I will sync down the latest sources and take a look.

[Simon Fraser (smfr)]

<rdar://problem/9320145>

[Adele Peterson]

Do we know of any cases where the original change caused a progression?  If not, maybe we should roll it out.

[Nico Weber]

jamesr tells me the original change doesn't revert cleanly because other changes have been made on top of it.

[Simon Fraser (smfr)]

The original change was related to not rounding to pixel values for transforms.

[Rik Cabanier]

Created attachment 90839 [details]
bug fix

[Nico Weber]

Missing a test case.

[Nico Weber]

Comment on attachment 90839 [details]
bug fix

(I'm not a reviewer, but I'm pretty sure the other folks would ask for a test as well. My attachment should be a good starting point.)

Also, putting |else| and |if| on the same line would be nice.

[Rik Cabanier]

his bug was introduced by the change that made '%' a true float instead of a fixed.
There's some strange code in the AutoTableLayout file that sometimes returns incorrect results (I didn't create this). Other code in WebKit sees these (large) results and ignores them.

I didn't realize that the code at the end already tries to fix this for certain cases so my change was wrong.

Adding a simple 'else' so my code doesn't get invoked fixes the problem.

[Rik Cabanier]

(In reply to comment #9)
> (From update of attachment 90839 [details])
> (I'm not a reviewer, but I'm pretty sure the other folks would ask for a test as well. My attachment should be a good starting point.)
> 
> Also, putting |else| and |if| on the same line would be nice.

What do you mean with |else| and |if| ?

[Nico Weber]

Comment on attachment 90839 [details]
bug fix

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

> Source/WebCore/rendering/AutoTableLayout.cpp:270
>      if (!remainingPercent && maxNonPercent)

You're saying

  else
  // comment
  if (foo)

I think 

  // comment
  else if (foo)

is nicer.

(And I guess

bool maxWidthValid = !remainingPercent && maxNonPercent;
  ...
  else if (maxWidthValid)

is even more WebKit-style.)

[Rik Cabanier]

Created attachment 90841 [details]
bug fix with testcase

[Simon Fraser (smfr)]

Comment on attachment 90841 [details]
bug fix with testcase

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

r- for lack of working test.

> Source/WebCore/rendering/AutoTableLayout.cpp:270
> +    } else
>      // if there was no remaining percent, maxWidth is invalid.
>      if (!remainingPercent && maxNonPercent)

I'd prefer the 'else if' together, and the comment after the condition

> LayoutTests/fast/table/auto-100-percent-width.html:8
> +function test()
> +{
> +    if (window.layoutTestController)
> +        layoutTestController.dumpAsText();
> +}

Where is the test code?

[Rik Cabanier]

(In reply to comment #14)
> (From update of attachment 90841 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90841&action=review
> 
> r- for lack of working test.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:270
> > +    } else
> >      // if there was no remaining percent, maxWidth is invalid.
> >      if (!remainingPercent && maxNonPercent)
> 
> I'd prefer the 'else if' together, and the comment after the condition

OK. I will change it

> 
> > LayoutTests/fast/table/auto-100-percent-width.html:8
> > +function test()
> > +{
> > +    if (window.layoutTestController)
> > +        layoutTestController.dumpAsText();
> > +}
> 
> Where is the test code?

Doesn't it take a dump of the layout?
The table in the body should be the test.

[Rik Cabanier]

Created attachment 90842 [details]
bug fix with comment moved

[Simon Fraser (smfr)]

Comment on attachment 90842 [details]
bug fix with comment moved

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

> Source/WebCore/rendering/AutoTableLayout.cpp:270
> +    } else if (!remainingPercent && maxNonPercent)
> +        // if there was no remaining percent, maxWidth is invalid.
>          maxWidth = intMaxForLength;        

Braces are preferred when a comment is included above a single-line clause.

> LayoutTests/fast/table/auto-100-percent-width.html:9
> +function test()
> +{
> +    if (window.layoutTestController)
> +        layoutTestController.dumpAsText();
> +}
> +</script>

The dumpAsText() makes this a text test, so the output doesn't show the sizes of the render objects. You can see the output above; how does that reveal whether the bug is fixed?

Part of your workflow should be to ensure that the LayoutTest reproduces failure without the fix in the code.

I think in this case you can just remove the entire <script> block. The <div> is also unnecessary.

[Rik Cabanier]

(In reply to comment #17)
> (From update of attachment 90842 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90842&action=review
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:270
> > +    } else if (!remainingPercent && maxNonPercent)
> > +        // if there was no remaining percent, maxWidth is invalid.
> >          maxWidth = intMaxForLength;        
> 
> Braces are preferred when a comment is included above a single-line clause.

Will do

> 
> > LayoutTests/fast/table/auto-100-percent-width.html:9
> > +function test()
> > +{
> > +    if (window.layoutTestController)
> > +        layoutTestController.dumpAsText();
> > +}
> > +</script>
> 
> The dumpAsText() makes this a text test, so the output doesn't show the sizes of the render objects. You can see the output above; how does that reveal whether the bug is fixed?
> 
> Part of your workflow should be to ensure that the LayoutTest reproduces failure without the fix in the code.
> 
> I think in this case you can just remove the entire <script> block. The <div> is also unnecessary.

I was wondering about that. I assumed that it was putting it out as binary or something.
I will fix the text.

[Rik Cabanier]

Created attachment 90855 [details]
corrected test + fixed style

[Nico Weber]

Comment on attachment 90855 [details]
corrected test + fixed style

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

> LayoutTests/ChangeLog:8
> +        * fast/table/auto-100-percent-width-expected.txt: Added.

the -expected file is missing from this patch

> LayoutTests/fast/table/auto-100-percent-width.html:4
> +<body onload="test()">

Drop onload=test()?

[Rik Cabanier]

Created attachment 90856 [details]
corrected test + fixed style

[Rik Cabanier]

(In reply to comment #20)
> (From update of attachment 90855 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90855&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        * fast/table/auto-100-percent-width-expected.txt: Added.
> 
> the -expected file is missing from this patch
> 
> > LayoutTests/fast/table/auto-100-percent-width.html:4
> > +<body onload="test()">
> 
> Drop onload=test()?

I already noticed that.
Fixed in latest patch

[Nico Weber]

Thanks!

[Simon Fraser (smfr)]

Comment on attachment 90856 [details]
corrected test + fixed style

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

> LayoutTests/fast/table/auto-100-percent-width.html:1
> +<!doctype HTML>

Should be !DOCTYPE html

> LayoutTests/fast/table/auto-100-percent-width.html:3
> +<head>
> +<title></title></head>

No need for these tags.

[Rik Cabanier]

Created attachment 90872 [details]
updated HTML file

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 90872 [details]:

http/tests/canvas/webgl/origin-clean-conformance.html bug 52117 (author: [e-mail])
http/tests/security/local-video-source-from-remote.html bug 59298 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 90872 [details]
updated HTML file

Clearing flags on attachment: 90872

Committed r84755: <http://trac.webkit.org/changeset/84755>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.