Created attachment 90598 [details] repro

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

<rdar://problem/9320145>

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

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

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

Created attachment 90839 [details] bug fix

Missing a test case.

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.

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.

(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 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.)

Created attachment 90841 [details] bug fix with testcase

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?

(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.

Created attachment 90842 [details] bug fix with comment moved

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.

(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.

Created attachment 90855 [details] corrected test + fixed style

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()?

Created attachment 90856 [details] corrected test + fixed style

(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

Thanks!

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.

Created attachment 90872 [details] updated HTML file

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 on attachment 90872 [details] updated HTML file Clearing flags on attachment: 90872 Committed r84755: <http://trac.webkit.org/changeset/84755>

All reviewed patches have been landed. Closing bug.