[webkit-reviews] review granted: [Bug 53668] Download bundles should be moved to their final destination when they finish : [Attachment 81044] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Jon Honeycutt
<jhoneycutt at apple.com>'s request for review:
Bug 53668: Download bundles should be moved to their final destination when
they finish
https://bugs.webkit.org/show_bug.cgi?id=53668

Attachment 81044: Patch
https://bugs.webkit.org/attachment.cgi?id=81044&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
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);
}