[webkit-reviews] review denied: [Bug 88116] Change overrideSizes to be content-box instead of border-box : [Attachment 145349] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 88116: Change overrideSizes to be content-box instead of border-box
https://bugs.webkit.org/show_bug.cgi?id=88116

Attachment 145349: Patch
https://bugs.webkit.org/attachment.cgi?id=145349&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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?