Bug 157580 – [Cairo] GraphicsContext3D::ImageExtractor should use the correct size for copying non-image surfaces

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[Cairo] GraphicsContext3D::ImageExtractor::extractImage() should use the correct size for copying

[Zan Dobersek]

Created attachment 278651 [details]
Patch

[Alex Christensen]

Comment on attachment 278651 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:229
> +            IntSize surfaceSize = cairoSurfaceSize(m_imageSurface.get());

This now has two calls to cairoSurfaceSize, one here and one on line 241.  Could the size change between these calls?

[Darin Adler]

Comment on attachment 278651 [details]
Patch

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

>> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:229
>> +            IntSize surfaceSize = cairoSurfaceSize(m_imageSurface.get());
> 
> This now has two calls to cairoSurfaceSize, one here and one on line 241.  Could the size change between these calls?

The two calls are calling cairoSurfaceSize on two different surfaces so I am not sure exactly what the point of your question is. Perhaps the idea is that since we create the surface in this code path, we already know the size, so we could slightly optimize by not calling cairoSurfaceSize compute it below, but that does not seem an important optimization to me.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:230
> +            RefPtr<cairo_surface_t> tmpSurface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, surfaceSize.width(), surfaceSize.height()));

I think we should prefer auto when storing the result of adoptRef. This code seems to also assume that cairo_image_surface_create will never return null. If that’s true, then we would typically prefer to use * so that adoptRef makes a Ref instead of a RefPtr.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:232
>              m_imageSurface = tmpSurface.release();

Should probably use WTFMove, not release.

[Zan Dobersek]

Comment on attachment 278651 [details]
Patch

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

>> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:230
>> +            RefPtr<cairo_surface_t> tmpSurface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, surfaceSize.width(), surfaceSize.height()));
> 
> I think we should prefer auto when storing the result of adoptRef. This code seems to also assume that cairo_image_surface_create will never return null. If that’s true, then we would typically prefer to use * so that adoptRef makes a Ref instead of a RefPtr.

Ref<> doesn't use the refIfNotNull<>()/deferIfNotNull<>() templates to ref/deref the object, but directly calls the ::ref() and ::deref() methods on the class. We overload those template helpers for Cairo objects (and objects from other APIs) to use appropriate ref/deref functions.

We also avoid using C++ references with forward-declared C API pointers, which would be the case with Ref<>.

[Zan Dobersek]

Created attachment 279008 [details]
Patch for landing

[Build Bot]

Comment on attachment 279008 [details]
Patch for landing

Attachment 279008 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1329429

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279009 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Zan Dobersek]

Comment on attachment 279008 [details]
Patch for landing

Clearing flags on attachment: 279008

Committed r200940: <http://trac.webkit.org/changeset/200940>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.