[webkit-reviews] review granted: [Bug 130424] Make image controls menu work in WK2 : [Attachment 227127] Patch v3 - More build fixes

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 130424: Make image controls menu work in WK2
https://bugs.webkit.org/show_bug.cgi?id=130424

Attachment 227127: Patch v3 - More build fixes
https://bugs.webkit.org/attachment.cgi?id=227127&action=review

------- Additional Comments from Tim Horton <thorton at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=227127&action=review


> Source/WebKit2/ChangeLog:47
> +	   (-[WKSharingServicePickerDelegate WebKit::]):

WebKit:: :|

> Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:57
> +void SharedMemory::Handle::clear()

maybe “release” is a better name than “clear”… I don’t know. More evident that
you’re losing something.

> Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:177
> +    // FIXME: What do we do if there are multiple items?

Please remove my silly comment (for now).

> Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:193
> +- (NSRect)sharingService:(NSSharingService *)sharingService
sourceFrameOnScreenForShareItem:(id <NSPasteboardWriting>)item
> +{
> +    notImplemented();
> +    return NSZeroRect;
> +}
> +
> +- (NSImage *)sharingService:(NSSharingService *)sharingService
transitionImageForShareItem:(id <NSPasteboardWriting>)item contentRect:(NSRect
*)contentRect
> +{
> +    notImplemented();
> +    return nil;
> +}

I think we can elide these.

> Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:354
> +    NSMenu* menu = m_servicesMenu ? m_servicesMenu.get() : [m_popup menu];

star on the wrong side

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3088
> +    m_contextMenu->replaceControlledImage(bitmap->createImage());

what happens in createImage if bitmapHandle.isNull()?