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.
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 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.
(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 :)
Created attachment 146808 [details] Patch for ewk_view_touchscreen.
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 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* ?
(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.
(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.
Created attachment 147743 [details] Patch for ewk_view_touchscreen. Patch which is modified for Bugzilla comments.
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.
(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 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 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.
(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.
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.
(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.
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.
Created attachment 149711 [details] Patch for ewk_view_touchscreen. This patch has to be applied after Bug 90054.
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.
Created attachment 150921 [details] Patch for adding APIs to enable and disable mouse event. Add missing "EAPI" to the APIs.
Created attachment 151010 [details] Patch for adding APIs to enable and disable mouse event. The patch is rebased.
Created attachment 151011 [details] Patch for adding APIs to enable and disable mouse event. remove unnecessary <WebCore/NotImplemented.h> line.
Created attachment 151012 [details] Patch for adding APIs to enable and disable mouse event.
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).
(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.
Created attachment 152488 [details] Patch for adding APIs to enable and disable mouse event. Modified for Christophe's comment.
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 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;
(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".
(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.
Created attachment 152494 [details] Patch for adding APIs to enable and disable mouse event. Modified for Christophe and Gyuyoung's comment.
Comment on attachment 152494 [details] Patch for adding APIs to enable and disable mouse event. LGTM.
Comment on attachment 152494 [details] Patch for adding APIs to enable and disable mouse event. Looks good to me now, thanks.
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?
(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.
Created attachment 156557 [details] Patch
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 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.
Created attachment 157181 [details] Patch
Created attachment 157196 [details] Patch
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 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 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.
Created attachment 157662 [details] Patch
(In reply to comment #44) > Created an attachment (id=157662) [details] > Patch Thank you. Looks fine now.
Comment on attachment 157662 [details] Patch Eunmi, could you add unit test as well? Nowadays, public APIs are being added with unit test.
(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.
Created attachment 161909 [details] Patch
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 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 on attachment 161909 [details] Patch If so, looks fine.
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
Created attachment 162659 [details] Patch
Comment on attachment 162659 [details] Patch Clearing flags on attachment: 162659 Committed r127842: <http://trac.webkit.org/changeset/127842>
All reviewed patches have been landed. Closing bug.