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:

WebKit2

Assignee:

Eunmi Lee <enmi.lee>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, gyuyoung.kim, jinwoo7.song, rakuco, ryuan.choi, sw0524.lee, webkit.review.bot

Priority:

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

Eunmi Lee

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

Attachments
Patch for ewk_view_touchscreen. (10.07 KB, patch) 2012-06-08 01:44 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch for ewk_view_touchscreen. (9.98 KB, patch) 2012-06-11 01:18 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch for ewk_view_touchscreen. (10.86 KB, patch) 2012-06-14 23:18 PDT, Eunmi Lee	gyuyoung.kim: review- gyuyoung.kim: commit-queue-	Details Formatted Diff Diff
Patch for ewk_view_touchscreen. (22.01 KB, patch) 2012-06-26 05:51 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch for ewk_view_touchscreen. (10.36 KB, patch) 2012-06-27 02:18 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch for adding APIs to enable and disable mouse event. (3.96 KB, patch) 2012-07-05 04:05 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch for adding APIs to enable and disable mouse event. (3.97 KB, patch) 2012-07-05 04:12 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch for adding APIs to enable and disable mouse event. (4.02 KB, patch) 2012-07-05 19:39 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch for adding APIs to enable and disable mouse event. (3.78 KB, patch) 2012-07-05 19:51 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch for adding APIs to enable and disable mouse event. (3.78 KB, patch) 2012-07-05 19:52 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch for adding APIs to enable and disable mouse event. (6.00 KB, patch) 2012-07-16 00:07 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch for adding APIs to enable and disable mouse event. (5.97 KB, patch) 2012-07-16 00:57 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (6.31 KB, patch) 2012-08-05 06:17 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (5.40 KB, patch) 2012-08-08 04:44 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (5.41 KB, patch) 2012-08-08 05:53 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (5.43 KB, patch) 2012-08-10 00:54 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (6.74 KB, patch) 2012-09-03 05:56 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (6.67 KB, patch) 2012-09-06 20:04 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Show Obsolete (17) View All Add attachment proposed patch, testcase, etc.

Eunmi Lee

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

Ryuan Choi

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

Eunmi Lee

Comment 3 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 :)

Eunmi Lee

Comment 4 2012-06-11 01:18:20 PDT

Created attachment 146808 [details] Patch for ewk_view_touchscreen.

Ryuan Choi

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

Gyuyoung Kim

Comment 6 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* ?

Eunmi Lee

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

Eunmi Lee

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

Eunmi Lee

Comment 9 2012-06-14 23:18:40 PDT

Created attachment 147743 [details] Patch for ewk_view_touchscreen. Patch which is modified for Bugzilla comments.

Ryuan Choi

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

Eunmi Lee

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

Chris Dumez

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

Gyuyoung Kim

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

Gyuyoung Kim

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

Eunmi Lee

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

Eunmi Lee

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

Eunmi Lee

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

Eunmi Lee

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

Eunmi Lee

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

Eunmi Lee

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

Eunmi Lee

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

Eunmi Lee

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

Eunmi Lee

Comment 23 2012-07-05 19:52:32 PDT

Created attachment 151012 [details] Patch for adding APIs to enable and disable mouse event.

Chris Dumez

Comment 24 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).

Eunmi Lee

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

Eunmi Lee

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

Chris Dumez

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

Gyuyoung Kim

Comment 28 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;

Eunmi Lee

Comment 29 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".

Eunmi Lee

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

Eunmi Lee

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

Chris Dumez

Comment 32 2012-07-16 01:02:20 PDT

Comment on attachment 152494 [details] Patch for adding APIs to enable and disable mouse event. LGTM.

Gyuyoung Kim

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

Jinwoo Song

Comment 34 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?

Eunmi Lee

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

Eunmi Lee

Comment 36 2012-08-05 06:17:32 PDT

Created attachment 156557 [details] Patch

Ryuan Choi

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

Eunmi Lee

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

Eunmi Lee

Comment 39 2012-08-08 04:44:51 PDT

Created attachment 157181 [details] Patch

Eunmi Lee

Comment 40 2012-08-08 05:53:08 PDT

Created attachment 157196 [details] Patch

Ryuan Choi

Comment 41 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?

Gyuyoung Kim

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

Eunmi Lee

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

Eunmi Lee

Comment 44 2012-08-10 00:54:07 PDT

Created attachment 157662 [details] Patch

Ryuan Choi

Comment 45 2012-08-14 05:05:23 PDT

(In reply to comment #44) > Created an attachment (id=157662) [details] > Patch Thank you. Looks fine now.

Gyuyoung Kim

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

Eunmi Lee

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

Eunmi Lee

Comment 48 2012-09-03 05:56:48 PDT

Created attachment 161909 [details] Patch

Gyuyoung Kim

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

Eunmi Lee

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

Gyuyoung Kim

Comment 51 2012-09-06 19:30:43 PDT

Comment on attachment 161909 [details] Patch If so, looks fine.

WebKit Review Bot

Comment 52 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

Eunmi Lee

Comment 53 2012-09-06 20:04:54 PDT

Created attachment 162659 [details] Patch

WebKit Review Bot

Comment 54 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>

WebKit Review Bot

Comment 55 2012-09-07 01:27:53 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.