Bug 150988 – Adopt CGIOSurfaceContextCreateImageReference to avoid unnecessary readback

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Adopt CGIOSurfaceContextCreateImageReference to avoid unnecessary readback

[Tim Horton]

Created attachment 264984 [details]
Patch

[Build Bot]

Comment on attachment 264984 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 264988 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Tim Horton]

Created attachment 265070 [details]
Patch

[Tim Horton]

Created attachment 265103 [details]
Patch

[Tim Horton]

Created attachment 265126 [details]
Patch

[Tim Horton]

Created attachment 265214 [details]
Patch

[Tim Horton]

Simon noted that he doesn't necessarily love 'sinkInto...'. Any suggestions from the crowd?

[Darin Adler]

Comment on attachment 265214 [details]
Patch

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

Some of those WTF::move might not be needed, although most clearly are.

Looks great!

> Source/WebCore/platform/DragImage.cpp:91
> +static DragImageRef createDragImageFromSnapshot(std::unique_ptr<ImageBuffer>&& snapshot, Node* node)

Since std::unique_ptr is a move-only class, not sure that using && for the argument type has any value.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:197
> +    return BitmapImage::create(image.get()).ptr();

You shouldn’t need this ".ptr()"; can’t you initialize a Ref<Image> from a RefPtr<Image> without that?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:219
> +    RetainPtr<CGImageRef> image = sinkIntoNativeImage(WTF::move(imageBuffer));
> +    return createBitmapImageAfterScalingIfNeeded(WTF::move(image), internalSize, logicalSize, backingStoreSize, resolutionScale, scaleBehavior);

I probably would have done this without the local variable. Would also have obviated the need for one of the WTF::move as well.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:234
> +    std::unique_ptr<IOSurface> surface = IOSurface::createFromImageBuffer(WTF::move(imageBuffer));
> +    return IOSurface::sinkIntoImage(WTF::move(surface));

Would read better as a one-liner.

    return IOSurface::sinkIntoImage(IOSurface::createFromImageBuffer(WTF::move(imageBuffer)));

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:278
> +    std::unique_ptr<IOSurface> surface = IOSurface::createFromImageBuffer(WTF::move(imageBuffer));
> +    RetainPtr<CGImageRef> image = IOSurface::sinkIntoImage(WTF::move(surface));

Would read better as a one-liner:

    auto image = IOSurface::sinkIntoImage(IOSurface::createFromImageBuffer(WTF::move(imageBuffer)));

> Source/WebCore/platform/graphics/cocoa/IOSurface.h:52
> +    static std::unique_ptr<IOSurface> createFromImageBuffer(std::unique_ptr<ImageBuffer>&&);

Since std::unique_ptr is a move-only class, not sure that using && for the argument type has any value.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:102
> +std::unique_ptr<IOSurface> IOSurface::createFromImageBuffer(std::unique_ptr<ImageBuffer>&& imageBuffer)

Since std::unique_ptr is a move-only class, not sure that using && for the argument type has any value.

[Tim Horton]

Fingers crossed. http://trac.webkit.org/changeset/194025