Bug 111591

Summary: [EFL][WK2] Separate WebView further from EwkView
Product: WebKit Reporter: Kenneth Rohde Christiansen <kenneth>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cmarcelo, dongseong.hwang, gyuyoung.kim, kalyan.kondapally, kondapallykalyan, lucas.de.marchi, menard, mikhail.pozdnyakov, rakuco, tmpsantos, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 112364    
Bug Blocks: 107662    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
Patch (work in progress)
none
Patch (work in progress)
none
Patch (work in progress)
none
Patch (work in progress)
none
Patch (work in progress)
none
Patch (work in progress)
none
Patch (work in progress)
none
WIP patch
none
patch
none
patch v2
none
patch v3
none
patch v4
none
patch v5
none
patch v6 none

Description Kenneth Rohde Christiansen 2013-03-06 09:47:06 PST
Now create WebView separately from EwkView.
Comment 1 Kenneth Rohde Christiansen 2013-03-06 09:47:47 PST
Created attachment 191776 [details]
WIP Patch
Comment 2 Kenneth Rohde Christiansen 2013-03-06 10:50:21 PST
Created attachment 191792 [details]
WIP Patch
Comment 3 Kenneth Rohde Christiansen 2013-03-06 10:58:18 PST
Created attachment 191794 [details]
Patch (work in progress)

Remove the WKViewConfig part for now.
Comment 4 Kenneth Rohde Christiansen 2013-03-06 11:04:16 PST
Created attachment 191796 [details]
Patch (work in progress)

Fix inspector crash
Comment 5 Mikhail Pozdnyakov 2013-03-06 11:55:02 PST
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.
Comment 6 Kenneth Rohde Christiansen 2013-03-06 12:38:45 PST
(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.
Comment 7 Kenneth Rohde Christiansen 2013-03-06 13:34:02 PST
Created attachment 191821 [details]
Patch (work in progress)
Comment 8 Kenneth Rohde Christiansen 2013-03-06 14:55:19 PST
Created attachment 191837 [details]
Patch (work in progress)
Comment 9 Kenneth Rohde Christiansen 2013-03-06 15:04:01 PST
Created attachment 191841 [details]
Patch (work in progress)

Minor changes, time for bed...
Comment 10 Kenneth Rohde Christiansen 2013-03-06 15:13:36 PST
Created attachment 191844 [details]
Patch (work in progress)

Realized that I forgot to add the missing adoptWK calls that Mikhail pointed out.
Comment 11 Kenneth Rohde Christiansen 2013-03-07 02:17:05 PST
Created attachment 191955 [details]
Patch (work in progress)
Comment 12 Mikhail Pozdnyakov 2013-03-07 05:12:32 PST
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()
Comment 13 Mikhail Pozdnyakov 2013-03-07 13:03:02 PST
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
Comment 14 Mikhail Pozdnyakov 2013-03-07 13:25:49 PST
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
Comment 15 Mikhail Pozdnyakov 2013-03-07 13:54:12 PST
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
Comment 16 Mikhail Pozdnyakov 2013-03-07 14:14:02 PST
Created attachment 192083 [details]
WIP patch

Fixed all above
Comment 17 Mikhail Pozdnyakov 2013-03-08 03:23:49 PST
Created attachment 192191 [details]
patch
Comment 18 Benjamin Poulain 2013-03-13 14:47:57 PDT
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
Comment 19 Caio Marcelo de Oliveira Filho 2013-03-14 06:42:50 PDT
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.
Comment 20 Mikhail Pozdnyakov 2013-03-15 02:32:55 PDT
Created attachment 193263 [details]
patch v2

WKView APIs are EFL-free now. Rebased. Will work correctly however only after bug112364 is fixed.
Comment 21 Caio Marcelo de Oliveira Filho 2013-03-15 07:24:34 PDT
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.
Comment 22 Caio Marcelo de Oliveira Filho 2013-03-15 07:28:26 PDT
> 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.
Comment 23 Mikhail Pozdnyakov 2013-03-18 05:04:26 PDT
Created attachment 193535 [details]
patch v3

Took Marcelo's proposal into consideration.
Comment 24 Mikhail Pozdnyakov 2013-03-18 05:06:29 PDT
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.
Comment 25 Mikhail Pozdnyakov 2013-03-18 05:47:23 PDT
(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..
Comment 26 Kenneth Rohde Christiansen 2013-03-18 05:49:02 PDT
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()
Comment 27 Caio Marcelo de Oliveira Filho 2013-03-18 07:11:03 PDT
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).
Comment 28 Mikhail Pozdnyakov 2013-03-18 07:47:38 PDT
Created attachment 193564 [details]
patch v4

Updated due to previous comments from Kenneth and Marcelo.
Comment 29 Mikhail Pozdnyakov 2013-03-18 07:51:05 PDT
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 :(
Comment 30 Mikhail Pozdnyakov 2013-03-18 08:08:22 PDT
Created attachment 193566 [details]
patch v5

Now there is no 'backing' in WTR.
Comment 31 Kenneth Rohde Christiansen 2013-03-18 08:13:39 PDT
Comment on attachment 193566 [details]
patch v5

LGTM Misha!
Comment 32 Caio Marcelo de Oliveira Filho 2013-03-18 09:44:45 PDT
Comment on attachment 193566 [details]
patch v5

LGTM.
Comment 33 Mikhail Pozdnyakov 2013-03-18 11:43:48 PDT
Created attachment 193620 [details]
patch v6

Rebased to master.
Comment 34 Benjamin Poulain 2013-03-19 13:56:24 PDT
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?
Comment 35 Caio Marcelo de Oliveira Filho 2013-03-19 14:32:43 PDT
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.
Comment 36 WebKit Review Bot 2013-03-19 14:58:12 PDT
Comment on attachment 193620 [details]
patch v6

Clearing flags on attachment: 193620

Committed r146265: <http://trac.webkit.org/changeset/146265>
Comment 37 WebKit Review Bot 2013-03-19 14:58:19 PDT
All reviewed patches have been landed.  Closing bug.