Bug 88631

Summary: [EFL][WK2] Add APIs to enable or disable the mouse events of the ewk_view.
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit2Assignee: Eunmi Lee <enmi.lee>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, jinwoo7.song, rakuco, ryuan.choi, sw0524.lee, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 84124    
Bug Blocks: 61838, 90662    
Attachments:
Description Flags
Patch for ewk_view_touchscreen.
none
Patch for ewk_view_touchscreen.
none
Patch for ewk_view_touchscreen.
gyuyoung.kim: review-, gyuyoung.kim: commit-queue-
Patch for ewk_view_touchscreen.
none
Patch for ewk_view_touchscreen.
none
Patch for adding APIs to enable and disable mouse event.
none
Patch for adding APIs to enable and disable mouse event.
none
Patch for adding APIs to enable and disable mouse event.
none
Patch for adding APIs to enable and disable mouse event.
none
Patch for adding APIs to enable and disable mouse event.
none
Patch for adding APIs to enable and disable mouse event.
none
Patch for adding APIs to enable and disable mouse event.
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Eunmi Lee 2012-06-08 01:31:48 PDT
I want to add ewk_view_touchscreen Evas_Object which inherits ewk_view and has touchscreen behavior.

The touchscreen behavior means that we can pan, flick and zoom the view by touching the device's screen,
and touch event can be sent to the webpage which consumes touch event using javascript.
The legacy behavior means that view is controlled by mouse. It is traditional behavior.
Comment 1 Eunmi Lee 2012-06-08 01:44:53 PDT
Created attachment 146507 [details]
Patch for ewk_view_touchscreen.

This patch is in progress, because it is based on Bug 84124 which is not reviewed yet.

I will re-base and update patch again after Bug 84124 is merged.
Comment 2 Ryuan Choi 2012-06-10 16:14:32 PDT
Comment on attachment 146507 [details]
Patch for ewk_view_touchscreen.

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:141
> +EAPI void ewk_view_touchscreen_enabled_set(Eina_Bool enable);

