[Webkit-unassigned] [Bug 209265] Implement web-share v2 for files

[bugzilla-daemon at webkit.org]

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

--- Comment #4 from Tim Horton <thorton at apple.com> ---
Comment on attachment 394009
  --> https://bugs.webkit.org/attachment.cgi?id=394009
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
> + at 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
> + at 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.

-- 
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/20200320/516ab2f7/attachment.htm>