[webkit-reviews] review granted: [Bug 206317] Clamp paddingBoxWidth/Height to a minimum of zero : [Attachment 388594] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sunny He <sunny_he at apple.com>'s
request for review:
Bug 206317: Clamp paddingBoxWidth/Height to a minimum of zero
https://bugs.webkit.org/show_bug.cgi?id=206317

Attachment 388594: Patch

https://bugs.webkit.org/attachment.cgi?id=388594&action=review




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 388594
  --> https://bugs.webkit.org/attachment.cgi?id=388594
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388594&action=review

> Source/WebCore/rendering/RenderBox.h:221
> +    LayoutUnit paddingBoxWidth() const { return std::max<LayoutUnit>(0,
width() - borderLeft() - borderRight() - verticalScrollbarWidth()); }
> +    LayoutUnit paddingBoxHeight() const { return std::max<LayoutUnit>(0,
height() - borderTop() - borderBottom() - horizontalScrollbarHeight()); }

A more economical way to write this is "std::max(0_lu,"

>> Source/WebCore/rendering/RenderReplaced.cpp:461
>> +	logicalWidth = std::max(0_lu, (logicalWidth - (marginStart + marginEnd
+ borderLeft() + borderRight() + verticalScrollbarWidth())));
> 
> I am confused how this works (or even worked before). The logical width here
(which is the content logical width) of an inline replaced box is supposed to
be the containing block's content box width - (margin start + border inline
start + padding inline start + padding inline end + border inline end + margin
end), since the constraint equation is as follows containing block's content
width = margin s + border s + padding s + content width + padding e + border e
+ margin e.
> It's even in the comment a few lines above.

I am going to say review+ here, but I am concerned that Alan is confused about
this, since this is an area he's expert on. Please follow up with him.