[Webkit-unassigned] [Bug 192683] Add a class to bundle download configuration parameters together

[bugzilla-daemon at webkit.org]

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

--- Comment #5 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 357274
  --> https://bugs.webkit.org/attachment.cgi?id=357274
Patch v2

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

Nice cleanup. Thanks for updating the soup/glib classes.

None of these comments are important, just got curious and noticed a couple nits:

> Source/WebKit/NetworkProcess/Downloads/Download.h:68
> -    Download(DownloadManager&, DownloadID, NetworkDataTask&, const PAL::SessionID& sessionID, const String& suggestedFilename = { });
> +    Download(DownloadManager&, DownloadID, NetworkDataTask&, const PAL::SessionID& sessionID, const String& suggestedFilename = { }, DownloadConfiguration&& = { });

Does it really ever make sense to create a Download without a DownloadConfiguration? I suspect not? Unless this really makes sense, it shouldn't be optional and should move before suggestedFilename.

> Source/WebKit/NetworkProcess/Downloads/Download.h:70
> -    Download(DownloadManager&, DownloadID, NSURLSessionDownloadTask*, const PAL::SessionID& sessionID, const String& suggestedFilename = { });
> +    Download(DownloadManager&, DownloadID, NSURLSessionDownloadTask*, const PAL::SessionID& sessionID, const String& suggestedFilename = { }, DownloadConfiguration&& = { });

Ditto.

> Source/WebKit/NetworkProcess/Downloads/DownloadManager.cpp:128
> -    auto download = std::make_unique<Download>(*this, downloadID, nullptr, sessionID);
> +    auto download = std::make_unique<Download>(*this, downloadID, nullptr, sessionID, String { }, WTFMove(configuration));

Yeah see, here you had to specify suggestedFilename even though it's not really important just to get to DownloadConfiguration.

> Source/WebKit/NetworkProcess/NetworkDataTask.h:120
> -    virtual void setPendingDownloadLocation(const String& filename, SandboxExtension::Handle&&, bool /*allowOverwrite*/) { m_pendingDownloadLocation = filename; }
> +    virtual void setPendingDownloadConfiguration(DownloadConfiguration&& downloadConfiguration) { m_pendingDownloadLocation = downloadConfiguration.destination; m_downloadConfiguration = WTFMove(downloadConfiguration); }

Hm, normally we limit our inlining to one-liners.

Looks like m_pendingDownloadLocation isn't really needed anymore and each use can be replaced by downloadConfiguration.destination, right? A bit more verbose, but it's often good practice to not make more copies of data than are really needed. Either way seems fine.

> Source/WebKit/NetworkProcess/NetworkDataTask.h:123
> +    DownloadConfiguration& downloadConfiguration() { return m_downloadConfiguration; }

I bet a lot would break if a caller were to mutate the DownloadConfiguration, right? Should probably return const DownloadConfiguration&.

-- 
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/20181214/f6387129/attachment-0001.html>