Bug 108062

Created attachment 185561 [details] WIP patch WIP as: it is better to be based on bug#107931 patch + there are still some areas to improve

Comment on attachment 185561 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=185561&action=review > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:41 > + return toAPI(view->evasObject()); I think we should probably ref the returned Evas Object since the is a *Create function. > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:50 > + return toAPI(view->evasObject()); Ditto. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:122 > +template <_Evas_Callback_Type EventType> I would remove leading '_'. Evas_Callback_Type exists. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:347 > + Evas_Object* evasObject = evas_object_smart_add(canvas, smart); Could we use RefPtr here to simplify memory handling? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:352 > + evas_object_del(evasObject); Would not be needed if we used a RefPtr. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:360 > + smartData->priv = new EwkView(evasObject, context, toImpl(pageGroupRef), behavior); Would be nice to remove the call to toImpl() here but I guess this is outside the scope of this patch? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:421 > +bool EwkView::isFromEwkView(const Evas_Object* evasObject) isEwkViewObject()? or isEwkView() ? I find the name a bit confusing. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:447 > Ewk_View_Smart_Data* EwkView::smartData() const Feels like this method should not be const. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1206 > + Ewk_View_Smart_Data* smartData = EwkView::smartData(evasObject); You should probably take a look at EVAS_SMART_DATA_ALLOC() macro: http://docs.enlightenment.org/auto/evas/group__Evas__Smart__Group.html#ga4aa4b72aafa0391144fb0171aeff2ccd It would be 1 line instead of 4. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1209 > + smartData = new Ewk_View_Smart_Data; I think we used calloc before because it initialized members to 0 and because it is freed on C side. Switching to new is wrong if this is really freed on C side. Anyway, using EVAS_SMART_DATA_ALLOC() as I proposed earlier would solve this issue as well. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1338 > +#define CHECK_COLOR(color, alpha) \ static inline function? > Source/WebKit2/UIProcess/API/efl/EwkView.h:114 > + Ewk_View_Smart_Data* smartData() const; non const? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:88 > Evas_Object* ewk_view_smart_add(Evas* canvas, Evas_Smart* smart, Ewk_Context* context) I think the behavior is different here. Before, the caller owned the returned Evas_Object, now it doesn't. I think the previous behavior made more sense. Probably we should ref the returned evas_object. > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:85 > +// FIXME: should have this in some generic place If the EwkContext::create() factory function took a WKContextRef in argument, we wouldn't need this function probably. I think it is duplicated now.

Comment on attachment 185561 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=185561&action=review > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:40 > + EwkView* view = EwkView::create(canvas, ewkContext(contextRef), pageGroupRef, EwkView::LegacyBehavior); > + if (!view) > + return 0; > + why not if (EwkView* view = ...) return toAPI... return 0; > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:90 > +// Aux functions. spell out aux > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:92 > +static PageViewMap& pageViewMap() maybe we should just call it allViews() like other platforms. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:133 > + static void unsubscribe(Evas_Object* evasObject) > + { > + evas_object_event_callback_del(evasObject, EventType, onEvent); > + } could/should these be inline? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:342 > +{ FromSmartClass? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:371 > + if (!isFromEwkView(evasObject)) isOfEwkViewType ?hmm > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:382 > +EwkView* EwkView::fromSmartData(const Ewk_View_Smart_Data* smartData) > +{ > + EINA_SAFETY_ON_NULL_RETURN_VAL(smartData, 0); > + > + return smartData->priv; > +} why is this EINA... better than just return smartData ? smartData->priv : 0; ? The method should probably be inline > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:406 > + api.sc.data = smartClassName; // It is used by type checking. s/by/for/ > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:413 > + api.mouse_wheel = onViewSmartMouseWheel; > + api.mouse_down = onViewSmartMouseDown; > + api.mouse_up = onViewSmartMouseUp; I would remove the Smart part of the name. handleViewMouseUpEvent? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1240 > + ASSERT(smartData->priv); // smartData->priv is EwkView instance to delete. is owned by EwkView ? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1298 > +#if USE(ACCELERATED_COMPOSITING) > + view->setNeedsSurfaceResize(); do we even work without accelerated compositing? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1358 > + EwkView* self = EwkView::fromSmartData(smartData); > + ASSERT(self); ASSERT inside fromSmartData? > Source/WebKit2/UIProcess/API/efl/EwkView.h:247 > + // Ewk_View_Smart_Class functions. s/functions/interface ?

