[webkit-reviews] review canceled: [Bug 113420] Regression(r142765) Broke Custom SVG cursors for Chromium : [Attachment 195444] Patch

[bugzilla-daemon at webkit.org]

Christophe Dumez <dchris at gmail.com> has canceled Christophe Dumez
<dchris at gmail.com>'s request for review:
Bug 113420: Regression(r142765) Broke Custom SVG cursors for Chromium
https://bugs.webkit.org/show_bug.cgi?id=113420

Attachment 195444: Patch
https://bugs.webkit.org/attachment.cgi?id=195444&action=review

------- Additional Comments from Christophe Dumez <dchris at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=195444&action=review


>> Source/WebCore/ChangeLog:18
>> +	    No new tests, not testable via layout tests.
> 
> I'd really like to add a test for this so we don't regress
nativeImageForCurrentFrame again in the future. You can actually use the
testcase from WK109894, as this patch fixes it too.

Ok, will do. Thanks.

>>> Source/WebCore/loader/icon/IconDatabase.cpp:306
>>> +#endif
>> 
>> This is quite ugly.	Perhaps we should define a NativeImagePassPtr type
instead?
> 
> I think the non-Skia ports will push back here. Because of the differences in
how RefPtrs/raw ptrs are passed, I can't think of a way around
NativeImagePassPtr.

I agree this is not nice. NativeImagePassPtr sounds like a good idea to make
this cleaner.

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:-453
>> -	    // ImageSource::createFrameAtIndex() allocated |m_frame| and passed

> 
> This was gross and I'm glad you're fixing it!
> 
> This comment makes me think we should be using OwnPtr and PassOwnPtr for the
return value of nativeImageForCurrentFrame, but due to the clone() calls I'm
not sure if it would work out cleanly. Did you consider OwnPtr?

I don't think PassOwnPtr would be suitable as the return value of
nativeImageForCurrentFrame(). In the BitmapImage case, the NativeImage is owned
by BitmapImage::FrameData and nativeImageForCurrentFrame() merely returns a
pointer to it. If we use OwnPtr/PassOwnPtr, then the caller takes ownership of
the NativeImage. In the case of BitmapImage, this would mean copying (aka
cloning) the NativeImage.

>> Source/WebCore/svg/graphics/SVGImage.cpp:156
>> +	return
buffer->copyImage(CopyBackingStore)->nativeImageForCurrentFrame();
> 
> We want to use DontCopyBackingStore here.
> CopyBackingStore makes a deep copy of all the ImageBuffer pixels, whereas we
just want to re-use them. Fortunately, DontCopyBackingStore refs the backing
store so we should be safe.

Ok. I merely reused the previous implementation that got dropped without
altering it.