Bug 59138

Summary: REGRESSION(81625): Tables are not rendered correctly
Product: WebKit Reporter: Nico Weber <thakis>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, adele, cabanier, commit-queue, eric, jamesr, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
repro
none
bug fix
thakis: review-
bug fix with testcase
simon.fraser: review-, simon.fraser: commit-queue-
bug fix with comment moved
simon.fraser: review-, simon.fraser: commit-queue-
corrected test + fixed style
cabanier: review-, cabanier: commit-queue-
corrected test + fixed style
simon.fraser: review+, simon.fraser: commit-queue-
updated HTML file none

Description Nico Weber 2011-04-21 14:17:46 PDT
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
Comment 1 Nico Weber 2011-04-21 14:18:04 PDT
Created attachment 90598 [details]
repro
Comment 2 Rik Cabanier 2011-04-21 14:33:18 PDT
I will sync down the latest sources and take a look.
Comment 3 Simon Fraser (smfr) 2011-04-21 14:50:14 PDT
<rdar://problem/9320145>
Comment 4 Adele Peterson 2011-04-22 11:25:40 PDT
Do we know of any cases where the original change caused a progression?  If not, maybe we should roll it out.
Comment 5 Nico Weber 2011-04-22 11:32:10 PDT
jamesr tells me the original change doesn't revert cleanly because other changes have been made on top of it.
Comment 6 Simon Fraser (smfr) 2011-04-22 12:05:30 PDT
The original change was related to not rounding to pixel values for transforms.
Comment 7 Rik Cabanier 2011-04-22 20:47:06 PDT
Created attachment 90839 [details]
bug fix
Comment 8 Nico Weber 2011-04-22 20:49:04 PDT
Missing a test case.
Comment 9 Nico Weber 2011-04-22 20:58:17 PDT
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.
Comment 10 Rik Cabanier 2011-04-22 21:07:48 PDT
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.
Comment 11 Rik Cabanier 2011-04-22 21:11:36 PDT
(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| ?
Comment 12 Nico Weber 2011-04-22 21:14:01 PDT
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.)
Comment 13 Rik Cabanier 2011-04-22 21:50:41 PDT
Created attachment 90841 [details]
bug fix with testcase
Comment 14 Simon Fraser (smfr) 2011-04-22 22:11:04 PDT
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?
Comment 15 Rik Cabanier 2011-04-22 22:22:22 PDT
(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.
Comment 16 Rik Cabanier 2011-04-22 22:28:20 PDT
Created attachment 90842 [details]
bug fix with comment moved
Comment 17 Simon Fraser (smfr) 2011-04-23 09:21:00 PDT
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.
Comment 18 Rik Cabanier 2011-04-23 12:30:02 PDT
(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.
Comment 19 Rik Cabanier 2011-04-23 12:30:46 PDT
Created attachment 90855 [details]
corrected test + fixed style
Comment 20 Nico Weber 2011-04-23 12:36:51 PDT
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()?
Comment 21 Rik Cabanier 2011-04-23 12:45:43 PDT
Created attachment 90856 [details]
corrected test + fixed style
Comment 22 Rik Cabanier 2011-04-23 12:48:24 PDT
(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
Comment 23 Nico Weber 2011-04-23 12:55:40 PDT
Thanks!
Comment 24 Simon Fraser (smfr) 2011-04-23 18:45:54 PDT
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.
Comment 25 Rik Cabanier 2011-04-23 22:55:56 PDT
Created attachment 90872 [details]
updated HTML file
Comment 26 WebKit Commit Bot 2011-04-24 01:16:20 PDT
The commit-queue encountered the following flaky tests while processing attachment 90872 [details]:

http/tests/canvas/webgl/origin-clean-conformance.html bug 52117 (author: enne@google.com)
http/tests/security/local-video-source-from-remote.html bug 59298 (author: eric.carlson@apple.com)
The commit-queue is continuing to process your patch.
Comment 27 WebKit Commit Bot 2011-04-24 01:17:49 PDT
Comment on attachment 90872 [details]
updated HTML file

Clearing flags on attachment: 90872

Committed r84755: <http://trac.webkit.org/changeset/84755>
Comment 28 WebKit Commit Bot 2011-04-24 01:17:56 PDT
All reviewed patches have been landed.  Closing bug.