Bug 209265 – Implement web-share v2 for files

• Component: WebKit2
• Status: RESOLVED FIXED

[Nikos Mouchtaris]

add to existing progress

[Nikos Mouchtaris]

Created attachment 393942 [details]
Patch

[Nikos Mouchtaris]

Created attachment 393946 [details]
Patch

[Nikos Mouchtaris]

Created attachment 394009 [details]
Patch

[Tim Horton]

Comment on attachment 394009 [details]
Patch

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

> Source/WebCore/page/Navigator.cpp:40
> +#include "ReadShareDataAsync.hpp"

We don't use the hpp extension in the WebKit project; C++ headers are just .h like C.

Also, I don't love the file name at first glance. Maybe I'll have a suggestion once I read the rest of the patch.

> Source/WebCore/page/Navigator.cpp:121
> +#if ENABLE(FILE_SHARE)
> +            return true;
> +#endif
>              return false;

Stick the `return false;` in an #else, it's weird to have two sequential return statements if ENABLE(FILE_SHARE) is true.

> Source/WebCore/page/Navigator.cpp:165
> +        m_shareData = WTFMove(shareData);
> +        m_promise = WTFMove(promise);
> +        
> +        m_loader = ReadShareDataAsync::create(*this, context);
> +        m_loader->start(m_shareData);

It seems wrong to store m_shareData and friends on Navigator. Navigator is page-global; what happens if you get multiple calls to share()? A better design might make use of C++ lambdas or something (or only allow one at a time and cancel the old one?) to store the state (in a completion handler passed to the to-be-renamed "ReadShareDataAsync" class).

> Source/WebCore/page/Navigator.cpp:189
> +    auto* frame = this->frame();

Should figure out how to share this with the code above. Maybe the aforementioned completion lambda always does these things, and you just call it synchronously from share() if you have no files?

> Source/WebCore/page/Navigator.h:55
> +    void finishedLoad(ExceptionOr<void> ret);

in this case I think you can elide the parameter name, but we generally prefer whole words instead of abbreviations like "ret"

> Source/WebCore/page/Navigator.h:73
> +    ShareDataWithParsedURL m_shareData;
> +    RefPtr<DeferredPromise> m_promise;
> +    RefPtr<ReadShareDataAsync> m_loader;
> +    

As previously mentioned, I'm a bit wary of adding this state to Navigator. At most, perhaps a vector of pending share-data-reading operations?

> Source/WebCore/page/ReadShareDataAsync.cpp:1
> +// Copyright © 2020 ___ORGANIZATIONNAME___ All rights reserved.

This is definitely not the right copyright header :)

> Source/WebCore/page/ReadShareDataAsync.hpp:1
> +// Copyright © 2020 ___ORGANIZATIONNAME___ All rights reserved.

Again with the copyright header

> Source/WebCore/page/ReadShareDataAsync.hpp:4
> +#pragma once
> +#ifndef ReadShareDataAsync_hpp
> +#define ReadShareDataAsync_hpp

You don't need #ifdefs if you have #pragma once

> Source/WebCore/page/ReadShareDataAsync.hpp:6
> +#include <stdio.h>

This is surprising!

> Source/WebCore/page/ReadShareDataAsync.hpp:17
> +class ReadShareDataAsync: private FileReaderLoaderClient, public RefCounted<ReadShareDataAsync>{

There's a bunch of style problems with this line, check out https://webkit.org/code-style-guidelines/#classes

> Source/WebCore/page/ReadShareDataAsync.hpp:32
> +    Navigator* m_client;

If we do the lambda-as-completion-handler thing we can avoid knowing about Navigator, which seems preferable

> Source/WebCore/page/ReadShareDataAsync.hpp:36
> +    String m_currentFileLoad;

What is this, a path? A filename? The variable name could be more clear.

> Source/WebCore/page/ReadShareDataAsync.hpp:37
> +    size_t m_doneSoFar = 0;

It should be more clear by the name what units this is in. Maybe something like m_loadedBytes or something?

> Source/WebKit/Shared/WebCoreArgumentCoders.h:129
> +class File;
>  struct ShareDataWithParsedURL;
> +struct RawFile;

These are both sorted wrong :) classes should be together up above, everything should be alphabetized

