[webkit-reviews] review denied: [Bug 43779] RenderImage::imageChanged invalidates wrong area : [Attachment 65341] Patch #1j (add RenderImageResource)

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 43779: RenderImage::imageChanged invalidates wrong area
https://bugs.webkit.org/show_bug.cgi?id=43779

Attachment 65341: Patch #1j (add RenderImageResource)
https://bugs.webkit.org/attachment.cgi?id=65341&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Hi Patrick,

unfortunately there are still some issues, that I missed before:

WebCore/rendering/RenderImageResource.cpp:36
 +  PassOwnPtr<RenderImageResource> RenderImageResource::create(RenderObject*
renderer, StyleImage* styleImage)
This solution is not a good idea, the RenderImageResource::create method,
should only ever created a RenderImageResource object, not a
RenderImageResourceStyleImage.
Otherwhise the design is pretty confusing. That in turn also means RenderImage
shouldn't receive the new StyleImage* constructor parameter, I think its better
if it would take a PassOwnPtr<RenderImageResource> that can be stored as
m_imageResource member variable.

The idea would look like this:

WebCore/rendering/RenderObject.cpp:110
 +	    RenderImage* image = new (arena) RenderImage(node,
contentData->image());
RenderImage* image = new (arena) RenderImage(node,
RenderImageResourceStyleImage::create(contentData->image())

WebCore/rendering/RenderObjectChildList.cpp:433
 +		    renderer = new (owner->renderArena())
RenderImage(owner->document(), content->image()); // anonymous object
renderer  = new (owner->renderArena()) RenderImage(owner->document(),
RenderImageResourceStyleImage::create(content->image()));

You'd also have to change the callsites that create a regular RenderImage
object:
html/HTMLEmbedElement.cpp:	  return new (arena) RenderImage(this);
html/HTMLImageElement.cpp:     return new (arena) RenderImage(this);
html/HTMLInputElement.cpp:	  return new (arena) RenderImage(this);
html/HTMLObjectElement.cpp:	   return new (arena) RenderImage(this);
wml/WMLImageElement.cpp:    return new (arena) RenderImage(this);

adding a RenderImageResource::create() argument.

I think this solution would be cleaner.