ShareableBitmap is necessary for several features of WebKit2. The Cairo backend should have an implementation of this class.
Created attachment 92452 [details] Patch
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?
Created attachment 92981 [details] Patch for landing
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?
(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.
(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!
Committed r86174: <http://trac.webkit.org/changeset/86174>