Bug 143620 – Share sheets from Share menus appear outside the browser window

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Share sheets from Share menus appear outside the browser window

This is because we don't implement the appropriate delegates to tell them which window is the source.

rdar://problem/20455592

[Brady Eidson]

Created attachment 250536 [details]
Patch v1

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKSharingServicePickerDelegate.mm:34:  "WebCore/NSSharingServiceSPI.h" already included at Source/WebKit2/UIProcess/mac/WKSharingServicePickerDelegate.mm:33  [build/include] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKSharingServicePickerDelegate.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3 in 14 files


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

[Brady Eidson]

Created attachment 250544 [details]
Patch v2

[Darin Adler]

Comment on attachment 250544 [details]
Patch v2

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

Looks like this doesn’t compile yet. So obviously, please don’t land until it does!

> Source/WebKit/mac/Misc/WebSharingServicePickerController.h:42
> +    WebSharingServicePickerClient(WebView *);

explicit please

Also should make this constructor be protected.

> Source/WebKit/mac/Misc/WebSharingServicePickerController.h:64
> +- (instancetype)initWithSharingServicePicker:(NSSharingServicePicker *)sharingServicePicker client:(WebSharingServicePickerClient*)pickerClient;

Can client be a reference instead of a pointer?

> Source/WebKit/mac/Misc/WebSharingServicePickerController.mm:44
> +    : m_webView(webView)

What prevents us from holding on to the WebView pointer after the WebView is deallocated?

> Source/WebKit/mac/Misc/WebSharingServicePickerController.mm:106
> +    _handleEditingReplacement = NO;

I don’t think we need this. Objective-C initializes everything to NO.

> Source/WebKit/mac/WebCoreSupport/WebContextMenuClient.h:41
> +class WebContextMenuClient : public WebCore::ContextMenuClient, public WebSharingServicePickerClient
>  {

Should move the brace up one line.

Can the derivation be private here instead of public?

> Source/WebKit/mac/WebView/WebHTMLView.mm:3365
> +    ContextMenuClient& contextMenuClient = page->contextMenuController().client();

I suggest using auto& here instead of uttering the type. Also probably should do the typecast here instead of down in the code below.

> Source/WebKit/mac/WebView/WebHTMLView.mm:3367
> +    Vector<ContextMenuItem> menuItemVector = contextMenuItemVector(coreMenu->platformDescription());

I suggest using auto here instead of uttering the type.

[Brady Eidson]

Created attachment 250650 [details]
Patch v3 - Review feedback and fixing older builds

[Brady Eidson]

Going to work through the build failures with EWS, then prep to land.

[Brady Eidson]

Created attachment 250659 [details]
Patch v4 - More feature guards.

[Brady Eidson]

Created attachment 250660 [details]
Patch v5 - Still more feature guards

[Brady Eidson]

Created attachment 250661 [details]
Patch v6 - Mo' better feature guards

[Brady Eidson]

Created attachment 250663 [details]
Patch v7 - Potentially landable

[WebKit Commit Bot]

Comment on attachment 250663 [details]
Patch v7 - Potentially landable

Clearing flags on attachment: 250663

Committed r182756: <http://trac.webkit.org/changeset/182756>

[Ahmad Saleem]

Landed - https://github.com/WebKit/WebKit/commit/95ef8bd83955487e2e8db1f9513bd0d8377d730d

and didn't backed out.

Marking this as "RESOLVED FIXED". Thanks!