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.
<rdar://problem/18604241>
Created attachment 239580 [details] patch
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.
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.
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.
(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.
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.
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 :|
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.
Yay, bugzilla success.
(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).
Created attachment 239893 [details] try to fix mavericks/ml build
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.
Comment on attachment 239893 [details] try to fix mavericks/ml build Does this actually break the windows build? Plz to not break. Otherwise, okay.
http://trac.webkit.org/changeset/174791