Bug 132362 – Handle selection services menu

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

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>

[Brady Eidson]

Created attachment 230430 [details]
Patch v1

[Tim Horton]

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...

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 230441 [details]
Patch v2 - Review feedback and hopefully more building

[Tim Horton]

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

[Brady Eidson]

(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 :)

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.