Bug 60293 - [Cairo][WebKit2] Add an implementation of ShareableBitmap for Cairo
: [Cairo][WebKit2] Add an implementation of ShareableBitmap for Cairo
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P3 Normal
Assigned To:
:
: Gtk
:
: 58242 59655 60546
  Show dependency treegraph
 
Reported: 2011-05-05 12:40 PST by
Modified: 2011-05-10 11:34 PST (History)


Attachments
Patch (16.86 KB, patch)
2011-05-05 12:56 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (17.09 KB, patch)
2011-05-10 10:52 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-05-05 12:40:00 PST
ShareableBitmap is necessary for several features of WebKit2. The Cairo backend should have an implementation of this class.
------- Comment #1 From 2011-05-05 12:56:22 PST -------
Created an attachment (id=92452) [details]
Patch
------- Comment #2 From 2011-05-10 09:16:03 PST -------
(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).

> 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?
------- Comment #3 From 2011-05-10 10:52:59 PST -------
Created an attachment (id=92981) [details]
Patch for landing
------- Comment #4 From 2011-05-10 11:03:47 PST -------
(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?
------- Comment #5 From 2011-05-10 11:04:34 PST -------
(In reply to comment #2)

Thanks for the comments!

> (From update of attachment 92452 [details] [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.
------- Comment #6 From 2011-05-10 11:18:34 PST -------
(In reply to comment #4)
> (From update of attachment 92981 [details] [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!
------- Comment #7 From 2011-05-10 11:32:19 PST -------
Committed r86174: <http://trac.webkit.org/changeset/86174>