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

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

WebCore/ChangeLog:8
 +	    At the moment RenderSVGImage inherit from RenderImage, which causes
wrong repainting.
At the moment RenderSVGImage inherits from RenderImage, which makes non-SVG
compatible assumptions about repainting, and thus has to be fixed to inherit
from RenderSVGModelObject.

WebCore/ChangeLog:10
 +	    This patch move the CachedImage from RenderImage into a separate
class.
s/move/moves/

WebCore/rendering/HitTestResult.cpp:250
 +	    if (renderImageResource->cachedImage() &&
!renderImageResource->cachedImage()->errorOccurred())
Please cache the result of renderImageResource->cachedImage() in a local
variable, to avoid calling it three times.

WebCore/rendering/RenderImage.cpp:60
 +	, m_cachedImage(RenderImageResource::create(this))
How about renaming m_cachedImage to m_imageResource to clarify.

WebCore/rendering/RenderImage.cpp:214
 +	    Image *image = m_cachedImage->image();
Image*.

WebCore/rendering/RenderImage.cpp:231
 +		if (m_cachedImage->errorOccurred() && !image->isNull() &&
(usableWidth >= image->width()) && (usableHeight >= image->height())) {
Superfluous braces (usableWidth... and (usableHeight.

WebCore/rendering/RenderImage.cpp: 
 +	    Image* img = image(cWidth, cHeight);
This doesn't look right. RenderImage::image() doesn't use the passed
width/height parameters, but RenderImageGeneratedContent does (which inherits
from RenderImage). I bet this change will break tests.

WebCore/rendering/RenderImage.cpp: 
 +	Image* img = image(rect.width(), rect.height());
Ditto.

WebCore/rendering/RenderImage.cpp: 
 +	context->drawImage(image(rect.width(), rect.height()),
style()->colorSpace(), rect, compositeOperator, useLowQualityScaling);
Ditto.

WebCore/rendering/RenderImageResource.h:54
 +	void setImageContainerSize(const IntSize& size) const { if
(m_cachedImage) m_cachedImage->setImageContainerSize(size); }
This needs to go in two lines, otherwhise it will raise a style error.