[webkit-reviews] review denied: [Bug 23658] webkit should support box-sizing:padding-box : [Attachment 157618] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Elliott Sprehn
<esprehn at chromium.org>'s request for review:
Bug 23658: webkit should support box-sizing:padding-box
https://bugs.webkit.org/show_bug.cgi?id=23658

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157618&action=review


R- for lack of tests and for the RenderLayer/RenderReplaced issues. Otherwise,
patch looks good.

I'd prefer to fix the table cases in this patch and just get it over with. The
hard part there is just writing tests to verify it's doing the right thing. But
if you'd prefer to do the table/table-cell cases in a follow-up patch, that's
fine too.

Looks like there's another use in RenderTable and one in RenderTableCell that
you missed. If you don't want to fix them in this patch, please add FIXMEs
there as well.

> Source/WebCore/rendering/RenderLayer.cpp:1967
> +	       baseWidth -= renderer->borderLogicalWidth();

This is setting the physical width below, so, this should just use the physical
border width.

> Source/WebCore/rendering/RenderLayer.cpp:1982
> +	       baseHeight -= renderer->borderLogicalHeight();

Ditto, but for height obviously.

> Source/WebCore/rendering/RenderReplaced.cpp:452
> +	       maxWidth = maxWidth + borderLeft() + borderRight();

This should just be borderLogicalWidth. The borderAndPaddingWidth call above is
wrong. It should be borderAndPaddingLogicalWidth.

> Source/WebCore/rendering/RenderTable.cpp:278
> +	   // FIXME: We might need to handle PADDING_BOX here.

I'm not too familiar with this code, but I think we do need to. Just need to
use borderLogicalWidth I think. Did you have trouble making a testcase?

> Source/WebCore/rendering/style/StyleBoxData.h:84
> +    unsigned m_boxSizing : 2; // EBoxSizing

Sigh. Sorry I didn't catch this.