Bug 132362

Summary: Handle selection services menu
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Attachments:
Description Flags
Patch v1
none
Patch v2 - Review feedback and hopefully more building none

Description Brady Eidson 2014-04-29 15:59:06 PDT
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>
Comment 1 Brady Eidson 2014-04-29 16:05:32 PDT
Created attachment 230430 [details]
Patch v1
Comment 2 Tim Horton 2014-04-29 16:12:42 PDT
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...
Comment 3 Brady Eidson 2014-04-29 16:48:34 PDT
(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.
Comment 4 Brady Eidson 2014-04-29 16:56:13 PDT
Created attachment 230441 [details]
Patch v2 - Review feedback and hopefully more building
Comment 5 Tim Horton 2014-04-29 16:59:59 PDT
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
Comment 6 Brady Eidson 2014-04-29 17:07:50 PDT
(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 7 WebKit Commit Bot 2014-04-29 18:04:44 PDT
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>
Comment 8 WebKit Commit Bot 2014-04-29 18:04:47 PDT
All reviewed patches have been landed.  Closing bug.