[Webkit-unassigned] [Bug 136372] [GTK] allow overwriting destination of download

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=136372


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #237460|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #12 from Carlos Garcia Campos <cgarcia at igalia.com>  2014-09-02 01:53:33 PST ---
(From update of attachment 237460)
View in context: https://bugs.webkit.org/attachment.cgi?id=237460&action=review

Once patch in bug #136423 lands we should probably check also that the destination file exists when decided destination callback is called.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:124
> +    case PROP_ALLOW_OVERWRITE:
> +        g_value_set_boolean(value, webkit_download_get_allow_overwrite(download));
>      default:

break;

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:162
> +        ASSERT(!webkit_download_get_allow_overwrite(download));
> +        webkit_download_set_allow_overwrite(download, m_allowOverwrite);
> +        ASSERT(webkit_download_get_allow_overwrite(download) == m_allowOverwrite);

Use g_assert() instead of ASSERT(), because ASSERT() is noop in release builds but we want unit tests to fail also in release builds.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:234
> +    static const char* filename = "test.pdf";
> +    GRefPtr<WebKitDownload> originalDownload = downloadLocalFileSuccessfully(test, filename);
> +    test->checkDestination(originalDownload.get(), filename);

I guess we could just create the file to ensure it exists instead of doing the download. That way we don't need to change the checkDestinationAndDeleteFile()

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:238
> +    test->m_allowOverwrite = true;
> +    GRefPtr<WebKitDownload> additionalDownload = downloadLocalFileSuccessfully(test, filename);
> +    test->checkDestinationAndDeleteFile(additionalDownload.get(), filename);

This should work when the destination file exists no matter if it was a previous download or created by us manually. I think it would simply the test

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:247
> +        , m_permitSuccess(false)

I would use m_expectedToFail, initialized to true

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:-222
> -    void receivedResponse(WebKitDownload* download)
> -    {
> -        DownloadTest::receivedResponse(download);
> -    }

Why did you delete this?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:269
> -        if (m_expectedError != WEBKIT_DOWNLOAD_ERROR_DESTINATION) {
> +        if (m_permitSuccess || m_expectedError != WEBKIT_DOWNLOAD_ERROR_DESTINATION) {

Since now we are going to handle several cases of destination failure, I think we could use our own internal enum, for errors, possible including None (if we really need to handle of not failure). Then we could have InvalidDestination to set here and invalid dest, and DestinationExists, to set the right destination here

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:285
> +    static const char* filename = "test.pdf";
> +    test->m_permitSuccess = true;
> +    GRefPtr<WebKitDownload> originalDownload = downloadLocalFileSuccessfully(test, filename);
> +    test->checkDestination(originalDownload.get(), filename);

Same here about the download, just create the file and then you don't need the special case of a DownloadErrorTest that doesn't fail.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:288
> +    test->m_permitSuccess = false;

I don't think this is working, this combination makes the test set an invalid destination in decideDestination(), so this will fail because of the invalid destination and not because of the destination file already exists.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:303
> +    test->checkDestinationAndDeleteFile(originalDownload.get(), filename);

I've noticed we are using this in all download error tests, but I guess we should check the files don't exist at this point, since they are supposed to be deleted by the web process when the download fails or is cancelled.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.