[webkit-reviews] review denied: [Bug 79490] Percent width/height SVG not always scaled on window resize : [Attachment 128762] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Florin Malita
<fmalita at google.com>'s request for review:
Bug 79490: Percent width/height SVG not always scaled on window resize
https://bugs.webkit.org/show_bug.cgi?id=79490

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=128762&action=review


Nice patch & testcases - here are some suggestions/questions:

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1453
> +    layoutPercentHeightDescendants();

Doesn't this cause changes for inline children of RenderBlocks in general? Are
we doing unnecessary work for HTML? Is HTML maybe affected as well by this bug
(inline percent height elements not reacting to size changes).

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:195
> +	      
containingBlock()->addPercentHeightDescendant(const_cast<RenderSVGRoot*>(this))
;

Excellent catch.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:288
> +	  
RenderBlock::removePercentHeightDescendant(const_cast<RenderSVGRoot*>(this));

Hrm, I'm slightly worried about this approach. What happens if the <svg> height
attribute gets dynamically changed from a percentage to a non-perc value?
Grepping through rendering/, shows another callee of
removePercentHeightDescendant: RenderBox::styleDidChange. It always
deregisteres itself as percent-height descendant, based on the oldStyle
information. The direct calls to oldStyle->logicalHeight().isPercent() have to
be avoided. See below for an idea how to fix it:

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:419
> +

Hm, this feels hackish. How about changing
RenderReplaced::computePreferredLogicalWidths() to avoid asking the style()
whether its dimensions are relative. Changing
if (style()->width().isPercent() || style()->height().isPercent() || ....
    m_minPrefLogWidth = 0;
..

to

if (hasRelativeLogicalWidth(style()) || hasRelativeLogicalHeight(style())))
    m_minPrefLogWidth = 0;

and move style()->logicalWidth().isPercent() etc. checks into
RenderBox::hasRelativeLogicalWidth/Height(RenderStyle*), mark it virtual and
override it in RenderSVGRoot, to respect SVG width/height properties. It needs
to be in RenderBox, so styleDidChange can use it as well.

if (oldStyle && (oldStyle->logicalHeight().isPercent() ..
-> if (oldStyle && hasRelativeLogicalWidth(oldStyle)...

This should fix all issues I'm having with it right now :-)

(If this shows hot in profiles, we can always refactor it to use non-virtual
calls to a fastHasRelativeLogicalWidth/Height inline implementation right in
RenderReplaced for non-SVG RenderReplaced objects to avoid both function call
overhead & virtual function call overhead)

> LayoutTests/svg/custom/svg-percent-scale-expected.html:16
> +    window.resizeTo(400, 300);

Oh my! I wasn't aware that this exists! In the past I always simulated that by
enclosing the <svg> in a <div> and changing that <div>'s width/height CSS
property, to simulate a window resize -- hence I never ran into this bug!