Bug 107931

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

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

Created attachment 184737 [details] Patch (work in progress) Feel free to review informally and test

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.

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.

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

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?

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

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

Created attachment 184956 [details] Patch

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

Created attachment 184962 [details] Patch

Comment on attachment 184962 [details] Patch LGTM

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?

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?

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.

Created attachment 184980 [details] Patch

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?

> 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

Created attachment 184987 [details] Patch

Created attachment 184992 [details] Patch

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?

Created attachment 184997 [details] Patch

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.

Comment on attachment 184997 [details] Patch Looks great! now need to get it landed :)

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

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

Created attachment 185195 [details] Patch Re-upload previous patch to verify ews.

Created attachment 185231 [details] rebased

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

Created attachment 185493 [details] Patch Fixed mine comments above. Also fixed possible regressions in unit tests (problem with WebView deletion inside PlatformView dtor).

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.

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

(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

Created attachment 185495 [details] Patch Took comments from Kenneth and Chris into consideration.

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.

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

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

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

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

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

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

Created attachment 185758 [details] patch Made corrections due to Benjamin's comment.

Any chance for a review?

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?

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.

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

Created attachment 186393 [details] Patch

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

Created attachment 186488 [details] Patch for landing

Comment on attachment 186488 [details] Patch for landing Clearing flags on attachment: 186488 Committed r141836: <http://trac.webkit.org/changeset/141836>

All reviewed patches have been landed. Closing bug.