Bug 43779 – RenderImage::imageChanged invalidates wrong area

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Patrick R. Gansterer]

Created attachment 63994 [details]
testcase

When you change the url of an SVG image a wrong area will be invalidated:

When you open the attached testcase you see an red rect on position 100,100 with the size 50,50. This is an image. When you click on it, it will change the url of the image. Now the area at 100,100 with the size 50,50 changed. You get the correct invalidate rect at ChromeClient::invalidateContentsAndWindow, but addional the rect(0,0,50,50). This requests an unneeded area for repaint.

The wrong rect is generated at http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderImage.cpp?rev=64272#L179.
Is it correct that this code is called for SVG?

[Nikolas Zimmermann]

Good catch, as RenderSVGImage::imageChanged(), repaints on its own, I think it's not needed.
Note, this bug also affects mac, when using "Quartz Debug" it's clearly visible.

The real problem is that RenderSVGImage inherits from RenderImage, instead it should be a RenderSVGModelObject (like RenderSVGContainer, RenderPath, etc..) to integrate better with SVGs render tree. The current design is ineffecient.

It's straightforward to fix, Patrick, do you have interesst to fix this?

[Patrick R. Gansterer]

(In reply to comment #1)
> Note, this bug also affects mac, when using "Quartz Debug" it's clearly visible.
I had the problem on WinCE where it caused big performance problems. :-/

> It's straightforward to fix, Patrick, do you have interesst to fix this?
Of course. I didn't know the correct fix.

I'll try to fix it and post a patch.

[Nikolas Zimmermann]

(In reply to comment #2) 
> I'll try to fix it and post a patch.

Excellent. I'd try to mimic RenderImage as much as possible.
You have to manage loading of the actual image on your own

If you grep for "m_cachedImage" in RenderImage, you see what you have to add to RenderSVGImage, besides letting it inherit from RenderSVGModelObject.

A general advice is to compare against already "correct" SVG renderers like RenderSVGContainer, RenderPath, how they handle layout/paint. What functions you have to implement etc..

If you run in trouble, just ask and/or post patches for discussion.

[Patrick R. Gansterer]

Created attachment 64021 [details]
Possible Patch (not working)

With this patch I don't get any updates on the screan. Only the inital state is drawn. Can you give me a hint about what is missing? THX!

[Nikolas Zimmermann]

Comment on attachment 64021 [details]
Possible Patch (not working)

Good first start, before I comment on your question, some issues:
Following RenderSVGImage methods can be removed now:
clippedOverflowRectForRepaint, it's already implemented by RenderSVGModelObject
computeRectForRepaint (ditto)
mapLocalToContainer (ditto)
absoluteRects (ditto)
absoluteQuads (ditto)
destroy (ditto)
styleDidChange (ditto)
updateFromElement (ditto)
nodeAtPoint (ditto)

This will make the code more readable :-)

Regarding your question, what do you mean exactly? Can you elaborate more? Images get drawn? Moving/animating/... images don't repaint correctly?

[Patrick R. Gansterer]

(In reply to comment #5)
> (From update of attachment 64021 [details])
> Following RenderSVGImage methods can be removed now:
> styleDidChange (ditto)
> updateFromElement (ditto)
They don't exist.
I think we can remove addFocusRingRects and nodeAtFloatPoint too.

> Regarding your question, what do you mean exactly?
The clickevent in the attached testcase does not work.

> Images get drawn?
The inital state of the image is drawn correctly. 

>Moving/animating/... images don't repaint correctly?
I didn't try, but I think so. I can try later...

[Patrick R. Gansterer]

I found the problem in the meantime:
ImageLoader checks for isImage(): http://trac.webkit.org/browser/trunk/WebCore/loader/ImageLoader.cpp?rev=62664#L229

[Patrick R. Gansterer]

Created attachment 64162 [details]
Patch #1 (add RenderImageResource)

[Patrick R. Gansterer]

Created attachment 64163 [details]
Patch #2 (remove Remove RenderImage inheritance)

depends on Patch #1

[WebKit Review Bot]

Attachment 64162 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderImageResource.h:54:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 64163 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3735078

[Early Warning System Bot]

Attachment 64163 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3736070

[WebKit Review Bot]

Attachment 64163 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3749061

[WebKit Review Bot]

Attachment 64163 [details] did not build on win:
Build output: http://queues.webkit.org/results/3762073

[Nikolas Zimmermann]

Comment on attachment 64162 [details]
Patch #1 (add RenderImageResource)

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.

[Nikolas Zimmermann]

Comment on attachment 64163 [details]
Patch #2 (remove Remove RenderImage inheritance)

Also some comments:
WebCore/rendering/RenderSVGImage.cpp:51
 +      m_cachedImage = RenderImageResource::create(this);
Can be moved in the initialization list.

WebCore/rendering/RenderSVGImage.cpp:56
 +      m_cachedImage.clear();
Not needed IIRC.

WebCore/rendering/RenderSVGImage.cpp:104
 +                  if (svgStyle->shapeRendering() == SR_CRISPEDGES)
Is that necessary for images? I don't think so.

WebCore/rendering/RenderSVGImage.cpp:169
 +      // This empty image may be cached by SVG filter effects which must be invalidated.
That change is not correct, you basically reverted to an older version of the imageChanged version.
The SVGResourcesCache::... call is correct.

WebCore/svg/SVGImageElement.cpp:174
 +      if (RenderSVGImage* imageObj = toRenderSVGImage(renderer())) {
Change to early return.

[Patrick R. Gansterer]

(In reply to comment #15)
> (From update of attachment 64162 [details])
> WebCore/rendering/RenderImage.cpp:61
>  +      m_cachedImage = RenderImageResource::create(this);
> No need to move this into the constructor body.
Is it safe to use this in initialization list? Some compiler throw warnings?

> 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.
They are called on all other places where images are used (e.g. Pasteboard, DragDrop, ...)
If you prefere it that way, I'll change it.

(In reply to comment #16)
> (From update of attachment 64163 [details])
> Also some comments:
> WebCore/rendering/RenderSVGImage.cpp:51
>  +      m_cachedImage = RenderImageResource::create(this);
> Can be moved in the initialization list.
Same as in RenderImage.

> WebCore/rendering/RenderSVGImage.cpp:104
>  +                  if (svgStyle->shapeRendering() == SR_CRISPEDGES)
> Is that necessary for images? I don't think so.
I copied fomr RenderPath. I'll remove it.

> WebCore/rendering/RenderSVGImage.cpp:169
>  +      // This empty image may be cached by SVG filter effects which must be invalidated.
> That change is not correct, you basically reverted to an older version of the imageChanged version.
> The SVGResourcesCache::... call is correct.
I think that happened when I rebased to head. I'll check it.

[Patrick R. Gansterer]

The RenderImageResource is missing in the xcode project files. Is there a way to add them without having a mac? I din't find something. :-(

[Nikolas Zimmermann]

Are you on IRC in the next hours? If yes, I can modify the Xcode proj, and send the difffs to you.

[Nikolas Zimmermann]

Created attachment 64192 [details]
Xcode changes

[Patrick R. Gansterer]

Created attachment 64206 [details]
Patch #1b (add RenderImageResource)

[Nikolas Zimmermann]

Comment on attachment 64206 [details]
Patch #1b (add RenderImageResource)

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.

[Nikolas Zimmermann]

Created attachment 64217 [details]
Xcode changes - part 2

[Patrick R. Gansterer]

Created attachment 64241 [details]
Patch #1c (add RenderImageResource)

[Patrick R. Gansterer]

Created attachment 64249 [details]
Patch #1d (add RenderImageResource)

Rebased to head

[WebKit Review Bot]

Attachment 64249 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderImageResource.cpp:91:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Created attachment 64254 [details]
Patch #1e (add RenderImageResource)

fixed wrong include

[Early Warning System Bot]

Attachment 64249 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3730114

[Early Warning System Bot]

Attachment 64254 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3765098

[Patrick R. Gansterer]

Created attachment 64259 [details]
Patch #1f (add RenderImageResource)

[Early Warning System Bot]

Attachment 64259 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3710112

[Patrick R. Gansterer]

Created attachment 64265 [details]
Patch #1g (add RenderImageResource)

[Early Warning System Bot]

Attachment 64265 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3726100

[Patrick R. Gansterer]

Created attachment 64272 [details]
Patch #1h (add RenderImageResource)

[WebKit Review Bot]

Attachment 64272 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3724115

[Nikolas Zimmermann]

Comment on attachment 64272 [details]
Patch #1h (add RenderImageResource)

Interessting, didn't think about the possibility to remove RenderImageGeneratedContent, with that removal your two new RenderImageResources classes make a lot more sense.
Though I still have a suggestion. I'd love to have an abstract RenderImageResource (renamed to RenderImageSource) class, and two other classes inheriting from it.

RenderImageSource (contains pure-virtual image/errorOccurred, etc.. functions)
RenderCachedImageSource (inherits from RenderImageSource)
RenderStyleImageSource (ditto)

The problem is that you're still storing the CachedResourceHandle<CachedImage> for RenderImageResourceStyleImage through inheritance, which wastes memory, as it's unused.
Your approach is fine in general, I'm just asking for one more class, to save memory, and some renames.

Do you agree with the idea?
I have another idea to avoid the toRenderFoo(renderer())->imageResource() dance, and would like to discuss that on IRC.

P.S. ClipboardGtk has problems, see gtk build output.

[Patrick R. Gansterer]

(In reply to comment #36)
> (From update of attachment 64272 [details])
> Interessting, didn't think about the possibility to remove RenderImageGeneratedContent, with that removal your two new RenderImageResources classes make a lot more sense.
:-)

> The problem is that you're still storing the CachedResourceHandle<CachedImage> for RenderImageResourceStyleImage through inheritance, which wastes memory, as it's unused.
I'm not sure if it's realy unused: RenderImageResourceStyleImage will call setCachedImage only if StyleImage::isCachedImage() is true. Otherwise the default CachedImage will be used.
I don't know the code exactly to say if that's correct. I only moved it from RenderImageGeneratedContent. ;-)

> Do you agree with the idea?
No problem.

[Nikolas Zimmermann]

(In reply to comment #37)
> (In reply to comment #36)
> > (From update of attachment 64272 [details] [details])
> > Interessting, didn't think about the possibility to remove RenderImageGeneratedContent, with that removal your two new RenderImageResources classes make a lot more sense.
> :-)
> 
> > The problem is that you're still storing the CachedResourceHandle<CachedImage> for RenderImageResourceStyleImage through inheritance, which wastes memory, as it's unused.
> I'm not sure if it's realy unused: RenderImageResourceStyleImage will call setCachedImage only if StyleImage::isCachedImage() is true. Otherwise the default CachedImage will be used.
> I don't know the code exactly to say if that's correct. I only moved it from RenderImageGeneratedContent. ;-)

Hah, that's just true, I overlooked that fact, so it's fine as is.

[Patrick R. Gansterer]

Created attachment 64315 [details]
Patch #1i (add RenderImageResource)

Added missing gtk change

[Patrick R. Gansterer]

Created attachment 64322 [details]
Patch #2b (remove Remove RenderImage inheritance)

[Patrick R. Gansterer]

Created attachment 65341 [details]
Patch #1j (add RenderImageResource)

[Nikolas Zimmermann]

Comment on attachment 65341 [details]
Patch #1j (add RenderImageResource)

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.

[Patrick R. Gansterer]

(In reply to comment #42)
> (From update of attachment 65341 [details])
> 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.

RenderImageResource::create needs a RenderObject* as a first parameter! Where should I get it from, when I use it in the constructor of RenderImage?

[Nikolas Zimmermann]

(In reply to comment #43)
> (In reply to comment #42)
> > (From update of attachment 65341 [details] [details])
> > 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.
> 
> RenderImageResource::create needs a RenderObject* as a first parameter! Where should I get it from, when I use it in the constructor of RenderImage?

Remove the RenderObject* paramater from the constructor, and add an initialize(RenderObject*) method, that can be called from the RenderImage constructor, passing "this" to the RenderImageResource.

[Patrick R. Gansterer]

Created attachment 65423 [details]
Patch #1k (add RenderImageResource)

[Early Warning System Bot]

Attachment 65423 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3806073

[Patrick R. Gansterer]

Created attachment 65425 [details]
Patch #1l (add RenderImageResource)

(In reply to comment #46)
> Attachment 65423 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/3806073

Ooops, missed a file.

[Nikolas Zimmermann]

Comment on attachment 65425 [details]
Patch #1l (add RenderImageResource)

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.

[Patrick R. Gansterer]

Created attachment 65547 [details]
Patch #1m (add RenderImageResource)

[Nikolas Zimmermann]

Comment on attachment 65547 [details]
Patch #1m (add RenderImageResource)

WebCore/rendering/RenderImageResource.cpp:31
 +  #include "RenderImageResourceStyleImage.h"
This include is unncessary.

WebCore/rendering/RenderImageResource.cpp:44
 +      if (m_cachedImage)
You should also ASSERT(m_renderer) here.

WebCore/rendering/RenderImageResource.cpp:87
 +      if (!m_cachedImage)
And here (before the if).

WebCore/rendering/RenderImageResource.h:31
 +  #include "StyleImage.h"
Superfluous.

WebCore/rendering/RenderImageResource.h:37
 +  class StyleImage;
Superflous.


WebCore/rendering/RenderImageResource.h:69
 +      RenderObject* renderer() { return m_renderer; }
I'd remove these accesor, and rather make m_renderer protected.

WebCore/rendering/RenderImageResourceStyleImage.cpp:47
 +      m_styleImage->removeClient(renderer());
Just use m_renderer here, once it's protected.

WebCore/rendering/RenderImageResourceStyleImage.cpp:53
 +      m_styleImage->addClient(renderer());
Just use the passed newRenderer here.

It's a pity you can't land on your own yet, otherwhise I'd give r+.
Please fix and upload a new version again, that I can r+/cq+.

[Patrick R. Gansterer]

Created attachment 65549 [details]
Patch #1n (add RenderImageResource)

(In reply to comment #50)
> WebCore/rendering/RenderImageResource.h:31
>  +  #include "StyleImage.h"
> Superfluous.
No, WrappedImagePtr is defined at StyleImage.h.

[Nikolas Zimmermann]

Comment on attachment 65549 [details]
Patch #1n (add RenderImageResource)

Looks good to me. Let's see if cq likes it too :-)

[WebKit Commit Bot]

Comment on attachment 65549 [details]
Patch #1n (add RenderImageResource)

Rejecting patch 65549 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
ld/Debug/WebKit.build/Objects-normal/i386/WebNSPasteboardExtras.o /Users/eseidel/Projects/CommitQueue/WebKit/mac/Misc/WebNSPasteboardExtras.mm normal i386 objective-c++ com.apple.compilers.gcc.4_2
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebKit.build/Debug/WebKit.build/Objects-normal/i386/WebNSAttributedStringExtras.o /Users/eseidel/Projects/CommitQueue/WebKit/mac/Misc/WebNSAttributedStringExtras.mm normal i386 objective-c++ com.apple.compilers.gcc.4_2
(2 failures)


Full output: http://queues.webkit.org/results/3710725

[Nikolas Zimmermann]

(In reply to comment #53)
> (From update of attachment 65549 [details])
> Rejecting patch 65549 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
> Last 500 characters of output:
> ld/Debug/WebKit.build/Objects-normal/i386/WebNSPasteboardExtras.o /Users/eseidel/Projects/CommitQueue/WebKit/mac/Misc/WebNSPasteboardExtras.mm normal i386 objective-c++ com.apple.compilers.gcc.4_2
>     Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebKit.build/Debug/WebKit.build/Objects-normal/i386/WebNSAttributedStringExtras.o /Users/eseidel/Projects/CommitQueue/WebKit/mac/Misc/WebNSAttributedStringExtras.mm normal i386 objective-c++ com.apple.compilers.gcc.4_2
> (2 failures)
> 
Ouch, RenderImageResource needs to be marked "PRIVATE" in the Xcode project file.
I guess you don't have a mac around to do that, Patrick?

[Patrick R. Gansterer]

(In reply to comment #54)
> Ouch, RenderImageResource needs to be marked "PRIVATE" in the Xcode project file.
> I guess you don't have a mac around to do that, Patrick?
no, sorry

[Patrick R. Gansterer]

Created attachment 65714 [details]
Patch #1o (add RenderImageResource)

[Nikolas Zimmermann]

Comment on attachment 65714 [details]
Patch #1o (add RenderImageResource)

As discussed on IRC, this approach is fine. The only drawback is the addition of one additional pointer, compared to the old solution -- but I think it's acceptable, given the higher level of abstraction it gives us.
Note: I ran all tests on Leopard locally, and it works just fine.

Landing for paroga.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66223 might have broken Qt Linux Release

[Patrick R. Gansterer]

Created attachment 65735 [details]
Patch #2c (remove Remove RenderImage inheritance)

rebased to head

[WebKit Review Bot]

Attachment 65735 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderSVGImage.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Created attachment 65739 [details]
Patch #2d (remove Remove RenderImage inheritance)

fixed include order

[Patrick R. Gansterer]

Created attachment 66010 [details]
Patch #2e (remove Remove RenderImage inheritance)

rebased to head

[Nikolas Zimmermann]

Created attachment 66208 [details]
Patch

[Dirk Schulze]

Comment on attachment 66208 [details]
Patch

Niko also fixed some suggestion, like better name for imageElt. So r=me

[Nikolas Zimmermann]

Landed in r66599.

[Simon Fraser (smfr)]

We've had two null m_imageResource crashes since this landed (bug 46140, bug 46120). How many more are lurking?

[Nikolas Zimmermann]

(In reply to comment #66)
> We've had two null m_imageResource crashes since this landed (bug 46140, bug 46120). How many more are lurking?

Sorry, but we obviously had no tests covering this, yet.

[Darin Adler]

We ran into another null imageResource crash involving the CSS content when the image is cached. Filing a new bug about it soon.

[Nikolas Zimmermann]

(In reply to comment #68)
> We ran into another null imageResource crash involving the CSS content when the image is cached. Filing a new bug about it soon.

Ouch, I'm happy to help with it, unless you already have a solution.
The only benefit is that we'll have better test coverage for these things soon, so they never come back...

Still I'm sorry, about the impact of this patch.

[Andy Estes]

Filed https://bugs.webkit.org/show_bug.cgi?id=47430 to track the crash Darin mentioned.

[Radar WebKit Bug Importer]

<rdar://problem/33758917>