<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add a class to bundle download configuration parameters together"
   href="https://bugs.webkit.org/show_bug.cgi?id=192683#c7">Comment # 7</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add a class to bundle download configuration parameters together"
   href="https://bugs.webkit.org/show_bug.cgi?id=192683">bug 192683</a>
              from <span class="vcard"><a class="email" href="mailto:david_quesada@apple.com" title="David Quesada <david_quesada@apple.com>"> <span class="fn">David Quesada</span></a>
</span></b>
        <pre>(In reply to Michael Catanzaro from <a href="show_bug.cgi?id=192683#c5">comment #5</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=357274&action=diff" name="attach_357274" title="Patch v2">attachment 357274</a> <a href="attachment.cgi?id=357274&action=edit" title="Patch v2">[details]</a></span>
> Patch v2

> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=357274&action=review">https://bugs.webkit.org/attachment.cgi?id=357274&action=review</a>

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

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

<span class="quote">> 
> > 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.</span >

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

<span class="quote">> 
> > 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&.</span >

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!</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>