> Source/WebKit/Shared/WebCoreArgumentCoders.h:561
> +template<> struct ArgumentCoder<Vector<WebCore::RawFile>> {
> +    static void encode(Encoder&, const Vector<WebCore::RawFile>&);
> +    static bool decode(Decoder&, Vector<WebCore::RawFile>&);
> +};

We tend to try to avoid putting encoders here now, is it possible this can go in WebCore proper? (Not always possible, if you depend on another encoder that's only available in WebKit)

> Source/WebKit/UIProcess/Cocoa/ShareableFileWrite.h:1
> +// Copyright © 2020 ___ORGANIZATIONNAME___ All rights reserved.

Nope

> Source/WebKit/UIProcess/Cocoa/ShareableFileWrite.h:8
> +@interface WKShareableFileWrite : NSObject

The part of speech for this class name is odd. I can see a "WKShareableFileWrite*r*", though I kind of wonder if this needs to be ObjC at all? (no super strong opinions about that part at the moment). Also, not sure it even needs to be factored into its own class, since it doesn't seem very reusable by anything other than WKShareSheet.

Also, the filename should match the class name.

> Source/WebKit/UIProcess/Cocoa/ShareableFileWrite.mm:12
> +@implementation WKShareableFileWrite

This file has lots of odd style, check out the style guide I linked above.

> Source/WebKit/UIProcess/Cocoa/ShareableFileWrite.mm:19
> +    NSArray *paths = NSSearchPathForDirectoriesInDomains(NSDocumentDirectory, NSUserDomainMask, YES);
> +    if (!paths || ![paths firstObject])
> +        return nil;
> +    
> +    NSString *documentsDirectory = [paths objectAtIndex:0]; // Get documents folder

Is this where we agreed was safest to put the files? I thought we were going to put it in a subdirectory of one of the WebKit data directories, not in the user's documents directory (which is user-owned and a very odd place to put temporary things).

> Source/WebKit/UIProcess/Cocoa/ShareableFileWrite.mm:30
> +

All of this code that touches the disk, after another round or two, will need careful review by a few other people. You should add Alex and Brady at least, but probably others too. Chris and Jessie, maybe?

> Source/WebKit/UIProcess/Cocoa/ShareableFileWrite.mm:31
> ++(NSString *)getFileDirectoryForSharing {

the "get" prefix usually indicates the existence of an out-argument in WebKit code. Perhaps just drop it?

> Source/WebKit/UIProcess/Cocoa/ShareableFileWrite.mm:66
> +    NSMutableDictionary *quarantineProperties = [[NSMutableDictionary alloc] init];

This allocation leaks

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:111
> +        NSURL *fileURL = [WKShareableFileWrite writeFileToShareableURL:file.fileName data:file.fileData->createNSData().get()];

This is doing disk I/O synchronously on the main thread, which is not a good practice for responsiveness' sake. Likely you want to start all of the writes on a secondary queue, and (asynchronously) wait for them all to complete before continuing.

[Tim Horton]

Also, the GTK/WPE errors are real. Here they are:

lib/libWebCore.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-767013ce-6.cpp.o):UnifiedSource-767013ce-6.cpp:function WebCore::Navigator::share(WebCore::ScriptExecutionContext&, WebCore::ShareData const&, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&): error: undefined reference to 'WebCore::ReadShareDataAsync::ReadShareDataAsync(WebCore::Navigator&, WebCore::ScriptExecutionContext&)'
lib/libWebCore.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-767013ce-6.cpp.o):UnifiedSource-767013ce-6.cpp:function WebCore::Navigator::share(WebCore::ScriptExecutionContext&, WebCore::ShareData const&, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&): error: undefined reference to 'WebCore::ReadShareDataAsync::start(WebCore::ShareDataWithParsedURL&)'


I think you may have forgotten to add your new file to Sources.txt, and instead added it to the Xcode target. You should add it to Sources.txt and then uncheck it in the target in Xcode.

[Nikos Mouchtaris]

Created attachment 394699 [details]
Patch

[Tim Horton]

Comment on attachment 394699 [details]
Patch

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

> Source/WebCore/page/Navigator.cpp:174
> +void Navigator::presentShareSheet(ExceptionOr<ShareDataWithParsedURL&> readData, Ref<DeferredPromise>&& promise)

