Bug 91345

Created attachment 154005 [details] Patch

Created attachment 154006 [details] Patch Regenerate patch with --binary for test PDF.

Comment on attachment 154006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154006&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:150 > + if (!download) > + return; > + ewk_download_unref(download); Nit: how about if (download) ewk_download_unref(download); > Source/WebKit2/UIProcess/API/efl/ewk_context_download_client.cpp:62 > + String destination = makeString("file://", String::fromUTF8(ewk_download_destination_get(download))); IIRC, makeString() is normally frowned upon: <https://bugs.webkit.org/show_bug.cgi?id=62527#c15>. Is some other code responsible for checking if the destination is valid? > Source/WebKit2/UIProcess/API/efl/ewk_download.cpp:72 > + ASSERT(!__ref); What if you build in Release mode? Is it OK for the de-ref not to happen? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_download.cpp:45 > +static const char* serverSuggestedFilename = "webkit-downloaded-file"; Couldn't this be made as const as testFilePath?

Comment on attachment 154006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154006&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:150 >> + ewk_download_unref(download); > > Nit: how about > if (download) > ewk_download_unref(download); Sure. >> Source/WebKit2/UIProcess/API/efl/ewk_context_download_client.cpp:62 >> + String destination = makeString("file://", String::fromUTF8(ewk_download_destination_get(download))); > > IIRC, makeString() is normally frowned upon: <https://bugs.webkit.org/show_bug.cgi?id=62527#c15>. Is some other code responsible for checking if the destination is valid? Good to know. Gyuyoung has been pushing me to use makeString() so I followed his advice. I'll fix it. >> Source/WebKit2/UIProcess/API/efl/ewk_download.cpp:72 >> + ASSERT(!__ref); > > What if you build in Release mode? Is it OK for the de-ref not to happen? I'm not sure what you mean. This assertion simply makes sure we are not calling delete on an object that's still referenced. This would indicate a programming error on our part so I think an assertion is correct here. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_download.cpp:45 >> +static const char* serverSuggestedFilename = "webkit-downloaded-file"; > > Couldn't this be made as const as testFilePath? Could you clarify? if you mean that I should define it as an array instead of a pointer (similarly to testFilePath), then I agree and I'll fix this. I don't really understand the "const" comment since it is already const.

Created attachment 154440 [details] Patch Take rakuco's feedback into consideration.

Comment on attachment 154440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154440&action=review A few style nits I noticed now. I see that the file names and the URIs are handled as UTF-8. Do you know how that plays out with languages such as Japanese or Korean (I know, if it's broken here it's broken in the rest of the API as well)? > Source/WebKit2/UIProcess/API/efl/ewk_download.cpp:157 > +Eina_Bool ewk_download_destination_set(Ewk_Download *download, const char* destination) Style nit: '*download' should be '* download'. > Source/WebKit2/UIProcess/API/efl/ewk_download.cpp:162 > + eina_stringshare_replace(&download->destination, destination); I still wonder whether if some part of this code should check if the destination is a valid path or not. > Source/WebKit2/UIProcess/API/efl/ewk_download.cpp:167 > +const char *ewk_download_suggested_filename_get(const Ewk_Download *download) Ditto here. > Source/WebKit2/UIProcess/API/efl/ewk_download.h:134 > +EAPI Eina_Bool ewk_download_destination_set(Ewk_Download *download, const char* destination); s/* destination/*destination/.

Comment on attachment 154440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154440&action=review > Source/WebKit2/UIProcess/API/efl/ewk_download.cpp:198 > + return static_cast<double>(download->downloaded) / static_cast<double>(contentLength); Is it enough to use static_cast for one operand ? > Source/WebKit2/UIProcess/API/efl/ewk_download.cpp:203 > + EINA_SAFETY_ON_NULL_RETURN_VAL(download, 0); 0 -> 0.0 ? > Source/WebKit2/UIProcess/API/efl/ewk_download.h:25 > + Missing file description. > Source/WebKit2/UIProcess/API/efl/ewk_download.h:185 > + * @return seconds since the download was started or 0 in case of failure. ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:30 > + * - "download,new", Ewk_Download*: reports that a new download has been requested. The client should set the Wrong alphabetical order.

Comment on attachment 154440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154440&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_download.cpp:162 >> + eina_stringshare_replace(&download->destination, destination); > > I still wonder whether if some part of this code should check if the destination is a valid path or not. I'm not sure what kind of check I can do here really. What will happen is that if the destination path is invalid, we won't be able to create the file later and the download will fail. Any kind of check I would do here would only be partial because you don't really know if it's valid until you try writing to the hard disk (e.g. in case of permission problem). >> Source/WebKit2/UIProcess/API/efl/ewk_download.cpp:198 >> + return static_cast<double>(download->downloaded) / static_cast<double>(contentLength); > > Is it enough to use static_cast for one operand ? True. >> Source/WebKit2/UIProcess/API/efl/ewk_download.cpp:203 >> + EINA_SAFETY_ON_NULL_RETURN_VAL(download, 0); > > 0 -> 0.0 ? This would be against WebKit coding style Gyuyoung :)

Created attachment 154538 [details] Patch Take Rakuco and Gyuyoung's feedback into consideration.

Kenneth, I know it's big but can you please find the courage to review this patch? :)

Comment on attachment 154538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154538&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:50 > +void ewk_view_download_new(Evas_Object* ewkView, Ewk_Download*); Nit : Wrong alphabetical order.

Created attachment 154846 [details] Patch Fix nit spotted by Gyuyoung and rebase on master.

Comment on attachment 154846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154846&action=review > Source/WebKit2/ChangeLog:17 > + The client application needs to listen for > + "download,new" signal on the view and set > + the download path for the download in the > + callback in order to accept it. If the signal > + is ignored or if the download path is not set > + the download will fail. That makes it more of a request then. "download,request" ? > Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:285 > + Ewk_Download* ewkDownload = ewk_download_new(download, m_viewWidget); ewk_download_request_new ? ewk_download_job_new > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:68 > + HashMap<uint64_t, Ewk_Download*> downloads; downloadJobs? > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:158 > + if (download) > + ewk_download_unref(download); why not make these methods accept null pointers?

Comment on attachment 154846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154846&action=review >> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:285 >> + Ewk_Download* ewkDownload = ewk_download_new(download, m_viewWidget); > > ewk_download_request_new ? ewk_download_job_new It is not really a request, except in it initial state, then it's an ongoing download. If you prefer Download_Job instead of Download, I don't mind. >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:158 >> + ewk_download_unref(download); > > why not make these methods accept null pointers? I think this is would be wrong API usage to call an unref method on NULL pointer. If I'm not mistaken, g_object_unref(NULL) will also cause warnings. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:32 > + * - "download,new", Ewk_Download*: reports that a new download has been requested. The client should set the "download,request" is a good idea, yes.

Created attachment 154932 [details] Patch Take Kenneth's feedback into consideration: - Rename Ewk_Download to Ewk_Download_Job (and all corresponding functions: ewk_download_job_*()) - Rename "download,new" signal to "download,request"

Comment on attachment 154932 [details] Patch Clearing flags on attachment: 154932 Committed r123882: <http://trac.webkit.org/changeset/123882>

All reviewed patches have been landed. Closing bug.