Bug 107685 – [EFL][WK2] RequestManagerClientEfl, DownloadManagerEfl and ContextHistoryClientEfl should be based on C API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Mikhail Pozdnyakov]

SSIA

[Mikhail Pozdnyakov]

Created attachment 184222 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 184222 [details]
patch

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

> Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.cpp:72
> -    ASSERT(context);
> +    ASSERT(m_soupRequestManager);

where is this change explained?

[Mikhail Pozdnyakov]

Comment on attachment 184222 [details]
patch

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

>> Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.cpp:72
>> +    ASSERT(m_soupRequestManager);
> 
> where is this change explained?

ASSERT(context); was erroneous here, as context is used already before this line

[Chris Dumez]

Comment on attachment 184222 [details]
patch

LGTM.

[Benjamin Poulain]

Comment on attachment 184222 [details]
patch

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.

[Mikhail Pozdnyakov]

Comment on attachment 184222 [details]
patch

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

>> Source/WebKit2/UIProcess/efl/ContextHistoryClientEfl.cpp:109
>> +ContextHistoryClientEfl::ContextHistoryClientEfl(WKContextRef context)
> 
> I see you include DownloadProxy.h in this file, is that intentional?

apparently not :) thanks for the catch

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

sure it is better

>> Source/WebKit2/UIProcess/efl/RequestManagerClientEfl.cpp:70
>> +    : m_soupRequestManager(WKContextGetSoupRequestManager(context))
> 
> This class uses WebSoupRequestManagerProxy directly. It looks like it could easily switch to the C API.

mm.. I guess it's in place already 'WKRetainPtr<WKSoupRequestManagerRef> m_soupRequestManager'

[Mikhail Pozdnyakov]

Created attachment 185419 [details]
patch v2

Took comments from Benjamin into consideration.

[Benjamin Poulain]

> mm.. I guess it's in place already 'WKRetainPtr<WKSoupRequestManagerRef> m_soupRequestManager'

Here:
    toImpl(m_soupRequestManager.get())->registerURIScheme(scheme);

[Mikhail Pozdnyakov]

(In reply to comment #8)
> > mm.. I guess it's in place already 'WKRetainPtr<WKSoupRequestManagerRef> m_soupRequestManager'
> 
> Here:
>     toImpl(m_soupRequestManager.get())->registerURIScheme(scheme);

indeed.. I haven't noticed :( Thanks for pointing out

[Mikhail Pozdnyakov]

Created attachment 185498 [details]
patch v3

Removed remaining WK2 internal usage in RequestManagerClientEfl::registerURLSchemeHandler.

[Benjamin Poulain]

Comment on attachment 185498 [details]
patch v3

Looks good.

Just remove #include "WebSoupRequestManagerProxy.h" from RequestManagerClientEfl.cpp.

[Mikhail Pozdnyakov]

Committed r141389: <http://trac.webkit.org/changeset/141389>