Handle selection service menu (click on selection overlays) This also combines the concepts of the "image services" and "selection services" as far as the menu and what to do after the menu returns. <rdar://problem/16727798>
Created attachment 230430 [details] Patch v1
Comment on attachment 230430 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=230430&action=review > Source/WebKit2/UIProcess/WebPageProxy.h:816 > + void replaceSelectionWithPasteboardData(const Vector<String>& types, const IPC::DataReference&); multiple types, one blob? I don't get it. > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:381 > + picker = adoptNS([[NSSharingServicePicker alloc] initWithItems:@[ nsImage.get() ]]); could share this line and have items be what this if() decides > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:384 > + NSAttributedString *selection = [[NSAttributedString alloc] initWithRTFD:selectionData documentAttributes:nil]; is this not a leak? does initWithItems: adopt? doesn't seem so above...
(In reply to comment #2) > (From update of attachment 230430 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230430&action=review > > > Source/WebKit2/UIProcess/WebPageProxy.h:816 > > + void replaceSelectionWithPasteboardData(const Vector<String>& types, const IPC::DataReference&); > > multiple types, one blob? I don't get it. Because WebCore is stupid, and somethings looks at one type, and other times looks at the other type. I originally coded it with multiple blobs, but each blob was the same... and there's not an IPC:: friendly way to have the same blob represented multiple times in a collection without duplicating the memory. It does seem less than ideal, but I don't want to fight any design battles until they're needed. > > > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:381 > > + picker = adoptNS([[NSSharingServicePicker alloc] initWithItems:@[ nsImage.get() ]]); > > could share this line and have items be what this if() decides Okay! > > > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:384 > > + NSAttributedString *selection = [[NSAttributedString alloc] initWithRTFD:selectionData documentAttributes:nil]; > > is this not a leak? does initWithItems: adopt? doesn't seem so above... Good catch. Last minute change.
Created attachment 230441 [details] Patch v2 - Review feedback and hopefully more building
Comment on attachment 230441 [details] Patch v2 - Review feedback and hopefully more building View in context: https://bugs.webkit.org/attachment.cgi?id=230441&action=review > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:253 > + RetainPtr<CGImageSourceRef> source = adoptCF(CGImageSourceCreateWithData((CFDataRef)data, NULL)); > + RetainPtr<CGImageRef> image = adoptCF(CGImageSourceCreateImageAtIndex(source.get(), 0, NULL)); nullptr
(In reply to comment #5) > (From update of attachment 230441 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230441&action=review > > > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:253 > > + RetainPtr<CGImageSourceRef> source = adoptCF(CGImageSourceCreateWithData((CFDataRef)data, NULL)); > > + RetainPtr<CGImageRef> image = adoptCF(CGImageSourceCreateImageAtIndex(source.get(), 0, NULL)); > > nullptr Coding style says NULL for C. While this is a C++ file, the API is C, and I think it's much more standard to use NULL with the CF apis. If that's no longer our thinking, coding style should be updated :)
Comment on attachment 230441 [details] Patch v2 - Review feedback and hopefully more building Clearing flags on attachment: 230441 Committed r167974: <http://trac.webkit.org/changeset/167974>
All reviewed patches have been landed. Closing bug.