[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 #7 from David Quesada <david_quesada at apple.com> ---
(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!

-- 
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/80ea092e/attachment.html>