[webkit-reviews] review denied: [Bug 26285] Removing an image's src attribute, or setting it to null, via scripting, doesn't change the image : [Attachment 221436] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jaehun Lim
<ljaehun.lim at samsung.com>'s request for review:
Bug 26285: Removing an image's src attribute, or setting it to null, via
scripting, doesn't change the image
https://bugs.webkit.org/show_bug.cgi?id=26285

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=221436&action=review


Thank you for looking into this, this is a good bug to fix.

I think that this change makes the code substantially more fragile and
confusing though. Some comments inline.

More importantly, this needs a lot more test coverage:

- what happens when initial src points to a 404 resource, and we later remove
the attribute?
- what happens when removing src from an element that's currently loading (this
test only runs after unload)?
- what are the DOM events being dispatched in all of these cases?

I think that adding such tests may prompt some code changes too.

> Source/WebCore/loader/ImageLoader.cpp:259
> +	   } else
> +	       updateRenderer();

The added code reads really strangely. It's essentially:

if (newImage == oldImage)
    updateRenderer();

How is this a good thing? What are the other cases when this function is called
with newImage == oldImage, and can updating the renderer have unintended
consequences for those?

And why do we get the same CachedImage in this case when removing the src?
Conceptually, there should be no CachedImage, because we are not loading
anything.