Bug 108062 – [EFL][WK2] Encapsulate Ewk View evas smart object code inside EwkView class

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

SSIA. Part of EwkView refactoring described at https://bugs.webkit.org/show_bug.cgi?id=107662#c1.

[Mikhail Pozdnyakov]

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

[Chris Dumez]

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.

[Kenneth Rohde Christiansen]

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 ?

[Chris Dumez]

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.

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

(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

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

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.

[Kenneth Rohde Christiansen]

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!

[Mikhail Pozdnyakov]

Created attachment 186042 [details]
patch

[Chris Dumez]

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? :)

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

Created attachment 186054 [details]
patch v2

Took comments from Chris into consideration.

[Chris Dumez]

Comment on attachment 186054 [details]
patch v2

LGTM.

[Kenneth Rohde Christiansen]

LGTM, too

[Mikhail Pozdnyakov]

Created attachment 186595 [details]
patch v3

Rebased.

[Kenneth Rohde Christiansen]

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

[Build Bot]

Comment on attachment 186595 [details]
patch v3

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

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

Created attachment 186622 [details]
patch v4

Took Kenneth's comments into consideration.

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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

[Mikhail Pozdnyakov]

Created attachment 186634 [details]
patch v5

Kenneth, thanks for careful review!

[Kenneth Rohde Christiansen]

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 :

[Mikhail Pozdnyakov]

Created attachment 186638 [details]
patch v6

Fixed comments above.

[Mikhail Pozdnyakov]

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

[Kenneth Rohde Christiansen]

Comment on attachment 186638 [details]
patch v6

LGTM

[Benjamin Poulain]

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

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

Created attachment 186724 [details]
patch v7

Took Benjamin's comments into consideration.

[Benjamin Poulain]

Comment on attachment 186724 [details]
patch v7

That seems to go in a good direction, I sign off on this for WebKit2.

[Kenneth Rohde Christiansen]

Comment on attachment 186724 [details]
patch v7

r=me as signed off by Benjamin

[WebKit Review Bot]

Comment on attachment 186724 [details]
patch v7

Clearing flags on attachment: 186724

Committed r141978: <http://trac.webkit.org/changeset/141978>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.