Bug 113420 – Regression(r142765) Broke Custom SVG cursors and SVG canvas drawing for Chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

http://trac.webkit.org/changeset/142765 broken Custom SVG cursors for Chromium.

Can be reproduced via:
http://jsfiddle.net/davidaco/ePzfr/7/

[Chris Dumez]

The reason seems to be that WebImage relies on Image::nativeImageForCurrentFrame() which is no longer implemented for SVGImage.

[Chris Dumez]

Doing a partial revert of r142765 (basically bringing back exact same implementation of SVGImage::nativeImageForCurrentFrame()) fixes the issue. However, I'm not sure this is what we want.

[Chris Dumez]

Created attachment 195354 [details]
Patch

Note sure this is the right approach but it does fix the regression (tested via http://jsfiddle.net/davidaco/ePzfr/7/).

[Philip Rogers]

Comment on attachment 195354 [details]
Patch

Unfortunately this isn't the best approach.

We need to re-architect how nativeImageForCurrentFrame works so that the frame cache can be cleared when it is no longer used. With the approach in this patch we will keep around a big chunk of memory indefinitely. One approach would be to return a ref counted image.

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 195354 [details])
> Unfortunately this isn't the best approach.
> 
> We need to re-architect how nativeImageForCurrentFrame works so that the frame cache can be cleared when it is no longer used. With the approach in this patch we will keep around a big chunk of memory indefinitely. One approach would be to return a ref counted image.

Changing the prototype of nativeImageForCurrentFrame() to return a ref counted image is likely going to result in a lot of changes as nativeImageForCurrentFrame() is a virtual method from Image.

Could we maybe introduce a new method specific to SVGImage that returns a ref counted image?

Then, we could handle SVGImage as a special case in WebImage by using Image::isSVGImage().

[Philip Rogers]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 195354 [details] [details])
> > Unfortunately this isn't the best approach.
> > 
> > We need to re-architect how nativeImageForCurrentFrame works so that the frame cache can be cleared when it is no longer used. With the approach in this patch we will keep around a big chunk of memory indefinitely. One approach would be to return a ref counted image.
> 
> Changing the prototype of nativeImageForCurrentFrame() to return a ref counted image is likely going to result in a lot of changes as nativeImageForCurrentFrame() is a virtual method from Image.

It's actually not as bad as it might seem, but I'll admit it isn't the easiest change :( I attempted it in WK109894 but was pulled into other regressions and never finished it. Another user of nativeImageForCurrentFrame() is canvas.drawPattern which also regressed with the SVG image change: https://bugs.webkit.org/show_bug.cgi?id=109894. Both of these cases were untested before which is how the image change was able to land :/

> Could we maybe introduce a new method specific to SVGImage that returns a ref counted image?

I think returning a ref counted image would have other benefits as well, such as codifying the lifetime of the returned value. It's not obvious to me who owns the returned native image at the moment, or what happens when the native image is destroyed.

> 
> Then, we could handle SVGImage as a special case in WebImage by using Image::isSVGImage().

I have a personal goal of removing all special-casing of svg image except where we're special-casing for svg's awesomeness :) Of course, my personal pie-in-the-sky won't prevent a well thought out special-case implementation for nativeImageForCurrentFrame, but it's worth keeping in mind.

[Chris Dumez]

Created attachment 195411 [details]
Patch

Philip, is that what you had in mind. For now, I only ported Skia code as it is a bit big.

[Chris Dumez]

Created attachment 195422 [details]
Patch

Build fix for Chromium tests.

[Chris Dumez]

Comment on attachment 195422 [details]
Patch

I'm preparing a slightly improved version.

[Chris Dumez]

Created attachment 195439 [details]
Patch

- Make NativeImageSkia constructors private and add factory methods since it is RefCounted. Also remove its copy constructor and assignment operator since RefCounted objects are not meant to be copied.

[Chris Dumez]

Created attachment 195444 [details]
Patch

Removed a useless #if USE(SKIA) from a Skia-specific file.

[Eric Seidel (no email)]

Comment on attachment 195444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195444&action=review

> Source/WebCore/loader/icon/IconDatabase.cpp:306
> +#if USE(SKIA)
> +PassRefPtr<NativeImage>
> +#else
> +NativeImagePtr
> +#endif

This is quite ugly.  Perhaps we should define a NativeImagePassPtr type instead?

[Philip Rogers]

Comment on attachment 195444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195444&action=review

Overall, I like where this is going. I've added some high-level comments below.

> 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.

>> 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.

> 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?

> 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.

[Chris Dumez]

Comment on attachment 195444 [details]
Patch

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.

[Chris Dumez]

Comment on attachment 195444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195444&action=review

>>> 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.

Hmm. Actually, the test case from Bug 109894 is failing if I use DontCopyBackingStore. It passes with CopyBackingStore.

[Chris Dumez]

Created attachment 195515 [details]
Patch

Take feedback into consideration.

[Stephen White]

Comment on attachment 195515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195515&action=review

> Source/WebCore/platform/graphics/BitmapImage.h:97
> +#if USE(SKIA)
> +    RefPtr<NativeImage> m_frame;
> +#else
>      NativeImagePtr m_frame;
> +#endif