This very Apple-platform-UI-focused name is not ideal for Navigator, which is very platform-independent. I don't have a proposal at the moment, but I think we can do better!

> Source/WebCore/page/Navigator.cpp:-153
> -    frame->page()->chrome().showShareSheet(shareData, [promise = WTFMove(promise)] (bool completed) {

This existing name is not great either (I guess that's where you got it from!), since Chrome is also cross-platform. At least it is /closer/ to the UI layer tho :P

> Source/WebCore/page/Navigator.h:55
> +    void finishedLoad(ExceptionOr<void> ret);

Get rid of the "ret"

> Source/WebCore/page/ReadShareDataAsync.cpp:57
> +//    m_shareData = nullptr;

Commented out?

> Source/WebCore/page/ReadShareDataAsync.h:33
> +namespace WebCore {

Should be a newline before this line

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2042
> +

Extraneous newline. There are a few of these throughout the patch. Take a peek and see if you find the others, I'm just going to mention this one.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.h:43
> +- (void)presentWithShareDataArray:(NSArray *)shareDataArray inRect:(WTF::Optional<WebCore::FloatRect>)rect;

If this is internal to the class it shouldn't need to be mentioned here (ObjC is weird)

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:114
> +        NSMutableArray *fileData = [[NSMutableArray alloc] init];

An alternative way (IDK if better, but I think so) to write this would be with dispatch_group, submitting a block per file in parallel. That would also let you avoid these arrays, because you could do it from this thread, capturing the NSData and filename in the block, no need to slap them into two parallel arrays. Then dispatch_group_notify to find out when they're all done.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:118
> +            [fileData addObject: file.fileData->createNSData().get()];

No spaces after :s in objc messages

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:121
> +        dispatch_async( dispatch_get_global_queue( DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^(void) {

I wonder if we want to make a queue for this purpose, or if this global one is right, or what! (I'm not sure, peek around and see what other parts of WebKit do).

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:126
> +                    [fileData release];
> +                    [fileNames release];

No way, use RetainPtr instead.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:144
> +- (void)presentWithShareDataArray:(NSArray *)shareDataArray inRect:(WTF::Optional<WebCore::FloatRect>)rect {

I think the parameter (and first part of the selector name) should be "sharing items" or "items" or something, similar to the share sheet API.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:340
> ++ (NSString *)getSharingDirectoryPath

This should not be a get-, because it doesn't have an out argument.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:345
> +#if PLATFORM(MAC)
> +    NSString *cacheDirectory = cacheDataVaultParentDirectory();
> +#else
> +    NSArray *paths = NSSearchPathForDirectoriesInDomains(NSCachesDirectory, NSUserDomainMask, YES);

In both cases this is writing in places that will not be deleted if someone uses the "reset Safari" button or WKWebView data deletion APIs, as far as I can tell, so I think you've still not /quite/ found the right place. I highly recommend talking to Chris/Brady/Alex (unless you did and there is something I am missing here).

I am expecting (but I could be wrong!) in the end that this will somehow interact with WebsiteDataStore and friends.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:386
> +    NSString *fileNameConcat = [NSString stringWithFormat:@"%@/%@", getTempDirectory, sanitizedFilename];

This is not the best way to concatenate path components. Check out NSString -stringByAppendingPathComponent: (we probably have something in WTF too, but I think it's fine to use Foundation stuff here)

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:392
> +    NSString *file = [WKShareSheet createFilename: fileName];

Extraneous spaces again :) You've got a lot of them, maybe find some way to search for them

[Nikos Mouchtaris]

Created attachment 394701 [details]
Patch

[Nikos Mouchtaris]

Created attachment 394708 [details]
Patch

[Nikos Mouchtaris]

Created attachment 394775 [details]
Patch

[Andy Estes]

Comment on attachment 394775 [details]
Patch

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

I'll spend more time later, but did have a few quick comments now.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:54
> +    NSString *_tempFileShareDirectory;

This should be a RetainPtr.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:115
> +        _tempFileShareDirectory = [[WKShareSheet createTempSharingDirectory] copy];

adoptNS([[WKShareSheet createTempSharingDirectory] copy])

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:226
> +    _tempFileShareDirectory = nil;

This only releases _tempFileShareDirectory if it's a RetainPtr.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:272
> +    NSMutableDictionary *quarantineProperties = [[NSMutableDictionary alloc] init];

auto quarantineProperties = adoptNS([[NSMutableDictionary alloc] init]);

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:283
> +    [quarantineProperties release];

No need for this if you use RetainPtr above.

[Alex Christensen]

Comment on attachment 394775 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:359
> ++ (NSURL *)writeFileToShareableURL:(NSString *)fileName data:(NSData *) fileData tempDirectory:(NSString *) tempDirectory

This should have ASSERT(!RunLoop::isMain()) in it to make sure we don't add spins.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:368
> +    NSString *sanitizedFilename = WebCore::ResourceResponseBase::sanitizeSuggestedFilename(fileName);

I think we should move this call to sanitizeSuggestedFilename closer to where the string is received from the web process to prevent us from doing anything else with it in the future.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:374
> +    [fileData writeToURL:fileURL options:NSDataWritingAtomic error:&error];

Who is responsible for deleting this file?

[Nikos Mouchtaris]

Created attachment 396028 [details]
Patch

[Tim Horton]

Comment on attachment 396028 [details]
Patch

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

> Source/WebCore/page/Navigator.cpp:174
> +void Navigator::presentShareSheet(ExceptionOr<ShareDataWithParsedURL&> readData, Ref<DeferredPromise>&& promise)

My previous comments about the odd platform-specificness of this name still stand

> Source/WebCore/page/Navigator.h:70
> +    std::atomic<bool> m_isPresentingShareSheet = false;

who's interacting with this from a secondary thread? can they not?

if your completion handler is on a secondary thread, you should probably bounce back to the main thread before doing anything, it seems likely wrong to resolve a promise on a non-main-thread too. and then you can avoid this odd atomic

> Source/WebCore/page/ReadShareDataAsync.cpp:104
> +        completionHandler(Exception { AbortError, "Abort due to fail of reading files."_s });

The grammar in this error message seems nonideal.

> Source/WebCore/page/ReadShareDataAsync.h:40
> +class ReadShareDataAsync: private FileReaderLoaderClient, public RefCounted<ReadShareDataAsync> {

This class name is still very weird. Maybe ShareDataReader? I don't think the asynchronousness needs to be in the name

> Source/WebCore/page/ReadShareDataAsync.h:57
> +    ScriptExecutionContext* m_context;

Is it OK for us to hold this bare pointer? What happens if we're still doing things and the ScriptExecutionContext is deleted?

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2070
>  bool ArgumentCoder<ShareDataWithParsedURL>::decode(Decoder& decoder, ShareDataWithParsedURL& settings)

Not your fault, but how did we end up with the argument name 'settings' in all of these methods?!

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:115
> +        _tempFileShareDirectory = adoptNS([[WKShareSheet createTempSharingDirectory] copy]);

All "temp"s should be written out, not abbreviated.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:118
> +        __block bool isSuccesful = true;

drop the is- prefix. Also successful has three 's'es

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:119
> +        auto queue = dispatch_queue_create("Write Shareable Files To Disk", DISPATCH_QUEUE_SERIAL);

This should be globally identifiable, and clear it's WebKit's. Maybe "com.apple.WebKit.WKShareSheet.FileWriter" or something? Look at others.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:134
> +        dispatch_group_notify(fileWriteGroup, queue, ^{
> +            dispatch_async(dispatch_get_main_queue(), ^{

you don't need two bounces here, can just write these two lines as one:

dispatch_group_notify(fileWriteGroup, dispatch_get_main_queue(), ^{

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:270
> +    // Workaround <rdar://problem/21735412>: NSURLDownload will automatically attach NSURLQuarantineProperties
> +    // to files we download if they are already quarantined. This adds a quarantine history item that the code
> +    // below will shadow. Since Safari needs to be able to remove all traces of download history, we need to
> +    // delete that entry before ours renders it inaccessible.

This comment seems irrelevant in WebKit

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:274
> +    auto quarantineProperties = adoptNS([[NSMutableDictionary alloc] init]);
> +    quarantineProperties.get()[(__bridge NSString *)kLSQuarantineTypeKey] = (__bridge NSString *)kLSQuarantineTypeWebDownload;
> +    quarantineProperties.get()[(__bridge NSString *)kLSQuarantineAgentBundleIdentifierKey] = WebCore::applicationBundleIdentifier();

IMO this would be better written:

auto quarantineProperties = @{
    (__bridge NSString *)kLSQuarantineTypeKey: (__bridge NSString *)kLSQuarantineTypeWebDownload,
    (__bridge NSString *)kLSQuarantineAgentBundleIdentifierKey]: WebCore::applicationBundleIdentifier()
};

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:314
> ++ (BOOL)removeTempSharingDirector:(NSString *)directoryPath

"director"! Also another "temp"

[Nikos Mouchtaris]

Created attachment 397098 [details]
Patch

[Nikos Mouchtaris]

Created attachment 397395 [details]
Patch

[Wenson Hsieh]

Comment on attachment 397395 [details]
Patch

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

> Source/WebCore/page/FileReaderLoaderWrapper.h:39
> +class Navigator;
> +class Blob;
> +class FileReaderLoader;
> +class ScriptExecutionContext;

Nit - forward declarations such as these are usually sorted in alphabetical order.

> Source/WebCore/page/FileReaderLoaderWrapper.h:41
> +class FileReaderLoaderWrapper: private FileReaderLoaderClient, public RefCounted<FileReaderLoaderWrapper> {

Nit - space after FileReaderLoaderWrapper

> Source/WebCore/page/FileReaderLoaderWrapper.h:48
> +    String fileName() { return m_fileName; }

Nit - const?

> Source/WebCore/page/Navigator.cpp:163
> +            m_loader = nullptr;

Do you need to null it out here? It looks like we'll set it soon afterwards anyways, which should destroy the preexisting one. (But perhaps it’s important that the old ShareDataReader is destroyed before the new one is created?)

> Source/WebCore/page/Navigator.cpp:171
> +    this->showShareData(shareData, WTFMove(promise));

Nit - I don’t think you need the "this->” here.

> Source/WebCore/page/Navigator.cpp:184
> +    frame->page()->chrome().showShareSheet(shareData, [promise = WTFMove(promise), this] (bool completed) {

Is there anything that guarantees frame and page exist here?

> Source/WebCore/page/Navigator.h:71
> +    bool m_isPresentingShareSheet = false;

Nit - we commonly write default initializers in WebKit like `bool m_isPresentingShareSheet { false };`.

The member variables should also be moved further down, below these private methods, to where the other private members.

> Source/WebCore/page/Navigator.h:-73
> -

Nit - looks like a stray whitespace change?

> Source/WebCore/page/ShareDataReader.cpp:47
> +void ShareDataReader::start(ScriptExecutionContext& context, ShareDataWithParsedURL shareData)

shareData should be a ShareDataWithParsedURL&& if you’re going to WTFMove it below.

> Source/WebCore/page/ShareDataReader.cpp:51
> +    int count = 0;

It would be slightly more efficient here to use `m_pendingFileLoads.reserveInitialCapacity()` here and `uncheckedAppend` below.

> Source/WebCore/page/ShareDataReader.cpp:60
> +    m_shareData.shareData.files.reserveCapacity(m_shareData.shareData.files.size());

Is this line necessary?

> Source/WebCore/page/ShareDataReader.cpp:75
> +    file.fileName = ResourceResponseBase::sanitizeSuggestedFilename(fileLoad->fileName());

Hm...I’m a tiny bit wary of this, since the web content process could be compromised. Is it possible to push the raw filename over IPC and do the file name sanitization in the UI process instead before attempting to use it?

> Source/WebCore/page/ShareDataReader.cpp:77
> +    file.fileData = WTFMove(sharedBuffer);

Nit - I think just `file.fileData = fileLoad->readBuffer();` would be clearer.

> Source/WebCore/page/ShareDataReader.cpp:81
> +    if (m_filesReadSoFar == (int)m_pendingFileLoads.size()) {

Nit - we commonly use static_cast instead of regular cast.

> Source/WebCore/page/ShareDataReader.h:40
> +class ShareDataReader: public RefCounted<ShareDataReader> {

Nit - space after the first ShareDataReader.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2035
> +    uint64_t dataSize;

Nit - I would suggest naming this “numberOfFiles” or “fileCount” instead.

Another option is to implement encode/decode templates on the RawFile struct — then you can have the Vector<> encoder template do its thing with the RawFiles.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:350
> ++ (NSURL *)writeFileToShareableURL:(NSString *)fileName data:(NSData *) fileData temporaryDirectory:(NSString *) temporaryDirectory

Nit - extra spaces before some of these argument names.

[Nikos Mouchtaris]

Created attachment 397869 [details]
Patch

[Wenson Hsieh]

Hm..the test failures on the Windows bot don’t look related to this patch.

> fast/dom/beforeload/video-before-load.html, imported/blink/fast/dom/Window/open-window-features-fuzz.html, fast/dom/focus-shift-crash.html, security/contentSecurityPolicy/blocks-video.html, fast/dom/beforeload/remove-video-in-beforeload-listener.html, http/tests/security/contentSecurityPolicy/media-src-blocked.html, http/tests/navigation/page-cache-video.html

[Tim Horton]

Comment on attachment 397869 [details]
Patch

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

> Source/WebCore/page/FileReaderLoaderWrapper.h:41
> +class FileReaderLoaderWrapper : private FileReaderLoaderClient, public RefCounted<FileReaderLoaderWrapper> {

Since this is in both patches, maybe we should peel it out and get it reviewed and landed, because a lot of the comments and concerns are shared between the two patches.

> Source/WebCore/page/Navigator.cpp:118
> +#if ENABLE(FILE_SHARE)

Is this ENABLE flag introduced somewhere?

(maybe it happened in an earlier patch)

> Source/WebCore/page/ShareDataReader.cpp:49
> +    m_filesReadSoFar = 0;

Do we actually need this member, or is it implicitly the length of m_shareData.files?

> Source/WebCore/page/ShareDataReader.cpp:59
> +        count += 1;

All these += 1s should just be post-increment `count++` (same below for m_filesReadSoFar)

> Source/WebCore/page/ShareDataReader.cpp:66
> +        if (auto completionHandler = WTFMove(m_completionHandler))

Should we clear m_pendingFileLoads in this case?

[Tim Horton]

Comment on attachment 397869 [details]
Patch

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

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2024
> +void ArgumentCoder<Vector<RawFile>>::encode(Encoder& encoder, const Vector<RawFile>& files)

I'm pretty sure you just have to implement coders for RawFile, not Vector<RawFile>, and then VectorArgumentCoder will Do The Right Thing for the vector, no?

[Nikos Mouchtaris]

Created attachment 398768 [details]
Patch

[Tim Horton]

Comment on attachment 398768 [details]
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?)

[Andy Estes]

Comment on attachment 398768 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:53
> +    RetainPtr<NSString> _temporaryFileShareDirectory;

I think this should be an NSURL.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:313
> +    if (![[NSFileManager defaultManager] fileExistsAtPath:directoryPath])
> +        return [[NSFileManager defaultManager] removeItemAtPath:directoryPath error:nil];

Don’t you mean to remove the directory if it exists, not if it doesn’t?

I think it’s ok to just unconditionally call -removeItemAtPath:error:. If the item doesn’t exist, NO will be returned.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:318
> ++ (NSString *)createTemporarySharingDirectory

I think this should return an NSURL.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:329
> +    if (![[NSFileManager defaultManager] fileExistsAtPath:temporaryDirectory]) {
> +        [[NSFileManager defaultManager] createDirectoryAtPath:temporaryDirectory withIntermediateDirectories:NO attributes:nil error:&error];
> +        if (error)
> +            return nil;
> +    }

FileSystem::createTemporaryDirectory actually creates a temporary directory, not just a name to a temporary directory. Why do we need to check if it exists then try to create it again? Wouldn’t the second attempt fail as well?

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:333
> ++ (NSString *)createRandomSharingDirectoryForFile:(NSString *)temporaryDirectory

I think this should return an NSURL.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:338
> +    NSString *dataPath = [temporaryDirectory stringByAppendingPathComponent:[NSString stringWithFormat:@"/%@", randomDirectory]];

-stringByAppendingPathComponent: will handle inserting a ‘/‘, so you don’t need to specify ‘/‘ in your format string. You end up with ‘//‘ in your path here, which is ok, but we should avoid doing this.

But really you should use -URLByAppendingPathComponent:.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:346
> +    if (![[NSFileManager defaultManager] fileExistsAtPath:dataPath]) {
> +        [[NSFileManager defaultManager] createDirectoryAtPath:dataPath withIntermediateDirectories:NO attributes:nil error:&error];
> +        if (error)
> +            return nil;
> +    }
> +    return dataPath;

So if a directory matching this UUID happens to already exist then we’ll just reuse it? Is that ok? What if another file by the same was already written into that directory?

Seems like we should be using createTemporaryDirectory() here too, which mostly guarantees to create a directory that didn’t previously exist (I say mostly because I think it does suffer from a time-of-use-time-of-check bug, but so does your solution).

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:355
> +    NSString *temporaryDirectoryForFile = [WKShareSheet createRandomSharingDirectoryForFile:temporaryDirectory];

Can you explain why we have two levels of temporary directories (temporaryDirectory and temporaryDirectoryForFile)? I guess it’s possible for more than one file to have the same name, and it’s easier to keep track of one top-level temporary directory rather than `n` different per-file temporary directories?

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:361
> +    NSString *file = [temporaryDirectoryForFile stringByAppendingPathComponent:fileName];
> +
> +    NSURL *fileURL = [NSURL fileURLWithPath:file];

I’d use -URLByAppendingPathComponent then not worry about converting from a string to a URL.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:366
> +    NSError *error = nil;
> +    [fileData writeToURL:fileURL options:NSDataWritingAtomic error:&error];
> +    if (error)
> +        return nil;

The canonical way to check for failure here is to check the return value of -writeToURL:error:, not whether error is non-nil.

Unless you plan to do something with `error` (like log it? maybe you should log here), you should just pass NULL for the error: parameter.

[Andy Estes]

(In reply to Andy Estes from comment #24)
> Comment on attachment 398768 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398768&action=review
> it does suffer from a time-of-use-time-of-check bug,
> but so does your solution).

time-of-check time-of-use, that is.

[Nikos Mouchtaris]

Created attachment 398827 [details]
Patch

[Andy Estes]

Comment on attachment 398827 [details]
Patch

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

> Source/WebCore/page/ShareDataReader.cpp:63
> +        if (auto completionHandler = WTFMove(m_completionHandler))

if (auto completionHandler = std::exchange(m_completionHandler, {}))

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:231
> +#if PLATFORM(IOS_FAMILY)
> +    [[NSFileManager defaultManager] removeItemAtURL:_temporaryFileShareDirectory.get() error:nil];
> +#endif

For the failure case, surely it’s ok to delete the temp directory.

For the success case, I think this deserves at least a comment, and probably a bug about how NSSharingServiceDelegate doesn’t have a callback to let us know when the temp file can be deleted.

I also think we need to decide if it’s ok to let these files accumulate in the temporary directory on Mac. Maybe it’s ok because we assume they’ll be cleaned up at some point, but maybe we need to disable the feature on Mac until we have the API we need to delete things safely (not advocating for that, per se, but it’s worth discussing).

I’m curious what other folks think about this.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:284
> +    int quarantineError = qtn_file_init_with_path(fq, fileURL.fileSystemRepresentation);

Nit: You can use makeScopeExit() to avoid having to call qtn_file_free() in a bunch of places.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:316
> +    NSString *randomDirectory = [prefix stringByAppendingString: createCanonicalUUIDString()];

Nit: no space after the colon.

And I’m not sure about the prefix. It looks like the prefix is an index number converted to a string, so you’ll get directory names like “1<UUID>”. Since you’ve already created a unique temporary parent directory, I think you can either use just a UUID, or just an index, but I don’t think you need both.

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:319
> +    NSURL *dataPath = [temporaryDirectory URLByAppendingPathComponent: randomDirectory];

Ditto.

> LayoutTests/imported/w3c/ChangeLog:12
> +        * web-platform-tests/web-share/canShare-files.https-expected.txt:
> +        * web-platform-tests/web-share/canShare.https-expected.txt:
> +

I don’t see these files in the patch.

[Nikos Mouchtaris]

Created attachment 398880 [details]
Patch

[Andy Estes]

Comment on attachment 398880 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WKShareSheet.mm:305
> +    quarantineError = qtn_file_apply_to_path(fq, fileURL.fileSystemRepresentation);

Do we need to return NO if this fails?

[EWS]

Committed r261412: <https://trac.webkit.org/changeset/261412>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398880 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63034560>