[webkit-reviews] review denied: [Bug 113420] Regression(r142765) Broke Custom SVG cursors and SVG canvas drawing for Chromium : [Attachment 196413] Patch

[bugzilla-daemon at webkit.org]

Stephen White <senorblanco at chromium.org> has denied  review:
Bug 113420: Regression(r142765) Broke Custom SVG cursors and SVG canvas drawing
for Chromium
https://bugs.webkit.org/show_bug.cgi?id=113420

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

------- Additional Comments from Stephen White <senorblanco at chromium.org>
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?