Created attachment 145349 [details] Patch

Comment on attachment 145349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145349&action=review > Source/WebCore/rendering/RenderBox.cpp:2289 > - return overrideHeight() - borderAndPaddingLogicalWidth(); > + return overrideLogicalContentHeight(); How did this work before? We're subtracting a width from a height? > LayoutTests/ChangeLog:9 > + Change overrideSizes to be content-box instead of border-box > + https://bugs.webkit.org/show_bug.cgi?id=88116 > + > + Reviewed by NOBODY (OOPS!). > + > + * platform/chromium-linux/tables/mozilla/bugs/bug131020-expected.png: > + * platform/chromium-win/tables/mozilla/bugs/bug131020-expected.txt: A sentence about how the test is changing would be helpful. E.g., "We were double counting the border in the height, but now we don't." or something. Should we add a test with padding?

Comment on attachment 145349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145349&action=review > Source/WebCore/rendering/RenderTableCell.cpp:237 > void RenderTableCell::setOverrideHeightFromRowHeight(LayoutUnit rowHeight) While you are renaming all the override functions to include "logical", this one should be renamed too. >> LayoutTests/ChangeLog:9 >> + * platform/chromium-linux/tables/mozilla/bugs/bug131020-expected.png: >> + * platform/chromium-win/tables/mozilla/bugs/bug131020-expected.txt: > > A sentence about how the test is changing would be helpful. E.g., "We were double counting the border in the height, but now we don't." or something. Should we add a test with padding? I agree with adding a sentence (especially since it is a nice progression). The test needs to be rebaselined on other platforms too. I hope you will add the missing test_expectations.txt / Skipped entries before landing.

Comment on attachment 145349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145349&action=review >> Source/WebCore/rendering/RenderBox.cpp:2289 >> + return overrideLogicalContentHeight(); > > How did this work before? We're subtracting a width from a height? We were just doing the wrong thing. It happens that we have poor test coverage of the combination of setting overrideHeight on table cells and having a different borderAndPaddingWidth than borderAndPaddingHeight. Sigh, I suppose I should figure out how to write a test for this. >>> LayoutTests/ChangeLog:9 >>> + * platform/chromium-win/tables/mozilla/bugs/bug131020-expected.txt: >> >> A sentence about how the test is changing would be helpful. E.g., "We were double counting the border in the height, but now we don't." or something. Should we add a test with padding? > > I agree with adding a sentence (especially since it is a nice progression). The test needs to be rebaselined on other platforms too. I hope you will add the missing test_expectations.txt / Skipped entries before landing. Will add the description and add it to test_expectations.txt, but adding it to Skipped is not very useful since I need it to run on the bots to get the new expected results, no?

Comment on attachment 145349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145349&action=review >>>> LayoutTests/ChangeLog:9 >>>> + * platform/chromium-win/tables/mozilla/bugs/bug131020-expected.txt: >>> >>> A sentence about how the test is changing would be helpful. E.g., "We were double counting the border in the height, but now we don't." or something. Should we add a test with padding? >> >> I agree with adding a sentence (especially since it is a nice progression). The test needs to be rebaselined on other platforms too. I hope you will add the missing test_expectations.txt / Skipped entries before landing. > > Will add the description and add it to test_expectations.txt, but adding it to Skipped is not very useful since I need it to run on the bots to get the new expected results, no? If you intent to do the rebaselining on the platform then yes you shouldn't touch the Skipped file. That said some maintainers prefer people to use Skipped files over test_expectations.txt which is why I mentioned the 2 files. It's better to disable the test on those platforms (following the maintainers' policy) than to turn the bot red.

Created attachment 145394 [details] Patch

Comment on attachment 145394 [details] Patch Might want to give Julien a chance to take a look too before landing.

Comment on attachment 145394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145394&action=review The change is fine though I wish there was a way to differentiate between border-box and content-box in our code to avoid such mistakes. > LayoutTests/fast/table/padding-height-and-override-height.html:12 > +// Firefox 12 has the same crazy behavior. Is this a bug or are tables just crazy? You may be calling for the craziness yourself by using an auto-table layout layout in quirks mode. If you want less craziness, the way to go is: doctype + table-layout: fixed on your <table>.

(In reply to comment #8) > (From update of attachment 145394 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145394&action=review > > The change is fine though I wish there was a way to differentiate between border-box and content-box in our code to avoid such mistakes. Only thing I can think of to improve this would be to get rid of height/width accessors entirely. It makes the code more verbose, but it's more clear what's going on.

Committed r119507: <http://trac.webkit.org/changeset/119507>

> Only thing I can think of to improve this would be to get rid of height/width accessors entirely. It makes the code more verbose, but it's more clear what's going on. That's one way or at least a short term solution. The other would be to stop using plain LayoutRect and have 2 dedicated wrapper classes that avoids being able to convert implicitly between content and border boxes.