Bug 91345 – [EFL][WK2] Add download client for Ewk_Context

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

We should define a download client and attach it to the Ewk_Context so that we can report information about downloads.

We should also add an API function to Ewk_Context so that the browser can trigger a download.

[Chris Dumez]

Created attachment 154005 [details]
Patch

[Chris Dumez]

Created attachment 154006 [details]
Patch

Regenerate patch with --binary for test PDF.

[Raphael Kubo da Costa (:rakuco)]

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?

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 154440 [details]
Patch

Take rakuco's feedback into consideration.

[Raphael Kubo da Costa (:rakuco)]

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/.

[Gyuyoung Kim]

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.

[Chris Dumez]

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 :)

[Chris Dumez]

Created attachment 154538 [details]
Patch

Take Rakuco and Gyuyoung's feedback into consideration.

[Chris Dumez]

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

[Gyuyoung Kim]

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.

[Chris Dumez]

Created attachment 154846 [details]
Patch

Fix nit spotted by Gyuyoung and rebase on master.

[Kenneth Rohde Christiansen]

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?

[Chris Dumez]

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.

[Chris Dumez]

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"

[WebKit Review Bot]

Comment on attachment 154932 [details]
Patch

Clearing flags on attachment: 154932

Committed r123882: <http://trac.webkit.org/changeset/123882>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.