[webkit-reviews] review denied: [Bug 107685] [EFL][WK2] RequestManagerClientEfl, DownloadManagerEfl and ContextHistoryClientEfl should be based on C API : [Attachment 184222] patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Mikhail Pozdnyakov
<mikhail.pozdnyakov at intel.com>'s request for review:
Bug 107685: [EFL][WK2] RequestManagerClientEfl, DownloadManagerEfl and
ContextHistoryClientEfl should be based on C API
https://bugs.webkit.org/show_bug.cgi?id=107685

Attachment 184222: patch
https://bugs.webkit.org/attachment.cgi?id=184222&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=184222&action=review


Looks like a step in the right direction. Some comments:

> Source/WebKit2/UIProcess/efl/ContextHistoryClientEfl.cpp:109
> -ContextHistoryClientEfl::ContextHistoryClientEfl(PassRefPtr<WebContext>
context)
> +ContextHistoryClientEfl::ContextHistoryClientEfl(WKContextRef context)

I see you include DownloadProxy.h in this file, is that intentional?

> Source/WebKit2/UIProcess/efl/ContextHistoryClientEfl.h:29
> +#include "WKRetainPtr.h"

I would use #include <WebKit2/WKRetainPtr.h> to simplify the include paths.
Then order the #includes.

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.h:29
> +#include "WKRetainPtr.h"

Ditto.

> Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.cpp:70
> -RequestManagerClientEfl::RequestManagerClientEfl(EwkContext* context)
> -    :
m_soupRequestManager(WKContextGetSoupRequestManager(toAPI(context->webContext()
.get())))
> +RequestManagerClientEfl::RequestManagerClientEfl(WKContextRef context)
> +    : m_soupRequestManager(WKContextGetSoupRequestManager(context))

This class uses WebSoupRequestManagerProxy directly. It looks like it could
easily switch to the C API.