Bug 98334 – [EFL][WK2] Fix destination path when download with suggested filename on the Minibrowser

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KyungTae Kim]

Currently, when try to download any file on EFL Minibrowser, the destination path becomes "file://".
That's because decideDestinationWithSuggestedFilename (ewk_context_download_client.cpp) sets the download->suggested_filename with passed file name,
then use the download->destination that is blank.
In this case, the destination path should be "file://" + (destination folder) + (suggested file name).

[KyungTae Kim]

Created attachment 167022 [details]
Patch

[Laszlo Gombos]

If file download working with this patch ? How have you tested it ?

[KyungTae Kim]

Surely I tested.

With the patch, the destination path becomes /tmp/(filename),
and I checked it with the links in http://nightly.webkit.org

When I click the link to r130337.tar.bz2">http://builds.nightly.webkit.org/files/trunk/src/WebKit-r130337.tar.bz2 ,
the file was downloaded as /tmp/WebKit-r130337.tar.bz2

[Chris Dumez]

Comment on attachment 167022 [details]
Patch

This change looks strange. I will look into it in more details when I get to work. In any case, it needs unit testing.

[Chris Dumez]

(In reply to comment #0)
> Currently, when try to download any file on EFL Minibrowser, the destination path becomes "file://".
> That's because decideDestinationWithSuggestedFilename (ewk_context_download_client.cpp) sets the download->suggested_filename with passed file name,
> then use the download->destination that is blank.
> In this case, the destination path should be "file://" + (destination folder) + (suggested file name).

This is because MiniBrowser probably does not handle the download signals. It is the browser's task to set the destination path (possible using the suggested file name). I believe you should do this in Minibrowser instead. Please read the doc the download signals on EwkView. If the browser does not set a destination path, I believe the default behavior is not to download anything (thus the empty save path).

[KyungTae Kim]

Created attachment 167038 [details]
Patch

[Chris Dumez]

Comment on attachment 167038 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:203
> +    Ewk_Download_Job* download = (Ewk_Download_Job*)event_info;

Star on wrong side

> Tools/MiniBrowser/efl/main.c:204
> +    const char* suggested_name =  ewk_download_job_suggested_filename_get(download);

Star on wrong side + extra space after = sign

> Tools/MiniBrowser/efl/main.c:206
> +        // FIX ME: The destination folder should be a specific folder user selected, but now set to '/tmp' temporary.

// FIXME: The destination folder should be selected by the user but we set it to '/tmp' for now.

> Tools/MiniBrowser/efl/main.c:207
> +        char* destination_path = malloc(strlen(suggested_name) + 6);

It would be nice to use eina string buffer for this:
http://docs.enlightenment.org/auto/eina/group__Eina__String__Buffer__Group.html

Note that destination_path does not seem to be freed currently.

> Tools/MiniBrowser/efl/main.c:209
> +            sprintf(destination_path, "/tmp/%s", suggested_name);

snprintf would have been safer but I prefer we use eina string buffer anyway.

> Tools/MiniBrowser/efl/main.c:210
> +            ewk_download_job_destination_set(download, destination_path);

It would be nice to print something on stdout like:
"Downloading to /tmp/xxxxxxx..."

> Tools/MiniBrowser/efl/main.c:212
> +    }

else case? If the download does not have a suggested name, we should probably download it anyway. Maybe generate a unique file name?

> Tools/MiniBrowser/efl/main.c:254
> +    evas_object_smart_callback_add(app->browser, "download,request", on_download_request, app);

How about listening for "download,finished" and "download,failed" as well in order to print something on stdout?

[Chris Dumez]

Comment on attachment 167038 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:201
> +on_download_request(void* user_data, Evas_Object* webview, void* event_info)

stars on wrong side

[KyungTae Kim]

Created attachment 167041 [details]
Patch

[Chris Dumez]

Comment on attachment 167041 [details]
Patch

A lot of the feedback was not taken into consideration in this new patch. Why is that? You should either take the feedback into consideration or comment why you disagree with the feedback.

[KyungTae Kim]

Created attachment 167048 [details]
Patch

[Chris Dumez]

Comment on attachment 167048 [details]
Patch

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

> Tools/ChangeLog:10
> +        The 'destination folder' should be a specific folder user selected, but now set to '/tmp' temporary.

"but now set to '/tmp' temporary." -> "but is set to '/tmp' for now". "temporary" is not correct here I believe.

> Tools/MiniBrowser/efl/main.c:205
> +    // FIX ME: destination folder should be user-selectable. Set it as "/tmp" temporary.  

This comment is still incorrect "FIX ME" -> "FIXME" and the last sentence is not correct English I believe. Please check the comment I proposed in my earlier review.

> Tools/MiniBrowser/efl/main.c:213
> +        eina_strbuf_append(destination_path, "Downloaded_File_From_EFL_Minibrowser");

This is going to cause overwriting trouble. You should probably generate a unique name, as I suggested in my earlier review. You can use mktemp() for example. You can use test_ewk2_download_job.cpp as reference.

> Tools/MiniBrowser/efl/main.c:273
> +    evas_object_smart_callback_add(app->browser, "download,request", on_download_request, app);

Please keep the signals sorted.

[KyungTae Kim]

Created attachment 167050 [details]
Patch

[KyungTae Kim]

Created attachment 167056 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 167056 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:219
> +    printf("Downloading: %s\n", eina_strbuf_string_get(destination_path));

Why not use *info* macro ?

> Tools/MiniBrowser/efl/main.c:227
> +    printf("Download finished: %s\n",  ewk_download_job_destination_get(download));

ditto.

> Tools/MiniBrowser/efl/main.c:234
> +    printf("Download failed: %s\n", ewk_download_job_destination_get(download));

ditto.

[KyungTae Kim]

Created attachment 167069 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 167069 [details]
Patch

LGTM.

[Chris Dumez]

Comment on attachment 167069 [details]
Patch

LGTM.

[KyungTae Kim]

Created attachment 167225 [details]
Patch

[WebKit Review Bot]

Comment on attachment 167225 [details]
Patch

Clearing flags on attachment: 167225

Committed r130472: <http://trac.webkit.org/changeset/130472>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.