Comment on attachment 185561 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=185561&action=review >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:92 >> +static PageViewMap& pageViewMap() > > maybe we should just call it allViews() like other platforms. I think we can get rid of this HashMap all together. The Map keeps the mapping between a WKPageRef and an Evas_Object. However, you can already get the Evas_object from the page via WebPageProxy::viewWidget(). We probably need a C API for this one though.

(In reply to comment #4) > (From update of attachment 185561 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185561&action=review > > >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:92 > >> +static PageViewMap& pageViewMap() > > > > maybe we should just call it allViews() like other platforms. > > I think we can get rid of this HashMap all together. The Map keeps the mapping between a WKPageRef and an Evas_Object. However, you can already get the Evas_object from the page via WebPageProxy::viewWidget(). We probably need a C API for this one though. Thanks Chris! Great finding :)

(In reply to comment #2) > (From update of attachment 185561 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185561&action=review > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:41 > > + return toAPI(view->evasObject()); > > I think we should probably ref the returned Evas Object since the is a *Create function. > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:50 > > + return toAPI(view->evasObject()); > > Ditto. > Right. > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:122 > > +template <_Evas_Callback_Type EventType> > > I would remove leading '_'. Evas_Callback_Type exists. Sure. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:347 > > + Evas_Object* evasObject = evas_object_smart_add(canvas, smart); > > Could we use RefPtr here to simplify memory handling? > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:352 > > + evas_object_del(evasObject); > > Would not be needed if we used a RefPtr. Do not fully agree as it would bring another ugliness like: smartData->priv = new EwkView(evasObject.release().leakRef(), context, toImpl(pageGroupRef), behavior); > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:360 > > + smartData->priv = new EwkView(evasObject, context, toImpl(pageGroupRef), behavior); > > Would be nice to remove the call to toImpl() here but I guess this is outside the scope of this patch? > Yeah let's do it within another patch > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:421 > > +bool EwkView::isFromEwkView(const Evas_Object* evasObject) > > isEwkViewObject()? or isEwkView() ? I find the name a bit confusing. thinking of it.. > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:447 > > Ewk_View_Smart_Data* EwkView::smartData() const > > Feels like this method should not be const. Agree, But this 'const' was already there. Removing of it is leading to further code modifications which are certainly beyond this bug scope. > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1206 > > + Ewk_View_Smart_Data* smartData = EwkView::smartData(evasObject); > > You should probably take a look at EVAS_SMART_DATA_ALLOC() macro: > http://docs.enlightenment.org/auto/evas/group__Evas__Smart__Group.html#ga4aa4b72aafa0391144fb0171aeff2ccd > > It would be 1 line instead of 4. Unfortunately EVAS_SMART_DATA_ALLOC() cannot be compiled with C++ compiler: priv_type * priv; \ priv = evas_object_smart_data_get(o); \ > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1209 > > + smartData = new Ewk_View_Smart_Data; > > I think we used calloc before because it initialized members to 0 and because it is freed on C side. Switching to new is wrong if this is really freed on C side. > Anyway, using EVAS_SMART_DATA_ALLOC() as I proposed earlier would solve this issue as well. > Ok, will put calloc back > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1338 > > +#define CHECK_COLOR(color, alpha) \ > > static inline function? Well ok, even though I would not touch it in this patch. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:88 > > Evas_Object* ewk_view_smart_add(Evas* canvas, Evas_Smart* smart, Ewk_Context* context) > > I think the behavior is different here. Before, the caller owned the returned Evas_Object, now it doesn't. I think the previous behavior made more sense. Probably we should ref the returned evas_object. > It's not that different as Evas_Object "owns" EwkView in terms of deletion. > > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:85 > > +// FIXME: should have this in some generic place > > If the EwkContext::create() factory function took a WKContextRef in argument, we wouldn't need this function probably. I think it is duplicated now. yeah..

(In reply to comment #3) > (From update of attachment 185561 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185561&action=review > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:40 > > + EwkView* view = EwkView::create(canvas, ewkContext(contextRef), pageGroupRef, EwkView::LegacyBehavior); > > + if (!view) > > + return 0; > > + > > why not > > if (EwkView* view = ...) > return toAPI... > return 0; ok. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:90 > > +// Aux functions. > > spell out aux ok. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:92 > > +static PageViewMap& pageViewMap() > > maybe we should just call it allViews() like other platforms. Removed it at all as proposed by Chris :) > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:133 > > + static void unsubscribe(Evas_Object* evasObject) > > + { > > + evas_object_event_callback_del(evasObject, EventType, onEvent); > > + } > > could/should these be inline? They are already inline as defined inside class declaration. > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:382 > > +EwkView* EwkView::fromSmartData(const Ewk_View_Smart_Data* smartData) > > +{ > > + EINA_SAFETY_ON_NULL_RETURN_VAL(smartData, 0); > > + > > + return smartData->priv; > > +} > > why is this EINA... better than just return smartData ? smartData->priv : 0; ? I was thinking of logging, but Ok. > > The method should probably be inline > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:406 > > + api.sc.data = smartClassName; // It is used by type checking. > > s/by/for/ > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:413 > > + api.mouse_wheel = onViewSmartMouseWheel; > > + api.mouse_down = onViewSmartMouseDown; > > + api.mouse_up = onViewSmartMouseUp; > > I would remove the Smart part of the name. > > handleViewMouseUpEvent? > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1240 > > + ASSERT(smartData->priv); // smartData->priv is EwkView instance to delete. > > is owned by EwkView ? Can be deleted only via deleting of its evasObject. (As API clients still operate with evasObject). > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1298 > > +#if USE(ACCELERATED_COMPOSITING) > > + view->setNeedsSurfaceResize(); > > do we even work without accelerated compositing? not sure, but the patch is not about that :) > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1358 > > + EwkView* self = EwkView::fromSmartData(smartData); > > + ASSERT(self); > > ASSERT inside fromSmartData? mmm.. no. smartData->priv can be 0 in theory. > > > Source/WebKit2/UIProcess/API/efl/EwkView.h:247 > > + // Ewk_View_Smart_Class functions. > > s/functions/interface ? sure

(In reply to comment #6) > (In reply to comment #2) > > (From update of attachment 185561 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=185561&action=review > > > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:41 > > > + return toAPI(view->evasObject()); > > > > I think we should probably ref the returned Evas Object since the is a *Create function. > > > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:50 > > > + return toAPI(view->evasObject()); > > > > Ditto. The evas object used to be returned un-refed before and the client code that we have (like WTR-EFL) is adapted to it (does not consider WKRetain WKRelease at all).. The main problem is in EFL way of evas object ref-counting: practically evas object has duplicated internal and external ref-counts http://docs.enlightenment.org/auto/evas/group__Evas__Object__Group__Basic.html Off course we'll have to reconsider our WKView API implementation, testing code and maybe RefPtr implementation for EvasObject to fix this mess. However I don't think this work should be done within this particular patch.

> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:413 > > + api.mouse_wheel = onViewSmartMouseWheel; > > + api.mouse_down = onViewSmartMouseDown; > > + api.mouse_up = onViewSmartMouseUp; > > I would remove the Smart part of the name. > > handleViewMouseUpEvent? > I would keep 'smart' in the name as it clarifies that those methods are smart class API implementation ('smart' here means EFL term - not real smartness :) ).

Created attachment 186010 [details] WIP 2 Included corrections due to comments from Chris and Kenneth (Thanks for having a look!). Changed the factory methods so that they return evas_object as it is more convenient for the clients. Renamed 'evasObjects' to 'viewObjects'.

Comment on attachment 186010 [details] WIP 2 View in context: https://bugs.webkit.org/attachment.cgi?id=186010&action=review > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:85 > +// FIXME: should have this in some generic place ahh.. forgot to delete this function.

Comment on attachment 186010 [details] WIP 2 View in context: https://bugs.webkit.org/attachment.cgi?id=186010&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:90 > +static inline PassRefPtr<EwkContext> defaultIfNull(WKContextRef contextRef) I wish I could find a better name > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:111 > + if (EINA_UNLIKELY(!smart)) { why not use UNLIKELY from WTF. Also I dont see the point, this is not a hotspot code path at all, quite the opposite > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:125 > + ASSERT(upperLimit); > + if (value < 0) > + value = 0; > + if (value > upperLimit) > + value = upperLimit; why not use one of the min/max methods. Doesnt WTF have a bound method or so? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:236 > + // This call may look wrong, but we really need to pass ViewIsVisible here. I dont like that line > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:333 > + Evas_Object* viewObject = evas_object_smart_add(canvas, smart); RefPtr? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:360 > +EwkView* EwkView::fromEvasObject(const Evas_Object* evasObject) WebKit usually does this the other way around: toEwkView(const Evas_... and as a free function. I guess you just need to make the isViewObject a free function > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:412 > +bool EwkView::isViewObject(const Evas_Object* evasObject) > +{ > + EINA_SAFETY_ON_NULL_RETURN_VAL(evasObject, false); this should be a free function > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1204 > + // Allocating with 'calloc' as will be deleted with 'free' as the API contract is that it should be deleted with 'free()'. > Source/WebKit2/UIProcess/API/efl/EwkView.h:110 > + static Evas_Object* createViewObject(Evas* canvas, Evas_Smart* smart, PassRefPtr<EwkContext> context, WKPageGroupRef pageGroupRef = 0, ViewBehavior behavior = EwkView::DefaultBehavior); > + static Evas_Object* createViewObject(Evas* canvas, PassRefPtr<EwkContext> context, WKPageGroupRef pageGroupRef = 0, ViewBehavior behavior = EwkView::DefaultBehavior); > + static Evas_Object* createViewObject(Evas* canvas, WKContextRef contextRef = 0, WKPageGroupRef pageGroupRef = 0, ViewBehavior behavior = EwkView::DefaultBehavior); wow, cant these be consolidates.. or at least have some comments attached!

Created attachment 186042 [details] patch

Comment on attachment 186042 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=186042&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:106 > + static Ewk_View_Smart_Class api = EWK_VIEW_SMART_CLASS_INIT_NAME_VERSION("Ewk_View"); Why aren't we using smartClassName here? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-114 > - : m_evasObject(evasObject) I think this was clearer before. We have 3 objects for the view: Evas_Object, EwkView and WebView. Calling something viewObject is just confusing IMHO. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:315 > +Evas_Object* EwkView::createViewObject(Evas* canvas, Evas_Smart* smart, PassRefPtr<EwkContext> context, WKPageGroupRef pageGroupRef, ViewBehavior behavior) viewObject -> EvasObject? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:338 > +Evas_Object* EwkView::createViewObject(Evas* canvas, PassRefPtr<EwkContext> context, WKPageGroupRef pageGroupRef, ViewBehavior behavior) Ditto. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:380 > +const Evas_Object* EwkView::viewObjectFromPage(WKPageRef page) I think Kenneth said he preferred 'to' functions instead of 'from' ones? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:383 > + return toImpl(page)->viewWidget(); So we'll add the C API for this one later? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1442 > + return smartData ? smartData->priv : 0; Could we replace the if condition with an assertion? ASSERT(smartData); I don't think it is acceptable to pass NULL to this function. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1445 > +bool isViewObject(const Evas_Object* evasObject) isViewEvasObject()? > Source/WebKit2/UIProcess/API/efl/EwkView.h:114 > + Ewk_View_Smart_Data* smartData() const; So this one is const? :)

(In reply to comment #14) > (From update of attachment 186042 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186042&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:106 > > + static Ewk_View_Smart_Class api = EWK_VIEW_SMART_CLASS_INIT_NAME_VERSION("Ewk_View"); > > Why aren't we using smartClassName here? It was not used there before, I mean it's 'Ewk_View' not 'Ewk2_View'. However I also think smartClassName is natural to be used there. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-114 > > - : m_evasObject(evasObject) > > I think this was clearer before. We have 3 objects for the view: Evas_Object, EwkView and WebView. Calling something viewObject is just confusing IMHO. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:315 > > +Evas_Object* EwkView::createViewObject(Evas* canvas, Evas_Smart* smart, PassRefPtr<EwkContext> context, WKPageGroupRef pageGroupRef, ViewBehavior behavior) > > viewObject -> EvasObject? > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:338 > > +Evas_Object* EwkView::createViewObject(Evas* canvas, PassRefPtr<EwkContext> context, WKPageGroupRef pageGroupRef, ViewBehavior behavior) > > Ditto. Ok. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:380 > > +const Evas_Object* EwkView::viewObjectFromPage(WKPageRef page) > > I think Kenneth said he preferred 'to' functions instead of 'from' ones? static const Evas_Object* toEvasObject(WKPageRef); > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:383 > > + return toImpl(page)->viewWidget(); > > So we'll add the C API for this one later? yep. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1442 > > + return smartData ? smartData->priv : 0; > > Could we replace the if condition with an assertion? ASSERT(smartData); I don't think it is acceptable to pass NULL to this function. > Well, I would leave it like this, as this function is for external usage. EwkView* view = toEwkView(whatever); if (view) and if we have assert we would need extra 'if (whatever)' beforehand Besides another version of toEwkView() does not have assertions and they both should be compliant. > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1445 > > +bool isViewObject(const Evas_Object* evasObject) > > isViewEvasObject()? ok. > > > Source/WebKit2/UIProcess/API/efl/EwkView.h:114 > > + Ewk_View_Smart_Data* smartData() const; > > So this one is const? :) This 'const' was already there. Removing it is leading to further code modifications which are certainly beyond this bug scope.

Created attachment 186054 [details] patch v2 Took comments from Chris into consideration.

Comment on attachment 186054 [details] patch v2 LGTM.

LGTM, too

Created attachment 186595 [details] patch v3 Rebased.

Comment on attachment 186595 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=186595&action=review > Source/WebKit2/ChangeLog:28 > + > + * UIProcess/API/C/efl/WKView.cpp: > + (WKViewCreate): > + (WKViewCreateWithFixedLayout): > + (WKViewGetPage): > + (WKViewCreateSnapshot): > + * UIProcess/API/efl/EwkView.cpp: > + (smartDataChanged): > + (defaultSmartClassInstance): > + (EwkViewEventHandler): > + (EwkViewEventHandler::subscribe): > + (EwkViewEventHandler::unsubscribe): > + (::onEvent): > + (EwkView::EwkView): > + (EwkView::~EwkView): > + (EwkView::createEvasObject): > + (EwkView::initSmartClassInterface): > + (EwkView::toEvasObject): > + (EwkView::smartData): could use a few words! > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:33 > - if (!ewkView) > + Evas_Object* evasObject = EwkView::createEvasObject(canvas, contextRef ? EwkContext::create(contextRef) : EwkContext::defaultContext(), pageGroupRef, EwkView::LegacyBehavior); > + if (!evasObject) what about default page group if no ref is given? > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:43 > - if (!ewkView) > + Evas_Object* evasObject = EwkView::createEvasObject(canvas, contextRef ? EwkContext::create(contextRef) : EwkContext::defaultContext(), pageGroupRef, EwkView::LegacyBehavior); > + if (!evasObject) > return 0; Can we make the contextRef code be on a separate line > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:172 > + EINA_SAFETY_ON_NULL_RETURN(smartData->api); > + EINA_SAFETY_ON_NULL_RETURN(smartData->api->focus_in); is the api ever supposed to be null? I can understand that some might not connect to focus_in on the other hand Why not just if (smartData->api->focus_in) smartData->api->focus_in(smartData); > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1241 > +void EwkView::onEvasSmartHide(Evas_Object* evasObject) > +{ > + Ewk_View_Smart_Data* smartData = EwkView::smartData(evasObject); > + EINA_SAFETY_ON_NULL_RETURN(smartData); > + > + evas_object_hide(smartData->base.clipper); > + evas_object_hide(smartData->image); > +} looks like we could jsut use static methods (it seems Qt is going in that direction)... I guess almost call can when we have some additional C API's like > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1258 > + evas_object_image_alpha_set(smartData->image, alpha < 255); > + view->page()->setDrawsBackground(red || green || blue); > + view->page()->setDrawsTransparentBackground(alpha < 255); WKViewSetDrawsBackground

Comment on attachment 186595 [details] patch v3 Attachment 186595 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16368674

(In reply to comment #20) > (From update of attachment 186595 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186595&action=review > > > Source/WebKit2/ChangeLog:28 > > + > > + * UIProcess/API/C/efl/WKView.cpp: > > + (WKViewCreate): > > + (WKViewCreateWithFixedLayout): > > + (WKViewGetPage): > > + (WKViewCreateSnapshot): > > + * UIProcess/API/efl/EwkView.cpp: > > + (smartDataChanged): > > + (defaultSmartClassInstance): > > + (EwkViewEventHandler): > > + (EwkViewEventHandler::subscribe): > > + (EwkViewEventHandler::unsubscribe): > > + (::onEvent): > > + (EwkView::EwkView): > > + (EwkView::~EwkView): > > + (EwkView::createEvasObject): > > + (EwkView::initSmartClassInterface): > > + (EwkView::toEvasObject): > > + (EwkView::smartData): > > could use a few words! > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:33 > > - if (!ewkView) > > + Evas_Object* evasObject = EwkView::createEvasObject(canvas, contextRef ? EwkContext::create(contextRef) : EwkContext::defaultContext(), pageGroupRef, EwkView::LegacyBehavior); > > + if (!evasObject) > > what about default page group if no ref is given? This is handled inside WebContext. > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:43 > > - if (!ewkView) > > + Evas_Object* evasObject = EwkView::createEvasObject(canvas, contextRef ? EwkContext::create(contextRef) : EwkContext::defaultContext(), pageGroupRef, EwkView::LegacyBehavior); > > + if (!evasObject) > > return 0; > > Can we make the contextRef code be on a separate line It could. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:172 > > + EINA_SAFETY_ON_NULL_RETURN(smartData->api); > > + EINA_SAFETY_ON_NULL_RETURN(smartData->api->focus_in); > > is the api ever supposed to be null? Actually nope and I think it should be assertion. I can understand that some might not connect to focus_in on the other hand > > Why not just > > if (smartData->api->focus_in) > smartData->api->focus_in(smartData); Agree. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1241 > > +void EwkView::onEvasSmartHide(Evas_Object* evasObject) > > +{ > > + Ewk_View_Smart_Data* smartData = EwkView::smartData(evasObject); > > + EINA_SAFETY_ON_NULL_RETURN(smartData); > > + > > + evas_object_hide(smartData->base.clipper); > > + evas_object_hide(smartData->image); > > +} > > looks like we could jsut use static methods (it seems Qt is going in that direction)... I guess almost call can when we have some additional C API's like those functions are part of the view behaviour and do not make any sense without view, that is why they are part of the class. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1258 > > + evas_object_image_alpha_set(smartData->image, alpha < 255); > > + view->page()->setDrawsBackground(red || green || blue); > > + view->page()->setDrawsTransparentBackground(alpha < 255); > > WKViewSetDrawsBackground Sure, but let's leave it for the next step. The current patch is already big.

Created attachment 186622 [details] patch v4 Took Kenneth's comments into consideration.

Comment on attachment 186622 [details] patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=186622&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:144 > + Evas_Event_Mouse_Down* downEvent = static_cast<Evas_Event_Mouse_Down*>(eventInfo); > + Ewk_View_Smart_Data* smartData = static_cast<Ewk_View_Smart_Data*>(data); > + if (smartData->api->mouse_down) > + smartData->api->mouse_down(smartData, downEvent); > } hmm, you dont need the downEvent if mouse_down doesnt exist Ewk_View_Smart_Data* smartData = toSmartData(data); <- with type assert or similar if (smartData->api->mouse_down) smartData->api->mouse_down(smartData, static_cast<Evas_Event_Mouse_Down*>(event));

Comment on attachment 186622 [details] patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=186622&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:371 > +Ewk_View_Smart_Data* EwkView::smartData(Evas_Object* evasObject) > +{ > + ASSERT(evasObject); > + > + if (!isViewEvasObject(evasObject)) > + return 0; > + > + return static_cast<Ewk_View_Smart_Data*>(evas_object_smart_data_get(evasObject)); > } why isnt this a free function? toSmartData(Evas... > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1129 > + smartData->priv = 0; hmm? comment > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1239 > + Ewk_View_Smart_Data* smartData = EwkView::smartData(evasObject); > + EINA_SAFETY_ON_NULL_RETURN(smartData); these feels like asserts instead... and the smartData probably already does that > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1416 > +bool isViewEvasObject(const Evas_Object* evasObject) isEwkEvasObject( ?

Created attachment 186634 [details] patch v5 Kenneth, thanks for careful review!

Comment on attachment 186634 [details] patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=186634&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:336 > + api.sc.add = handleEvasAdd; > + api.sc.del = handleEvasDelete; I think we should all these handleEvasObjectAdd, like we use EwkView below. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1120 > + smartData->priv = 0; why no comment > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1227 > +void EwkView::handleEvasColorSet(Evas_Object* evasObject, int red, int green, int blue, int alpha) unsigned? > Source/WebKit2/UIProcess/API/efl/EwkView.h:248 > + // Ewk_View_Smart_Class callback interface. Replace . with :

Created attachment 186638 [details] patch v6 Fixed comments above.

(In reply to comment #27) > (From update of attachment 186634 [details]) > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1227 > > +void EwkView::handleEvasColorSet(Evas_Object* evasObject, int red, int green, int blue, int alpha) > > unsigned? > This is callback type from EFL, cannot modify function signature.

Comment on attachment 186638 [details] patch v6 LGTM

Comment on attachment 186638 [details] patch v6 View in context: https://bugs.webkit.org/attachment.cgi?id=186638&action=review > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:32 > +static inline WKViewRef createWKView(Evas* canvas, WKContextRef contextRef, WKPageGroupRef pageGroupRef, EwkView::ViewBehavior behavior) > { > - EwkView* ewkView = ewk_view_base_add(canvas, contextRef, pageGroupRef, EwkView::LegacyBehavior); > - if (!ewkView) > + RefPtr<EwkContext> context = contextRef ? EwkContext::create(contextRef) : EwkContext::defaultContext(); Something bother me about the way you use contextRef. If you are passing a null WKContextRef to an API, you are using it wrong. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:71 > +#include <wtf/Compiler.h> This should not be needed. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1238 > + red = clampTo(red, 0, alpha); > + green = clampTo(green, 0, alpha); > + blue = clampTo(blue, 0, alpha); Why do you clamp color value by alpha? It is a little weird to get colors without color profile. Is that in a default color space? > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1425 > + if (UNLIKELY(!evasSmart)) { > + EINA_LOG_CRIT("%p (%s) is not a smart object!", evasObject, evasObjectType ? evasObjectType : "(null)"); > + return false; > + } > + > + const Evas_Smart_Class* smartClass = evas_smart_class_get(evasSmart); > + if (UNLIKELY(!smartClass)) { > + EINA_LOG_CRIT("%p (%s) is not a smart class object!", evasObject, evasObjectType ? evasObjectType : "(null)"); > + return false; > + } > + > + if (UNLIKELY(smartClass->data != smartClassName)) { > + EINA_LOG_CRIT("%p (%s) is not of an ewk_view (need %p, got %p)!", evasObject, evasObjectType ? evasObjectType : "(null)", > + smartClassName, smartClass->data); > + return false; > + } Unless the branches are creating a hot path, which I highly doubt given the code, UNLIKELY is a bad idea. > Source/WebKit2/UIProcess/API/efl/EwkView.h:226 > + ~EwkView(); // Will be deleted when Evas Object is deleted. This comment is not helping the reader. If you want to document the object lifecycle, make a complete explanation in a visible place (above the class declaration for example). > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:103 > + WKContextRef contextRef = toAPI(page()->process()->context()); > + m_inspectorView = EwkView::createEvasObject(ecore_evas_get(m_inspectorWindow), contextRef ? EwkContext::create(contextRef) : EwkContext::defaultContext(), toAPI(inspectorPageGroup()), EwkView::LegacyBehavior); You get null context our of WebProcessProxy->context()?

> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1238 > > + red = clampTo(red, 0, alpha); > > + green = clampTo(green, 0, alpha); > > + blue = clampTo(blue, 0, alpha); > > Why do you clamp color value by alpha? > It is a little weird to get colors without color profile. Is that in a default color space? This is probably correct. As I remember from EFL times, the color are premultiplied by the alpha or something similar.

> > + RefPtr<EwkContext> context = contextRef ? EwkContext::create(contextRef) : EwkContext::defaultContext(); > > Something bother me about the way you use contextRef. > > If you are passing a null WKContextRef to an API, you are using it wrong. This is temporary till we get the other things cleaned up. This is not exposed in public API. > Unless the branches are creating a hot path, which I highly doubt given the code, UNLIKELY is a bad idea. I agree and already voiced this to Mikhail before.

Btw, thanks for the review Benjamin; we are on it.

(In reply to comment #31) > (From update of attachment 186638 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186638&action=review > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:32 > > +static inline WKViewRef createWKView(Evas* canvas, WKContextRef contextRef, WKPageGroupRef pageGroupRef, EwkView::ViewBehavior behavior) > > { > > - EwkView* ewkView = ewk_view_base_add(canvas, contextRef, pageGroupRef, EwkView::LegacyBehavior); > > - if (!ewkView) > > + RefPtr<EwkContext> context = contextRef ? EwkContext::create(contextRef) : EwkContext::defaultContext(); > > Something bother me about the way you use contextRef. > > If you are passing a null WKContextRef to an API, you are using it wrong.\ I'm not passing null and this is a check for it. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:71 > > +#include <wtf/Compiler.h> > > This should not be needed. yeah, if I remove UNLIKELY > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1238 > > + red = clampTo(red, 0, alpha); > > + green = clampTo(green, 0, alpha); > > + blue = clampTo(blue, 0, alpha); > > Why do you clamp color value by alpha? > It is a little weird to get colors without color profile. Is that in a default color space? This code was there before I just refactored it so that it uses 'clapTo' instead of home-brewed analogue. > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1425 > > + if (UNLIKELY(!evasSmart)) { > > + EINA_LOG_CRIT("%p (%s) is not a smart object!", evasObject, evasObjectType ? evasObjectType : "(null)"); > > + return false; > > + } > > + > > + const Evas_Smart_Class* smartClass = evas_smart_class_get(evasSmart); > > + if (UNLIKELY(!smartClass)) { > > + EINA_LOG_CRIT("%p (%s) is not a smart class object!", evasObject, evasObjectType ? evasObjectType : "(null)"); > > + return false; > > + } > > + > > + if (UNLIKELY(smartClass->data != smartClassName)) { > > + EINA_LOG_CRIT("%p (%s) is not of an ewk_view (need %p, got %p)!", evasObject, evasObjectType ? evasObjectType : "(null)", > > + smartClassName, smartClass->data); > > + return false; > > + } > > Unless the branches are creating a hot path, which I highly doubt given the code, UNLIKELY is a bad idea. Well, agree. this was also inherited from the current implementation however. > > > Source/WebKit2/UIProcess/API/efl/EwkView.h:226 > > + ~EwkView(); // Will be deleted when Evas Object is deleted. > > This comment is not helping the reader. I will elaborate :) > > If you want to document the object lifecycle, make a complete explanation in a visible place (above the class declaration for example). > > > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:103 > > + WKContextRef contextRef = toAPI(page()->process()->context()); > > + m_inspectorView = EwkView::createEvasObject(ecore_evas_get(m_inspectorWindow), contextRef ? EwkContext::create(contextRef) : EwkContext::defaultContext(), toAPI(inspectorPageGroup()), EwkView::LegacyBehavior); > > You get null context our of WebProcessProxy->context()? mmm.. right, think it has to be valid context. I'll check.

Created attachment 186724 [details] patch v7 Took Benjamin's comments into consideration.

Comment on attachment 186724 [details] patch v7 That seems to go in a good direction, I sign off on this for WebKit2.

Comment on attachment 186724 [details] patch v7 r=me as signed off by Benjamin

Comment on attachment 186724 [details] patch v7 Clearing flags on attachment: 186724 Committed r141978: <http://trac.webkit.org/changeset/141978>

All reviewed patches have been landed. Closing bug.