[webkit-reviews] review denied: [Bug 22981] SVG-as-image redraw broken: svg-as-background-5.html LayoutTest fails : [Attachment 26257] Patch, testcases, changelog

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 22981: SVG-as-image redraw broken: svg-as-background-5.html LayoutTest
fails
https://bugs.webkit.org/show_bug.cgi?id=22981

Attachment 26257: Patch, testcases, changelog
https://bugs.webkit.org/attachment.cgi?id=26257&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -	   curr->first->imageChanged(static_cast<WrappedImagePtr>(this));
> +	   // FIXME: what rect to use?
> +	   curr->first->imageChanged(static_cast<WrappedImagePtr>(this),
IntRect(0, 0, -1, -1));

Seems clear that passing no specific rect is the right thing to do. Why is
there any doubt?

> +    virtual void changedInRect(const Image* image, const IntRect& rect);

Please omit the names "image" and "rect" here.

> +// Returns a rect produced by mapping and scaling r from the bounds of
srcRect to the bounds of destRect
> +static IntRect mapRect(const IntRect& r, const IntRect& srcRect, const
IntRect& destRect)
> +{
> +    float widthMult = (float)destRect.width() / (float)srcRect.width();
> +    float heightMult = (float)destRect.height() / (float)srcRect.height();
> +
> +    FloatRect mappedRect(destRect.x() + (r.x() - srcRect.x()) * widthMult,
> +			    destRect.y() + (r.y() - srcRect.y()) * heightMult,
> +			    r.width() * widthMult,
> +			    r.height() * heightMult);
> +    
> +    return enclosingIntRect(mappedRect);
> +}

Seems weak to be converting back and forth to int like this. Formatting here is
also a bit strange. Use of float vs. double seems a bit arbitrary, perhaps
driven by the type FloatRect. I'm not fond of the "mult" abbreviation here, nor
of the use of C-style casts to convert an int to a float.

> +    virtual void chromeDestroyed()
> +    {
> +	   // There is no setClient() on Chrome, so we can't clear its client
explicitly. We have to wait for
> +	   // this callback.
> +	   delete this;
> +    }

We could add a setClient() on Chrome if we like. We could also consider making
the SVGImage itself be the ChromeClient, using private inheritance perhaps?

I'm going to say r=me because I had enough comments that it seems worth
revising another round.