Bug 137582 – Implement selection services menu for Legacy WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Tim Horton]

Now that ServicesOverlayController lives in WebCore, we can implement the requisite ChromeClient callbacks in Legacy WebKit to make the menu appear, and hook up the system bits.

[Radar WebKit Bug Importer]

<rdar://problem/18604241>

[Tim Horton]

Created attachment 239580 [details]
patch

[Tim Horton]

Comment on attachment 239580 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239580&action=review

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.h:3
> + * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
> + * Copyright (C) 2008, 2010 Nokia Corporation and/or its subsidiary(-ies)

This is wrong.

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.h:14
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of

This is wrong.

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:3
> + * Copyright (C) 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
> + * Copyright (C) 2008, 2010 Nokia Corporation and/or its subsidiary(-ies)

This is wrong.

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:14
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of

This is wrong.

[WebKit Commit Bot]

Attachment 239580 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:30:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 239580 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239580&action=review

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.h:61
> +    virtual void sharingServicePickerWillBeDestroyed(WebSharingServicePickerController *) override;
> +    virtual WebCore::Page* pageForSharingServicePicker(WebSharingServicePickerController *) override;
> +    virtual NSWindow *windowForSharingServicePicker(WebSharingServicePickerController *) override;
> +    virtual WebCore::FloatRect screenRectForCurrentSharingServicePickerItem(WebSharingServicePickerController *) override;
> +    virtual NSImage *imageForCurrentSharingServicePickerItem(WebSharingServicePickerController *) override;

I think these should take the WebSharingServicePickerController& as a reference.

I think the functions that return Objective-C objects should return RetainPtrs to better indicate the lifetime.

Can pageForSharingServicePicker return a Page&? Maybe it should return a MainFrame& ?

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:91
> +    static NeverDestroyed<NSAttributedString *> attributedString([[NSAttributedString alloc] initWithString:@"a"]);

Never use NeverDestroyed with pointers. Also, I think it's fine if this isn't static (No need for it to take up memory).

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:97
> +    static NSAttributedString *attributedStringWithRichContent;

Ditto, I don't think this needs to be static.

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:100
> +        static NeverDestroyed<NSImage *> image([[NSImage alloc] init]);

Same thing here.

> Source/WebKit/mac/WebView/WebView.mm:8496
> +- (WebSelectionServiceController*)_selectionServiceController

This should return a reference.

[Anders Carlsson]

Comment on attachment 239580 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239580&action=review

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.h:61
> +    virtual void sharingServicePickerWillBeDestroyed(WebSharingServicePickerController *) override;
> +    virtual WebCore::Page* pageForSharingServicePicker(WebSharingServicePickerController *) override;
> +    virtual NSWindow *windowForSharingServicePicker(WebSharingServicePickerController *) override;
> +    virtual WebCore::FloatRect screenRectForCurrentSharingServicePickerItem(WebSharingServicePickerController *) override;
> +    virtual NSImage *imageForCurrentSharingServicePickerItem(WebSharingServicePickerController *) override;

I think these should take the WebSharingServicePickerController& as a reference.

I think the functions that return Objective-C objects should return RetainPtrs to better indicate the lifetime.

Can pageForSharingServicePicker return a Page&? Maybe it should return a MainFrame& ?

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:91
> +    static NeverDestroyed<NSAttributedString *> attributedString([[NSAttributedString alloc] initWithString:@"a"]);

Never use NeverDestroyed with pointers. Also, I think it's fine if this isn't static (No need for it to take up memory).

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:97
> +    static NSAttributedString *attributedStringWithRichContent;

Ditto, I don't think this needs to be static.

> Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:100
> +        static NeverDestroyed<NSImage *> image([[NSImage alloc] init]);

Same thing here.

> Source/WebKit/mac/WebView/WebView.mm:8496
> +- (WebSelectionServiceController*)_selectionServiceController

This should return a reference.

[Tim Horton]

