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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Feb 11 00:29:01 PST 2015


--- Comment #2 from Grzegorz Czajkowski <g.czajkowski at samsung.com> ---
Comment on attachment 246314
  --> https://bugs.webkit.org/attachment.cgi?id=246314

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>

More information about the webkit-unassigned mailing list