Bug 98334 - [EFL][WK2] Fix destination path when download with suggested filename on the Minibrowser
Summary: [EFL][WK2] Fix destination path when download with suggested filename on the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 98493
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-03 17:58 PDT by KyungTae Kim
Modified: 2012-10-05 00:56 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.88 KB, patch)
2012-10-03 19:19 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (2.68 KB, patch)
2012-10-03 23:18 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (2.71 KB, patch)
2012-10-03 23:51 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (3.59 KB, patch)
2012-10-04 01:27 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (3.60 KB, patch)
2012-10-04 01:44 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (3.59 KB, patch)
2012-10-04 02:08 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (3.82 KB, patch)
2012-10-04 04:00 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (3.86 KB, patch)
2012-10-04 19:11 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description KyungTae Kim 2012-10-03 17:58:09 PDT
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).
Comment 1 KyungTae Kim 2012-10-03 19:19:00 PDT
Created attachment 167022 [details]
Patch
Comment 2 Laszlo Gombos 2012-10-03 21:31:00 PDT
If file download working with this patch ? How have you tested it ?
Comment 3 KyungTae Kim 2012-10-03 21:43:56 PDT
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
Comment 4 Chris Dumez 2012-10-03 21:49:01 PDT
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.
Comment 5 Chris Dumez 2012-10-03 21:52:36 PDT
(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).
Comment 6 KyungTae Kim 2012-10-03 23:18:49 PDT
Created attachment 167038 [details]
Patch
Comment 7 Chris Dumez 2012-10-03 23:39:11 PDT
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?
Comment 8 Chris Dumez 2012-10-03 23:39:34 PDT
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
Comment 9 KyungTae Kim 2012-10-03 23:51:06 PDT
Created attachment 167041 [details]
Patch
Comment 10 Chris Dumez 2012-10-04 00:34:27 PDT
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.
Comment 11 KyungTae Kim 2012-10-04 01:27:44 PDT
Created attachment 167048 [details]
Patch
Comment 12 Chris Dumez 2012-10-04 01:42:36 PDT
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.
Comment 13 KyungTae Kim 2012-10-04 01:44:11 PDT
Created attachment 167050 [details]
Patch
Comment 14 KyungTae Kim 2012-10-04 02:08:30 PDT
Created attachment 167056 [details]
Patch
Comment 15 Gyuyoung Kim 2012-10-04 02:50:35 PDT
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.
Comment 16 KyungTae Kim 2012-10-04 04:00:22 PDT
Created attachment 167069 [details]
Patch
Comment 17 Gyuyoung Kim 2012-10-04 04:07:18 PDT
Comment on attachment 167069 [details]
Patch

LGTM.
Comment 18 Chris Dumez 2012-10-04 06:47:21 PDT
Comment on attachment 167069 [details]
Patch

LGTM.
Comment 19 KyungTae Kim 2012-10-04 19:11:29 PDT
Created attachment 167225 [details]
Patch
Comment 20 WebKit Review Bot 2012-10-04 23:58:47 PDT
Comment on attachment 167225 [details]
Patch

Clearing flags on attachment: 167225

Committed r130472: <http://trac.webkit.org/changeset/130472>
Comment 21 WebKit Review Bot 2012-10-04 23:58:52 PDT
All reviewed patches have been landed.  Closing bug.