Bug 111591 – [EFL][WK2] Separate WebView further from EwkView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kenneth Rohde Christiansen]

Now create WebView separately from EwkView.

[Kenneth Rohde Christiansen]

Created attachment 191776 [details]
WIP Patch

[Kenneth Rohde Christiansen]

Created attachment 191792 [details]
WIP Patch

[Kenneth Rohde Christiansen]

Created attachment 191794 [details]
Patch (work in progress)

Remove the WKViewConfig part for now.

[Kenneth Rohde Christiansen]

Created attachment 191796 [details]
Patch (work in progress)

Fix inspector crash

[Mikhail Pozdnyakov]

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

> Source/WebKit2/UIProcess/API/efl/EwkView.h:102
> +    static EwkView* create(WebKit::WebView*, Evas* canvas, PassRefPtr<EwkContext> context, Evas_Smart* smart = 0);

Not sure about it. So far we have evas object that owns ewkview and ewkview owns webview, at least it is clearly determined how all 
those objects are created and deleted. Now it becomes more complicated.. like if WKViewCreateEvasObject was invoked web view memory management is done one way and if WKViewCreateEvasObject wasn't invoked it should be done totally different.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:94
> +    return o;

o?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:97
> +static Evas_Object* internal_add_with_context(Evas* canvas, Ewk_Context* context, Evas_Smart* smart)

createView? createEvasObject?

> Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:109
> +    WKViewSetThemePath(wkView, WKStringCreateWithUTF8CString(TEST_THEME_DIR "/default.edj"));

adopt

