[webkit-reviews] review denied: [Bug 43779] RenderImage::imageChanged invalidates wrong area : [Attachment 65425] Patch #1l (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 65425: Patch #1l (add RenderImageResource)
https://bugs.webkit.org/attachment.cgi?id=65425&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Almost there, some comments left:

WebCore/rendering/RenderImageResource.cpp:40
 +	ASSERT(m_render);
The assertion is now wrong and has to be removed (also it should have said
m_renderer :-)

WebCore/rendering/RenderImageResource.h:31
 +  #include "Noncopyable.h"
It's include <wtf/Noncopyable.h> - this one won't compile on mac.

WebCore/rendering/RenderImageResource.h:43
 +	static PassOwnPtr<RenderImageResource> create() { return adoptPtr(new
RenderImageResource()); }
The WebKit style would be to make multiple lines here:
static PassOwnPtr<RenderImageResource> create()
{
    return adoptPtr(new RenderImageResource);
}

You should also omit the () braces.

WebCore/rendering/RenderImageResourceStyleImage.h:40
 +	static PassOwnPtr<RenderImageResource> create(StyleImage* styleImage) {
return adoptPtr(new RenderImageResourceStyleImage(styleImage)); }
Same as the other create function, please use multiple lines, and also add a
newline between the virtual dtor and the create function.

WebCore/rendering/RenderImageResourceStyleImage.h:52
 +	virtual WrappedImagePtr imagePtr() const { return m_styleImage ?
m_styleImage->data() : 0; }
Why are you checking m_styleImage here? The constructor already ASSERTS the
object is available.

WebCore/rendering/RenderImageResourceStyleImage.cpp:43
 +	m_styleImage->addClient(renderer());
This is not going to work. renderer() is 0 at this point, initialize has to be
called first.
So you also need an initialize function here, a virtual one, that executes this
code.

If you have further problems, let's discuss on IRC.