[webkit-reviews] review denied: [Bug 118808] [cairo] canvas drawing on itself doesn't work with accelerated canvas : [Attachment 207969] updated patch: extractImage still didn't work when getting data from an accelerated canvas

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied arno.
<a.renevier at samsung.com>'s request for review:
Bug 118808: [cairo] canvas drawing on itself doesn't work with accelerated
canvas
https://bugs.webkit.org/show_bug.cgi?id=118808

Attachment 207969: updated patch: extractImage still didn't work when getting
data from an accelerated canvas
https://bugs.webkit.org/attachment.cgi?id=207969&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=207969&action=review


Looks pretty good, just a few minor points.

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:224
> +    RefPtr<cairo_surface_t> newSurface =
adoptRef(cairo_surface_create_similar(originalSurface,
> +	   cairo_surface_get_content(originalSurface),
> +	   size.width(), size.height()));

This can be one line.

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:267
> +    int width = 0;
> +    int height = 0;
> +    switch (cairo_surface_get_type(surface)) {
> +    case CAIRO_SURFACE_TYPE_IMAGE:
> +	   width = cairo_image_surface_get_width(surface);
> +	   height = cairo_image_surface_get_height(surface);
> +	   break;
> +#if ENABLE(ACCELERATED_2D_CANVAS)
> +    case CAIRO_SURFACE_TYPE_GL:
> +	   width = cairo_gl_surface_get_width(surface);
> +	   height = cairo_gl_surface_get_height(surface);
> +	   break;
> +#endif
> +    default:
> +	   ASSERT_NOT_REACHED();
> +	   break;
> +    }
> +    return IntSize(width, height);

This can just be:

    switch (cairo_surface_get_type(surface)) {
    case CAIRO_SURFACE_TYPE_IMAGE:
	return IntSize(cairo_image_surface_get_width(surface),
cairo_image_surface_get_height(surface));
#if ENABLE(ACCELERATED_2D_CANVAS)
    case CAIRO_SURFACE_TYPE_GL:
	return IntSize(cairo_gl_surface_get_width(surface),
cairo_gl_surface_get_height(surface));
#endif
    default:
	ASSERT_NOT_REACHED();
	return IntSize();
    }

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:225
> +	   RefPtr<cairo_surface_t> tmpSurface =
adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, m_imageWidth,
m_imageHeight));
> +	   RefPtr<cairo_t> cr = adoptRef(cairo_create(tmpSurface.get()));
> +	   cairo_set_source_surface(cr.get(), m_imageSurface.get(), 0, 0);
> +	   cairo_paint(cr.get());
> +	   m_imageSurface = tmpSurface;

Could you use copyRectFromOneSurfaceToAnother? Don't forget to use the source
operator.

> Source/WebCore/platform/graphics/gtk/ImageBufferGtk.cpp:53
> +	   IntSize size = cairoSurfaceSize(surface);
> +	   RefPtr<cairo_surface_t> newSurface =
adoptRef(cairo_image_surface_create(CAIRO_FORMAT_RGB24, size.width(),
size.height()));
> +	   RefPtr<cairo_t> cr = adoptRef(cairo_create(newSurface.get()));
> +	   cairo_set_source_surface(cr.get(), surface, 0, 0);
> +	   cairo_paint(cr.get());
> +	   pixbuf = adoptGRef(cairoSurfaceToGdkPixbuf(newSurface.get()));

This is a lot less efficient if the surface is already an image.