Bug 107685 - [EFL][WK2] RequestManagerClientEfl, DownloadManagerEfl and ContextHistoryClientEfl should be based on C API
Summary: [EFL][WK2] RequestManagerClientEfl, DownloadManagerEfl and ContextHistoryClie...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on:
Blocks: 107657 107666
  Show dependency treegraph
 
Reported: 2013-01-23 06:08 PST by Mikhail Pozdnyakov
Modified: 2013-01-31 00:51 PST (History)
7 users (show)

See Also:


Attachments
patch (10.54 KB, patch)
2013-01-23 06:39 PST, Mikhail Pozdnyakov
benjamin: review-
Details | Formatted Diff | Diff
patch v2 (11.01 KB, patch)
2013-01-30 00:48 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v3 (11.41 KB, patch)
2013-01-30 07:03 PST, Mikhail Pozdnyakov
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>