[webkit-reviews] review denied: [Bug 46775] getBoundingClientRect does not work with SVG <text> : [Attachment 69828] The same patch as before, with just a little more cleanup (two indentation fixes in JavaScript)

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Cosmin Truta
<ctruta at chromium.org>'s request for review:
Bug 46775: getBoundingClientRect does not work with SVG <text>
https://bugs.webkit.org/show_bug.cgi?id=46775

Attachment 69828: The same patch as before, with just a little more cleanup
(two indentation fixes in JavaScript)
https://bugs.webkit.org/attachment.cgi?id=69828&action=review

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

> LayoutTests/svg/custom/getBoundingClientRect-js.svg:19
> +		   var img =
document.createElementNS("http://www.w3.org/2000/svg", "image");

I'd propose to split this in several tests, one for getBoundingClientRect on
text, one for image, one for g, one for rect, one for text.
If this ever breaks, it's not easy to spot what exactly is broken.

> WebCore/dom/Element.cpp:490
> +	       const FloatRect& localRect = static_cast<const
SVGStyledLocatableElement*>(svgElement)->getBBox();

I think we should introduce a "bool boundingBoxForSVGElement(FloatRect&)"
method in SVGElement, that includes these checks. It feels awkward to have this
logic in Element.cpp.

This could would simplify to:
if (isSVGElement()) {
    FloatRect localRect;
    if (static_cast<const
SVGElement*>(this)->boundingBoxForSVGElement(localRect))
	quads.append(renderer()->localToAbsoluteQuad(localRect));
}

The SVGElement::boundingBoxForSVGElement method would utilize,
isStyledLocatable() cast itself to a SVGStyledLocatableElement, and return
getBBox().
Additionally it checks the isText() case, for both cases it fills the passed in
localRect reference, and returns true. Otherwhise it'll return false.

> WebCore/svg/SVGElement.h:58
> +	   virtual bool isText() const { return false; }

This is not needed. In the new boundingBoxForSVGElement() method, you can use:
if (hasTagName(SVGNames::textTag)) {
    SVGTextElement* textElement = static_cast<SVGTextElement*>(element);
...

> WebCore/svg/SVGStyledTransformableElement.cpp:-103
> -SVGElement* SVGStyledTransformableElement::nearestViewportElement() const
> -{
> -    return SVGTransformable::nearestViewportElement(this);
> -}
> -
> -SVGElement* SVGStyledTransformableElement::farthestViewportElement() const
> -{
> -    return SVGTransformable::farthestViewportElement(this);
> -}
> -
> -FloatRect SVGStyledTransformableElement::getBBox(StyleUpdateStrategy
styleUpdateStrategy) const
> -{
> -    return SVGTransformable::getBBox(this, styleUpdateStrategy);
> -}
> -

Good catches!

> WebCore/svg/SVGTextElement.h:46
> +	   virtual bool isText() const { return true; }

Not needed, see the other comments.