[webkit-reviews] review denied: [Bug 93735] Positioned Replaced Elements That Aren't RenderReplaced get Incorrect Width : [Attachment 203537] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Robert Hogan <robert at webkit.org>'s
request for review:
Bug 93735: Positioned Replaced Elements That Aren't RenderReplaced get
Incorrect Width
https://bugs.webkit.org/show_bug.cgi?id=93735

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

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


I would use intrinsicSize() subclassing more. You might want to investigate how
intrinsicSize() is called. It may be worthwhile to turn it into
intrinsicLogicalSize() to simplify subclassing, especially if nobody is really
using the physical version of it.

> Source/WebCore/rendering/RenderBox.cpp:2447
> +    // Some replaced renderers, such as text controls, need to calculate
their 'intrinsic' height during layout.
> +    updateIntrinsicLogicalHeight();

I don't get why you had to add this. Can't you just do this inside
RenderTextControl's computeLogicalHeight?

> Source/WebCore/rendering/RenderBox.cpp:3052
> +static bool isReplacedElement(const RenderBox* child)
> +{
> +    return child->isReplaced() || (child->node() &&
child->node()->isElementNode() &&
toElement(child->node())->isFormControlElement() && !child->isFieldset());
> +}

Let's add a FIXME about how we'd like this to just be isReplaced() eventually
and file a followup bug about it too.

> Source/WebCore/rendering/RenderBox.cpp:3056
> -    if (isReplaced()) {
> +    if (isReplacedElement(this)) {

I think it's worth a comment about why we're not just using isReplaced.

> Source/WebCore/rendering/RenderBox.cpp:3398
> -    if (isReplaced()) {
> +    if (isReplacedElement(this)) {

Ditto.

> Source/WebCore/rendering/RenderBox.h:405
> +    virtual LayoutUnit intrinsicLogicalWidth() const { return
style()->isHorizontalWritingMode() ? intrinsicSize().width() :
intrinsicSize().height(); }
> +    virtual LayoutUnit intrinsicLogicalHeight() const { return
style()->isHorizontalWritingMode() ? intrinsicSize().height() :
intrinsicSize().width(); }

I don't get why these have to become virtual. Can't you just override the
already virtual intrinsicSize()? When someone queries for the height on objects
that only subclassed to provide a width, you now have two virtual function
calls (one to call intrinsicLogicalHeight(), and then a second virtual function
call to intrinsicSize()(.

> Source/WebCore/rendering/RenderBox.h:406
> +    virtual void updateIntrinsicLogicalHeight() { };

Don't get why this is needed.

> Source/WebCore/rendering/RenderButton.h:61
> +protected:
> +    virtual LayoutUnit intrinsicLogicalWidth() const { return
maxPreferredLogicalWidth(); }

Just subclass intrinsicSize() instead.

> Source/WebCore/rendering/RenderMenuList.h:61
> +protected:
> +    virtual LayoutUnit intrinsicLogicalWidth() const { return
maxPreferredLogicalWidth(); }

Just subclass intrinsicSize() instead.

> Source/WebCore/rendering/RenderTextControl.cpp:146
> -void RenderTextControl::computeLogicalHeight(LayoutUnit logicalHeight,
LayoutUnit logicalTop, LogicalExtentComputedValues& computedValues) const
> +void RenderTextControl::updateIntrinsicLogicalHeight()

Seems like you could have just done the update in computeLogicalHeight without
an extra function?

> Source/WebCore/rendering/RenderTextControl.h:69
> +    virtual void updateIntrinsicLogicalHeight() OVERRIDE;
> +    virtual LayoutUnit intrinsicLogicalHeight() const { return
m_intrinsicLogicalHeight; }
> +    virtual LayoutUnit intrinsicLogicalWidth() const { return
maxPreferredLogicalWidth(); }

Definitely could just subclass intrinsicSize() here.