Bug 107931 – [EFL][WK2] Introduce a WebView class as counterpart for WKViewRef

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

[Kenneth Rohde Christiansen]

This is part of the transition of putting the EFL API entirely on top of the WK C API + a few EFL extensions (WKView...).

Next steps:
- Add C API for more WebView methods.
- Move non-evas code from EwkView into WebView
- Move smart object related code from ewk_view into EwkView
- Clean ups...

[Kenneth Rohde Christiansen]

I will upload my WIP patch as it has to be rebased on top of Mikhail's renaming.

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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

Feel free to review informally and test

[WebKit Review Bot]

Attachment 184737 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h', u'Source/WebKit2/UIProcess/API/C/efl/WKView.cpp', u'Source/WebKit2/UIProcess/API/C/efl/WKView.h', u'Source/WebKit2/UIProcess/API/efl/EwkView.cpp', u'Source/WebKit2/UIProcess/API/efl/EwkView.h', u'Source/WebKit2/UIProcess/API/efl/ewk_view.cpp', u'Source/WebKit2/UIProcess/API/efl/ewk_view_private.h', u'Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp', u'Tools/TestWebKitAPI/PlatformWebView.h', u'Tools/TestWebKitAPI/efl/PlatformWebView.cpp', u'Tools/WebKitTestRunner/PlatformWebView.h', u'Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp', u'Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp']" exit_code: 1
Source/WebKit2/UIProcess/API/efl/EwkView.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/efl/EwkView.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/efl/EwkView.cpp:116:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

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

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:118
> +WebView::WebView(WebContext* context, PageClient* pageClient, WebPageGroup* pageGroup, Evas_Object* evas)

We should really call it evasObject, not evas since evas is the name we use for the canvas.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:142
> +    m_webPageProxy->setThemePath(toImpl(theme)->string().utf8().data());

You're using WebString here but we are trying to avoid C++ classes usage.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:580
> +        WKRetainPtr<WKStringRef> wkTheme = WKStringCreateWithUTF8CString(theme);

Needs to be adopted.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:587
> +    WKStringRef customEncoding = WKPageCopyCustomTextEncodingName(wkPage());

We should probably use a WKRetainPtr and adopt to avoid leaking on early return (the WKStringIsEmpty() if check).

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:591
> +    m_customEncoding = WKEinaSharedString(AdoptWK, customEncoding);

No longer needs to be adopted if you use a WKRetainPtr earlier.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:598
> +    WKPageSetCustomTextEncodingName(wkPage(), WKStringCreateWithUTF8CString(encoding.utf8().data()));

Value returned by WKStringCreateWithUTF8CString() needs to be adopted.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:922
> +    WKRetainPtr<WKURLRef> wkActiveURL = WKPageCopyActiveURL(wkPage());

Needs to be adopted.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:923
> +    WKStringRef wkURLString = WKURLCopyString(wkActiveURL.get());

I would prefer if we use WKRetainPtr and adopt rather than having to call WKRelease() manually.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:930
> +    m_url = WKEinaSharedString(AdoptWK, wkURLString);

No longer needs to be adopted if you use WKRetainPtr earlier.

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:76
> +    evas_object_del(WKViewGetEvasObject(m_view));

This does not look right.

[Kenneth Rohde Christiansen]

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

[Chris Dumez]

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

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:131
> +    m_webPageProxy->close();

So we don't free m_evasObject here?

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:922
> +    WKRetainPtr<WKURLRef> wkActiveURL = WKPageCopyActiveURL(wkPage());

Needs to be adopted.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:923
> +    WKRetainPtr<WKStringRef> wkURLString = WKURLCopyString(wkActiveURL.get());

Ditto.

> Source/WebKit2/UIProcess/API/efl/EwkView.h:106
> +class WebView {

Shouldn't this be RefCounted?

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:78
> +    evas_object_del(WKViewGetEvasObject(m_view));

Shouldn't we WKRelease(m_view)? If the WebView destructor frees the evasObject and if it is RefCounted, it should work, right?

[Kenneth Rohde Christiansen]

> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:131
> > +    m_webPageProxy->close();
> 
> So we don't free m_evasObject here?

I tried :-) but gave up for now, we end up double freeing

