[webkit-reviews] review denied: [Bug 202142] Fix ServiceWorker downloads : [Attachment 446033] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied  review:
Bug 202142: Fix ServiceWorker downloads
https://bugs.webkit.org/show_bug.cgi?id=202142

Attachment 446033: Patch

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




--- Comment #33 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 446033
  --> https://bugs.webkit.org/attachment.cgi?id=446033
Patch

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

Needs file operations to be on a different run loop.

> Source/WebKit/NetworkProcess/NetworkLoad.cpp:59
> +NetworkLoad::NetworkLoad(NetworkLoadClient& client, NetworkSession&
networkSession, const
Function<RefPtr<NetworkDataTask>(NetworkDataTaskClient&)>& createTask)

nit: I think this can be a scoped lambda to save an allocation.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerDownloadTask.cpp:71
> +    FileSystem::deleteFile(m_pendingDownloadLocation);

I don't think cancelling deletes the existing file.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerDownloadTask.cpp:85
> +    m_state = State::Running;

Should we do this only if the state was suspended?
Also, this doesn't do anything to actually start the download.	Should it?

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerDownloadTask.cpp:135
> +    size_t bytesWritten = FileSystem::writeToFile(m_downloadFile,
data.data(), data.size());

Does this happen on the main thread?  We need to assert that this is not the
main run loop or we will introduce bad hangs.  Same with all file operations
here.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerDownloadTask.cpp:162
> +	   m_sandboxExtension->revoke();
> +	   m_sandboxExtension = nullptr;

nit: I feel like std::exchange could be nicely used several places in this
file.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerDownloadTask.cpp:184
> +    FileSystem::deleteFile(m_pendingDownloadLocation);

ditto, I think you shouldn't delete the file here.

>> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:434
>> +	// FIXME: We might want to keep the service worker alive until the
download ends.
> 
> Probably.

Definitely.

>> Source/WebKit/Scripts/generate-unified-sources.sh:17
>> +UnifiedSourceCppFileCount=112
> 
> What is this for? Why is it included in this patch?

This is needed to compile successfully with Xcode.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:15463
> +				2CAECB6627465AE400AB78D0 /*
UnifiedSource112.cpp in Sources */,

nit: you may consider adding more than one so we don't need to add another one
for a little while.  Like 5.

> Tools/WebKitTestRunner/TestController.h:671
> +    long long m_downloadTotalBytesWritten { -1 };

nit: I'd put std::optional<uint64_t>