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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 26262: Revised patch
https://bugs.webkit.org/attachment.cgi?id=26262&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -	   curr->first->imageChanged(static_cast<WrappedImagePtr>(this));
> +	   curr->first->imageChanged(static_cast<WrappedImagePtr>(this),
IntRect(0, 0, -1, -1));

I think using this special value for IntRect as a signal is ugly. At the call
site programmers have no way to know what the -1/-1 size means. Even if I look
in the the header file, I have to follow the thread of calls all the way to
CachedResourceClient::imageChanged; in many cases the caller is calling another
function that forwards a rectangle a couple of levels, and so won't see the
comment.

A named constant would be better, unknownChangedRect perhaps or
entireImageChangedRect. Even better would be a design that didn't use a magic
value. We typically refer to that as "in-band signaling" and sometimes go out
of our way to avoid it. For example, you could have two separate imageChanged
functions, one with and one without a rect. But given all the affected classes,
I guess you don't want that.

If you do want to preserve the in-band signaling but don't want to use the
named constant, an economical way to do it would be to make the argument a
const IntRect* = 0 and use the null pointer to mean unknown/entire-image. This
means that you can't make the mistake of using the special rectangle as a
rectangle, because it will do a null dereference. I also think that the value 0
is a more clear way to say "I don't have a rectangle".

And you could have a default argument so we wouldn't ever have to specify
IntRect(0, 0, -1, -1) explicitly. Perhaps you omitted it to help you remember
to specify the rectangle whenever possible. That's an argument against a
default argument.

> +	   if (rect.width() != -1 && rect.height() != -1) {

I find it a little strange that this checks both the width and height, but
neither x nor y. Why not just width, or all four? Maybe if you had
entireImageChangedRect, you could also have isEntireImageChangedRect function
or use if (rect == entireImageChangedRect). Either would be clearer than the
check for -1.

> +    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;
> +    }

Can the image outlast the chrome client? If not, why not? If so, then you need
to set m_image->m_chromeClient to 0 here to make sure the image doesn't access
a deleted chrome client object.

Can the chrome client outlast the image? If not, why not? If so, then I think
you need some null checks on m_image and you need ~SVGImage to clear out
m_chromeClient->m_image to make sure the chrome client object doesn't access
the deleted image.

I'm going to say r=me, but I'm worried about those lifetime issues, and I'd
like to hear what you have to say about them.