Bug 164490 – [Cocoa] Support wide gamut for Drag Image UI

• Component: Images
• Status: RESOLVED FIXED

[Megan Gardner]

Support wide gamut for Drag Image UI

[Megan Gardner]

Created attachment 294093 [details]
Patch

[Tim Horton]

Comment on attachment 294093 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.h:101
> +    RetainPtr<NSImage> snapshotNSImageDirect() override;

System things usually call this "NoCopy". Maybe we should follow? Alternatively, we have the BackingStoreCopy enum.

[Simon Fraser (smfr)]

Comment on attachment 294093 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Updated a few depricated constants.

deprecated.

> Source/WebCore/platform/mac/DragImageMac.mm:122
> +            [image->snapshotNSImage() drawInRect:destRect fromRect:NSMakeRect(0, 0, size.width(), size.height()) operation:NSCompositingOperationSourceOver fraction:1.0];

Can't the be snapshotNSImageDirect?

[Megan Gardner]

Created attachment 294116 [details]
Patch

[Darin Adler]

Comment on attachment 294116 [details]
Patch

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

Looks good, some fixes needed to build, and some other ideas for improvement.

> Source/WebCore/ChangeLog:8
> +        Added a new kind of NSImage from Bitmap that does not copy.

Should reword. Not entirely clear what "NSImage from Bitmap" means here. If you mean "BitmapImage" then I think you should write that, rather than "Bitmap", which is the name of a WebKit class unrelated to graphics. But I think maybe the comment should be more like this:

    Added option to create an NSImage without copying the backing store.

But also, nothing in this change log mentions that we are now using the new option when creating a drag image, nor does the comment explain why this is desirable, which it should. The question "why" is typically the most important one to answer.

> Source/WebCore/ChangeLog:9
> +        Fixed a few errors for wide gamut support on mac.

Capitalization: Mac

Also, this is a vague comment, not clear what kind of errors we mean.

> Source/WebCore/ChangeLog:10
> +        Updated a few depricated constants.

Spelling: deprecated

We didn’t update the constants. I think you mean something more like this:

    Updated code to not used deprecated constants any more.

> Source/WebCore/ChangeLog:12
> +        This is volitile UI, which does not currently have a structure to test with.

Spelling: volatile

I don’t know exactly what it means to say "this is volatile UI"; I think you are saying that we don’t currently have a way to test what the drag image looks like in LayoutTests. Did you talk to others on the team about how to make that testable? I think we should consider doing that.

> Source/WebCore/ChangeLog:25
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * platform/graphics/BitmapImage.h:
> +        * platform/graphics/Image.h:
> +        (WebCore::Image::snapshotNSImage):
> +        * platform/graphics/ImageBuffer.h:
> +        * platform/graphics/ImageDefines.h: Added.
> +        * platform/graphics/cg/GraphicsContextCG.cpp:
> +        (WebCore::extendedSRGBColorSpaceRef):
> +        * platform/graphics/mac/ImageMac.mm:
> +        (WebCore::BitmapImage::snapshotNSImage):
> +        * platform/mac/DragImageMac.mm:
> +        (WebCore::createDragImageFromImage):

While not all participants in the project agree, I think that it's easiest to understand a patch if each file and function has its own separate comment here in the change log.

> Source/WebCore/platform/graphics/ImageDefines.h:33
> +enum BackingStoreCopy {
> +    CopyBackingStore, // Guarantee subsequent draws don't affect the copy.
> +    DontCopyBackingStore // Subsequent draws may affect the copy.
> +};

We should consider changing this definition to something more like this:

    enum class BackingStoreCopyPolicy { Copy, DontCopy };

> Source/WebCore/platform/graphics/mac/ImageMac.mm:28
>  #import "BitmapImage.h"
> +#import "ImageDefines.h"

This is not typical header naming for WebKit. We don’t name headers XXXDefines.h except in the rare case where the header is full of only preprocessor definitions (FeatureDefines.h).

Rather than suggest a different name for this header, I would suggest moving the BackingStoreCopy enumeration to GraphicsTypes.h rather than adding a new header and putting it there. Should make the patch considerably smaller and simpler.

> Source/WebCore/platform/graphics/mac/ImageMac.mm:138
> +        auto data = tiffRepresentation({ nativeImage });
> +        if (!data)
> +            return nullptr;
> +        return adoptNS([[NSImage alloc] initWithData:(NSData*)data.get()]);

Even when we copy, I am not sure that we should do it with the tiffRepresentation function, although I suppose it is safer not to change this.

> Source/WebCore/platform/mac/DragImageMac.mm:38
> +#import "ImageBuffer.h"

I don’t see anything below that requires adding this import. Perhaps left over from an earlier version of the patch?

> Source/WebCore/platform/mac/DragImageMac.mm:123
> +            [image->snapshotNSImage(DontCopyBackingStore) drawInRect:destRect fromRect:NSMakeRect(0, 0, size.width(), size.height()) operation:NSCompositingOperationSourceOver fraction:1.0];