First off, does NativeImageSkia really need to be refcounted?  Considering that NativeImagePtr has been a bare pointer up until now, and that it remains a bare pointer on other ports (on CG it's a struct CGImage*, on QT it's a QPixmap*), why is Skia special?  Does our calling code do something that the other ports don't?  If so, I'd argue that calling code needs to be fixed.

Secondly, unless you really need shared ownership, OwnPtr/PassOwnPtr might be a better choice than RefPtr/PassRefPtr.  If you must change this, I'd recommend doing it to the typedef in NativeImagePtr.h.  If NativeImagePtr really needs to be changed, it should be done everywhere.  You're changing it only for BitmapImage (and inflicting a new #ifdef to this common code).

[Chris Dumez]

(In reply to comment #17)
> (From update of attachment 195515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195515&action=review
> 
> > Source/WebCore/platform/graphics/BitmapImage.h:97
> > +#if USE(SKIA)
> > +    RefPtr<NativeImage> m_frame;
> > +#else
> >      NativeImagePtr m_frame;
> > +#endif
> 
> First off, does NativeImageSkia really need to be refcounted?  Considering that NativeImagePtr has been a bare pointer up until now, and that it remains a bare pointer on other ports (on CG it's a struct CGImage*, on QT it's a QPixmap*), why is Skia special?  Does our calling code do something that the other ports don't?  If so, I'd argue that calling code needs to be fixed.

Please see implementation of SVGImage::nativeImageForCurrentFrame(). It is only enabled for Skia at the moment, because only Skia uses a smart pointer for the NativeImage. The implementation of SVGImage::nativeImageForCurrentFrame() passes ownership of the native image to the caller as we do not keep it internally (nor to we want to for memory usage reasons).

> 
> Secondly, unless you really need shared ownership, OwnPtr/PassOwnPtr might be a better choice than RefPtr/PassRefPtr.

As I commented in https://bugs.webkit.org/show_bug.cgi?id=113420#c14, using OwnPtr/PassOwnPtr would be problematic for BitmapImage::nativeImageForCurrentFrame(). BitmapImage owns the returned image only only returns a reference to it. We do not transfer ownership to the caller.

The main issue here is that SVGImage::nativeImageForCurrentFrame() and BitmapImage::nativeImageForCurrentFrame() behave differently. The first one transfer ownership of the native image to the caller while the latter does not. This is the reason why we switched to returning a RefPtr/PassRefPtr. This way, we don't have to worry about memory management and we don't have to handle SVGImage as a special case.

> If you must change this, I'd recommend doing it to the typedef in NativeImagePtr.h.  If NativeImagePtr really needs to be changed, it should be done everywhere.  You're changing it only for BitmapImage (and inflicting a new #ifdef to this common code).

I don't fully understand this one. I currently have a typedef PassRefPtr<NativeImage> NativeImagePassPtr, and AFAIK, I have replaced all instances for NativeImagePassPtr by PassRefPtr<NativeImage> where possible and for Skia. It is true that I still have #ifdefs for the RefPtr<NativeImage> case. We could add another typedef for this one in NativeImagePtr.h but I'm not sure how to name it. Should I call it simply NativeImagePtr ?

[Chris Dumez]

(In reply to comment #17)
> (From update of attachment 195515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195515&action=review
> 
> > Source/WebCore/platform/graphics/BitmapImage.h:97
> > +#if USE(SKIA)
> > +    RefPtr<NativeImage> m_frame;
> > +#else
> >      NativeImagePtr m_frame;
> > +#endif
> 
> First off, does NativeImageSkia really need to be refcounted?  Considering that NativeImagePtr has been a bare pointer up until now, and that it remains a bare pointer on other ports (on CG it's a struct CGImage*, on QT it's a QPixmap*), why is Skia special?  Does our calling code do something that the other ports don't?  If so, I'd argue that calling code needs to be fixed.

I forgot the mention that Skia is not really special here. It is just the only code I have ported in this patch as it is a lot of work to do it for all ports. If we choose this approach, this should probably be done incrementally.

It would be bad to keep returning a raw pointer and have nativeImageForCurrentFrame() transfer ownership if it is an SVGImage, and not transfer it if it is a BitmapImage. One solution that Philip proposed (and implemented in this patch) is to return a PassRefPtr to solve memory management issues. I could not find a better solution yet but I'm happy to go another way if someone does.

Personally, I think returning a smart pointer here is less bug prone. Before, we had nativeImageForCurrentFrame() that did not transfer ownership to the caller and ImageFrame::asNewNativeImage() that did. Both return a NativeImagePtr.

[Chris Dumez]

Created attachment 195560 [details]
Patch

- Add "typedef RefPtr<NativeImage> NativeImagePtr" for Skia to avoid #ifdef in BitmapImage.h.
- Replace NativeImagePtr by NativeImagePassPtr where suitable in non Skia specific code to make porting easier. For now, this does not change the behavior as NativeImagePassPtr is a typedef to NativeImagePtr for non-Skia code.

[Chris Dumez]

Note that Mac (CG) uses a CGImageRef type which is already refcounted. Therefore, changes needed for it would likely be minimal. We could probably use something like:
#if USE(CG)
typedef RetainPtr<CGImageRef> NativeImagePassPtr;
typedef RetainPtr<CGImageRef> NativeImagePtr;
#endif

The effort for Cairo would be similar as for Skia. Basically WebCore::NativeImageCairo would need to subclass RefCounted.

[Stephen White]

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

OK, so I'm trying to understand the ownership semantics here, so bear with me through my SVG-ignorance.

For most implementations of Image, nativeImageForCurrentFrame() simply returns (without copying) the underlying native image class.  For SVGImage, it seems as if it will now always allocate an ImageBuffer, render the SVG DOM, copy the ImageBuffer's backing store, and return the native image underlying it.  I can vaguely see how this will make all callers of Image (eg., canvas etc) transparently work with SVG images (vague due to my handwavey understanding of SVG).  But it does change the semantics a lot, and I don't understand the cacheing model.  Does this mean we will re-render the SVG DOM on every call to nativeImageForCurrentFrame(), even if the underlying SVG DOM has not changed?  Or am I missing some cacheing elsewhere?

Previously, most callers which received a NativeImagePtr assumed that they were not taking ownership.  SVG seems to now require an exception to that rule, which makes me wonder whether the fault lies in the SVG calling code, not in the platform code. Usually when we want a ref-countable image, we use Image itself, not NativeImagePtr.  Is that not possible in this case?

> >> 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.

[Stephen White]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 195515 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=195515&action=review
> > 
> > > Source/WebCore/platform/graphics/BitmapImage.h:97
> > > +#if USE(SKIA)
> > > +    RefPtr<NativeImage> m_frame;
> > > +#else
> > >      NativeImagePtr m_frame;
> > > +#endif
> > 
> > First off, does NativeImageSkia really need to be refcounted?  Considering that NativeImagePtr has been a bare pointer up until now, and that it remains a bare pointer on other ports (on CG it's a struct CGImage*, on QT it's a QPixmap*), why is Skia special?  Does our calling code do something that the other ports don't?  If so, I'd argue that calling code needs to be fixed.
> 
> Please see implementation of SVGImage::nativeImageForCurrentFrame(). It is only enabled for Skia at the moment, because only Skia uses a smart pointer for the NativeImage. The implementation of SVGImage::nativeImageForCurrentFrame() passes ownership of the native image to the caller as we do not keep it internally (nor to we want to for memory usage reasons).
> 
> > 
> > Secondly, unless you really need shared ownership, OwnPtr/PassOwnPtr might be a better choice than RefPtr/PassRefPtr.
> 
> As I commented in https://bugs.webkit.org/show_bug.cgi?id=113420#c14, using OwnPtr/PassOwnPtr would be problematic for BitmapImage::nativeImageForCurrentFrame(). BitmapImage owns the returned image only only returns a reference to it. We do not transfer ownership to the caller.

Sorry for not reading the earlier comments.  I think I see now why an OwnPtr will not work:  since sometimes you want the caller to own it (SVG), and sometimes you don't (everything else).

> The main issue here is that SVGImage::nativeImageForCurrentFrame() and BitmapImage::nativeImageForCurrentFrame() behave differently. The first one transfer ownership of the native image to the caller while the latter does not. This is the reason why we switched to returning a RefPtr/PassRefPtr. This way, we don't have to worry about memory management and we don't have to handle SVGImage as a special case.
> 
> > If you must change this, I'd recommend doing it to the typedef in NativeImagePtr.h.  If NativeImagePtr really needs to be changed, it should be done everywhere.  You're changing it only for BitmapImage (and inflicting a new #ifdef to this common code).
> 
> I don't fully understand this one. I currently have a typedef PassRefPtr<NativeImage> NativeImagePassPtr, and AFAIK, I have replaced all instances for NativeImagePassPtr by PassRefPtr<NativeImage> where possible and for Skia. It is true that I still have #ifdefs for the RefPtr<NativeImage> case. We could add another typedef for this one in NativeImagePtr.h but I'm not sure how to name it. Should I call it simply NativeImagePtr ?

Actually, I was thinking something more Skia-specific, in the #if USE(SKIA) section:

typedef RefPtr<NativeImageSkia> NativeImagePtr;

The fully-generic RefPtr version may not work for CG, for example, since I think you'll need to keep NativeImagePtr as a CGImageRef.

I would like to see all the other RefPtr<NativeImageSkia> and RefPtr<NativeImage> that you have in the code currently to just NativeImagePtr.  Would that work?

[Chris Dumez]

(In reply to comment #23)
> (In reply to comment #18)
> > (In reply to comment #17)
> Actually, I was thinking something more Skia-specific, in the #if USE(SKIA) section:
> 
> typedef RefPtr<NativeImageSkia> NativeImagePtr;

I believe this is what I did in my latest patch.

> 
> The fully-generic RefPtr version may not work for CG, for example, since I think you'll need to keep NativeImagePtr as a CGImageRef.

Yes, as I said in #21, CG would probably use RetainPtr instead of RefPtr that it could still remain generic from outside NativeImahePtr.h, e.g.:
#if USE(CG)
typedef RetainPtr<CGImageRef> NativeImagePassPtr;
typedef RetainPtr<CGImageRef> NativeImagePtr;
#endif

> 
> I would like to see all the other RefPtr<NativeImageSkia> and RefPtr<NativeImage> that you have in the code currently to just NativeImagePtr.  Would that work?

Sure, that would work. However, if I'm not mistaken, RefPtr<NativeImageSkia> only remains in Skia-specific files (no #ifdefs, at least in my latest patch). Do we really want to use NativeImagePtr for them as well? I see the point of NativeImagePtr / NativeImagePassPtr in generic code (to avoid #ifdefs). But I personally think it is slightly less readable than RefPtr<NativeImageSkia> in Skia files.

[Stephen White]

(In reply to comment #15)
> (From update of attachment 195444 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195444&action=review
> 
> >>> 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.
> 
> Hmm. Actually, the test case from Bug 109894 is failing if I use DontCopyBackingStore. It passes with CopyBackingStore.

OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size(), 1);
 153    if (!buffer) // failed to allocate image
 154        return 0;
 155
 156    draw(buffer->context(), rect(), rect(), ColorSpaceDeviceRGB, CompositeSourceOver, BlendModeNormal);
 157
 158    return buffer->copyImage(CopyBackingStore)->nativeImageForCurrentFrame();

Even with CopyBackingStore, this looks problematic to me.  You have a temporary ImageBuffer, from which you copy out a temporary Image, and return its current frame.  For plain old images (or BitmapImageSingleFrameSkia), this won't copy the pixels, so it looks like they would normally be freed along with the Image.

Perhaps the only reason it's not problematic is that the rest of your patch now makes even non-SVGImage NativeImagePtrs refcounted, so the ownership is transferred out.

(OTOH, I could be wrong about all this.)

[Chris Dumez]

(In reply to comment #25)
> (In reply to comment #15)
> > (From update of attachment 195444 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=195444&action=review
> > 
> > >>> 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.
> > 
> > Hmm. Actually, the test case from Bug 109894 is failing if I use DontCopyBackingStore. It passes with CopyBackingStore.
> 
> OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size(), 1);
>  153    if (!buffer) // failed to allocate image
>  154        return 0;
>  155
>  156    draw(buffer->context(), rect(), rect(), ColorSpaceDeviceRGB, CompositeSourceOver, BlendModeNormal);
>  157
>  158    return buffer->copyImage(CopyBackingStore)->nativeImageForCurrentFrame();
> 
> Even with CopyBackingStore, this looks problematic to me.  You have a temporary ImageBuffer, from which you copy out a temporary Image, and return its current frame.  For plain old images (or BitmapImageSingleFrameSkia), this won't copy the pixels, so it looks like they would normally be freed along with the Image.
> 
> Perhaps the only reason it's not problematic is that the rest of your patch now makes even non-SVGImage NativeImagePtrs refcounted, so the ownership is transferred out.

Yes, this works because ownership is transferred. See the implementation of nativeImageForCurrentFrame() for BitmapImageSingleFrameSkia:
PassRefPtr<NativeImage> BitmapImageSingleFrameSkia::nativeImageForCurrentFrame()
{
    return m_nativeImage; // m_nativeImage is a RefPtr<NativeImageSkia>.
}

The temporary BitmapImageSingleFrameSkia holds a ref to the NativeImage. The NativeImage gets ref'd when returned to the caller. Then the temporary BitmapImageSingleFrameSkia gets destroyed and decreases the ref count of the native image. The NativeImage does not go away because the caller of nativeImageForCurrentFrame() still holds a reference to it.

[Stephen White]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #18)
> > > (In reply to comment #17)
> > Actually, I was thinking something more Skia-specific, in the #if USE(SKIA) section:
> > 
> > typedef RefPtr<NativeImageSkia> NativeImagePtr;
> 
> I believe this is what I did in my latest patch.

No, what you have is:

typedef NativeImageSkia NativeImage;

...

typedef RefPtr<NativeImage> NativeImagePtr;

What I meant was to do all the skia-specific stuff in one place, and skip the creation of NativeImage altogether:

typedef RefPtr<NativeImageSkia> NativeImagePtr;
typedef PassRefPtr<NativeImageSkia> NativeImagePassPtr;

then just have the lower block be

#if !USE(SKIA)
typedef NativeImagePtr NativeImagePassPtr;
#endif

We could reconsider the introduction of NativeImage when this is expanded to other ports, but since NativeImage is never going to work for CG AFAICT (since it can't use RefPtr), I would consider this carefully.

> > I would like to see all the other RefPtr<NativeImageSkia> and RefPtr<NativeImage> that you have in the code currently to just NativeImagePtr.  Would that work?
> 
> Sure, that would work. However, if I'm not mistaken, RefPtr<NativeImageSkia> only remains in Skia-specific files (no #ifdefs, at least in my latest patch). Do we really want to use NativeImagePtr for them as well? I see the point of NativeImagePtr / NativeImagePassPtr in generic code (to avoid #ifdefs). But I personally think it is slightly less readable than RefPtr<NativeImageSkia> in Skia files.

I see your point -- it is more specific that way, especially if they were already using NativeImageSkia*, or assuming skia-specific functionality.  In generic code, though, I would prefer the use of NativeImagePtr and NativeImagePassPtr over RefPtr<NativeImage> and PassRefPtr<NativeImage> (especially if we get rid of NativeImage for now).

[Chris Dumez]

(In reply to comment #27)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > (In reply to comment #18)
> > > > (In reply to comment #17)
> > > Actually, I was thinking something more Skia-specific, in the #if USE(SKIA) section:
> > > 
> > > typedef RefPtr<NativeImageSkia> NativeImagePtr;
> > 
> > I believe this is what I did in my latest patch.
> 
> No, what you have is:
> 
> typedef NativeImageSkia NativeImage;
> 
> ...
> 
> typedef RefPtr<NativeImage> NativeImagePtr;
> 
> What I meant was to do all the skia-specific stuff in one place, and skip the creation of NativeImage altogether:
> 
> typedef RefPtr<NativeImageSkia> NativeImagePtr;
> typedef PassRefPtr<NativeImageSkia> NativeImagePassPtr;
> 
> then just have the lower block be
> 
> #if !USE(SKIA)
> typedef NativeImagePtr NativeImagePassPtr;
> #endif
> 
> We could reconsider the introduction of NativeImage when this is expanded to other ports, but since NativeImage is never going to work for CG AFAICT (since it can't use RefPtr), I would consider this carefully.

I see your point. I'll update the patch accordingly, thanks.

[Chris Dumez]

Created attachment 195580 [details]
Patch

Take Stephen White's feedback into consideration.

[Stephen White]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #15)
> > > (From update of attachment 195444 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=195444&action=review
> > > 
> > > >>> 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.
> > > 
> > > Hmm. Actually, the test case from Bug 109894 is failing if I use DontCopyBackingStore. It passes with CopyBackingStore.
> > 
> > OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size(), 1);
> >  153    if (!buffer) // failed to allocate image
> >  154        return 0;
> >  155
> >  156    draw(buffer->context(), rect(), rect(), ColorSpaceDeviceRGB, CompositeSourceOver, BlendModeNormal);
> >  157
> >  158    return buffer->copyImage(CopyBackingStore)->nativeImageForCurrentFrame();
> > 
> > Even with CopyBackingStore, this looks problematic to me.  You have a temporary ImageBuffer, from which you copy out a temporary Image, and return its current frame.  For plain old images (or BitmapImageSingleFrameSkia), this won't copy the pixels, so it looks like they would normally be freed along with the Image.
> > 
> > Perhaps the only reason it's not problematic is that the rest of your patch now makes even non-SVGImage NativeImagePtrs refcounted, so the ownership is transferred out.
> 
> Yes, this works because ownership is transferred. See the implementation of nativeImageForCurrentFrame() for BitmapImageSingleFrameSkia:
> PassRefPtr<NativeImage> BitmapImageSingleFrameSkia::nativeImageForCurrentFrame()
> {
>     return m_nativeImage; // m_nativeImage is a RefPtr<NativeImageSkia>.
> }
> 
> The temporary BitmapImageSingleFrameSkia holds a ref to the NativeImage. The NativeImage gets ref'd when returned to the caller. Then the temporary BitmapImageSingleFrameSkia gets destroyed and decreases the ref count of the native image. The NativeImage does not go away because the caller of nativeImageForCurrentFrame() still holds a reference to it.

All true, which makes me wonder why DontCopyBackingStore doesn't work, since we're refcounting things at the NativeImageSkia level now, so we shouldn't be dependent upon the Image or ImageBuffer lifetime.  Could you investigate a little to see why DontCopyBackingStore doesn't work?  I think using CopyBackingStore is going to incur a pretty heavy performance penalty.

[Chris Dumez]

(In reply to comment #30)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (In reply to comment #15)
> > > > (From update of attachment 195444 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=195444&action=review
> > > > 
> > > > >>> 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.
> > > > 
> > > > Hmm. Actually, the test case from Bug 109894 is failing if I use DontCopyBackingStore. It passes with CopyBackingStore.
> > > 
> > > OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size(), 1);
> > >  153    if (!buffer) // failed to allocate image
> > >  154        return 0;
> > >  155
> > >  156    draw(buffer->context(), rect(), rect(), ColorSpaceDeviceRGB, CompositeSourceOver, BlendModeNormal);
> > >  157
> > >  158    return buffer->copyImage(CopyBackingStore)->nativeImageForCurrentFrame();
> > > 
> > > Even with CopyBackingStore, this looks problematic to me.  You have a temporary ImageBuffer, from which you copy out a temporary Image, and return its current frame.  For plain old images (or BitmapImageSingleFrameSkia), this won't copy the pixels, so it looks like they would normally be freed along with the Image.
> > > 
> > > Perhaps the only reason it's not problematic is that the rest of your patch now makes even non-SVGImage NativeImagePtrs refcounted, so the ownership is transferred out.
> > 
> > Yes, this works because ownership is transferred. See the implementation of nativeImageForCurrentFrame() for BitmapImageSingleFrameSkia:
> > PassRefPtr<NativeImage> BitmapImageSingleFrameSkia::nativeImageForCurrentFrame()
> > {
> >     return m_nativeImage; // m_nativeImage is a RefPtr<NativeImageSkia>.
> > }
> > 
> > The temporary BitmapImageSingleFrameSkia holds a ref to the NativeImage. The NativeImage gets ref'd when returned to the caller. Then the temporary BitmapImageSingleFrameSkia gets destroyed and decreases the ref count of the native image. The NativeImage does not go away because the caller of nativeImageForCurrentFrame() still holds a reference to it.
> 
> All true, which makes me wonder why DontCopyBackingStore doesn't work, since we're refcounting things at the NativeImageSkia level now, so we shouldn't be dependent upon the Image or ImageBuffer lifetime.  Could you investigate a little to see why DontCopyBackingStore doesn't work?  I think using CopyBackingStore is going to incur a pretty heavy performance penalty.

To be fair, it was using CopyBackingStore before so it is not going to be slower :)

I had a look at the code:
Here is the Skia implementation of ImageBuffer::copyImage():
PassRefPtr<Image> ImageBuffer::copyImage(BackingStoreCopy copyBehavior, ScaleBehavior) const
{
    return BitmapImageSingleFrameSkia::create(*m_data.m_platformContext.bitmap(), copyBehavior == CopyBackingStore, m_resolutionScale);
}

If we use DontCopyBackingStore, it looks to me that the BitmapImageSingleFrameSkia becomes invalid once the ImageBuffer gets destroyed.

In our case, the buffer gets destroyed when SVGImage::nativeImageForCurrentFrame() returns.

[Chris Dumez]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #26)
> > > (In reply to comment #25)
> > > > (In reply to comment #15)
> > > > > (From update of attachment 195444 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=195444&action=review
> > > > > 
> > > > > >>> 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.
> > > > > 
> > > > > Hmm. Actually, the test case from Bug 109894 is failing if I use DontCopyBackingStore. It passes with CopyBackingStore.
> > > > 
> > > > OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size(), 1);
> > > >  153    if (!buffer) // failed to allocate image
> > > >  154        return 0;
> > > >  155
> > > >  156    draw(buffer->context(), rect(), rect(), ColorSpaceDeviceRGB, CompositeSourceOver, BlendModeNormal);
> > > >  157
> > > >  158    return buffer->copyImage(CopyBackingStore)->nativeImageForCurrentFrame();
> > > > 
> > > > Even with CopyBackingStore, this looks problematic to me.  You have a temporary ImageBuffer, from which you copy out a temporary Image, and return its current frame.  For plain old images (or BitmapImageSingleFrameSkia), this won't copy the pixels, so it looks like they would normally be freed along with the Image.
> > > > 
> > > > Perhaps the only reason it's not problematic is that the rest of your patch now makes even non-SVGImage NativeImagePtrs refcounted, so the ownership is transferred out.
> > > 
> > > Yes, this works because ownership is transferred. See the implementation of nativeImageForCurrentFrame() for BitmapImageSingleFrameSkia:
> > > PassRefPtr<NativeImage> BitmapImageSingleFrameSkia::nativeImageForCurrentFrame()
> > > {
> > >     return m_nativeImage; // m_nativeImage is a RefPtr<NativeImageSkia>.
> > > }
> > > 
> > > The temporary BitmapImageSingleFrameSkia holds a ref to the NativeImage. The NativeImage gets ref'd when returned to the caller. Then the temporary BitmapImageSingleFrameSkia gets destroyed and decreases the ref count of the native image. The NativeImage does not go away because the caller of nativeImageForCurrentFrame() still holds a reference to it.
> > 
> > All true, which makes me wonder why DontCopyBackingStore doesn't work, since we're refcounting things at the NativeImageSkia level now, so we shouldn't be dependent upon the Image or ImageBuffer lifetime.  Could you investigate a little to see why DontCopyBackingStore doesn't work?  I think using CopyBackingStore is going to incur a pretty heavy performance penalty.
> 
> To be fair, it was using CopyBackingStore before so it is not going to be slower :)
> 
> I had a look at the code:
> Here is the Skia implementation of ImageBuffer::copyImage():
> PassRefPtr<Image> ImageBuffer::copyImage(BackingStoreCopy copyBehavior, ScaleBehavior) const
> {
>     return BitmapImageSingleFrameSkia::create(*m_data.m_platformContext.bitmap(), copyBehavior == CopyBackingStore, m_resolutionScale);
> }
> 
> If we use DontCopyBackingStore, it looks to me that the BitmapImageSingleFrameSkia becomes invalid once the ImageBuffer gets destroyed.
> 
> In our case, the buffer gets destroyed when SVGImage::nativeImageForCurrentFrame() returns.

I don't know much about this but maybe it would make sense to have a "StealBackingStore" action to avoid copying for such case (temporary ImageBuffer)? Skia seems to have SkBitmap::swap() that may be useful in this case.

As I said, I don't know much about this so it may be a silly idea :)

This may be a bit out of scope for this patch since I'm basically restoring the previous SVGImage::nativeImageForCurrentFrame() implementation that got removed in r142765.

[Philip Rogers]

Comment on attachment 195580 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195580&action=review

I think this is getting very close. Do you mind updating this with the comments I made below?

@senorblanco: Are you convinced we're doing the right thing here? If so, do you mind doing a final once-over?

I investigated whether we have a security issue on the other platforms and I think they look good after all. Cursors re-request the native image on every update. Canvas patterns ref the Image so caching the native image in the Image does not result in a use after free.

> LayoutTests/svg/canvas/canvas-pattern-svg.html:4
> +  <title>createPattern repeat test</title>

Can you change this to:
<title>Test for WK113420: SVG-based patterns should be drawn correctly.</title>

> LayoutTests/svg/canvas/canvas-pattern-svg.html:16
> +      var p;

nit: can you change this to 'pattern'?

> LayoutTests/svg/canvas/canvas-pattern-svg.html:18
> +      //ctx.strokeWidth(1.0);

Remove this.

> LayoutTests/svg/canvas/canvas-pattern-svg.html:45
> +       }

Indentation is a bit off here.

> Source/WebCore/ChangeLog:20
> +

You will also need to override SVGImageForContainer::nativeImageForCurrentFrame with an implementation like this:
virtual NativeImagePassPtr nativeImageForCurrentFrame() OVERRIDE
{
   return m_image->nativeImageForCurrentFrame();
}

It looks like the EWS flaked out, but I suspect the new test would have failed without this.

> Source/WebCore/ChangeLog:22
> +

Can you add function-level changelog entries for the non-trivial changes here? This will help us decipher what we were thinking a year from now :)

> Source/WebCore/platform/image-decoders/ImageDecoder.h:117
>          // FrameData::clear()).

Is this comment still valid?

> Source/WebCore/svg/graphics/SVGImage.cpp:152
> +    OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size(), 1);

size() will return the intrinsic size of the image which is probably what we want, but the spec only specifies this for bitmap images.
http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#dom-context-2d-createpattern

I think size() is correct but please double-check my logic here :) The previous implementation wrong for this but size() has been updated to return the intrinsic size.

> Source/WebCore/svg/graphics/SVGImage.cpp:158
> +    return buffer->copyImage(CopyBackingStore)->nativeImageForCurrentFrame();

Can you file a bug for DontCopyBackingStore not working and add a fixme like so:
// FIXME: WK(bug#): We should use DontCopyBackingStore here.

[Chris Dumez]

Comment on attachment 195580 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195580&action=review

I'll reupload a patch with the improvements you suggested, thanks.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:117
>>          // FrameData::clear()).
> 
> Is this comment still valid?

Yes, this is still what happens even if using a smart pointer like Skia now.

[Chris Dumez]

Created attachment 195891 [details]
Patch

New iteration of the patch based on Philip's feedback.

[Stephen White]

Comment on attachment 195891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195891&action=review

> LayoutTests/svg/canvas/canvas-pattern-svg-expected.txt:1
> +There should be one big square below containing four squares. Top left square should be filled with 3 rows of 3 circles. Top right square should have one column of 3 circles. The bottom left square should have one row with three circles. The bottom right square should have one circle in the top-left corner.

IWBN if this was a reftest or a script test, to avoid platform-specific baselines.

> Source/WebCore/loader/icon/IconDatabase.cpp:302
> +NativeImagePassPtr IconDatabase::synchronousNativeIconForPageURL(const String& pageURLOriginal, const IntSize& size)

Sorry for the bikeshedding, but perhaps this should be named PassNativeImagePtr?  Would match the naming of RefPtr/PassRefPtr better.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:44
> +// "NativeImage", it is a typedef to this type. It has an SkBitmap, and also

Nit:  this comment is stale (there is no "NativeImage" anymore).

> Source/WebCore/svg/graphics/SVGImage.cpp:145
> +// Passes ownership of the NativeImage to the caller so NativeImagePassPtr needs

Nit:  comment is now wrong (there is no NativeImage type anymore).

[Philip Rogers]

(In reply to comment #36)
> IWBN if this was a reftest or a script test, to avoid platform-specific baselines.
> 

I don't think this can be a reftest. Patterns draw into an intermediate buffer before being stamped out, the alpha is ever-so-slightly different which will cause a reftest to fail.

[Chris Dumez]

Philip, do you also prefer PassNativeImagePtr instead of NativeImagePassPtr?

I don't mind making the change, I just want to make sure everyone is in agreement before going through it :)

[Philip Rogers]

(In reply to comment #38)
> Philip, do you also prefer PassNativeImagePtr instead of NativeImagePassPtr?
> 
> I don't mind making the change, I just want to make sure everyone is in agreement before going through it :)

I don't think it's a huge deal, but I do agree with senorblanco that PassNativeImagePtr is a bit better. If you can make those changes though, I'm ready to r+ so we can land this today :)

[Philip Rogers]

Comment on attachment 195891 [details]
Patch

R+ but please address senorblanco's concerns.

[Chris Dumez]

(In reply to comment #40)
> (From update of attachment 195891 [details])
> R+ but please address senorblanco's concerns.

Ok, I'll address senorblanco's comments and reupload. Note however that we cannot really land it until the dependency (Bug 113492) lands or we may have a few crashes in the tests (at least I have them locally) :/

[Chris Dumez]

Created attachment 196192 [details]
Patch

Addressed SenorBlanco's comment (except the ref test). Waiting for dependency to land before setting cq flag.

[Chris Dumez]

Created attachment 196413 [details]
Patch

Requesting review again as the rebasing was not trivial now that BitmapImageSingleFrameSkia was removed.

[Philip Rogers]

Comment on attachment 196413 [details]
Patch

r=me. Would you like me to check the commit flag?

[Stephen White]

Comment on attachment 196413 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196413&action=review

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:52
> +    ,  m_resizeRequests(0)

Nit:  spacing looks off here.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:62
> +SkBitmap NativeImageSkia::deepSkBitmapCopy(const SkBitmap& bitmap)
> +{
> +    SkBitmap tmp;
> +    if (!bitmap.deepCopyTo(&tmp, bitmap.config()))
> +        bitmap.copyTo(&tmp, bitmap.config());
> +
> +    return tmp;

This doesn't feel like it belongs here, since it has nothing to do with NativeImageSkia.

How about we make NativeImageSkia always deal with shallow copies, and move this function into the places which need deep copying (ie., ImageBufferSkia.cpp)?

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:63
> +    PassRefPtr<NativeImageSkia> clone(CopyBehavior copyBehavior) const
> +    {
> +        return adoptRef(new NativeImageSkia(copyBehavior == CopyPixels ? deepSkBitmapCopy(m_image) : m_image, m_resolutionScale, m_resizedImage, m_cachedImageInfo, m_resizeRequests));
> +    }

It looks like this is always called with DoNotCopyPixels.  I suggest getting rid of the argument altogether, and always do shallow copying.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:552
> +    RefPtr<BitmapImage> opaqueImage = BitmapImage::create(NativeImageSkia::create(drawBitmap, NativeImageSkia::CopyPixels));

Does this actually need deep copying?

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:559
> +    RefPtr<BitmapImage> alphaImage = BitmapImage::create(NativeImageSkia::create(drawBitmap, NativeImageSkia::CopyPixels));

Same here?

[Chris Dumez]

Comment on attachment 196413 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196413&action=review

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:62
>> +    return tmp;
> 
> This doesn't feel like it belongs here, since it has nothing to do with NativeImageSkia.
> 
> How about we make NativeImageSkia always deal with shallow copies, and move this function into the places which need deep copying (ie., ImageBufferSkia.cpp)?

Personally, I liked that pixels copying was codified in the API. Right now, it is just a comment saying "please be careful this constructor makes a shallow copy". It is easy for the developer to overlook. But anyways, I'll move this the ImageBufferSkia.

>> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:559
>> +    RefPtr<BitmapImage> alphaImage = BitmapImage::create(NativeImageSkia::create(drawBitmap, NativeImageSkia::CopyPixels));
> 
> Same here?

Yes, this one does. I'll need to create another SkBitmap for alphaImage.

[Chris Dumez]

Created attachment 196448 [details]
Patch

Take Stephen's feedback into consideration.

[Stephen White]

Comment on attachment 196448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196448&action=review

Looks great.  The rest are just nits; we can do the copyImage() refactor in another patch if you like.  r=me

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:273
> +    const SkBitmap& bitmap = *m_data.m_platformContext.bitmap();
> +    RefPtr<Image> image = BitmapImage::create(NativeImageSkia::create(drawNeedsCopy(m_context.get(), context) ? deepSkBitmapCopy(bitmap) : bitmap));

Nit:  I think this could now simply call ImageBuffer::copyImage(drawNeedsCopy(m_context.get(), context)), no?

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:281
> +    const SkBitmap& bitmap = *m_data.m_platformContext.bitmap();
> +    RefPtr<Image> image = BitmapImage::create(NativeImageSkia::create(drawNeedsCopy(m_context.get(), context) ? deepSkBitmapCopy(bitmap) : bitmap));

Same here.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:53
> +    // This constructor does a shallow copy of the passed-in SkBitmap (ie., it

Nit:  technically this is a function, or factory function, not a constructor.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:61
> +    // This constructor does a shallow copy of the internal SkBitmap (ie., it

Nit:  same here.

[Chris Dumez]

Comment on attachment 196448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196448&action=review

>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:273
>> +    RefPtr<Image> image = BitmapImage::create(NativeImageSkia::create(drawNeedsCopy(m_context.get(), context) ? deepSkBitmapCopy(bitmap) : bitmap));
> 
> Nit:  I think this could now simply call ImageBuffer::copyImage(drawNeedsCopy(m_context.get(), context)), no?

Actually, resolutionScale might be different. draw uses 1 for resolutionScale but copyImage() uses m_resolutionScale.

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:53
>> +    // This constructor does a shallow copy of the passed-in SkBitmap (ie., it
> 
> Nit:  technically this is a function, or factory function, not a constructor.

Oops, I moved the comment from the constructor and forgot to update this part.

[Chris Dumez]

Comment on attachment 196448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196448&action=review

>>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:273
>>> +    RefPtr<Image> image = BitmapImage::create(NativeImageSkia::create(drawNeedsCopy(m_context.get(), context) ? deepSkBitmapCopy(bitmap) : bitmap));
>> 
>> Nit:  I think this could now simply call ImageBuffer::copyImage(drawNeedsCopy(m_context.get(), context)), no?
> 
> Actually, resolutionScale might be different. draw uses 1 for resolutionScale but copyImage() uses m_resolutionScale.

The CG implementation seems to call copyImage() and m_resolutionScale here so it should be fine. However, I'd prefer to keep this out of this patch as this is not strictly related and it may change the behavior.

[Chris Dumez]

Created attachment 196464 [details]
Patch for landing

Fixed the comments but did not refactor the ImageBufferSkia draw() methods.

[Stephen White]

Comment on attachment 196448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196448&action=review

>>>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:273
>>>> +    RefPtr<Image> image = BitmapImage::create(NativeImageSkia::create(drawNeedsCopy(m_context.get(), context) ? deepSkBitmapCopy(bitmap) : bitmap));
>>> 
>>> Nit:  I think this could now simply call ImageBuffer::copyImage(drawNeedsCopy(m_context.get(), context)), no?
>> 
>> Actually, resolutionScale might be different. draw uses 1 for resolutionScale but copyImage() uses m_resolutionScale.
> 
> The CG implementation seems to call copyImage() and m_resolutionScale here so it should be fine. However, I'd prefer to keep this out of this patch as this is not strictly related and it may change the behavior.

Ahh, good point.

[WebKit Review Bot]

Comment on attachment 196464 [details]
Patch for landing

Clearing flags on attachment: 196464

Committed r147622: <http://trac.webkit.org/changeset/147622>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.