[Webkit-unassigned] [Bug 43779] RenderImage::imageChanged invalidates wrong area

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43779


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #64162|review?                     |review-
               Flag|                            |




--- Comment #15 from Nikolas Zimmermann <zimmermann at kde.org>  2010-08-11 23:59:21 PST ---
(From update of attachment 64162)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.