Summary: | Share sheets from Share menus appear outside the browser window | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||||||||
Component: | WebKit Misc. | Assignee: | Brady Eidson <beidson> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | ahmad.saleem792, bdakin, commit-queue, thorton | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Brady Eidson
2015-04-10 15:12:31 PDT
Created attachment 250536 [details]
Patch v1
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.
Created attachment 250544 [details]
Patch v2
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. Created attachment 250650 [details]
Patch v3 - Review feedback and fixing older builds
Going to work through the build failures with EWS, then prep to land. Created attachment 250659 [details]
Patch v4 - More feature guards.
Created attachment 250660 [details]
Patch v5 - Still more feature guards
Created attachment 250661 [details]
Patch v6 - Mo' better feature guards
Created attachment 250663 [details]
Patch v7 - Potentially landable
Comment on attachment 250663 [details] Patch v7 - Potentially landable Clearing flags on attachment: 250663 Committed r182756: <http://trac.webkit.org/changeset/182756> Landed - https://github.com/WebKit/WebKit/commit/95ef8bd83955487e2e8db1f9513bd0d8377d730d and didn't backed out. Marking this as "RESOLVED FIXED". Thanks! |