Bug 192683 – Add a class to bundle download configuration parameters together

• Status: NEW
• Component: WebKit2

[David Quesada]

After deciding the destination for a download, the UI process currently sends three things to the Network process - the destination path, a sandbox extension, and whether or not to allow overwriting an existing file. These things could be folded into one data structure, so it can be more easily expanded if we have more configuration about the download to send to the Network process in the future. For example, Cocoa could do this with the NSProgress URL. Conditionally adding a member to the theoretical "DownloadConfiguration" structure will require less code churn than conditionally adding a new parameter to every method signature where we pass the three current pieces of configuration.

[Radar WebKit Bug Importer]

<rdar://problem/46711066>

[David Quesada]

Created attachment 357265 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[David Quesada]

Created attachment 357274 [details]
Patch v2

Try to fix gtk and wpm builds.

[Michael Catanzaro]

Comment on attachment 357274 [details]
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&.

[David Quesada]

Created attachment 357279 [details]
Patch v2.1

Almost exactly the same, but with a speculative attempt to try to fix the mac 32bit build. I'm just attaching it here to run it through EWS, since it seems the failure is only caused by the additional files this patch adds to the unified sources.

[David Quesada]

(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 357274 [details]
> 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.

Good point. I'll investigate placing the configuration before the filename as a required argument.

> 
> > 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.

I had noticed this and thought the same thing. I thought I had a FIXME lying around that I would remove m_pendingDownloadLocation before sending this patch out. I guess I didn't and it slipped my mind. I'll also look into this. (Then setPendingDownloadConfiguration() can return to an inlined one-liner.)

> 
> > 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're right - it wouldn't be good for this to be unexpectedly mutated. The point of exposing as mutable was to enable moving the DownloadConfiguration from the NetworkDataTaskCocoa to create the Download when a data task is converted into a download. (See NetworkSessionCocoa.mm diffs) I wasn't sure what the best way to arrange this code was. I'll look at this some more to see if I can figure out a better way.

Thanks for the comments!

[David Quesada]

Created attachment 357285 [details]
Patch v2.2

Another attempt to fix the mac builds.