[Webkit-unassigned] [Bug 131162] [EFL][WK2] Crash when "Download Linked File" from MiniBrowser context menu is clicked

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=131162

--- Comment #3 from Ryuan Choi <ryuan.choi at navercorp.com> ---
Comment on attachment 246314
  --> https://bugs.webkit.org/attachment.cgi?id=246314
Patch

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

Thanks for the patch.
I leave some comments.

> Source/WebKit2/ChangeLog:15
> +        Propose a new callbacks which are view independent and detach EFL's DownloadMaager
> +        from EwkView. It makes DownloadManagerEFL consistent with WKContextDownload.
> +        Fix the crash and restore download functionality.

I am not sure.
Doesn't we have any use cases which need to distinguish which ewk_view(tab) requests this download?

> Source/WebKit2/ChangeLog:32
> +        Old tests do not match new feature. Separate bug will be introduced.

Bug ID?

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:374
> +EAPI void ewk_context_download_callbacks_set(Ewk_Context* context,
> +    Ewk_Download_Requested_Cb download_requested_func,
> +    Ewk_Download_Failed_Cb download_failed_func,
> +    Ewk_Download_Cancelled_Cb download_cancelled_func,
> +    Ewk_Download_Finished_Cb download_finished_func,
> +    void* data);

EFL looks prefer the spaces to align first letter of first parameter with others if we define API using multiple line.

For example, in Ecore_File.h
EAPI Eina_Bool ecore_file_download(const char *url,
                                   const char *dst,
                                   ...);

And style of * is wrong for the first parameter.

> Source/WebKit2/UIProcess/API/efl/ewk_download_job_private.h:74
> +    EwkDownloadJob(WKDownloadRef download);

Let's add "explicit"

>> Tools/MiniBrowser/efl/main.c:852
>> +    const Ewk_Error *error = downloadError->error;
> 
> I don't think "const" modifier brings benefits here.

Instead, how about define `error` field of Ewk_Download_Job_Error with "const" modifier?

>> Tools/MiniBrowser/efl/main.c:2302
>> +    ewk_context_download_callbacks_set(context, on_download_request, on_download_failed, 0, on_download_finished, window);
> 
> I'd rather move them to context related code in elm_main(), there we already have
> 
>     ewk_context_process_model_set(context, EWK_PROCESS_MODEL_MULTIPLE_SECONDARY);
>     ewk_context_favicon_database_directory_set(context, NULL);

+1

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150212/e4255290/attachment-0002.html>