Bug 108062 - [EFL][WK2] Encapsulate Ewk View evas smart object code inside EwkView class
Summary: [EFL][WK2] Encapsulate Ewk View evas smart object code inside EwkView class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on:
Blocks: 107657 107662
  Show dependency treegraph
 
Reported: 2013-01-28 00:59 PST by Mikhail Pozdnyakov
Modified: 2013-02-06 01:56 PST (History)
9 users (show)

See Also:


Attachments
WIP patch (57.60 KB, patch)
2013-01-30 14:10 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
WIP 2 (66.78 KB, patch)
2013-02-01 04:43 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch (72.70 KB, patch)
2013-02-01 07:23 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v2 (67.34 KB, patch)
2013-02-01 08:29 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v3 (68.47 KB, patch)
2013-02-05 04:21 PST, Mikhail Pozdnyakov
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch v4 (69.30 KB, patch)
2013-02-05 07:27 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v5 (71.88 KB, patch)
2013-02-05 08:54 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v6 (72.10 KB, patch)
2013-02-05 09:08 PST, Mikhail Pozdnyakov
benjamin: review-
Details | Formatted Diff | Diff
patch v7 (72.04 KB, patch)
2013-02-05 16:34 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2013-01-28 00:59:57 PST
SSIA. Part of EwkView refactoring described at https://bugs.webkit.org/show_bug.cgi?id=107662#c1.
Comment 1 Mikhail Pozdnyakov 2013-01-30 14:10:54 PST
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 2 Chris Dumez 2013-01-31 01:00:08 PST
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 3 Kenneth Rohde Christiansen 2013-01-31 01:19:33 PST
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 4 Chris Dumez 2013-01-31 06:27:55 PST
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.
Comment 5 Mikhail Pozdnyakov 2013-01-31 06:34:20 PST
(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 :)
Comment 6 Mikhail Pozdnyakov 2013-01-31 11:57:18 PST
(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..
Comment 7 Mikhail Pozdnyakov 2013-01-31 12:08:31 PST
(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
Comment 8 Mikhail Pozdnyakov 2013-01-31 13:33:20 PST
(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.
Comment 9 Mikhail Pozdnyakov 2013-01-31 13:38:29 PST
> > 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 :) ).
Comment 10 Mikhail Pozdnyakov 2013-02-01 04:43:53 PST
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 11 Mikhail Pozdnyakov 2013-02-01 04:46:00 PST
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 12 Kenneth Rohde Christiansen 2013-02-01 05:05:41 PST
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!
Comment 13 Mikhail Pozdnyakov 2013-02-01 07:23:01 PST
Created attachment 186042 [details]
patch
Comment 14 Chris Dumez 2013-02-01 07:50:17 PST
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? :)
Comment 15 Mikhail Pozdnyakov 2013-02-01 08:24:54 PST
(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.
Comment 16 Mikhail Pozdnyakov 2013-02-01 08:29:08 PST
Created attachment 186054 [details]
patch v2

Took comments from Chris into consideration.
Comment 17 Chris Dumez 2013-02-01 08:45:20 PST
Comment on attachment 186054 [details]
patch v2

LGTM.
Comment 18 Kenneth Rohde Christiansen 2013-02-01 08:54:06 PST
LGTM, too
Comment 19 Mikhail Pozdnyakov 2013-02-05 04:21:45 PST
Created attachment 186595 [details]
patch v3

Rebased.
Comment 20 Kenneth Rohde Christiansen 2013-02-05 05:34:53 PST
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 21 Build Bot 2013-02-05 05:43:19 PST
Comment on attachment 186595 [details]
patch v3

Attachment 186595 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16368674
Comment 22 Mikhail Pozdnyakov 2013-02-05 06:56:58 PST
(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.
Comment 23 Mikhail Pozdnyakov 2013-02-05 07:27:07 PST
Created attachment 186622 [details]
patch v4

Took Kenneth's comments into consideration.
Comment 24 Kenneth Rohde Christiansen 2013-02-05 07:40:29 PST
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 25 Kenneth Rohde Christiansen 2013-02-05 07:48:34 PST
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( ?
Comment 26 Mikhail Pozdnyakov 2013-02-05 08:54:36 PST
Created attachment 186634 [details]
patch v5

Kenneth, thanks for careful review!
Comment 27 Kenneth Rohde Christiansen 2013-02-05 09:00:08 PST
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 :
Comment 28 Mikhail Pozdnyakov 2013-02-05 09:08:06 PST
Created attachment 186638 [details]
patch v6

Fixed comments above.
Comment 29 Mikhail Pozdnyakov 2013-02-05 09:09:43 PST
(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 30 Kenneth Rohde Christiansen 2013-02-05 09:12:17 PST
Comment on attachment 186638 [details]
patch v6

LGTM
Comment 31 Benjamin Poulain 2013-02-05 15:33:33 PST
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()?
Comment 32 Kenneth Rohde Christiansen 2013-02-05 15:35:43 PST
> > 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.
Comment 33 Kenneth Rohde Christiansen 2013-02-05 15:39:01 PST
> > +    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.
Comment 34 Kenneth Rohde Christiansen 2013-02-05 15:47:43 PST
Btw, thanks for the review Benjamin; we are on it.
Comment 35 Mikhail Pozdnyakov 2013-02-05 16:22:30 PST
(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.
Comment 36 Mikhail Pozdnyakov 2013-02-05 16:34:01 PST
Created attachment 186724 [details]
patch v7

Took Benjamin's comments into consideration.
Comment 37 Benjamin Poulain 2013-02-05 21:23:19 PST
Comment on attachment 186724 [details]
patch v7

That seems to go in a good direction, I sign off on this for WebKit2.
Comment 38 Kenneth Rohde Christiansen 2013-02-06 00:28:13 PST
Comment on attachment 186724 [details]
patch v7

r=me as signed off by Benjamin
Comment 39 WebKit Review Bot 2013-02-06 01:56:32 PST
Comment on attachment 186724 [details]
patch v7

Clearing flags on attachment: 186724

Committed r141978: <http://trac.webkit.org/changeset/141978>
Comment 40 WebKit Review Bot 2013-02-06 01:56:41 PST
All reviewed patches have been landed.  Closing bug.