Bug 114734 – [WK2] Make EFL WKView API shareable between ports

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Mikhail Pozdnyakov]

The EFL WKView API and its implementation (WebView class) can be shared between ports using Coord Graphics code path (QT, EFL).

[Mikhail Pozdnyakov]

Created attachment 198722 [details]
WIP

[Mikhail Pozdnyakov]

Comment on attachment 198722 [details]
WIP

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

> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.cpp:109
> +#if USE(ACCELERATED_COMPOSITING)

this should be removed.

[Kenneth Rohde Christiansen]

Comment on attachment 198722 [details]
WIP

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

very nice

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:82
> +PassRefPtr<WebView> WebView::create(WebContext* context, WebPageGroup* pageGroup)
> +{
> +#if PLATFORM(EFL)
> +    return adoptRef(new WebViewEfl(context, pageGroup));
> +#else
> +    return adoptRef(new WebView(context, pageGroup));
> +#endif
> +}

why not let EFL implement a different create method where it instantiates it's own derived class?

[Mikhail Pozdnyakov]

(In reply to comment #3)
> (From update of attachment 198722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198722&action=review
> 
> very nice
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:82
> > +PassRefPtr<WebView> WebView::create(WebContext* context, WebPageGroup* pageGroup)
> > +{
> > +#if PLATFORM(EFL)
> > +    return adoptRef(new WebViewEfl(context, pageGroup));
> > +#else
> > +    return adoptRef(new WebView(context, pageGroup));
> > +#endif
> > +}
> 
> why not let EFL implement a different create method where it instantiates it's own derived class?

Than it would require another method being invoked from WKViewCreate which I did not wanted, so far WKView.cpp/WKView.h is not aware of existing WebViewEFL.
Secondly in EFL we never want to create WebView which is not WebViewEFL.

[Mikhail Pozdnyakov]

Created attachment 198822 [details]
patch

WebView::create is defined for EFL inside WebViewEfl as recommended by Kenneth. Also included WKViewEfl files that were accidentally missing in the previous patch :P

[Mikhail Pozdnyakov]

Comment on attachment 198822 [details]
patch

argh.. forgot to move paintToCairoSurface implementation to WebViewEfl..

[Mikhail Pozdnyakov]

Created attachment 198824 [details]
patch

[Jocelyn Turcotte]

Comment on attachment 198824 [details]
patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.h:162
> +    virtual void doneWithKeyEvent(const NativeWebKeyboardEvent&, bool) OVERRIDE;
> +#if ENABLE(TOUCH_EVENTS)
> +    virtual void doneWithTouchEvent(const NativeWebTouchEvent&, bool wasEventHandled) OVERRIDE;
> +#endif

I thought about this problem for a while to know how we could share the view between Qt and EFL and I came up with the conclusions:
- It's not a complete view API unless it also allows event handling
- We can't expose NativeWeb*Event types directly in the API, we have to wrap them in C types
- I prefer to have a Qt-specific view that converts QEvents directly to NativeWeb*Events instead of having to maintain a in-the-middle C wrapper for events and do additional conversions.

So with this I don't think it's a good idea to share the view between ports.
What do you think?

[Mikhail Pozdnyakov]

(In reply to comment #8)
> (From update of attachment 198824 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198824&action=review
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.h:162
> > +    virtual void doneWithKeyEvent(const NativeWebKeyboardEvent&, bool) OVERRIDE;
> > +#if ENABLE(TOUCH_EVENTS)
> > +    virtual void doneWithTouchEvent(const NativeWebTouchEvent&, bool wasEventHandled) OVERRIDE;
> > +#endif
> 
> I thought about this problem for a while to know how we could share the view between Qt and EFL and I came up with the conclusions:
> - It's not a complete view API unless it also allows event handling
> - We can't expose NativeWeb*Event types directly in the API, we have to wrap them in C types
> - I prefer to have a Qt-specific view that converts QEvents directly to NativeWeb*Events instead of having to maintain a in-the-middle C wrapper for events and do additional conversions.
> 
> So with this I don't think it's a good idea to share the view between ports.
> What do you think?

The C API creation work for NativeWeb*Events types is ongoing https://bugs.webkit.org/show_bug.cgi?id=108915. 

I don't think it should be a stopper for this patch as at the moment nothing prevents qt from converting QEvents directly (as a temporary solution) same way as it does now. 

Qt-specific view could be based on the existing WebView (WKView APIs) and handle events via accessing page from it (until events C APIs are ready) and at the same time a rather big part of the functionality which is already implemented inside WebView can be shared and re-used. I think it is beneficial even though WKView API is not 100% completed at the moment.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #8)
> - I prefer to have a Qt-specific view that converts QEvents directly to NativeWeb*Events instead of having to maintain a in-the-middle C wrapper for events and do additional conversions.

What about having functions that look like (made up example):

WKViewSendTouchEvent(WKViewRef view, const QTouchEvent& event);

in a WKViewQt.h header? Those functions would convert to internal NativeWebTouchEvent and call the common WebView function. Similar approach can be applied to client callbacks related to events.

[Jesus Sanchez-Palencia]

Comment on attachment 198824 [details]
patch

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

LGTM!

> Source/WebKit2/ChangeLog:60
> +        (WebKit::WebView::paintToCairoSurface):

This shouldn't be here, right?

> Source/WebKit2/ChangeLog:150
> +        * UIProcess/efl/WebViewEfl.cpp: Added.

I don't see WebViewEfl::paintToCairoSurface listed here. It seems the Changelog wasn't updated after you moved this code. ;)

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:56
> +    char* debugVisualsEnvironment = getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS");

const char* ?

[Jocelyn Turcotte]

(In reply to comment #10)
> What about having functions that look like (made up example):
> 
> WKViewSendTouchEvent(WKViewRef view, const QTouchEvent& event);
> 
> in a WKViewQt.h header? Those functions would convert to internal NativeWebTouchEvent and call the common WebView function. Similar approach can be applied to client callbacks related to events.

We talked a bit on IRC and I tend to agree that sharing an API would have its benefits, but this made me think a bit again.

This sounds like it would work, but what would we be gaining out of sharing this code more than with the abstraction already provided by PageClient and CoordinatedGraphicsScene?

If you look at WebView.cpp in the patch above, most of what it does is delegating calls to WebPageProxy and CoordinatedGraphicsScene on one side, and forward callbacks from PageClient on the other side.

It's difficult for me to guess how it's going to look in the future, but the way I see it, to get something out of sharing the same view class we have to share everything between CoreIPC and the C API. That would mean avoiding port-specific headers/functions and get rid of the need for NativeWeb*Event and all Qt/EFL type dependencies under the C API.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #12)
> This sounds like it would work, but what would we be gaining out of sharing this code more than with the abstraction already provided by PageClient and CoordinatedGraphicsScene?

Note that this changes benefit out-of-trunk ports (like Nix), making a clearer interface where we can rely upon, and reducing the amount of "Nix-specific but still generic code" we have to provide.


> If you look at WebView.cpp in the patch above, most of what it does is delegating calls to WebPageProxy and CoordinatedGraphicsScene on one side, and forward callbacks from PageClient on the other side.

One example of benefit I could remember is the code for handling the transformations correctly when painting: I think some bugs we faced developing WebKitNix were also faced by other ports before. This would be an example of shared code.


> It's difficult for me to guess how it's going to look in the future, but the way I see it, to get something out of sharing the same view class we have to share everything between CoreIPC and the C API. That would mean avoiding port-specific headers/functions and get rid of the need for NativeWeb*Event and all Qt/EFL type dependencies under the C API.

I didn't quite grasp the comment about "share everything between CoreIPC and the C API".

One typical way to benefit is with the TestWebKitAPI and WebKitTestRunner tools, we can significatly reduce boilerplate (usually repeated among ports) by using WKView.

In case of event feeding, if we manage to have a C-API way to get events in, we can use that. Qt could still have their own way to feed Q*Events directly via specific functions, and those would save whatever extra data they need into NativeWeb*Events.

[Mikhail Pozdnyakov]

(In reply to comment #11)
> (From update of attachment 198824 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198824&action=review
> 
> LGTM!
thanks for having a look!
> 
> > Source/WebKit2/ChangeLog:60
> > +        (WebKit::WebView::paintToCairoSurface):
> 
> This shouldn't be here, right?
> 
> > Source/WebKit2/ChangeLog:150
> > +        * UIProcess/efl/WebViewEfl.cpp: Added.
> 
> I don't see WebViewEfl::paintToCairoSurface listed here. It seems the Changelog wasn't updated after you moved this code. ;)
you're right (ooops!)
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:56
> > +    char* debugVisualsEnvironment = getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS");
> 
> const char* ?
that is the existing code that was just moved, and I think it's just keeping of the getenv semantics : char* getenv (const char* name);

[Mikhail Pozdnyakov]

Created attachment 198967 [details]
patch

Updated due to comments from Jesus.

[Jesus Sanchez-Palencia]

(In reply to comment #15)
> Created an attachment (id=198967) [details]
> patch
> 
> Updated due to comments from Jesus.

It looks good, I guess all you need now is to sync with Jocelyn about the Platform events issue he was concerned.

[Jocelyn Turcotte]

(In reply to comment #13)
> I didn't quite grasp the comment about "share everything between CoreIPC and the C API".

The reason that we have NativeWeb*Event is because the abstraction point for events lies at the WebPageProxy level, to be able to return the unhandled event instances through PageClient::doneWith*Event.
So what I meant is that all ports, if using the same WebView class, can essentially raise the abstraction point up to the C API and there is no need to have port-specific ways of handling events.

My other point is that I think that this is the only way that we can end up with an API opaque enough to allow it to be maintainable, otherwise we'll have too many different code paths and we'll need to consider all of them for each modification. Especially considering the lower quality of tests that we have had for platform event handling code in the past.
The slight differences in the way Qt and EFL are using PageViewportController and how it makes modifications to this class difficult is an example of that.

In other words, I think that it only make sense to share the same view class if we avoid port-specific behaviors for the same features.

That said, this patch does seem like a step in the right direction.

[WebKit Commit Bot]

Comment on attachment 198967 [details]
patch

Clearing flags on attachment: 198967

Committed r148963: <http://trac.webkit.org/changeset/148963>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.