Download bundles should be moved to their final destination when they finish

[Jon Honeycutt]

Download bundles created by the CFNetwork port should be moved to their final location when they finish downloading.

[Jon Honeycutt]

Created attachment 81044 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 81044 [details]
Patch

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

> Source/WebKit2/WebProcess/Downloads/win/DownloadWin.cpp:35
> +#if USE(CFNETWORK)

If this is CFNetwork- and Windows-specific, maybe this file should be Downloads\cf\win\DownloadsCFNetWin.cpp. That would match what has been done with GraphicsContext.

> Source/WebKit2/WebProcess/Downloads/win/DownloadWin.cpp:36
> +    ASSERT(!m_bundlePath.isEmpty() && !m_destination.isEmpty());

Please use two ASSERTs.

> Source/WebKit2/WebProcess/Downloads/win/DownloadWin.cpp:40
> +    // Try to move the bundle to the final filename.
> +    if (MoveFileEx(m_bundlePath.charactersWithNullTermination(), m_destination.charactersWithNullTermination(), 0))
> +        return;

That should be ::MoveFileExW.

Should we pass MOVEFILE_COPY_ALLOWED? And if the UI process said we're allowed to overwrite an existing file, shouldn't we also pass MOVEFILE_REPLACE_EXISTING? (That would require passing allowOverwrite to platformDidFinish.)

Is it a bug that we're not calling didCreateDestination in this case?

> Source/WebKit2/WebProcess/Downloads/win/DownloadWin.cpp:57
> +    bool reportBundlePathAsFinalPath = true;
> +
> +    if (MoveFileEx(m_bundlePath.charactersWithNullTermination(), m_destination.charactersWithNullTermination(), 0))
> +        reportBundlePathAsFinalPath = false;
> +
> +    // We either need to report our final filename as the bundle filename or the updated destination filename.
> +    if (reportBundlePathAsFinalPath)
> +        didCreateDestination(m_bundlePath);
> +    else
> +        didCreateDestination(m_destination);

Same comments here re: MoveFileEx.

I think the extra boolean is just making things more complicated. You could just do:

if (::MoveFileExW(...))
    didCreateDestination(m_destination);
else {
    // The move failed, so the final destination is just the bundle's current path.
    didCreateDestination(m_bundlePath);
}

[Jon Honeycutt]

(In reply to comment #2)
> (From update of attachment 81044 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81044&action=review
> 
> > Source/WebKit2/WebProcess/Downloads/win/DownloadWin.cpp:35
> > +#if USE(CFNETWORK)
> 
> If this is CFNetwork- and Windows-specific, maybe this file should be Downloads\cf\win\DownloadsCFNetWin.cpp. That would match what has been done with GraphicsContext.

OK, moved.

> 
> > Source/WebKit2/WebProcess/Downloads/win/DownloadWin.cpp:36
> > +    ASSERT(!m_bundlePath.isEmpty() && !m_destination.isEmpty());
> 
> Please use two ASSERTs.

Fixed.

> 
> > Source/WebKit2/WebProcess/Downloads/win/DownloadWin.cpp:40
> > +    // Try to move the bundle to the final filename.
> > +    if (MoveFileEx(m_bundlePath.charactersWithNullTermination(), m_destination.charactersWithNullTermination(), 0))
> > +        return;
> 
> That should be ::MoveFileExW.

Fixed.

> 
> Should we pass MOVEFILE_COPY_ALLOWED? And if the UI process said we're allowed to overwrite an existing file, shouldn't we also pass MOVEFILE_REPLACE_EXISTING? (That would require passing allowOverwrite to platformDidFinish.)

We didn't do this in old WebKit, but we probably should. I'll pass them.

> 
> Is it a bug that we're not calling didCreateDestination in this case?

No, we've already called didCreateDestination with the original destination. It's only if the destination changes that we need it call it.

> 
> > Source/WebKit2/WebProcess/Downloads/win/DownloadWin.cpp:57
> > +    bool reportBundlePathAsFinalPath = true;
> > +
> > +    if (MoveFileEx(m_bundlePath.charactersWithNullTermination(), m_destination.charactersWithNullTermination(), 0))
> > +        reportBundlePathAsFinalPath = false;
> > +
> > +    // We either need to report our final filename as the bundle filename or the updated destination filename.
> > +    if (reportBundlePathAsFinalPath)
> > +        didCreateDestination(m_bundlePath);
> > +    else
> > +        didCreateDestination(m_destination);
> 
> Same comments here re: MoveFileEx.

Fixed.

> 
> I think the extra boolean is just making things more complicated. You could just do:
> 
> if (::MoveFileExW(...))
>     didCreateDestination(m_destination);
> else {
>     // The move failed, so the final destination is just the bundle's current path.
>     didCreateDestination(m_bundlePath);
> }

Fixed.

Thanks! I'll upload another patch shortly.

[Jon Honeycutt]

Created attachment 81150 [details]
Patch v2

[Jon Honeycutt]

Landed in <http://trac.webkit.org/changeset/77585>.