<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Support wide gamut for Drag Image UI"
href="https://bugs.webkit.org/show_bug.cgi?id=164490">bug 164490</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #294116 Flags</td>
<td>review?, commit-queue?
</td>
<td>review+, commit-queue-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Support wide gamut for Drag Image UI"
href="https://bugs.webkit.org/show_bug.cgi?id=164490#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Support wide gamut for Drag Image UI"
href="https://bugs.webkit.org/show_bug.cgi?id=164490">bug 164490</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=294116&action=diff" name="attach_294116" title="Patch">attachment 294116</a> <a href="attachment.cgi?id=294116&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=294116&action=review">https://bugs.webkit.org/attachment.cgi?id=294116&action=review</a>
Looks good, some fixes needed to build, and some other ideas for improvement.
<span class="quote">> Source/WebCore/ChangeLog:8
> + Added a new kind of NSImage from Bitmap that does not copy.</span >
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.
<span class="quote">> Source/WebCore/ChangeLog:9
> + Fixed a few errors for wide gamut support on mac.</span >
Capitalization: Mac
Also, this is a vague comment, not clear what kind of errors we mean.
<span class="quote">> Source/WebCore/ChangeLog:10
> + Updated a few depricated constants.</span >
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.
<span class="quote">> Source/WebCore/ChangeLog:12
> + This is volitile UI, which does not currently have a structure to test with.</span >
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.
<span class="quote">> 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):</span >
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.
<span class="quote">> 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.
> +};</span >
We should consider changing this definition to something more like this:
enum class BackingStoreCopyPolicy { Copy, DontCopy };
<span class="quote">> Source/WebCore/platform/graphics/mac/ImageMac.mm:28
> #import "BitmapImage.h"
> +#import "ImageDefines.h"</span >
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.
<span class="quote">> Source/WebCore/platform/graphics/mac/ImageMac.mm:138
> + auto data = tiffRepresentation({ nativeImage });
> + if (!data)
> + return nullptr;
> + return adoptNS([[NSImage alloc] initWithData:(NSData*)data.get()]);</span >
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.
<span class="quote">> Source/WebCore/platform/mac/DragImageMac.mm:38
> +#import "ImageBuffer.h"</span >
I don’t see anything below that requires adding this import. Perhaps left over from an earlier version of the patch?
<span class="quote">> 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];</span >
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.
<span class="quote">> Source/WebCore/platform/mac/DragImageMac.mm:133
> + auto dragImage = image->snapshotNSImage(DontCopyBackingStore);
> [dragImage.get() setSize:(NSSize)size];
> return dragImage;</span >
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.
<span class="quote">> Source/WebKit2/ChangeLog:8
> + Fixed a few errors with the new wide gamut support in SharableBitmap.</span >
Spelling: ShareableBitmap
Same complaint as above. Not specific enough to tell me what the patch does.
<span class="quote">> Source/WebKit2/ChangeLog:9
> + Added support for wide gamut when in the create Image path.</span >
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.
<span class="quote">> Source/WebKit2/ChangeLog:10
> + Updated a few depricated constants.</span >
Spelling: deprecated
Same comment as above about what we did with the deprecated constants.
<span class="quote">> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:42
> - if ((flags & ShareableBitmap::SupportsExtendedColor) && screenSupportsExtendedColor()) {
> + if (flags & ShareableBitmap::SupportsExtendedColor) {</span >
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?
<span class="quote">> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:62
> +static CGColorSpaceRef colorSpaceType(ShareableBitmap::Flags flags)</span >
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.
<span class="quote">> 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;</span >
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.
<span class="quote">> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:77
> + CGColorSpaceRef colorSpace = colorSpaceType(m_flags);</span >
Could use auto here instead of writing out CGColorSpaceRef. But even better to just call this inside the function the way we call bitmapInfo.
<span class="quote">> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:59
> + flags |= screenSupportsExtendedColor(frame.mainFrame().view()) ? ShareableBitmap::SupportsExtendedColor : 0;</span >
I think this would be slightly clearer as an if statement rather than a trinary. What do you think?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>