[webkit-reviews] review granted: [Bug 209265] Implement web-share v2 for files : [Attachment 398768] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 209265: Implement web-share v2 for files
https://bugs.webkit.org/show_bug.cgi?id=209265

Attachment 398768: Patch

https://bugs.webkit.org/attachment.cgi?id=398768&action=review




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

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

> Source/WebCore/page/Navigator.cpp:149
> +    if (!window || !window->consumeTransientActivation() ||
m_isPresentingShareSheet) {

I still (as in comment #7) object to the Cocoa-platform-specific "share sheet"
terminology in cross-platform code. Maybe "has{Outstanding|Pending}Share" or
something?

> Source/WebCore/page/Navigator.h:35
> +class FileReaderLoader;

I think this needs to be removed now?

> Source/WebCore/page/ShareDataReader.cpp:75
> +    m_filesReadSoFar++;

Like I asked in comment #20, do we need m_filesReadSoFar? Extra state to keep
in sync is always annoying if avoidable.

> Source/WebCore/page/ShareDataReader.h:51
> +    ShareDataWithParsedURL m_shareData;

Should be a empty newline between private methods and members.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:114
> +	   _temporaryFileShareDirectory = adoptNS([[WKShareSheet
createTemporarySharingDirectory] copy]);

No need for the copy or adopt, just assign the autoreleased result of
-createTemporarySharingDirectory to the RetainPtr member variable.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:118
> +	   auto queue =
dispatch_queue_create("com.apple.WebKit.WKShareSheet.ShareableFileWriter",
DISPATCH_QUEUE_SERIAL);

I believe this queue leaks. You should use OSObjectPtr (like RetainPtr) and
adoptOSObject.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:369
> +    if (![WKShareSheet setQuarantineInformationForFilePath:file])
> +	   return nil;

If we wrote the file and then this fails, should we delete the file? (Maybe we
already will?)