[webkit-reviews] review requested: [Bug 22981] SVG-as-image redraw broken: svg-as-background-5.html LayoutTest fails : [Attachment 26262] Revised patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has asked	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 26262: Revised patch
https://bugs.webkit.org/attachment.cgi?id=26262&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
(In reply to comment #3)
> (From update of attachment 26257 [review])
> > -	     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?

Removed.

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

Done.

> > +// 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.

I tidied this up a bit, but I think at least the width and height computations
need
to do float-int conversions. As for double vs. float, all of the coordinate
math I could
find uses floats. Much of it uses C-style cases too.

> > +	 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.

That would require adding null checks on m_client in lots of places (the
contract, for
now, seems to be that Chrome::m_client is never null).

> We could also consider making the SVGImage itself be the ChromeClient, using
> private inheritance perhaps?

I think that will result in lifetime issues. ChromeClient is not refcounted,
but SVGImage is, and the image gets destroyed before the Chrome.

So without fairly significant Chrome work, this seems the simplest solution for
now.