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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Excellent idea.

r-, because of following issues:
WebCore/ChangeLog:8
 +	    Move the CachedImage from RenderImage into a separate class.
The ChangeLog should contain more information, especially the why of this
change.
You should also explicitely name RenderImageResource here, so it's clear where
the code moved.

WebCore/loader/ImageLoader.cpp:229
 +	if (RenderObject* renderer = m_element->renderer()) {
Change to early return style. if renderrer is not existant, just return 0.

WebCore/loader/ImageLoader.cpp:247
 +	if (RenderImageResource* imageResource = renderImageResource()) {
Same here.

WebCore/rendering/RenderImage.cpp:61
 +	m_cachedImage = RenderImageResource::create(this);
No need to move this into the constructor body.

WebCore/rendering/RenderImage.cpp:70
 +	m_cachedImage.clear();
This is automatically done IIRC. Please double check, but I think it can be
removed.

WebCore/rendering/RenderImage.h:46
 +	void setCachedImage(CachedImage* image) {
m_cachedImage->setCachedImage(image); }
Who calls these methods? It seems to me, all callers of these functions could
now operate on RenderImageResource. Please check twice.

WebCore/rendering/RenderImage.h:47
 +	CachedImage* cachedImage() { return m_cachedImage->cachedImage(); }
Ditto.

WebCore/rendering/RenderImage.h:49
 +	virtual bool hasImage() const { return m_cachedImage->hasImage(); }
Ditto.

WebCore/rendering/RenderImage.h:54
 +	virtual Image* image(int /* width */ = 0, int /* height */ = 0) {
return m_cachedImage->image(); }
Ditto.

WebCore/rendering/RenderImage.h:55
 +	virtual bool errorOccurred() const { return
m_cachedImage->errorOccurred(); }
Ditto.

It should be possible to move all these virtual methods away from RenderObject
now, no?WebCore/rendering/RenderImage.h:86
 +	virtual bool usesImageContainerSize() const { return
m_cachedImage->usesImageContainerSize(); }
Same for the bunch of methods in the next lines.

WebCore/rendering/RenderImageResource.cpp:35
 +  PassOwnPtr<RenderImageResource> RenderImageResource::create(RenderObject*
render)
You can move this inline into the header.

WebCore/rendering/RenderImageResource.cpp:41
 +	: m_render(render)
I'd prefer "m_renderer" here.

WebCore/rendering/RenderImageResource.cpp:61
 +	if (m_cachedImage) {
You can early return here, if m_cachedImage is 0.