[Webkit-unassigned] [Bug 164490] Support wide gamut for Drag Image UI
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Nov 7 22:39:05 PST 2016
https://bugs.webkit.org/show_bug.cgi?id=164490
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #294116|review?, commit-queue? |review+, commit-queue-
Flags| |
--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 294116
--> https://bugs.webkit.org/attachment.cgi?id=294116
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?
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161108/a44623b3/attachment-0001.html>
More information about the webkit-unassigned
mailing list