Bug 107685

Summary: [EFL][WK2] RequestManagerClientEfl, DownloadManagerEfl and ContextHistoryClientEfl should be based on C API
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107657, 107666    
Attachments:
Description Flags
patch
benjamin: review-
patch v2
none
patch v3 benjamin: review+, benjamin: commit-queue-

Description Mikhail Pozdnyakov 2013-01-23 06:08:00 PST
SSIA
Comment 1 Mikhail Pozdnyakov 2013-01-23 06:39:35 PST
Created attachment 184222 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2013-01-24 08:02:22 PST
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?
Comment 3 Mikhail Pozdnyakov 2013-01-24 08:35:08 PST
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
Comment 4 Chris Dumez 2013-01-25 02:47:54 PST
Comment on attachment 184222 [details]
patch

LGTM.
Comment 5 Benjamin Poulain 2013-01-29 14:12:20 PST
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.
Comment 6 Mikhail Pozdnyakov 2013-01-30 00:42:17 PST
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'
Comment 7 Mikhail Pozdnyakov 2013-01-30 00:48:30 PST
Created attachment 185419 [details]
patch v2

Took comments from Benjamin into consideration.
Comment 8 Benjamin Poulain 2013-01-30 00:52:38 PST
> mm.. I guess it's in place already 'WKRetainPtr<WKSoupRequestManagerRef> m_soupRequestManager'

Here:
    toImpl(m_soupRequestManager.get())->registerURIScheme(scheme);
Comment 9 Mikhail Pozdnyakov 2013-01-30 01:31:10 PST
(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
Comment 10 Mikhail Pozdnyakov 2013-01-30 07:03:09 PST
Created attachment 185498 [details]
patch v3

Removed remaining WK2 internal usage in RequestManagerClientEfl::registerURLSchemeHandler.
Comment 11 Benjamin Poulain 2013-01-30 12:55:34 PST
Comment on attachment 185498 [details]
patch v3

Looks good.

Just remove #include "WebSoupRequestManagerProxy.h" from RequestManagerClientEfl.cpp.
Comment 12 Mikhail Pozdnyakov 2013-01-31 00:51:35 PST
Committed r141389: <http://trac.webkit.org/changeset/141389>