WebKit/Efl is using enable_get.
Comment 3 Eunmi Lee 2012-06-10 23:43:37 PDT
(In reply to comment #2)
> (From update of attachment 146507 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146507&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:141
> > +EAPI void ewk_view_touchscreen_enabled_set(Eina_Bool enable);
> 
> WebKit/Efl is using enable_get.

OK, I will follow WebKit/EFL style :)
Comment 4 Eunmi Lee 2012-06-11 01:18:20 PDT
Created attachment 146808 [details]
Patch for ewk_view_touchscreen.
Comment 5 Ryuan Choi 2012-06-11 23:14:50 PDT
Comment on attachment 146808 [details]
Patch for ewk_view_touchscreen.

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:165
> +EAPI Eina_Bool ewk_view_smart_class_init(Ewk_View_Smart_Class *api);

WebKit/Efl have similar API, ewk_view_base_smart_set.

Isn't it better to make them similar (or same)?

> Source/WebKit2/UIProcess/API/efl/ewk_view_touchscreen.cpp:23
> +#include "NotImplemented.h"

<WebCore/NotImplemented.h> looks better.
Comment 6 Gyuyoung Kim 2012-06-11 23:46:59 PDT
Comment on attachment 146808 [details]
Patch for ewk_view_touchscreen.

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:101
> +static bool g_enableTouchscreen = false;

AFAIK, g_ is not WebKit coding style. If I am this patch owner, I would implement this by using enum type.

For example,
  enum Mode { TouchScreenMode, NormalMode };

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:151
> + * Sets the smart class api, enabling view to be inherited.

s/api/API/g

> Source/WebKit2/UIProcess/API/efl/ewk_view_touchscreen.cpp:35
> +    // The ewk_view_touchscreen does not support mouse wheel event.

Using FIXME:

> Source/WebKit2/UIProcess/API/efl/ewk_view_touchscreen.cpp:42
> +    return true;

Is *false* better than *true* ?
Comment 7 Eunmi Lee 2012-06-14 05:35:34 PDT
(In reply to comment #5)
> (From update of attachment 146808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146808&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:165
> > +EAPI Eina_Bool ewk_view_smart_class_init(Ewk_View_Smart_Class *api);
> 
> WebKit/Efl have similar API, ewk_view_base_smart_set.
> 
> Isn't it better to make them similar (or same)?

I think ewk_view_smart_class_set() is better, so I will change to that.
I don't want to add "base" to the name because ewk_view is not a base object in the WebKit2. (it is base in the WebKit1)
and, I agree that "set" is better than "init" :)

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_touchscreen.cpp:23
> > +#include "NotImplemented.h"
> 
> <WebCore/NotImplemented.h> looks better.

OK, I will change to that.
Comment 8 Eunmi Lee 2012-06-14 05:47:26 PDT
(In reply to comment #6)
> (From update of attachment 146808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146808&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:101
> > +static bool g_enableTouchscreen = false;
> 
> AFAIK, g_ is not WebKit coding style. If I am this patch owner, I would implement this by using enum type.
> 
> For example,
>   enum Mode { TouchScreenMode, NormalMode };
> 

OK, I will try to change static variable using DEFINE_STATIC_LOCAL() and enum. :)

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:151
> > + * Sets the smart class api, enabling view to be inherited.
> 
> s/api/API/g
>

OK.
 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_touchscreen.cpp:35
> > +    // The ewk_view_touchscreen does not support mouse wheel event.
> 
> Using FIXME:

OK.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_touchscreen.cpp:42
> > +    return true;
> 
> Is *false* better than *true* ?

Yes, right. false is better. I will change to false.
Comment 9 Eunmi Lee 2012-06-14 23:18:40 PDT
Created attachment 147743 [details]
Patch for ewk_view_touchscreen.

Patch which is modified for Bugzilla comments.
Comment 10 Ryuan Choi 2012-06-19 23:09:07 PDT
Comment on attachment 147743 [details]
Patch for ewk_view_touchscreen.

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:173
> +EAPI Eina_Bool ewk_view_smart_class_set(Ewk_View_Smart_Class *api);

Basically, this is way to go.

However, we can not inherit ewk_view although this is exposed.
It's because ewk_view_base_add have some logic to initialize.

In order to expose this successfully,
I believe that we should move initialization logic from ewk_view_base_add to _ewk_view_smart_add.
Comment 11 Eunmi Lee 2012-06-22 20:19:08 PDT
(In reply to comment #10)
> (From update of attachment 147743 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147743&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:173
> > +EAPI Eina_Bool ewk_view_smart_class_set(Ewk_View_Smart_Class *api);
> 
> Basically, this is way to go.
> 
> However, we can not inherit ewk_view although this is exposed.
> It's because ewk_view_base_add have some logic to initialize.
> 
> In order to expose this successfully,
> I believe that we should move initialization logic from ewk_view_base_add to _ewk_view_smart_add.

Thank you for your comment :)

I will upload patch again after changing codes to support inheritance correctly.
Comment 12 Chris Dumez 2012-06-25 04:41:54 PDT
Comment on attachment 147743 [details]
Patch for ewk_view_touchscreen.

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:103
> +    static Ewk_View_Type ewkViewType = EWK_VIEW_TYPE_LEGACY;

Do we really want to default to legacy? LEGACY sounds like "deprecated" and it feels weird that it is the default.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:139
> + * is EWK_VIEW_TYPE_LEGACY or legacy behavior if type is EWK_VIEW_TYPE_TOUCHSCREEN.

Comment is wrong. It reverses legacy and touchscreen.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:153
> + *

We should document which type is the default one.

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:34
> +const char ewkViewLegacyName[] = "Ewk_View_Legacy";

Should be static.

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:35
> +const char ewkViewTouchscreenName[] = "Ewk_View_Touchscreen";

Ditto.
Comment 13 Gyuyoung Kim 2012-06-25 20:56:31 PDT
Comment on attachment 147743 [details]
Patch for ewk_view_touchscreen.

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

Set '-' because above comments.

> Source/WebKit2/UIProcess/API/efl/ewk_view_touchscreen.cpp:29
> +using namespace WebCore;

I wonder why do you add WebCore namespace. I can't find WebCore usage in this file.

> Source/WebKit2/UIProcess/API/efl/ewk_view_touchscreen.cpp:31
> +static Ewk_View_Smart_Class g_parentSmartClass = EWK_VIEW_SMART_CLASS_INIT_NULL;

g_ is GTK port style. Please keep to consistent with WK1 EFL style.
Comment 14 Gyuyoung Kim 2012-06-25 21:00:22 PDT
(In reply to comment #13)
> (From update of attachment 147743 [details])
> > Source/WebKit2/UIProcess/API/efl/ewk_view_touchscreen.cpp:29
> > +using namespace WebCore;
> 
> I wonder why do you add WebCore namespace. I can't find WebCore usage in this file.

Oops, this comment needs to check further. I can't check it on my local pc because of my new office environment now.

And, the reason to set r- because we need to reduce pending reviewed patches.
Comment 15 Eunmi Lee 2012-06-26 05:40:13 PDT
Thanks for your comment.

(In reply to comment #12)
> (From update of attachment 147743 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147743&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:103
> > +    static Ewk_View_Type ewkViewType = EWK_VIEW_TYPE_LEGACY;
> 
> Do we really want to default to legacy? LEGACY sounds like "deprecated" and it feels weird that it is the default.
> 
I want to default to legacy because I think webkit2-efl is usually tested on the desktop PC and TOUCHSCREEN mode has to be implemented more.
By the way, does LEGACY sound like "deprecated"?
I considered for name and chose LEGACY like webkit2-qt, but I'm not sure it is good name.
Would you recommend word instead of "legacy"?

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:139
> > + * is EWK_VIEW_TYPE_LEGACY or legacy behavior if type is EWK_VIEW_TYPE_TOUCHSCREEN.
> 
> Comment is wrong. It reverses legacy and touchscreen.

It's my mistake. I will fix it.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:153
> > + *
> 
> We should document which type is the default one.

Yes. I will add.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:34
> > +const char ewkViewLegacyName[] = "Ewk_View_Legacy";
> 
> Should be static.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:35
> > +const char ewkViewTouchscreenName[] = "Ewk_View_Touchscreen";
> 
> Ditto.

I will fix it.
Comment 16 Eunmi Lee 2012-06-26 05:41:58 PDT
(In reply to comment #13)
> (From update of attachment 147743 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147743&action=review
> 
> Set '-' because above comments.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_touchscreen.cpp:29
> > +using namespace WebCore;
> 
> I wonder why do you add WebCore namespace. I can't find WebCore usage in this file.

Yes right. I will remove it.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_touchscreen.cpp:31
> > +static Ewk_View_Smart_Class g_parentSmartClass = EWK_VIEW_SMART_CLASS_INIT_NULL;
> 
> g_ is GTK port style. Please keep to consistent with WK1 EFL style.

ok, so I will replace g_parentSmartClass to _parentSmartClass.
Comment 17 Eunmi Lee 2012-06-26 05:51:21 PDT
Created attachment 149517 [details]
Patch for ewk_view_touchscreen.

I've made new patch, but I think it is better to divide patch into patch for inheritance and patch for touchscreen.

So, I will update patch again.
Comment 18 Eunmi Lee 2012-06-27 02:18:46 PDT
Created attachment 149711 [details]
Patch for ewk_view_touchscreen.

This patch has to be applied after Bug 90054.
Comment 19 Eunmi Lee 2012-07-05 04:05:29 PDT
Created attachment 150918 [details]
Patch for adding APIs to enable and disable mouse event.

Add APIs to enable and disable the mouse event instead of adding ewk_view_touchscreen.
I will make patch for touch event as a separated patch.
Comment 20 Eunmi Lee 2012-07-05 04:12:00 PDT
Created attachment 150921 [details]
Patch for adding APIs to enable and disable mouse event.

Add missing "EAPI" to the APIs.
Comment 21 Eunmi Lee 2012-07-05 19:39:03 PDT
Created attachment 151010 [details]
Patch for adding APIs to enable and disable mouse event.

The patch is rebased.
Comment 22 Eunmi Lee 2012-07-05 19:51:50 PDT
Created attachment 151011 [details]
Patch for adding APIs to enable and disable mouse event.

remove unnecessary <WebCore/NotImplemented.h> line.
Comment 23 Eunmi Lee 2012-07-05 19:52:32 PDT
Created attachment 151012 [details]
Patch for adding APIs to enable and disable mouse event.
Comment 24 Chris Dumez 2012-07-13 07:44:27 PDT
Comment on attachment 151012 [details]
Patch for adding APIs to enable and disable mouse event.

Is there a good reason to use a static variable instead of making it a per-view setting? a per-view setting would feel more consistent and would be more flexible (e.g. if you want to temporarily disable mouse events for the view during an operation).
Comment 25 Eunmi Lee 2012-07-15 22:47:12 PDT
(In reply to comment #24)
> (From update of attachment 151012 [details])
> Is there a good reason to use a static variable instead of making it a per-view setting? a per-view setting would feel more consistent and would be more flexible (e.g. if you want to temporarily disable mouse events for the view during an operation).

I added static variable to enable/disable the mouse event before creating view, because the event callbacks are added when view is created.

Well, now that you mention it, I can enable/disable the mouse event per-view by adding and deleting the event callbacks.

I will try to modify like that.
Comment 26 Eunmi Lee 2012-07-16 00:07:29 PDT
Created attachment 152488 [details]
Patch for adding APIs to enable and disable mouse event.

Modified for Christophe's comment.
Comment 27 Chris Dumez 2012-07-16 00:19:36 PDT
Comment on attachment 152488 [details]
Patch for adding APIs to enable and disable mouse event.

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

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Add APIs to enable or disable the mouse event of the ewk_view.

"the mouse event" -> "mouse events"

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:53
> +    bool isMouseEventEnabled;

"areMouseEventsEnabled" would be better.
Comment 28 Gyuyoung Kim 2012-07-16 00:25:20 PDT
Comment on attachment 152488 [details]
Patch for adding APIs to enable and disable mouse event.

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:405
> + * @return @c EINA_TRUE on success or @c EINA_FALSE on failure

Don't you need to mention EINA_FALSE can be returned when current status is same with "enabled" status.

973    if (priv->isMouseEventEnabled == enabled)
974        return false;
Comment 29 Eunmi Lee 2012-07-16 00:43:52 PDT
(In reply to comment #27)
> (From update of attachment 152488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152488&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        [EFL][WK2] Add APIs to enable or disable the mouse event of the ewk_view.
> 
> "the mouse event" -> "mouse events"
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:53
> > +    bool isMouseEventEnabled;
> 
> "areMouseEventsEnabled" would be better.

This comment is for English grammar. Thanks.
I will change all "event" to "events".
Comment 30 Eunmi Lee 2012-07-16 00:46:47 PDT
(In reply to comment #28)
> (From update of attachment 152488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152488&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:405
> > + * @return @c EINA_TRUE on success or @c EINA_FALSE on failure
> 
> Don't you need to mention EINA_FALSE can be returned when current status is same with "enabled" status.
> 
> 973    if (priv->isMouseEventEnabled == enabled)
> 974        return false;

I've thought about that again, and I think #973 line is not a failure case.
So, I think it is better to return true in this case.
I will fix the code instead of modifying comments.
Comment 31 Eunmi Lee 2012-07-16 00:57:35 PDT
Created attachment 152494 [details]
Patch for adding APIs to enable and disable mouse event.

Modified for Christophe and Gyuyoung's comment.
Comment 32 Chris Dumez 2012-07-16 01:02:20 PDT
Comment on attachment 152494 [details]
Patch for adding APIs to enable and disable mouse event.

LGTM.
Comment 33 Gyuyoung Kim 2012-07-16 01:08:07 PDT
Comment on attachment 152494 [details]
Patch for adding APIs to enable and disable mouse event.

Looks good to me now, thanks.
Comment 34 Jinwoo Song 2012-07-26 01:55:48 PDT
I like this patch, but actually, some touch devices use the mouse wheel event to scroll the screen. So it is not unusual to enable the wheel event in the touch device. In my thought, it seems better to enable and disable the mouse events related to down, move and up except the wheel. How do you think about this?
Comment 35 Eunmi Lee 2012-08-05 06:02:48 PDT
(In reply to comment #34)
> I like this patch, but actually, some touch devices use the mouse wheel event to scroll the screen. So it is not unusual to enable the wheel event in the touch device. In my thought, it seems better to enable and disable the mouse events related to down, move and up except the wheel. How do you think about this?

Thanks for your comment :)

I agree with you and the wheel event don't have to be disabled for touch.
I will modify the patch for that.
Comment 36 Eunmi Lee 2012-08-05 06:17:32 PDT
Created attachment 156557 [details]
Patch
Comment 37 Ryuan Choi 2012-08-05 20:10:33 PDT
Comment on attachment 156557 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:412
> +    if (smartData->priv->areMouseEventsEnabled)
> +        _ewk_view_mouse_events_connect(smartData);

Should we need to check smartData->priv->areMouseEventsEnabled ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1357
> +    if (enabled)
> +        _ewk_view_mouse_events_connect(smartData);
> +    else
> +        _ewk_view_mouse_events_disconnect(smartData);

How do you think about merging these into one internal function such as _ewk_view_mouse_events_callbacks_change ?

If then, priv->areMouseEventsEnabled can be checked only one time in _ewk_view_mouse_events_callbacks_change.
Comment 38 Eunmi Lee 2012-08-08 04:25:04 PDT
Comment on attachment 156557 [details]
Patch

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

Thanks, I will upload new patch again!

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:412
>> +        _ewk_view_mouse_events_connect(smartData);
> 
> Should we need to check smartData->priv->areMouseEventsEnabled ?

If I merge the code to one function, I can remove checking code.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1357
>> +        _ewk_view_mouse_events_disconnect(smartData);
> 
> How do you think about merging these into one internal function such as _ewk_view_mouse_events_callbacks_change ?
> 
> If then, priv->areMouseEventsEnabled can be checked only one time in _ewk_view_mouse_events_callbacks_change.

I understand that you want to reduce the number of code lines.
then, I can do everything in this function and remove internal functions.
Comment 39 Eunmi Lee 2012-08-08 04:44:51 PDT
Created attachment 157181 [details]
Patch
Comment 40 Eunmi Lee 2012-08-08 05:53:08 PDT
Created attachment 157196 [details]
Patch
Comment 41 Ryuan Choi 2012-08-08 07:06:39 PDT
Comment on attachment 157196 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:404
> +    ewk_view_mouse_events_enabled_set(ewkView, true);
>  #define CONNECT(s, c) evas_object_event_callback_add(ewkView, s, c, smartData)

Isn't it better to add empty line?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1434
> +#define CONNECT(s, c) evas_object_event_callback_add(ewkView, s, c, smartData)
> +        CONNECT(EVAS_CALLBACK_MOUSE_DOWN, _ewk_view_on_mouse_down);
> +        CONNECT(EVAS_CALLBACK_MOUSE_UP, _ewk_view_on_mouse_up);
> +        CONNECT(EVAS_CALLBACK_MOUSE_MOVE, _ewk_view_on_mouse_move);
> +#undef CONNECT
> +    } else {
> +#define DISCONNECT(s, c) evas_object_event_callback_del(ewkView, s, c)
> +        DISCONNECT(EVAS_CALLBACK_MOUSE_DOWN, _ewk_view_on_mouse_down);
> +        DISCONNECT(EVAS_CALLBACK_MOUSE_UP, _ewk_view_on_mouse_up);
> +        DISCONNECT(EVAS_CALLBACK_MOUSE_MOVE, _ewk_view_on_mouse_move);
> +#undef DISCONNECT

I know that CONNECT has been used, but readability looks not good to me.
If we don't have any reason, how about calling functions without macro?
Comment 42 Gyuyoung Kim 2012-08-08 17:54:08 PDT
Comment on attachment 157196 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1434
>> +#undef DISCONNECT
> 
> I know that CONNECT has been used, but readability looks not good to me.
> If we don't have any reason, how about calling functions without macro?

Frankly, if this macro can be used by other functions, this is good to use this macro. But, current implementation just undefines this macro as soon as this is used. So, I also think we don't need to use CONNECT and DISCONNECT macro.
Comment 43 Eunmi Lee 2012-08-08 20:08:39 PDT
Comment on attachment 157196 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:404
>>  #define CONNECT(s, c) evas_object_event_callback_add(ewkView, s, c, smartData)
> 
> Isn't it better to add empty line?

OK, I will fix it.

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1434
>>> +#undef DISCONNECT
>> 
>> I know that CONNECT has been used, but readability looks not good to me.
>> If we don't have any reason, how about calling functions without macro?
> 
> Frankly, if this macro can be used by other functions, this is good to use this macro. But, current implementation just undefines this macro as soon as this is used. So, I also think we don't need to use CONNECT and DISCONNECT macro.

Yes, I just use that macro because that style is used in the ewk_view.cpp in the WK1 and WK2.
If it is not necessary to use, I can use function name directly without macro.
Comment 44 Eunmi Lee 2012-08-10 00:54:07 PDT
Created attachment 157662 [details]
Patch
Comment 45 Ryuan Choi 2012-08-14 05:05:23 PDT
(In reply to comment #44)
> Created an attachment (id=157662) [details]
> Patch

Thank you.

Looks fine now.
Comment 46 Gyuyoung Kim 2012-08-23 08:12:13 PDT
Comment on attachment 157662 [details]
Patch

Eunmi, could you add unit test as well? Nowadays, public APIs are being added with unit test.
Comment 47 Eunmi Lee 2012-09-03 04:12:35 PDT
(In reply to comment #46)
> (From update of attachment 157662 [details])
> Eunmi, could you add unit test as well? Nowadays, public APIs are being added with unit test.

sure, I will prepare tests for this patch.
Comment 48 Eunmi Lee 2012-09-03 05:56:48 PDT
Created attachment 161909 [details]
Patch
Comment 49 Gyuyoung Kim 2012-09-03 18:01:10 PDT
Comment on attachment 161909 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1545
> +Eina_Bool ewk_view_mouse_events_enabled_set(Evas_Object* ewkView, Eina_Bool enabled)

WK1 efl uses _enable_set / _get.
Comment 50 Eunmi Lee 2012-09-06 19:27:24 PDT
Comment on attachment 161909 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1545
>> +Eina_Bool ewk_view_mouse_events_enabled_set(Evas_Object* ewkView, Eina_Bool enabled)
> 
> WK1 efl uses _enable_set / _get.

I told about that with Ryuan but I didn't add comments in this bug.

Actually, WK1 efl uses both _enable_set/get and _enabled_set/get.
so, I decided to use _enabled_set/get because it is more similar with WebKit style.
Comment 51 Gyuyoung Kim 2012-09-06 19:30:43 PDT
Comment on attachment 161909 [details]
Patch

If so, looks fine.
Comment 52 WebKit Review Bot 2012-09-06 19:43:55 PDT
Comment on attachment 161909 [details]
Patch

Rejecting attachment 161909 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
es).
1 out of 4 hunks FAILED -- saving rejects to file Source/WebKit2/UIProcess/API/efl/ewk_view.cpp.rej
patching file Source/WebKit2/UIProcess/API/efl/ewk_view.h
Hunk #1 succeeded at 630 (offset 10 lines).
patching file Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp
Hunk #2 succeeded at 266 with fuzz 2 (offset 29 lines).

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

Full output: http://queues.webkit.org/results/13776490
Comment 53 Eunmi Lee 2012-09-06 20:04:54 PDT
Created attachment 162659 [details]
Patch
Comment 54 WebKit Review Bot 2012-09-07 01:27:47 PDT
Comment on attachment 162659 [details]
Patch

Clearing flags on attachment: 162659

Committed r127842: <http://trac.webkit.org/changeset/127842>
Comment 55 WebKit Review Bot 2012-09-07 01:27:53 PDT
All reviewed patches have been landed.  Closing bug.