[Webkit-unassigned] [Bug 171100] Implement the Web Share API

[bugzilla-daemon at webkit.org]

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

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

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

> Source/WebCore/page/Chrome.h:61
> +struct ShareDataWithParsedURL;

This should be sorted to the right place (alphabetically).

> Source/WebCore/page/ChromeClient.h:106
> +struct ShareDataWithParsedURL;

Sorting.

> Source/WebCore/page/Navigator.cpp:117
> +        m_frame->page()->chrome().runShareSheet(*m_frame, shareData, [documentReference = makeWeakPtr(m_frame->document()), promise = WTFMove(promise)] (bool completed) mutable {

Why do you capture the document? Also what's the 'mutable' about? (there might be a good reason?)

> Source/WebCore/page/Navigator.cpp:122
> +            promise->reject(Exception { AbortError, "Aborted by AbortSignal."_s });

Where did the weird string "Aborted by AbortSignal" come from?

> Source/WebKit/Scripts/webkit/messages.py:385
> +        'WebCore::ShareData': ['<WebCore/ShareData.h>'],

This one is not needed, this is just for overrides where the left and right hand side don't match.

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:500
> +// Defaults to false

Is this comment a lie? (it is)

> Source/WebKit/UIProcess/WebPageProxy.h:185
> +struct ShareData;

sooooooooooooooort

> Source/WebKit/UIProcess/WebPageProxy.h:1481
> +    void runShareSheet(uint64_t frameID, const WebCore::ShareDataWithParsedURL&,  uint64_t contextID);

You've got one extra space before the uint64_t

Also I think we should rename this to callbackID everywhere (it's the ... ID... of the... callback. it doesn't provide any context). Ideally it would also have a typedef instead of being a uint64_t everywhere.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:66
> +#import <WebCore/ShareData.h>

Sort (should go with other WebCore includes). Not sure how the stylebot didn't complain about this one, since this is totally something it knows about (perhaps unlike the others)

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:28
> +#import <UIKit/UIViewController.h>

It's slightly unusual to import a single UIKit header, IMO, but maybe it's OK. <UIKit/UIKit.h> is a safe bet, though.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:37
> +class TextStream;

Why?

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:50
> + at property (nonatomic, assign) id <WKShareSheetDelegate> delegate;

I would put properties after all the methods. Especially it feels weird if init isn't first. Also I think I'd separate these differently:

-init

-present
-dismiss

@properties

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:76
> +        [shareDataArray addObject:(NSString*)data.shareData.title];

Lots of ObjC stars on the wrong side (should be (NSString *)) throughout this function.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:107
> +    [[UIViewController _viewControllerForFullScreenPresentationFromView:_view.get().get()] dismissViewControllerAnimated:YES completion:nil];

.getAutoreleased() returns an autoreleased raw pointer, so you can avoid this weird .get().get() (the first one returns a very-shortlived-and-unnecessary RetainPtr).

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:127
> +    [self _dismissDisplayAnimated:NO];

How do we ever get here with an existing presented VC?

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:129
> +    _presentationViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view.get().get()];

Ditto.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3694
> +    

A few places you're adding unnecessary whitespace. I think maybe from when you had the listener.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6043
> +static uint64_t nextShareSheetContextId()

s/context/callback, like I said above

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6094
> +    ASSERT(callback);

I don't think there's any need for this assert, we're just about to crash if it's null, so there's no point crashing one line earlier only in debug builds :) No idea why the person above did that.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1076
> +    void runShareSheet(WebCore::Frame&, uint64_t frameID, WebCore::ShareDataWithParsedURL&, WTF::CompletionHandler<void(bool)>&& callback);

Why does this take a WebCore::Frame? it doesn't look like you need it for anything.

ACTUALLY if I follow it through, you don't need the frameID for anything either, right? I think these are vestiges of the code you copied.  This is the fun part where after growing for a while, your patch collapses to be smaller :)

> Source/WebKit/WebProcess/WebPage/WebPage.h:1735
> +    void runShareSheetResponse(bool wasCompleted, uint64_t contextId);

This is definitely not the right place for a function. WebKit style puts all member variables below all member functions.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3016
> +- (BOOL)webShareEnabled

I don't think there's any need for WebKitLegacy SPI to turn this on/off. I don't think we'll likely implement for WebKitLegacy.

-- 
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/20180822/c228a453/attachment.html>