Bug 109559 – [EFL][WK2] Introduce WKViewClient C API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 188036 [details]
Patch

[Chris Dumez]

Created attachment 188037 [details]
Patch

Quickly add "explicit" to one of the constructor before Gyuyoung notices :)

[Kenneth Rohde Christiansen]

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?

[Mikhail Pozdnyakov]

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?

[Chris Dumez]

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.

[Chris Dumez]

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.

[Mikhail Pozdnyakov]

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

[Chris Dumez]

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 :-)

[Chris Dumez]

Created attachment 188058 [details]
Patch

Take feedback into consideration.

[Mikhail Pozdnyakov]

Comment on attachment 188058 [details]
Patch

looks OK

[Kenneth Rohde Christiansen]

Comment on attachment 188058 [details]
Patch

LGTM

[Anders Carlsson]

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.

[WebKit Review Bot]

Comment on attachment 188058 [details]
Patch

Clearing flags on attachment: 188058

Committed r142750: <http://trac.webkit.org/changeset/142750>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.