Bug 60293 – [Cairo][WebKit2] Add an implementation of ShareableBitmap for Cairo

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

ShareableBitmap is necessary for several features of WebKit2. The Cairo backend should have an implementation of this class.

[Martin Robinson]

Created attachment 92452 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 92452 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        * platform/graphics/cairo/CairoUtilities.cpp:
> +        (WebCore::copyCairoImageSurface): Added this helper which creates a deep
> +        copy of a Cairo image surface.

Would be good to mention where this code came from (though it's fairly obvious).

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:162
> +cairo_surface_t* copyCairoImageSurface(cairo_surface_t* originalSurface)

This should return a PassRefPtr<cairo_surface_t>.

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:167
> +    cairo_surface_t* newSurface = cairo_image_surface_create(cairo_image_surface_get_format(originalSurface), 

And this should be a RefPtr<cairo_surface_t>.

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:99
> +    cairo_pattern_t* pattern = cairo_pattern_create_for_surface(surface);

Looks like this is being leaked? (RefPtr to the rescue!)

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:112
> +        if (cairo_t* shadowContext = shadow->beginShadowLayer(context, destRect)) {
> +            drawPatternToCairoContext(shadowContext, pattern, destRect, 1);
> +            shadow->endShadowLayer(context);
> +        }

Do we need to destroy shadowContext? Maybe endShadowLayer does that for you?

> Source/WebKit2/Shared/ShareableBitmap.h:113
> +    cairo_surface_t* createCairoSurface();

This should return a PassRefPtr.

> Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:56
> +    const FloatRect destRect = FloatRect(dstPoint, srcRect.size());

No need for the "const" here. And you can just use straight construction syntax, rather than construction+assignment.

> Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:57
> +    context.platformContext()->drawSurfaceToContext(surfaceCopy.get(), destRect, FloatRect(srcRect), &context);

I think the explicit FloatRect() isn't needed.

> Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:62
> +    cairo_surface_t* image = cairo_image_surface_create_for_data(static_cast<unsigned char*>(data()),

This should be a RefPtr.

> Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:68
> +    static cairo_user_data_key_t dataKey;

Why static?

[Martin Robinson]

Created attachment 92981 [details]
Patch for landing

[Adam Roben (:aroben)]

Comment on attachment 92981 [details]
Patch for landing

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

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:112
> +        if (cairo_t* shadowContext = shadow->beginShadowLayer(context, destRect)) {
> +            drawPatternToCairoContext(shadowContext, pattern.get(), destRect, 1);
> +            shadow->endShadowLayer(context);
> +        }

I guess we don't need to do anything to clean up shadowContext? I didn't see a reply to my question on the other patch.

> Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:68
> +    static cairo_user_data_key_t dataKey;

Why static?

[Martin Robinson]

(In reply to comment #2)

Thanks for the comments!

> (From update of attachment 92452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92452&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        * platform/graphics/cairo/CairoUtilities.cpp:
> > +        (WebCore::copyCairoImageSurface): Added this helper which creates a deep
> > +        copy of a Cairo image surface.
> 
> Would be good to mention where this code came from (though it's fairly obvious).

I've updated the ChangeLog to be more explicit here.

> > Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:162
> > +cairo_surface_t* copyCairoImageSurface(cairo_surface_t* originalSurface)
> 
> This should return a PassRefPtr<cairo_surface_t>.

Fixed.

> > Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:167
> > +    cairo_surface_t* newSurface = cairo_image_surface_create(cairo_image_surface_get_format(originalSurface), 
> 
> And this should be a RefPtr<cairo_surface_t>.

Fixed.

> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:99
> > +    cairo_pattern_t* pattern = cairo_pattern_create_for_surface(surface);
> 
> Looks like this is being leaked? (RefPtr to the rescue!)

Nice catch. I've changed this to a RefPtr.

> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:112
> > +        if (cairo_t* shadowContext = shadow->beginShadowLayer(context, destRect)) {
> > +            drawPatternToCairoContext(shadowContext, pattern, destRect, 1);
> > +            shadow->endShadowLayer(context);
> > +        }
> 
> Do we need to destroy shadowContext? Maybe endShadowLayer does that for you?

endShadowLayer should take care of it. It owns shadowContext.

> > Source/WebKit2/Shared/ShareableBitmap.h:113
> > +    cairo_surface_t* createCairoSurface();
> 
> This should return a PassRefPtr.

Fixed.

> > Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:56
> > +    const FloatRect destRect = FloatRect(dstPoint, srcRect.size());
> 
> No need for the "const" here. And you can just use straight construction syntax, rather than construction+assignment.

Fixed.

> > Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:57
> > +    context.platformContext()->drawSurfaceToContext(surfaceCopy.get(), destRect, FloatRect(srcRect), &context);
> 
> I think the explicit FloatRect() isn't needed.

Fixed.

> > Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:62
> > +    cairo_surface_t* image = cairo_image_surface_create_for_data(static_cast<unsigned char*>(data()),
> 
> This should be a RefPtr.

Fixed.

> > Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:68
> > +    static cairo_user_data_key_t dataKey;
> 
> Why static?

The Cairo documentation suggests this is the best way to use the API. I did not want to deviate from that. I left it as static in the final patch.

[Martin Robinson]

(In reply to comment #4)
> (From update of attachment 92981 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92981&action=review
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:112
> > +        if (cairo_t* shadowContext = shadow->beginShadowLayer(context, destRect)) {
> > +            drawPatternToCairoContext(shadowContext, pattern.get(), destRect, 1);
> > +            shadow->endShadowLayer(context);
> > +        }
> 
> I guess we don't need to do anything to clean up shadowContext? I didn't see a reply to my question on the other patch.
> 
> > Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:68
> > +    static cairo_user_data_key_t dataKey;
> 
> Why static?

Hopefully I've answered your questions above. I'll begin the process of landing the patch. Feel free to ping me if you have further questions. Thanks again!

[Martin Robinson]

Committed r86174: <http://trac.webkit.org/changeset/86174>