Bug 143620 - Share sheets from Share menus appear outside the browser window
Summary: Share sheets from Share menus appear outside the browser window
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-04-10 15:12 PDT by Brady Eidson
Modified: 2022-10-25 08:51 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 (34.74 KB, patch)
2015-04-10 15:23 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v2 (34.78 KB, patch)
2015-04-10 17:14 PDT, Brady Eidson
darin: review+
Details | Formatted Diff | Diff
Patch v3 - Review feedback and fixing older builds (35.39 KB, patch)
2015-04-13 10:19 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v4 - More feature guards. (35.88 KB, patch)
2015-04-13 11:06 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v5 - Still more feature guards (36.26 KB, patch)
2015-04-13 11:41 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v6 - Mo' better feature guards (36.33 KB, patch)
2015-04-13 11:44 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v7 - Potentially landable (42.63 KB, patch)
2015-04-13 12:18 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2015-04-10 15:12:31 PDT
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
Comment 1 Brady Eidson 2015-04-10 15:23:37 PDT
Created attachment 250536 [details]
Patch v1
Comment 2 WebKit Commit Bot 2015-04-10 15:25:50 PDT
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.
Comment 3 Brady Eidson 2015-04-10 17:14:48 PDT
Created attachment 250544 [details]
Patch v2
Comment 4 Darin Adler 2015-04-10 19:53:44 PDT
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.
Comment 5 Brady Eidson 2015-04-13 10:19:51 PDT
Created attachment 250650 [details]
Patch v3 - Review feedback and fixing older builds
Comment 6 Brady Eidson 2015-04-13 10:20:34 PDT
Going to work through the build failures with EWS, then prep to land.
Comment 7 Brady Eidson 2015-04-13 11:06:11 PDT
Created attachment 250659 [details]
Patch v4 - More feature guards.
Comment 8 Brady Eidson 2015-04-13 11:41:54 PDT
Created attachment 250660 [details]
Patch v5 - Still more feature guards
Comment 9 Brady Eidson 2015-04-13 11:44:53 PDT
Created attachment 250661 [details]
Patch v6 - Mo' better feature guards
Comment 10 Brady Eidson 2015-04-13 12:18:57 PDT
Created attachment 250663 [details]
Patch v7 - Potentially landable
Comment 11 WebKit Commit Bot 2015-04-13 14:24:09 PDT
Comment on attachment 250663 [details]
Patch v7 - Potentially landable

Clearing flags on attachment: 250663

Committed r182756: <http://trac.webkit.org/changeset/182756>
Comment 12 Ahmad Saleem 2022-10-25 08:51:51 PDT
Landed - https://github.com/WebKit/WebKit/commit/95ef8bd83955487e2e8db1f9513bd0d8377d730d

and didn't backed out.

Marking this as "RESOLVED FIXED". Thanks!