[webkit-reviews] review denied: [Bug 15849] CSS 2.1: Support replaced elements with relative intrinsic sizes : [Attachment 96622] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 10 12:15:02 PDT 2011


Dave Hyatt <hyatt at apple.com> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 15849: CSS 2.1: Support replaced elements with relative intrinsic sizes
https://bugs.webkit.org/show_bug.cgi?id=15849

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=96622&action=review

Just a few comments.

> Source/WebCore/rendering/RenderBox.cpp:668
> +    if (!style())
> +	   return false;

Just remove this. style can never be null.

> Source/WebCore/rendering/RenderBox.cpp:672
> +    if (isRenderPart())
> +	   return
toRenderPart(const_cast<RenderBox*>(this))->embeddedContentBox();

isRenderPart() is virtual, so I think it would be cleaner to just make
needsPreferredWidthsRecalculation virtual and subclass it for RenderPart. 
Given that padding is almost never a percentage, the isRenderPart check is
always being hit anyway, so you're already paying the virtual function
invocation cost once. Might as well make the code cleaner then.

> Source/WebCore/rendering/RenderBox.h:123
> +    virtual void computeIntrinsicRatioInformation(FloatSize& /*
intrinsicRatio */, bool& /* isPercentageIntrinsicSize */) const { }

Is it right for this to be public?

> Source/WebCore/rendering/RenderReplaced.cpp:192
> +int RenderReplaced::computeIntrinsicWidth(RenderBox* contentRenderer, bool
includeMaxWidth) const

You've made a function called "computeIntrinsicWidth" that then invokes
"computeReplacedLogicalWidth...".  In a vertical writing mode, "logical width"
is actually height. There's an inconsistency here that needs to be dealt with.
Either you just misnamed this function, or you intended for it to be truly
physical, in which case what it's doing isn't correct for vertical writing
modes.

> Source/WebCore/rendering/RenderReplaced.cpp:204
> +int RenderReplaced::computeIntrinsicHeight(RenderBox* contentRenderer) const

> +{

Same comment here as for computeIntrinsicWidth.


More information about the webkit-reviews mailing list