Bug 109559

Summary: [EFL][WK2] Introduce WKViewClient C API
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit EFLAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, benjamin, gyuyoung.kim, kenneth, laszlo.gombos, lucas.de.marchi, mikhail.pozdnyakov, rakuco, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107657    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2013-02-12 02:44:02 PST
In order to decouple EFL's PageClient from EwkView, we need to introduce a WKViewClient C API so that PageClient can interact with WebView instead of EwkView and so that the EwkView can simply use WKViewClient.
Comment 1 Chris Dumez 2013-02-13 02:26:58 PST
Created attachment 188036 [details]
Patch
Comment 2 Chris Dumez 2013-02-13 02:36:27 PST
Created attachment 188037 [details]
Patch

Quickly add "explicit" to one of the constructor before Gyuyoung notices :)
Comment 3 Kenneth Rohde Christiansen 2013-02-13 04:15:15 PST
Comment on attachment 188036 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        PageClient and EwkView. In the end, PageClient should only interact with

when completed, Page...

> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:36
> +
> +EwkView* ViewClientEfl::toEwkView(const void* clientInfo)

As this is the default EFL implementation shoulnd this be moved to the API/efl directory?
Comment 4 Mikhail Pozdnyakov 2013-02-13 04:18:43 PST
Comment on attachment 188037 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:61
> +    viewClient.clientInfo = this;

it does not need "this" it needs EwkView, right?

> Source/WebKit2/UIProcess/efl/ViewClientEfl.h:37
> +class ViewClientEfl {

I am not convinced we need this class at all, could EwkView itself somehow implement client interface?
Comment 5 Chris Dumez 2013-02-13 04:20:51 PST
Comment on attachment 188036 [details]
Patch

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

>> Source/WebKit2/ChangeLog:10
>> +        PageClient and EwkView. In the end, PageClient should only interact with
> 
> when completed, Page...

Hmm. Why not but I'm not convinced "In the end" is incorrect.

>> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:36
>> +EwkView* ViewClientEfl::toEwkView(const void* clientInfo)
> 
> As this is the default EFL implementation shoulnd this be moved to the API/efl directory?

All our EFL clients are currently in this folder, so I added ViewClientEfl here. We may decide to move all of them to API/efl at some point but at least it is consistent for now.
Comment 6 Chris Dumez 2013-02-13 04:27:17 PST
Comment on attachment 188037 [details]
Patch

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

>> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:61
>> +    viewClient.clientInfo = this;
> 
> it does not need "this" it needs EwkView, right?

For the current callbacks we have, yes. But when we start adding more callbacks, this may change. Having the ViewClientEfl is more flexible and more forward looking IMHO.

>> Source/WebKit2/UIProcess/efl/ViewClientEfl.h:37
>> +class ViewClientEfl {
> 
> I am not convinced we need this class at all, could EwkView itself somehow implement client interface?

Well, so far we have split clients from Ewk classes to avoid cluttering their implementation. For example, look at EwkContext and DownloadManagerEfl / ContextHistoryClientEfl / NetworkInfoProvider ...

I think it is nice to have a clear separation between the clients and the Ewk classes. This is what we have been doing so far and I don't see why this case is different.
Comment 7 Mikhail Pozdnyakov 2013-02-13 04:28:02 PST
Comment on attachment 188037 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/ViewClientEfl.h:39
> +    PassOwnPtr<ViewClientEfl> create(EwkView* view)

mmm.. maybe I'm missing something, but who calls it? I cannot find it in the patch..
Comment 8 Chris Dumez 2013-02-13 04:29:03 PST
Comment on attachment 188037 [details]
Patch

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

>> Source/WebKit2/UIProcess/efl/ViewClientEfl.h:39
>> +    PassOwnPtr<ViewClientEfl> create(EwkView* view)
> 
> mmm.. maybe I'm missing something, but who calls it? I cannot find it in the patch..

Oops :-)
Comment 9 Chris Dumez 2013-02-13 04:48:39 PST
Created attachment 188058 [details]
Patch

Take feedback into consideration.
Comment 10 Mikhail Pozdnyakov 2013-02-13 04:59:40 PST
Comment on attachment 188058 [details]
Patch

looks OK
Comment 11 Kenneth Rohde Christiansen 2013-02-13 05:00:45 PST
Comment on attachment 188058 [details]
Patch

LGTM
Comment 12 Anders Carlsson 2013-02-13 07:59:30 PST
Comment on attachment 188058 [details]
Patch

It's a little weird to use a C API to do this since it seems like an implementation detail, but I'll let it slide since it's not cross platform code.
Comment 13 WebKit Review Bot 2013-02-13 08:18:24 PST
Comment on attachment 188058 [details]
Patch

Clearing flags on attachment: 188058

Committed r142750: <http://trac.webkit.org/changeset/142750>
Comment 14 WebKit Review Bot 2013-02-13 08:18:30 PST
All reviewed patches have been landed.  Closing bug.