Bug 88116 - Change overrideSizes to be content-box instead of border-box
Summary: Change overrideSizes to be content-box instead of border-box
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks: 87679
  Show dependency treegraph
 
Reported: 2012-06-01 11:53 PDT by Ojan Vafai
Modified: 2012-06-05 11:36 PDT (History)
6 users (show)

See Also:


Attachments
Patch (169.85 KB, patch)
2012-06-01 11:59 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (173.55 KB, patch)
2012-06-01 15:55 PDT, Ojan Vafai
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2012-06-01 11:53:34 PDT
Change overrideSizes to be content-box instead of border-box
Comment 1 Ojan Vafai 2012-06-01 11:59:30 PDT
Created attachment 145349 [details]
Patch
Comment 2 Tony Chang 2012-06-01 12:18:22 PDT
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 3 Julien Chaffraix 2012-06-01 12:25:16 PDT
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 4 Ojan Vafai 2012-06-01 14:28:04 PDT
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 5 Julien Chaffraix 2012-06-01 15:35:44 PDT
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.
Comment 6 Ojan Vafai 2012-06-01 15:55:38 PDT
Created attachment 145394 [details]
Patch
Comment 7 Tony Chang 2012-06-01 16:43:27 PDT
Comment on attachment 145394 [details]
Patch

Might want to give Julien a chance to take a look too before landing.
Comment 8 Julien Chaffraix 2012-06-01 16:54:09 PDT
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>.
Comment 9 Ojan Vafai 2012-06-05 11:24:42 PDT
(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.
Comment 10 Ojan Vafai 2012-06-05 11:32:24 PDT
Committed r119507: <http://trac.webkit.org/changeset/119507>
Comment 11 Julien Chaffraix 2012-06-05 11:36:35 PDT
> 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.