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

[bugzilla-daemon at webkit.org]

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

--- Comment #9 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 347525
  --> https://bugs.webkit.org/attachment.cgi?id=347525
Patch

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

Overall, this looks great! A few minor comments:

> Source/WebCore/loader/EmptyClients.cpp:440
> +void EmptyChromeClient::runShareSheet(Frame&, ShareDataWithParsedURL&, WTF::CompletionHandler<void(bool)>&&)

Nit - the WTF:: here too.

> Source/WebCore/loader/EmptyClients.h:140
> +    void runShareSheet(Frame&, ShareDataWithParsedURL&, WTF::CompletionHandler<void(bool)>&&) final;

Nit - you shouldn't need an explicit WTF:: here.

> Source/WebKit/UIProcess/WebShareSheetResultListenerProxy.cp:49
> +void WebOpenPanelResultListenerProxy::chooseFiles(const Vector<WTF::String>& filenames, const String& displayString, const API::Data* iconImageData)

Nit - WTF:: here too.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4647
> +    NSMutableArray *shareDataArray = [[NSMutableArray alloc] init];

Nit - it looks like this leaks? You could consider changing this to:

auto shareDataArray = adoptNS([[NSMutableArray alloc] init]);

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4670
> +    ASSERT(_shareSheet.get() == shareSheet);

Nit - I don't think the .get() is necessary here.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:58
> +}

Nit - extra {}

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:68
> +    WKContentView *_view;

You could consider making this a WeakObjCPtr if WKShareSheet can outlive its WKContentView.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:72
> +#pragma clang diagnostic push

I don't think you need these 3 #pragma's

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:85
> +- (void)dealloc

Nit - I don't think you need to override -dealloc here.

> Source/WebKit/WebProcess/WebPage/WebPage.h:175
> +struct ShareData;

Hm...you shouldn't need to forward declare this if you're already including <WebCore/ShareData.h> above.

> Source/WebKit/WebProcess/WebPage/WebShareSheetResultListener.h:44
> +    void disconnectFromPage() { m_page = 0; }

m_page = nullptr;

If WebPage always outlives its WebShareSheetResultListener, I would suggest using a reference; otherwise, if this can potentially outlive its WebPage, you might want to consider holding a WeakPtr.

> Source/WebKitLegacy/ios/WebCoreSupport/WebChromeClientIOS.mm:160
> +void WebChromeClientIOS::runShareSheet(Frame&, ShareDataWithParsedURL&,  CompletionHandler<void(bool)>&&)

Nit - extra space before CompletionHandler

-- 
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/20180820/c8e2d7c5/attachment.html>