[Cairo] GraphicsContext3D::ImageExtractor::extractImage() should use the correct size for copying
Created attachment 278651 [details] Patch
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?
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.
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<>.
Created attachment 279008 [details] Patch for landing
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.
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
Comment on attachment 279008 [details] Patch for landing Clearing flags on attachment: 279008 Committed r200940: <http://trac.webkit.org/changeset/200940>
All reviewed patches have been landed. Closing bug.