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

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied youenn fablet
<youennf at gmail.com>'s request for review:
Bug 202142: Fix ServiceWorker downloads
https://bugs.webkit.org/show_bug.cgi?id=202142

Attachment 446336: Patch

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




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

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

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerDownloadTask.cpp:50
> +    , m_writeQueue(WorkQueue::create("ServiceWorkerDownloadTask"))

Could we share the queue if there are multiple downloads happening at the same
time?

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerDownloadTask.cpp:72
> +    m_writeQueue->dispatch([protectedThis = Ref { *this }, function =
WTFMove(function)]() mutable {

nit: I don't think this needs to be mutable.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerDownloadTask.cpp:79
> +    m_writeQueue->dispatch([this, protectedThis = Ref { *this }] {

ASSERT(isMainRunLoop()); outside, ASSERT(!isMainRunLoop()); inside.

Let's add these to all the functions here.

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

?

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerDownloadTask.cpp:126
> +	   if (allowOverwrite && FileSystem::fileExists(filename))
> +	       FileSystem::deleteFile(filename);

Do we need to delete the file?	Should we fail if we don't allow overwrite and
the file exists?

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

I really don't think we delete the file when failing or cancelling.