[Webkit-unassigned] [Bug 189443] Implement the Web Share API for mac

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=189443

--- Comment #2 from Tim Horton <thorton at apple.com> ---
Comment on attachment 349217
  --> https://bugs.webkit.org/attachment.cgi?id=349217
Patch

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

> Source/WebKit/Shared/WebPreferencesDefaultValues.h:175
> -#if PLATFORM(IOS) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
> +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

You can't do this, because there are platforms other than ours. You can instead change it to:

#if PLATFORM(COCOA) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

which will be Mac and iOS but not watch/GTK/etc.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:206
> + at interface WKWebView () <NSFilePromiseProviderDelegate, NSDraggingSource, WKShareSheetDelegate>

This isn't about ENABLE(DRAG_SUPPORT), so it doesn't go here. Stick it up above with WebViewImplDelegate.

ACTUALLY you don't need this here at all because it's in WKWebViewInternal.h, right?

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.h:617
> +    RetainPtr<WKShareSheet> _shareSheet;

This is a member variable, it doesn't belong amongst functions. Move it down further.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2721
> +    
> +    _shareSheet = adoptNS([[WKShareSheet alloc] initWithView:(NSView<WKShareSheetDelegate> *)view]);

You should assert that the view conforms to WKShareSheetDelegate before this nasty cast. Also, you only need the cast when setting the delegate; initWithView doesn't care about protocol conformance.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:26
> -#if PLATFORM(IOS) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
> +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

Ditto the thing I said before about PLATFORM(COCOA)

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:43
> + at interface WKShareSheet : NSObject<NSSharingServicePickerDelegate>

We usually put spaces before the protocol conformance list (NSObject <NSSharingServicePickerDelegate>)

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:-29
> -#if PLATFORM(IOS) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

PLATFORM(COCOA) thing

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:32
> +#if PLATFORM(IOS)
>  #import "UIKitSPI.h"
> +#endif

Usually platform-specific includes go in a section separated by newlines below the others

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:94
> +    _sharingServicePickerForMenu = [[NSSharingServicePicker alloc] initWithItems:shareDataArray.get()];

You're missing an adoptNS here (this is initialized with a +1 reference count, and you're sticking it in a RetainPtr, which immediately retains it (to +2); it will never make it back to zero, so it will leak).

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:98
> +    NSRect mouseLocationRect = {[NSEvent mouseLocation], CGSizeMake(1.0, 1.0)};

Let's use NSMakeRect instead of weird curly braces.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:100
> +    NSRect mouseLocationInView = [_view.getAutoreleased() convertRect:mouseLocationInWindow fromView:nil];

No need for the .getAutoreleased()

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:118
> +/* Invoked when the user has selected a service and before it is executed. Service will be nil if the picker was dismissed.
> + */

No need for this comment.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:170
>  - (void)invokeShareSheetWithResolution:(BOOL)resolved

How does your test work without this implemented? (I see below that it perhaps does not)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180908/65dcbc9d/attachment.html>