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).
Created attachment 167022 [details] Patch
If file download working with this patch ? How have you tested it ?
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 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.
(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).
Created attachment 167038 [details] Patch
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 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
Created attachment 167041 [details] Patch
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.
Created attachment 167048 [details] Patch
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.
Created attachment 167050 [details] Patch
Created attachment 167056 [details] Patch
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.
Created attachment 167069 [details] Patch
Comment on attachment 167069 [details] Patch LGTM.
Created attachment 167225 [details] Patch
Comment on attachment 167225 [details] Patch Clearing flags on attachment: 167225 Committed r130472: <http://trac.webkit.org/changeset/130472>
All reviewed patches have been landed. Closing bug.