[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 #2 from Grzegorz Czajkowski <g.czajkowski at samsung.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

I like the idea of detaching DownloadManagerEFL from EwkView! Generally looks promising, added few minor comments. I'd like to listen Gyuyoung, Ryuan comments on that.
This patch lacks of API tests but I believe they could be done in a separate patch.

> Source/WebKit2/UIProcess/API/efl/ewk_download_job.h:155
> + * will be called on the Ewk_Download_Job.

Ewk_Download_Job ? I think you meant "Ewk_Context"

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:138
> +    clientCallbacks.m_requested = nullptr;
> +    clientCallbacks.m_failed = nullptr;
> +    clientCallbacks.m_cancelled = nullptr;
> +    clientCallbacks.m_finished = nullptr;
> +    clientCallbacks.m_userData = nullptr;

Please define ClientDownloadCallbacks' constructor or use memset(&clientCallbacks, 0, sizeof(ClientDownloadCallbacks)) for that.

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.h:37
> +struct ClientDownloadCallbacks {

Since we don't expose those callbacks outside DownloadManagerEfl.h/cpp I'd prefer to move the declaration to the cpp file and use forward declaration (struct ClientDownloadsCallbacks;)

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.h:51
> +    void setCallbacks(Ewk_Download_Requested_Cb, Ewk_Download_Failed_Cb, Ewk_Download_Cancelled_Cb,  Ewk_Download_Finished_Cb, void* userData);

setClientCallbacks() ?

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.h:69
> +    ClientDownloadCallbacks clientCallbacks;

m_clientCallbacks as it's class member.

> Tools/ChangeLog:13
> +        Adapt download caalbcks to new callback mechanism

typo: caalbcks -> callbacks

> Tools/MiniBrowser/efl/main.c:850
> +on_download_failed(Ewk_Download_Job_Error *downloadError, void *user_data)

MiniBrowser follows C coding style (downloadError -> download_error).

> Tools/MiniBrowser/efl/main.c:852
> +    const Ewk_Error *error = downloadError->error;

I don't think "const" modifier brings benefits here.

> Tools/MiniBrowser/efl/main.c:2302
> +    // Set callbacks for download events.
> +    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);

-- 
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/20150211/6bfd1407/attachment-0002.html>