Bug 111591

Created attachment 191776 [details] WIP Patch

Created attachment 191792 [details] WIP Patch

Created attachment 191794 [details] Patch (work in progress) Remove the WKViewConfig part for now.

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

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.

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

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

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

Created attachment 191841 [details] Patch (work in progress) Minor changes, time for bed...

Created attachment 191844 [details] Patch (work in progress) Realized that I forgot to add the missing adoptWK calls that Mikhail pointed out.

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

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

Created attachment 192083 [details] WIP patch Fixed all above

Created attachment 192191 [details] patch

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

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.

Created attachment 193263 [details] patch v2 WKView APIs are EFL-free now. Rebased. Will work correctly however only after bug112364 is fixed.

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.

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

Created attachment 193535 [details] patch v3 Took Marcelo's proposal into consideration.

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.

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

Created attachment 193564 [details] patch v4 Updated due to previous comments from Kenneth and Marcelo.

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

Created attachment 193566 [details] patch v5 Now there is no 'backing' in WTR.

Comment on attachment 193566 [details] patch v5 LGTM Misha!

Comment on attachment 193566 [details] patch v5 LGTM.

Created attachment 193620 [details] patch v6 Rebased to master.

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 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 on attachment 193620 [details] patch v6 Clearing flags on attachment: 193620 Committed r146265: <http://trac.webkit.org/changeset/146265>

All reviewed patches have been landed. Closing bug.