We will need to include <wtf/mac/AppKitCompatibilityDeclarations.h> in WebCorePrefix.h like we do in WebKit2Prefix.h if we want to use NSCompositingOperationSourceOver, otherwise we can’t build with older versions of the macOS SDK.

> Source/WebCore/platform/mac/DragImageMac.mm:133
> +    auto dragImage = image->snapshotNSImage(DontCopyBackingStore);
>      [dragImage.get() setSize:(NSSize)size];
>      return dragImage;

Why is it safe to return an image without copying the backing store? I am not claiming it is unsafe, but it’s not obvious to me why it is OK.

> Source/WebKit2/ChangeLog:8
> +        Fixed a few errors with the new wide gamut support in SharableBitmap.

Spelling: ShareableBitmap

Same complaint as above. Not specific enough to tell me what the patch does.

> Source/WebKit2/ChangeLog:9
> +        Added support for wide gamut when in the create Image path.

Not sure exactly what "in the create Image path" means. Maybe you mean in the functions that create images? Not sure why image is capitalized.

> Source/WebKit2/ChangeLog:10
> +        Updated a few depricated constants.

Spelling: deprecated

Same comment as above about what we did with the deprecated constants.

> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:42
> -    if ((flags & ShareableBitmap::SupportsExtendedColor) && screenSupportsExtendedColor()) {
> +    if (flags & ShareableBitmap::SupportsExtendedColor) {

Why is this change OK? Nothing in the change log explains what was wrong before and why this is better.

Maybe the idea now is that callers are all now required to be careful to only pass SupportsExtendedColor when there is a screen that supports it?

> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:62
> +static CGColorSpaceRef colorSpaceType(ShareableBitmap::Flags flags)

This function does not return a "color space type", so the name colorSpaceType is not a good name for it. I suggest just colorSpace, by analogy with the function just above named bitmapInfo. And also, like that function, I suggest we just call it in place, instead of putting its result into a local variable.

> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:70
>      CGColorSpaceRef colorSpace;
> -    if (m_flags & ShareableBitmap::SupportsExtendedColor)
> +    
> +    if (flags & ShareableBitmap::SupportsExtendedColor)
>          colorSpace = extendedSRGBColorSpaceRef();
>      else
>          colorSpace = sRGBColorSpaceRef();
> +    return colorSpace;

Why add the blank line after the declaration? I like it better without.

But actually, this function would be even better without a local variable. Should just use return. And use our early return style, without an else.

> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:77
> +    CGColorSpaceRef colorSpace = colorSpaceType(m_flags);

Could use auto here instead of writing out CGColorSpaceRef. But even better to just call this inside the function the way we call bitmapInfo.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:59
> +    flags |= screenSupportsExtendedColor(frame.mainFrame().view()) ? ShareableBitmap::SupportsExtendedColor : 0;

I think this would be slightly clearer as an if statement rather than a trinary. What do you think?

[Said Abou-Hallawa]

Comment on attachment 294116 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/ImageMac.mm:141
> +    return adoptNS([[NSImage alloc] initWithCGImage:nativeImage.get() size:NSZeroSize]);

initWithCGImage returns an NSImage with NSImageRep of class NSCGImageSnapshotRep. On 2x display, it doubles the image size. Please see https://bugs.webkit.org/show_bug.cgi?id=162109#c32 and https://bugs.webkit.org/show_bug.cgi?id=162109#c34. Is this what you want?

> Source/WebCore/platform/mac/DragImageMac.mm:131
> +    auto dragImage = image->snapshotNSImage(DontCopyBackingStore);

I think this is going to break the API test: WebKit2.FindMatches. Make sure you pass all the API tests before landing.

[Megan Gardner]

Created attachment 294441 [details]
Patch

[Megan Gardner]

Created attachment 294446 [details]
Patch

[Tim Horton]

Comment on attachment 294446 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Fixed an error in the support define for wide gamut on Mac.

Maybe "use extended sRGB on macOS Sierra"? Or some such.

> Source/WebKit2/ChangeLog:8
> +        Fixed an error in the gating for the new wide gamut support in ShareableBitmap.

?

> Source/WebKit2/ChangeLog:9
> +        We should always respect the flags straight out, and not make decisions later, it can lead to mismatched data and data storage.

Maybe "Always respect the stored extended color flag instead of re-computing whether a buffer should use extended color." or something?

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:59
> +    if (screenSupportsExtendedColor(frame.mainFrame().view()))

is screenSupportsExtendedColor actually *any*ScreenSupportsExtendedColor? Otherwise, what if you drag from a sRGB to P3 display?

[WebKit Commit Bot]

Comment on attachment 294446 [details]
Patch

Clearing flags on attachment: 294446

Committed r208596: <http://trac.webkit.org/changeset/208596>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.