> > Source/WebKit2/UIProcess/API/efl/EwkView.h:106
> > +class WebView {
> 
> Shouldn't this be RefCounted?

Could be, but maybe it should be an APIObject instead as it is supposed to be used with the C API.

> > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:78
> > +    evas_object_del(WKViewGetEvasObject(m_view));
> 
> Shouldn't we WKRelease(m_view)? If the WebView destructor frees the evasObject and if it is RefCounted, it should work, right?

That won't work yet, as said above, I tried and gave up. If you want to give it a try, feel free, but it can also be done later.

[Kenneth Rohde Christiansen]

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

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

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:131
>> +    m_webPageProxy->close();
> 
> So we don't free m_evasObject here?

No, for reasons discussed on IRC

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:922
>> +    WKRetainPtr<WKURLRef> wkActiveURL = WKPageCopyActiveURL(wkPage());
> 
> Needs to be adopted.

Fixed locally

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:923
>> +    WKRetainPtr<WKStringRef> wkURLString = WKURLCopyString(wkActiveURL.get());
> 
> Ditto.

Fixed locally

>> Source/WebKit2/UIProcess/API/efl/EwkView.h:106
>> +class WebView {
> 
> Shouldn't this be RefCounted?

We should probably move it to APIObject in another patch

[Kenneth Rohde Christiansen]

Created attachment 184956 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 184956 [details]
Patch

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

LGTM

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:65
> +Evas_Object* WKViewGetEvasObject(WKViewRef viewRef)

could we have a comment, saying it is temporary?

> Source/WebKit2/UIProcess/API/efl/EwkView.h:105
> +class WebView {

guess should be inherited from APIObject (but maybe not necessary within the current patch).

> Source/WebKit2/UIProcess/API/efl/EwkView.h:112
> +    WKPageRef pageRef() { return toAPI(m_webPageProxy.get()); }

const method?

> Source/WebKit2/UIProcess/API/efl/EwkView.h:118
> +    // FIXME: Remove when possible.

right

[Kenneth Rohde Christiansen]

Created attachment 184962 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 184962 [details]
Patch

LGTM

[Chris Dumez]

Comment on attachment 184962 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:32
> +    return ewk_view_base_add(canvas, contextRef, pageGroupRef, EwkView::LegacyBehavior)->wkView();

This looks like it is leaking, isn't it? ewk_view_base_add() creates a EwkView and then we return a member. When is the EwkView destroyed?

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:37
> +    return ewk_view_base_add(canvas, contextRef, pageGroupRef, EwkView::DefaultBehavior)->wkView();

Ditto.

> Source/WebKit2/UIProcess/API/efl/EwkView.h:283
> +    OwnPtr<WebKit::WebView> m_webView;

Why isn't this a RefPtr? WebView is refcounted, right?

[Chris Dumez]

Comment on attachment 184962 [details]
Patch

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

> Tools/TestWebKitAPI/efl/PlatformWebView.cpp:-76
> -    evas_object_del(m_view);

Shouldn't we call WKRelease(m_view) here?

> Tools/TestWebKitAPI/efl/PlatformWebView.cpp:76
> +    evas_object_del(WKViewGetEvasObject(m_view));

This frees the Evas_Object inside the EwkView but I don't see where the EwkView is actually freed. And why doesn't the EwkView free the Evas_Object itself (it should own it, right?).

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:-76
> -    evas_object_del(m_view);

Shouldn't we call WKRelease(m_view) here?

[Kenneth Rohde Christiansen]

Comment on attachment 184962 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:32
>> +    return ewk_view_base_add(canvas, contextRef, pageGroupRef, EwkView::LegacyBehavior)->wkView();
> 
> This looks like it is leaking, isn't it? ewk_view_base_add() creates a EwkView and then we return a member. When is the EwkView destroyed?

It is being deleted by the smart object: Source/WebKit2/UIProcess/API/efl/ewk_view.cpp: delete smartData->priv;

>> Source/WebKit2/UIProcess/API/efl/EwkView.h:283
>> +    OwnPtr<WebKit::WebView> m_webView;
> 
> Why isn't this a RefPtr? WebView is refcounted, right?

Sure, will fix

>> Tools/TestWebKitAPI/efl/PlatformWebView.cpp:-76
>> -    evas_object_del(m_view);
> 
> Shouldn't we call WKRelease(m_view) here?

Because so far we free from the smart object, which frees the EwkView which frees the WebView.

>> Tools/TestWebKitAPI/efl/PlatformWebView.cpp:76
>> +    evas_object_del(WKViewGetEvasObject(m_view));
> 
> This frees the Evas_Object inside the EwkView but I don't see where the EwkView is actually freed. And why doesn't the EwkView free the Evas_Object itself (it should own it, right?).

Because we haven't consolidated the smart object code and the EwkView yet; that is what Mikhail is working on.

[Kenneth Rohde Christiansen]

Created attachment 184980 [details]
Patch

[Chris Dumez]

Comment on attachment 184980 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:32
> +    return ewk_view_base_add(canvas, contextRef, pageGroupRef, EwkView::LegacyBehavior)->wkView();

I believe WKRetain() should be called here. The convention is that a Create function in the C API returns an object that needs to be unref'd by the caller.

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:37
> +    return ewk_view_base_add(canvas, contextRef, pageGroupRef, EwkView::DefaultBehavior)->wkView();

Ditto.

> Source/WebKit2/UIProcess/API/efl/EwkView.h:105
> +class WebView : public APIObject {

Why are we adding this class to EwkView.h/cpp? Could we move it to its own file to avoid cluttering the existing code? EwkView file is already fairly big on its own.

> Tools/TestWebKitAPI/efl/PlatformWebView.cpp:-76
> -    evas_object_del(m_view);

I believe WKRelease(m_view) will be needed here.

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:-76
> -    evas_object_del(m_view);

I believe WKRelease() will be needed here once the Create function calls WKRetain().

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:78
> +    evas_object_del(WKViewGetEvasObject(m_view));

Since the patch is likely going to land with this workaround. Would it be possible to share how it is supposed to look like in the end? What approach have you chosen to cleanly handle the Evas_object memory handling?

[Kenneth Rohde Christiansen]

> I believe WKRetain() should be called here. The convention is that a Create function in the C API returns an object that needs to be unref'd by the caller.

Very good point; fixed.

> > Source/WebKit2/UIProcess/API/efl/EwkView.h:105
> > +class WebView : public APIObject {
> 
> Why are we adding this class to EwkView.h/cpp? Could we move it to its own file to avoid cluttering the existing code? EwkView file is already fairly big on its own.

Yes, I could probably do that. Let me discuss with Mikhail

> Since the patch is likely going to land with this workaround. Would it be possible to share how it is supposed to look like in the end? What approach have you chosen to cleanly handle the Evas_object memory handling?

Let's discuss this on IRC

[Kenneth Rohde Christiansen]

Created attachment 184987 [details]
Patch

[Kenneth Rohde Christiansen]

Created attachment 184992 [details]
Patch

[Chris Dumez]

Comment on attachment 184992 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.h:105
> +class WebView : public APIObject {

Why is this still in EwkView.h?

[Kenneth Rohde Christiansen]

Created attachment 184997 [details]
Patch

[Chris Dumez]

Comment on attachment 184997 [details]
Patch

LGTM. I think this is a good first step since we need to go incrementally. Thanks for the hard work Kenneth.

[Mikhail Pozdnyakov]

Comment on attachment 184997 [details]
Patch

Looks great! now need to get it landed :)

[Build Bot]

Comment on attachment 184997 [details]
Patch

Attachment 184997 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16163885

New failing tests:
fast/workers/worker-lifecycle.html

[Mikhail Pozdnyakov]

(In reply to comment #26)
> (From update of attachment 184997 [details])
> Attachment 184997 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://queues.webkit.org/results/16163885
> 
> New failing tests:
> fast/workers/worker-lifecycle.html

has to be unrelated: only efl-specific code was touched.

[Mikhail Pozdnyakov]

Created attachment 185195 [details]
Patch

Re-upload previous patch to verify ews.

[Mikhail Pozdnyakov]

Created attachment 185231 [details]
rebased

[Mikhail Pozdnyakov]

Comment on attachment 185231 [details]
rebased

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:32
> +    WKViewRef viewRef = ewk_view_base_add(canvas, contextRef, pageGroupRef, EwkView::LegacyBehavior)->wkView();

ouch! one more problem :( ewk_view_base_add can return '0', so the result should be checked first before invoking wkView()..

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:39
> +    WKViewRef viewRef = ewk_view_base_add(canvas, contextRef, pageGroupRef, EwkView::DefaultBehavior)->wkView();

ditto.

> Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:102
> +    m_inspectorView = ewk_view_base_add(ecore_evas_get(m_inspectorWindow), toAPI(page()->process()->context()), toAPI(inspectorPageGroup()), EwkView::LegacyBehavior)->view();

ditto

[Mikhail Pozdnyakov]

Created attachment 185493 [details]
Patch

Fixed mine comments above. Also fixed possible regressions in unit tests (problem with WebView deletion inside PlatformView dtor).

[Chris Dumez]

Comment on attachment 185493 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:556
> +    WKRetainPtr<WKStringRef> wkEncoding = adoptWK(WKStringCreateWithUTF8CString(encoding.utf8().data()));

We could use adoptWK(toCopiedAPI(encoding)) here. It is a bit shorter.

[Kenneth Rohde Christiansen]

Comment on attachment 185493 [details]
Patch

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

> Tools/TestWebKitAPI/efl/PlatformWebView.cpp:81
> +    WKRelease(m_view);
> +
> +    // FIXME: The C WKView API currently creates the Evas_Object, so we have to destruct it
> +    // (and its dependencies EwkView and WebKit::WebView) this way, until this get fixed.
> +    evas_object_del(WKViewGetEvasObject(m_view));

shouldn't you get the evas object first? This looks wrong

[Mikhail Pozdnyakov]

(In reply to comment #33)
> (From update of attachment 185493 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185493&action=review
> 
> > Tools/TestWebKitAPI/efl/PlatformWebView.cpp:81
> > +    WKRelease(m_view);
> > +
> > +    // FIXME: The C WKView API currently creates the Evas_Object, so we have to destruct it
> > +    // (and its dependencies EwkView and WebKit::WebView) this way, until this get fixed.
> > +    evas_object_del(WKViewGetEvasObject(m_view));
> 
> shouldn't you get the evas object first? This looks wrong

well yeah, would look better, even though technically it does not matter

[Mikhail Pozdnyakov]

Created attachment 185495 [details]
Patch

Took comments from Kenneth and Chris into consideration.

[Benjamin Poulain]

Comment on attachment 185495 [details]
Patch

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

I am a bit confused with what you are doing.

WebPage is the WebKit abstraction to a page, it is the window to the content. The other platforms have a view that is the native View for the WebPage.
If you look at Mac's WKView for example, it is dead simple. It simply map the native "things" to the WebKit "things".

In your case, can you explain what are the responsibilities of the WK C layer? Why does it exists?
Then what is the relations between the WKView, EwkView and ewk_view? Why is there 3 abstractions for the exact same thing?

> Source/WebKit2/ChangeLog:58
> +            object construction has been solves.

solves -> solved.

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:36
> +    // Retain as the contract with *Create functions is that returned objects much to be released after use.

Not sure about EFL convention, but if that is the common pattern, you don't need that comment.

CoreFoundation has similar convention. Anything with Create and Copy return a +1 ref-ed object.

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:46
> +    // Retain as the contract with *Create functions is that returned objects much to be released after use.

Ditto.

[Mikhail Pozdnyakov]

(In reply to comment #36)
> (From update of attachment 185495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185495&action=review
> 
> I am a bit confused with what you are doing.
> 
> WebPage is the WebKit abstraction to a page, it is the window to the content. The other platforms have a view that is the native View for the WebPage.
> If you look at Mac's WKView for example, it is dead simple. It simply map the native "things" to the WebKit "things".
> 
> In your case, can you explain what are the responsibilities of the WK C layer? Why does it exists?
> Then what is the relations between the WKView, EwkView and ewk_view? Why is there 3 abstractions for the exact same thing?

That was the plan :)
1) Existing EwkViewImpl class code and Ewk_view smart object code will be
distributed between two new classes: WebView and EwkView 

2) WebView class is intended to be minimal and just expose the WK2 internals we need; it will be EFL-agnostic meaning we avoid Evas constructs in it.

3) WebView class will not be exported, instead it will have exported wrapping C WKView API(api/c/efl), including client API to be used by EwkView.

4) EwkView class will include the glue code between EFL (Evas) and WebView. Besides EwkView will support WebView client interface.

[Mikhail Pozdnyakov]

(In reply to comment #37)
> (In reply to comment #36)
> > (From update of attachment 185495 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=185495&action=review
> > 
> > I am a bit confused with what you are doing.
> > 
> > WebPage is the WebKit abstraction to a page, it is the window to the content. The other platforms have a view that is the native View for the WebPage.
> > If you look at Mac's WKView for example, it is dead simple. It simply map the native "things" to the WebKit "things".
> > 
> > In your case, can you explain what are the responsibilities of the WK C layer? Why does it exists?
> > Then what is the relations between the WKView, EwkView and ewk_view? Why is there 3 abstractions for the exact same thing?
> 
> That was the plan :)
> 1) Existing EwkViewImpl class code and Ewk_view smart object code will be
> distributed between two new classes: WebView and EwkView 
> 
> 2) WebView class is intended to be minimal and just expose the WK2 internals we need; it will be EFL-agnostic meaning we avoid Evas constructs in it.
> 
> 3) WebView class will not be exported, instead it will have exported wrapping C WKView API(api/c/efl), including client API to be used by EwkView.
> 
> 4) EwkView class will include the glue code between EFL (Evas) and WebView. Besides EwkView will support WebView client interface.

Kenneth, you might want to add something to this explanation.

[Kenneth Rohde Christiansen]

> In your case, can you explain what are the responsibilities of the WK C layer? Why does it exists?
> Then what is the relations between the WKView, EwkView and ewk_view? Why is there 3 abstractions for the exact same thing?

A few extra comments to what Mikhail already said.

ewk_view will go away, or be merged with EwkView. EwkView is basically the private class for the public EFL API.

We want the ability to use our port without requiring Evas_Object/smart classes/X11 etc, with a simple lower-level C API allowing us to draw to an opengl context (we are not there yet). The specialized Evas_Object based API will be based on top of this.

[Gyuyoung Kim]

(In reply to comment #39)
>> ewk_view will go away, or be merged with EwkView. EwkView is basically the private class for the public EFL API.

> We want the ability to use our port without requiring Evas_Object/smart classes/X11 etc, with a simple lower-level C API allowing us to draw to an opengl context (we are not there yet). The specialized Evas_Object based API will be based on top of this.

I have ambiguous thing. I understand that you guys're suggesting the plan as below, right ?

WK C-API <-> WebView <-> EwkView <-> ewk_XXX 
(Each role was explained by comment #37)

Basically, I agree to move to this plan in order to be placed top of WK C-APIs. But, it looks you guys want to hide smart classes to private EwkView or replace smart class function with smart callback eventually. If so, frankly I'm not sure if this change can replace all existing functions of smart class, or satisfies all requirements from EFL applications. So, I would suggest that you guys talk with EFL community when you guys start to hide smart class as well.

[Mikhail Pozdnyakov]

(In reply to comment #40)
> (In reply to comment #39)
> >> ewk_view will go away, or be merged with EwkView. EwkView is basically the private class for the public EFL API.
> 
> > We want the ability to use our port without requiring Evas_Object/smart classes/X11 etc, with a simple lower-level C API allowing us to draw to an opengl context (we are not there yet). The specialized Evas_Object based API will be based on top of this.
> 
> I have ambiguous thing. I understand that you guys're suggesting the plan as below, right ?
> 
> WK C-API <-> WebView <-> EwkView <-> ewk_XXX 
> (Each role was explained by comment #37)
> 
> Basically, I agree to move to this plan in order to be placed top of WK C-APIs. But, it looks you guys want to hide smart classes to private EwkView or replace smart class function with smart callback eventually. If so, frankly I'm not sure if this change can replace all existing functions of smart class, or satisfies all requirements from EFL applications. So, I would suggest that you guys talk with EFL community when you guys start to hide smart class as well.

There is NO changes to the current external API planned as part of this.

This is just refactoring (re-organization) of the current implementation.
Please take a look at WIP patch at bug#108062 for details.
(Moreover we can consider bug#108062 as a continuation of work for converting  efl objects to c++ classes.)

[Gyuyoung Kim]

(In reply to comment #41)

> There is NO changes to the current external API planned as part of this.
> 
> This is just refactoring (re-organization) of the current implementation.
> Please take a look at WIP patch at bug#108062 for details.
> (Moreover we can consider bug#108062 as a continuation of work for converting  efl objects to c++ classes.)

Ok, I see. There was misunderstanding to me. Looks fine. Thanks.

[Mikhail Pozdnyakov]

Created attachment 185758 [details]
patch

Made corrections due to Benjamin's comment.

[Kenneth Rohde Christiansen]

Any chance for a review?

[Simon Hausmann]

Comment on attachment 185758 [details]
patch

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

I wonder if it's possible to break down this patch into smaller pieces. A few comments below.

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:36
> -    return toAPI(ewk_view_base_add(canvas, contextRef, pageGroupRef, EwkView::LegacyBehavior));
> +    EwkView* ewkView = ewk_view_base_add(canvas, contextRef, pageGroupRef, EwkView::LegacyBehavior);
> +    if (!ewkView)
> +        return 0;
> +
> +    return static_cast<WKViewRef>(WKRetain(ewkView->wkView()));

So in the long run this function would change signature to just take a WKContextRef and pagegroupref and not instantiate the EwkView anymore?

> Source/WebKit2/UIProcess/API/C/efl/WKView.h:48
> +WK_EXPORT void WKViewSuspendActiveDOMObjectsAndAnimations(WKViewRef);
> +WK_EXPORT void WKViewResumeActiveDOMObjectsAndAnimations(WKViewRef);

This doesn't look like something that is specific to the ELF port nor to the view, especially given that the implementation is in WebPageProxy. Should this be a function of WKPage? (WKPagePrivate.h?)

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

How does that compare to the existing WKPageCreateSnapshotOfVisibleContent ?

> Source/WebKit2/UIProcess/efl/WebView.cpp:65
> +void WebView::setThemePath(WKStringRef theme)
> +{
> +    m_webPageProxy->setThemePath(toWTFString(theme).utf8().data());
> +}

Is this something that should go into preferences instead?

[Chris Dumez]

Comment on attachment 185758 [details]
patch

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

>> Source/WebKit2/UIProcess/API/C/efl/WKView.h:53
>> +WK_EXPORT WKImageRef WKViewCreateSnapshot(WKViewRef);
> 
> How does that compare to the existing WKPageCreateSnapshotOfVisibleContent ?

Hmm. I did not know about that one. However, WKPageCreateSnapshotOfVisibleContent() is unimplemented and marked for removal (Bug 66979). BTW, this is not a new function, this patch merely removes the argument name.

[Kenneth Rohde Christiansen]

Comment on attachment 185758 [details]
patch

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

>> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:36
>> +    return static_cast<WKViewRef>(WKRetain(ewkView->wkView()));
> 
> So in the long run this function would change signature to just take a WKContextRef and pagegroupref and not instantiate the EwkView anymore?

Yes that is correct

>> Source/WebKit2/UIProcess/API/C/efl/WKView.h:48
>> +WK_EXPORT void WKViewResumeActiveDOMObjectsAndAnimations(WKViewRef);
> 
> This doesn't look like something that is specific to the ELF port nor to the view, especially given that the implementation is in WebPageProxy. Should this be a function of WKPage? (WKPagePrivate.h?)

That could be done if other ports find that useful

>> Source/WebKit2/UIProcess/efl/WebView.cpp:65
>> +}
> 
> Is this something that should go into preferences instead?

In the long run we want to get rid of the Edje theming and just use one nice default theme like Chrome does

[Kenneth Rohde Christiansen]

Created attachment 186393 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186393 [details]
Patch

Rejecting attachment 186393 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 186393, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
Log
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/TestWebKitAPI/PlatformWebView.h
patching file Tools/TestWebKitAPI/efl/PlatformWebView.cpp
patching file Tools/WebKitTestRunner/PlatformWebView.h
patching file Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp
patching file Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Anders Carlsson']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16368419

[Kenneth Rohde Christiansen]

Created attachment 186488 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 186488 [details]
Patch for landing

Clearing flags on attachment: 186488

Committed r141836: <http://trac.webkit.org/changeset/141836>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.