(In reply to comment #5)
> (From update of attachment 239580 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239580&action=review
> 
> > Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.h:61
> > +    virtual void sharingServicePickerWillBeDestroyed(WebSharingServicePickerController *) override;
> > +    virtual WebCore::Page* pageForSharingServicePicker(WebSharingServicePickerController *) override;
> > +    virtual NSWindow *windowForSharingServicePicker(WebSharingServicePickerController *) override;
> > +    virtual WebCore::FloatRect screenRectForCurrentSharingServicePickerItem(WebSharingServicePickerController *) override;
> > +    virtual NSImage *imageForCurrentSharingServicePickerItem(WebSharingServicePickerController *) override;
> 
> I think these should take the WebSharingServicePickerController& as a reference.
> 
> I think the functions that return Objective-C objects should return RetainPtrs to better indicate the lifetime.

Both seem reasonable.

> Can pageForSharingServicePicker return a Page&? Maybe it should return a MainFrame& ?

I'm totally unclear on this here.

> > Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:91
> > +    static NeverDestroyed<NSAttributedString *> attributedString([[NSAttributedString alloc] initWithString:@"a"]);
> 
> Never use NeverDestroyed with pointers. Also, I think it's fine if this isn't static (No need for it to take up memory).

Fine with me. De-staticed everything.

> > Source/WebKit/mac/WebView/WebView.mm:8496
> > +- (WebSelectionServiceController*)_selectionServiceController
> 
> This should return a reference.

Sure.

[Tim Horton]

Created attachment 239586 [details]
patch

With Anders' changes. However, Anders thinks I should look into putting more of this in WebCore to share with WebKit2 (so, having the Services ChromeClient overrides call directly back into WebCore in the Legacy WebKit case, or jump to the UI process and call back into WebCore in the WebKit2 case, and put the context menu code/NSSharingServicePicker code in WebCore). I'm not sure this is going to be super easy so I'm leaving this patch here in case I need to fall back to it.

[Tim Horton]

Comment on attachment 239586 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239586&action=review

> Source/WebKit2/ChangeLog:15
> +        Add a note that we should transition to using replaceSelectionWithAttributedString.

These comments are flipped :|

[Tim Horton]

Comment on attachment 239586 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239586&action=review

> Source/WebKit2/ChangeLog:15
> +        Add a note that we should transition to using replaceSelectionWithAttributedString.

These comments are flipped :|

[Tim Horton]

Comment on attachment 239586 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239586&action=review

> Source/WebKit2/ChangeLog:15
> +        Add a note that we should transition to using replaceSelectionWithAttributedString.

These comments are flipped :|

[Tim Horton]

Comment on attachment 239586 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239586&action=review

> Source/WebKit2/ChangeLog:15
> +        Add a note that we should transition to using replaceSelectionWithAttributedString.

These comments are flipped :|

[WebKit Commit Bot]

Attachment 239586 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Yay, bugzilla success.

[Tim Horton]

(In reply to comment #8)
> Created an attachment (id=239586) [details]
> patch
> 
> With Anders' changes. However, Anders thinks I should look into putting more of this in WebCore to share with WebKit2 (so, having the Services ChromeClient overrides call directly back into WebCore in the Legacy WebKit case, or jump to the UI process and call back into WebCore in the WebKit2 case, and put the context menu code/NSSharingServicePicker code in WebCore). I'm not sure this is going to be super easy so I'm leaving this patch here in case I need to fall back to it.

Doing this is pretty messy, and involves hugely changing how Legacy WebKit image services work, and image services and selection services and telephone number services for WebKit2, and is all in all a much much bigger change. I think we should do it separately, in the future, but try to get this patch landed sooner (it doesn't make things much worse, since WebKit1 already has a sharing service picker implementation there for image services).

[Tim Horton]

Created attachment 239893 [details]
try to fix mavericks/ml build

[WebKit Commit Bot]

Attachment 239893 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebSelectionServiceController.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 239893 [details]
try to fix mavericks/ml build

Does this actually break the windows build?  Plz to not break.

Otherwise, okay.

[Tim Horton]

http://trac.webkit.org/changeset/174791