> Source/WebKit2/UIProcess/efl/WebView.cpp:77
> +{

assertion here?

> Source/WebKit2/UIProcess/efl/WebView.cpp:288
> +    if (!m_ewkView)

think assertion is enough here. WebView will own its focus state after bug110877 is fixed.

> Source/WebKit2/UIProcess/efl/WebView.cpp:296
> +    if (!m_ewkView)

ditto

> Tools/TestWebKitAPI/efl/PlatformWebView.cpp:69
> +    WKViewSetThemePath(m_view, WKStringCreateWithUTF8CString(THEME_DIR "/default.edj"));

adopt?

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:67
> +        WKPageSetUseFixedLayout(WKViewGetPage(m_view), true);

that is great that we use WKPageSetUseFixedLayout and drop this annoying ViewBehavior, I guess it could be done even as a separate patch.

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> View in context: https://bugs.webkit.org/attachment.cgi?id=191794&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.h:102
> > +    static EwkView* create(WebKit::WebView*, Evas* canvas, PassRefPtr<EwkContext> context, Evas_Smart* smart = 0);
> 
> Not sure about it. So far we have evas object that owns ewkview and ewkview owns webview, at least it is clearly determined how all those objects are created and deleted.

That is unchanged.

> Now it becomes more complicated.. like if WKViewCreateEvasObject was invoked web view memory management is done one way and if WKViewCreateEvasObject wasn't invoked it should be done totally different.

huh? It makes it possible to use the C API without EFL, which is the whole point. If you don't invoke it, you have to EwkView nor an Evas_Object.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:97
> > +static Evas_Object* internal_add_with_context(Evas* canvas, Ewk_Context* context, Evas_Smart* smart)
> 
> createView? createEvasObject?

It will go away when the EwkView doesn't store the EwkContext but just the C API one.

> > Source/WebKit2/UIProcess/efl/WebView.cpp:288
> > +    if (!m_ewkView)
> 
> think assertion is enough here. WebView will own its focus state after bug110877 is fixed.

Yes, it can be removed then, right now it will crash without this.

[Kenneth Rohde Christiansen]

Created attachment 191821 [details]
Patch (work in progress)

[Kenneth Rohde Christiansen]

Created attachment 191837 [details]
Patch (work in progress)

[Kenneth Rohde Christiansen]

Created attachment 191841 [details]
Patch (work in progress)

Minor changes, time for bed...

[Kenneth Rohde Christiansen]

Created attachment 191844 [details]
Patch (work in progress)

Realized that I forgot to add the missing adoptWK calls that Mikhail pointed out.

[Kenneth Rohde Christiansen]

Created attachment 191955 [details]
Patch (work in progress)

[Mikhail Pozdnyakov]

Comment on attachment 191955 [details]
Patch (work in progress)

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:33
> +    WKViewRef wkView = toAPI(new WebView(toImpl(contextRef), toImpl(pageGroupRef)));

WebView is API object, we need adoption here. think it's worth having WebView::create()

[Mikhail Pozdnyakov]

Comment on attachment 191955 [details]
Patch (work in progress)

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

> Source/WebKit2/UIProcess/API/efl/EwkView.h:208
> +    EwkView(WebKit::WebView*, Evas_Object* evasObject);

PassRefPtr

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:101
> +    WKViewRef wkView = WKViewCreate(WKContextCreate(), 0);

RetainPtr

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:111
> +    WKViewRef wkView = WKViewCreate(ewkContext->wkContext(), 0);

RetainPtr

[Mikhail Pozdnyakov]

Comment on attachment 191955 [details]
Patch (work in progress)

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:101
>> +    WKViewRef wkView = WKViewCreate(WKContextCreate(), 0);
> 
> RetainPtr

WKContextCreate() needs adoption

[Mikhail Pozdnyakov]

Comment on attachment 191955 [details]
Patch (work in progress)

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:40
> +    EwkView* ewkView = EwkView::create(toImpl(viewRef), evas, smart);

check for null

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:101
>>> +    WKViewRef wkView = WKViewCreate(WKContextCreate(), 0);
>> 
>> RetainPtr
> 
> WKContextCreate() needs adoption

view also needs adoption, WKViewCreateEvasObject will retain it

[Mikhail Pozdnyakov]

Created attachment 192083 [details]
WIP patch

Fixed all above

[Mikhail Pozdnyakov]

Created attachment 192191 [details]
patch

[Benjamin Poulain]

Comment on attachment 192191 [details]
patch

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

Why not I guess. Signed off by me for WebKit2.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:263
> +    // Remove when possible.

+FIXME

[Caio Marcelo de Oliveira Filho]

It seems that when you use an EFL web view, you want it to be an EvasObject, so I think should be way more simpler to make the EFL API use the WKViewCreate by itself instead of making the calling code do that.

EFL API would expose a way to get the WKViewRef/WKPageRef in order to use other WK2 C API calls.

In this scenario we'll have:

- WKViewRef/WebView as the "raw" web view API. One can use WKViewCreate() to use them.
- ewk_view_*/EwkView as the EFL web view API. It uses WKViewCreate() itself, it exposes its WKViewRef and WKPageRef for sharing code in tests and places that make a "PlatformWebView" abstraction and use WK2 C API.

This approach avoids the (IMHO) confusing WKViewCreateEvasObject() function, that does a lot of stuff behind that name: sligthly change ownership expectation about WKView, should be called only once per WebView, change the WK2 clients by itself.

[Mikhail Pozdnyakov]

Created attachment 193263 [details]
patch v2

WKView APIs are EFL-free now. Rebased. Will work correctly however only after bug112364 is fixed.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 193263 [details]
patch v2

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:35
> +    WKPageSetUseFixedLayout(webView->pageRef(), true);

I think this shouldn't be here.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:273
> +    // Remove when possible.

It was pointed out in other review. Add a "FIXME: " here.

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:65
> +    m_view = WKViewCreate(context, pageGroup);
> +    m_window = ewkCreateEvasObject(m_view, ecore_evas_get(m_backing), /* smart */ 0);

My suggestion was more in the line of having this code written as:

    m_window = ewkCreateWebView(context, pageGroup, ecore_evas_get, /* smart */ 0);
    m_view = ewkGetWKView(m_window); // Note that this do not transfer ownership.
    // or having an ewkGetWKPage().

In other hands, when using EWK, we use a create function specific of EWK that will call WKViewCreate (and possibly set WKPageSetUseFixedLayout() and friends.

[Caio Marcelo de Oliveira Filho]

> In other hands, when using EWK, we use a create function specific of EWK that will call WKViewCreate (and possibly set WKPageSetUseFixedLayout() and friends.

"In other words" I meant.

[Mikhail Pozdnyakov]

Created attachment 193535 [details]
patch v3

Took Marcelo's proposal into consideration.

[Mikhail Pozdnyakov]

Comment on attachment 193535 [details]
patch v3

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:34
> +    RefPtr<WebView> webView = WebView::create(toImpl(contextRef), toImpl(pageGroupRef));

mm, no need to have RefPtr anymore.

[Mikhail Pozdnyakov]

(In reply to comment #24)
> (From update of attachment 193535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193535&action=review
> 
> > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:34
> > +    RefPtr<WebView> webView = WebView::create(toImpl(contextRef), toImpl(pageGroupRef));
> 
> mm, no need to have RefPtr anymore.

on the other hand WKContextCreate.. functions use the same pattern, so can be leaved as it is I guess..

[Kenneth Rohde Christiansen]

Comment on attachment 193535 [details]
patch v3

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

> Source/WebKit2/ChangeLog:10
> +
> +        EFL is not present in WKView API.
> +        WebView does not keep evas object.
> +

I would explain this better:

Main points being:
 - The WKView API has no dependency on EFL types
 - WebView does not store the Evas_Object

> Source/WebKit2/UIProcess/API/C/efl/WKView.h:81
> -
>  WK_EXPORT WKImageRef WKViewCreateSnapshot(WKViewRef);

We need to change this method (later) to not depend on EFL

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:273
> +    // Remove when possible.

FIXME:

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:92
> -Evas_Object* ewk_view_smart_add(Evas* canvas, Evas_Smart* smart, Ewk_Context* context, Ewk_Page_Group* pageGroup)
> +Evas_Object* ewkCreateEvasObject(WKContextRef context, WKPageGroupRef pageGroup, Evas* canvas, Evas_Smart* smart)

maybe just ewkCreate ? or ewkViewCreate(). Are these internal/private methods? then maybe just _ewk_create() or similar

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:38
> +
> +EAPI Evas_Object* ewkCreateEvasObject(WKContextRef, WKPageGroupRef, Evas*, Evas_Smart*);
> +EAPI WKViewRef ewkGetWKView(Evas_Object*);
> +

Either we should use EFL naming or WK2 naming.

_ewk_view_create()
_ewk_view_wkview_get()

[Caio Marcelo de Oliveira Filho]

Comment on attachment 193535 [details]
patch v3

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

I think we are converging to the right direction. :-)  Added a couple of comments on top of Kenneth comments. I think after this round we should get WK2 Owner to review again and land this.

>>> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:34
>>> +    RefPtr<WebView> webView = WebView::create(toImpl(contextRef), toImpl(pageGroupRef));
>> 
>> mm, no need to have RefPtr anymore.
> 
> on the other hand WKContextCreate.. functions use the same pattern, so can be leaved as it is I guess..

Yes. Leave WebView::create() returning RefPtr, because WKViewCreate should return a WKViewRef.

> Tools/TestWebKitAPI/PlatformWebView.h:54
> +typedef struct _Evas_Object Evas_Object;
>  typedef WKViewRef PlatformWKView;
> -typedef Ecore_Evas* PlatformWindow;
> +typedef Ecore_Evas* PlatformBacking;
> +typedef Evas_Object* PlatformWindow;

I don't think you should create another typedef here. If I recall correctly, Ecore_Evas* conceptually represents the PlatformWindow, and should keep that way.

You should either (1) make the Evas_Object* (of type ewk_*) be the PlatformWKView, (2) keep it as extra member, or (3) make a way to go from WKView -> ewk_type.

Given that you need Ewk for certain operations and for deletion, and there's no shared code that relies on PlatformWKView, I say you go for (1).

[Mikhail Pozdnyakov]

Created attachment 193564 [details]
patch v4

Updated due to previous comments from Kenneth and Marcelo.

[Mikhail Pozdnyakov]

Comment on attachment 193564 [details]
patch v4

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

> Tools/WebKitTestRunner/PlatformWebView.h:108
> +    PlatformBacking m_backing;

forgot to get rid of backing here :(

[Mikhail Pozdnyakov]

Created attachment 193566 [details]
patch v5

Now there is no 'backing' in WTR.

[Kenneth Rohde Christiansen]

Comment on attachment 193566 [details]
patch v5

LGTM Misha!

[Caio Marcelo de Oliveira Filho]

Comment on attachment 193566 [details]
patch v5

LGTM.

[Mikhail Pozdnyakov]

Created attachment 193620 [details]
patch v6

Rebased to master.

[Benjamin Poulain]

Comment on attachment 193620 [details]
patch v6

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

Quickly skimmed over this. This looks ok for WebKit2. Go ahead with a review.

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:70
> +    if (m_usingFixedLayout)
> +        resizeTo(800, 600);
> +

Uh?

[Caio Marcelo de Oliveira Filho]

Comment on attachment 193620 [details]
patch v6

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

Let's get this going.

> Source/WebKit2/ChangeLog:23
> +            WKViewRef is passed to EwkView constructor from outside rather than
> +            created inside.

Optional: I would go even further and make EwkView create the WKViewRef itself, so the EWKViewCreate() function is just a wrapper to EwkView::create() call.

[WebKit Review Bot]

Comment on attachment 193620 [details]
patch v6

Clearing flags on attachment: 193620

Committed r146265: <http://trac.webkit.org/changeset/146265>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.