Bug 102643

Summary:

[EFL][WK2] Implement gesture recognizer.

Product:

WebKit

Reporter:

Eunmi Lee <enmi.lee>

Component:

WebKit EFL

Assignee:

Eunmi Lee <enmi.lee>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, cdumez, commit-queue, d-r, gyuyoung.kim, gyuyoung.kim, hugo.lima, kenneth, lauro.neto, lucas.de.marchi, luciano.wolf, marcelo.lira, mikhail.pozdnyakov, nick.diego, rakuco, rniwa, ryuan.choi, thiago.lacerda, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

110085

Bug Blocks:

106864, 107101, 111702

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2	none
Patch	none
Patch	none
Patch	none
Patch	none

Eunmi Lee

Reported 2012-11-18 22:16:04 PST

I will make patch to add gesture_start, gesture_end and gesture_move APIs to the ewk_view's smart class. We can tap, pan, flick and zoom using gesture APIs when TILED_BACKING_STORE is enabled.

Attachments
Patch (13.36 KB, patch) 2012-11-21 23:15 PST, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (13.37 KB, patch) 2012-11-23 00:04 PST, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (13.42 KB, patch) 2012-12-05 22:21 PST, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (35.30 KB, patch) 2013-01-14 00:51 PST, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (36.98 KB, patch) 2013-01-14 03:16 PST, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (36.99 KB, patch) 2013-01-15 02:14 PST, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (37.67 KB, patch) 2013-01-20 22:56 PST, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (37.62 KB, patch) 2013-01-23 00:51 PST, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (34.65 KB, patch) 2013-02-18 23:23 PST, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (34.65 KB, patch) 2013-03-04 22:17 PST, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (20.21 KB, patch) 2013-03-15 04:13 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (26.02 KB, patch) 2013-03-19 01:25 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (25.94 KB, patch) 2013-07-03 22:53 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (25.88 KB, patch) 2013-07-23 18:52 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (26.14 KB, patch) 2013-07-25 02:28 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (24.97 KB, patch) 2013-07-25 23:11 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (602.92 KB, application/zip) 2013-07-26 12:43 PDT, Build Bot	no flags	Details
Patch (21.18 KB, patch) 2013-07-28 23:11 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (20.67 KB, patch) 2013-07-29 06:55 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (20.66 KB, patch) 2013-07-29 19:53 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (24.00 KB, patch) 2013-07-30 03:46 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (1.31 MB, application/zip) 2013-07-30 04:10 PDT, Build Bot	no flags	Details
Patch (23.95 KB, patch) 2013-07-30 05:25 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (23.98 KB, patch) 2013-07-30 06:56 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (24.30 KB, patch) 2013-08-01 02:39 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (20.34 KB, patch) 2013-08-02 04:11 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Show Obsolete (23) View All Add attachment proposed patch, testcase, etc.

Eunmi Lee

Comment 1 2012-11-21 23:15:29 PST

Created attachment 175595 [details] Patch

Eunmi Lee

Comment 2 2012-11-23 00:04:57 PST

Created attachment 175740 [details] Patch

Gyuyoung Kim

Comment 3 2012-12-04 00:25:55 PST

Comment on attachment 175740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175740&action=review > Source/WebKit2/UIProcess/API/efl/GestureHandler.cpp:44 > + UNUSED_PARAM(event); WebKit prefers to remove parameter name if there is no macro inside function. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:237 > +#else Need to add UNUSED_PARAM > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:248 > +#else ditto.

Gyuyoung Kim

Comment 4 2012-12-04 00:26:31 PST

CC'ing Kenneth, Kenneth, how do you think about this patch ?

Kenneth Rohde Christiansen

Comment 5 2012-12-04 00:35:23 PST

Comment on attachment 175740 [details] Patch Huh? It is impossible to see how this is supposed to evolve. Gestures for interacting with the web view need to work together and cannot be looked upon as fully separate gestures. I am not convinced this is the right direction

Eunmi Lee

Comment 6 2012-12-04 18:40:05 PST

(In reply to comment #5) > (From update of attachment 175740 [details]) > Huh? It is impossible to see how this is supposed to evolve. Gestures for interacting with the web view need to work together and cannot be looked upon as fully separate gestures. I am not convinced this is the right direction 1. I understand your question is "why gestures are fed using smart API from outside?". The main reason of that is to support "C++ API layer of Tizen"(Bada). If we merge the C++ layer, the ewk_view can not get the event directly and all events are fed by C++ layer. So, we need route to feed gestures to the ewk_view. 2. Are you worried about to interact with W3C touch events? We can enable and disable the gestures for condition of touch events' preventDefault. 3. I have a plan to make bugs for each gesture after merging this patch. but do you think it is good to add all codes in this bug for understanding? If so, I will add all codes about gestures here.

Eunmi Lee

Comment 7 2012-12-05 20:51:30 PST

Comment on attachment 175740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175740&action=review >> Source/WebKit2/UIProcess/API/efl/GestureHandler.cpp:44 >> + UNUSED_PARAM(event); > > WebKit prefers to remove parameter name if there is no macro inside function. OK, I will remove it. >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:237 >> +#else > > Need to add UNUSED_PARAM Thanks I will add it. >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:248 >> +#else > > ditto. ditto

Eunmi Lee

Comment 8 2012-12-05 22:21:13 PST

Created attachment 177938 [details] Patch

Dominik Röttsches (drott)

Comment 9 2013-01-10 09:09:59 PST

What I am concerned about, and I think that's also what Kenneth partially means: Can you combine multiple gestures? Can you for example send two "start" signals, one for PAN, one for PINCH? Or can you transition between them? On a touchscreen, PAN is one finger, PINCH would be two finger, so they're separate. But if you imagine a trackpad, one finger usually means pointer movement, two finger means PAN but can also mean PINCH - how can you transition between the two or change from one to the other? Or do you have to decide for example at the beginning of the gesture wheter it's going to be PAN or PINCH? Kenneth: Which gestures do you think need to "work together"?

Eunmi Lee

Comment 10 2013-01-13 23:57:42 PST

(In reply to comment #9) > What I am concerned about, and I think that's also what Kenneth partially means: Can you combine multiple gestures? Can you for example send two "start" signals, one for PAN, one for PINCH? Or can you transition between them? > > On a touchscreen, PAN is one finger, PINCH would be two finger, so they're separate. But if you imagine a trackpad, one finger usually means pointer movement, two finger means PAN but can also mean PINCH - how can you transition between the two or change from one to the other? Or do you have to decide for example at the beginning of the gesture wheter it's going to be PAN or PINCH? > > Kenneth: Which gestures do you think need to "work together"? The main idea of this patch is that "all things for recognizing the gesture are done outside of the web view". That means, the web view does not have to know the number of touch points and decide current state is Pan or Pinch and the web view just processes the gesture APIs when they are called. Answering for your question - "two finger means PAN but can also mean PINCH", I think your question is that you want to scroll the page when we move the two finger together within same distance and zoom the page when we change the distance between fingers. That can be processed in the PINCH state. we can process scroll or zoom for each two finger's movement. By the way, I've decide to change this patch. I think it is not efficient to depend on applications or libraries outside of web view to recognize gestures, so I am going to make "gesture recognizer" inside webkit2 efl port (like webkit2 qt port) to provide same behavior for touches to applications and libraries which use webkit2 efl.

Eunmi Lee

Comment 11 2013-01-14 00:03:26 PST

(In reply to comment #10) > (In reply to comment #9) > > What I am concerned about, and I think that's also what Kenneth partially means: Can you combine multiple gestures? Can you for example send two "start" signals, one for PAN, one for PINCH? Or can you transition between them? > > > > On a touchscreen, PAN is one finger, PINCH would be two finger, so they're separate. But if you imagine a trackpad, one finger usually means pointer movement, two finger means PAN but can also mean PINCH - how can you transition between the two or change from one to the other? Or do you have to decide for example at the beginning of the gesture wheter it's going to be PAN or PINCH? > > > > Kenneth: Which gestures do you think need to "work together"? > > The main idea of this patch is that "all things for recognizing the gesture are done outside of the web view". > That means, the web view does not have to know the number of touch points and decide current state is Pan or Pinch and the web view just processes the gesture APIs when they are called. > > Answering for your question - "two finger means PAN but can also mean PINCH", > I think your question is that you want to scroll the page when we move the two finger together within same distance and zoom the page when we change the distance between fingers. > That can be processed in the PINCH state. we can process scroll or zoom for each two finger's movement. > > By the way, > I've decide to change this patch. > I think it is not efficient to depend on applications or libraries outside of web view to recognize gestures, > so I am going to make "gesture recognizer" inside webkit2 efl port (like webkit2 qt port) to provide same behavior for touches to applications and libraries which use webkit2 efl. Additionally, I thought that the web view can not recognize gesture with C++ API layer of Tizen because web view ca not get the raw event directly in that case, but it is possible to recognize gesture if we use Touch Events of webkit instead of raw event from Evas.

Eunmi Lee

Comment 12 2013-01-14 00:51:36 PST

Created attachment 182520 [details] Patch

Eunmi Lee

Comment 13 2013-01-14 01:21:43 PST

Comment on attachment 182520 [details] Patch This patch is in a work-in-process.

Eunmi Lee

Comment 14 2013-01-14 03:16:02 PST

Created attachment 182537 [details] Patch

Eunmi Lee

Comment 15 2013-01-15 02:14:27 PST

Created attachment 182722 [details] Patch Change to use screen position instead of contents position because contents can be changed before getting touch events result.

Kenneth Rohde Christiansen

Comment 16 2013-01-18 03:15:16 PST

Comment on attachment 182722 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182722&action=review > Source/WebKit2/PlatformEfl.cmake:89 > UIProcess/efl/FormClientEfl.cpp > + UIProcess/efl/GestureHandler.cpp > + UIProcess/efl/GestureRecognizer.cpp > UIProcess/efl/InputMethodContextEfl.cpp Are these WebKit2 dependent? Can we somehow in the future share code with Qt? > Source/WebKit2/UIProcess/efl/GestureRecognizer.cpp:2 > + * Copyright (C) 2013 Samsung Electronics. All rights reserved. This code looks to be inspired by the Qt code...

Kenneth Rohde Christiansen

Comment 17 2013-01-18 04:01:02 PST

(In reply to comment #15) > Created an attachment (id=182722) [details] > Patch > > Change to use screen position instead of contents position because contents can be changed before getting touch events result. Be aware that screen positions might be in actually device positions or UI positions.

Kenneth Rohde Christiansen

Comment 18 2013-01-18 04:03:07 PST

> Kenneth: Which gestures do you think need to "work together"? Almost all :-) 1 finger -> pan, tap, double tap 2 fingers -> pinch include move, possible turn into pan when fingers are very close, otherwise do scale and move at the same time. 3 fingers -> possible tread as pan You should be able to move from say pan to pinch, and the other way around

Eunmi Lee

Comment 19 2013-01-20 20:08:08 PST

(In reply to comment #16) > (From update of attachment 182722 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182722&action=review > > > Source/WebKit2/PlatformEfl.cmake:89 > > UIProcess/efl/FormClientEfl.cpp > > + UIProcess/efl/GestureHandler.cpp > > + UIProcess/efl/GestureRecognizer.cpp > > UIProcess/efl/InputMethodContextEfl.cpp > > Are these WebKit2 dependent? Can we somehow in the future share code with Qt? > Some codes are depends on WebKit2 - such as TouchEvents class (WebPlatformTouchPoint, NativeWebTouchEvent) They can be changed to use common structure and shared with WebKit1, but I think I don't have to implement gesture for WebKit1 EFL now and gesture codes can be refactored when WebKit1 EFL wants to use that. For sharing code with Qt, I think some codes to recognize the gestures can be shared with Qt, but it is not easy to make it because: - Qt port is using Qt's data structures, so they have to be changed to use common structures. - Qt is using QQuickFlickable and QQuickItem for flicking and scaling, but EFL does not have such libraries, so caller codes have to be refactored to support both Qt and EFL. So, codes can be shared after refactoring in the future. > > Source/WebKit2/UIProcess/efl/GestureRecognizer.cpp:2 > > + * Copyright (C) 2013 Samsung Electronics. All rights reserved. > > This code looks to be inspired by the Qt code... Yes, right. I have to add Copyright for Nokia. :)

Eunmi Lee

Comment 20 2013-01-20 20:11:20 PST

(In reply to comment #17) > (In reply to comment #15) > > Created an attachment (id=182722) [details] [details] > > Patch > > > > Change to use screen position instead of contents position because contents can be changed before getting touch events result. > > Be aware that screen positions might be in actually device positions or UI positions. OK, thank you. :)

Eunmi Lee

Comment 21 2013-01-20 22:56:42 PST

Created attachment 183716 [details] Patch Add Nokia in the copyright of gesture files.

Eunmi Lee

Comment 22 2013-01-23 00:51:14 PST

Created attachment 184174 [details] Patch Rebase patch

Antonio Gomes

Comment 23 2013-01-27 19:49:25 PST

Comment on attachment 184174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184174&action=review Some nits: > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:150 > + WebKit::GestureRecognizer* gestureRecognizer() { return m_gestureRecognizer.get(); } should be const? > Source/WebKit2/UIProcess/efl/GestureRecognizer.cpp:62 > + for (int i = 0; i < numberOfTouchPoints; ++i) you lack {} wrapping 'for'

Eunmi Lee

Comment 24 2013-02-17 23:58:28 PST

Comment on attachment 184174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184174&action=review >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:150 >> + WebKit::GestureRecognizer* gestureRecognizer() { return m_gestureRecognizer.get(); } > > should be const? Thanks for your review :) I will update this patch for changed Efl port and apply your comments.

Chris Dumez

Comment 25 2013-02-18 00:00:25 PST

Comment on attachment 184174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184174&action=review >>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:150 >>> + WebKit::GestureRecognizer* gestureRecognizer() { return m_gestureRecognizer.get(); } >> >> should be const? > > Thanks for your review :) > I will update this patch for changed Efl port and apply your comments. Actually, it was discussed on the mailing list that we should not make the getter const if we return a non-const pointer. I think the current patch is correct.

Chris Dumez

Comment 26 2013-02-18 00:03:39 PST

I was referring to https://lists.webkit.org/pipermail/webkit-dev/2012-October/022576.html

Mikhail Pozdnyakov

Comment 27 2013-02-18 01:32:26 PST

Comment on attachment 184174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184174&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:76 > +#if ENABLE(TOUCH_EVENTS) && USE(TILED_BACKING_STORE) USE(TILED_BACKING_STORE) is not needed since coord graphics code path is always used in EFL WK2. > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:149 > +#if USE(TILED_BACKING_STORE) ditto >>>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:150 >>>> + WebKit::GestureRecognizer* gestureRecognizer() { return m_gestureRecognizer.get(); } >>> >>> should be const? >> >> Thanks for your review :) >> I will update this patch for changed Efl port and apply your comments. > > Actually, it was discussed on the mailing list that we should not make the getter const if we return a non-const pointer. I think the current patch is correct. Agree with Chris, that should not be const indeed. > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:286 > +#if USE(TILED_BACKING_STORE) should be removed as well, and everywhere within the patch > Source/WebKit2/UIProcess/efl/GestureHandler.h:32 > +#include <wtf/Vector.h> think it could be also forward declared > Source/WebKit2/UIProcess/efl/GestureHandler.h:52 > + GestureHandler(EwkViewImpl*); explicit :) > Source/WebKit2/UIProcess/efl/GestureRecognizer.cpp:72 > + } else if (event.type() == WebEvent::TouchMove) { "switch case" would look better imho > Source/WebKit2/UIProcess/efl/PanGestureHandler.cpp:57 > + if (points.size() < 1) { ==0? > Source/WebKit2/UIProcess/efl/PanGestureHandler.h:40 > +class PanGestureHandler : private GestureHandler { why do you need private inheritance here? > Source/WebKit2/UIProcess/efl/PinchGestureHandler.h:40 > +class PinchGestureHandler : private GestureHandler { and here > Source/WebKit2/UIProcess/efl/TapGestureHandler.h:39 > +class TapGestureHandler : private GestureHandler { and here

Mikhail Pozdnyakov

Comment 28 2013-02-18 01:49:12 PST

Comment on attachment 184174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184174&action=review > Source/WebKit2/UIProcess/efl/GestureRecognizer.cpp:107 > + if (m_currentGesture == TapGesture) what about current handler instead? > Source/WebKit2/UIProcess/efl/GestureRecognizer.h:76 > + TapGestureHandler m_tapGestureHandler; oh, so why than they need common base interface??? if they are all declared and used with their own types

Mikhail Pozdnyakov

Comment 29 2013-02-18 02:00:19 PST

Comment on attachment 184174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184174&action=review > Source/WebKit2/UIProcess/efl/GestureRecognizer.h:55 > + virtual ~GestureRecognizer() { } is anyone inherited from GestureRecognizer? guess it should not need virtual > Source/WebKit2/UIProcess/efl/GestureRecognizer.h:74 > + Gesture m_currentGesture; think we don't need it. >> Source/WebKit2/UIProcess/efl/GestureRecognizer.h:76 >> + TapGestureHandler m_tapGestureHandler; > > oh, so why than they need common base interface??? if they are all declared and used with their own types to be more clear: they all(including Gesture m_currentGesture) could be replaced with OwnPtr<GestureHandler> m_currentHandler;

Chris Dumez

Comment 30 2013-02-18 03:15:57 PST

Comment on attachment 184174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184174&action=review > Source/WebKit2/UIProcess/efl/PanGestureHandler.cpp:47 > + if (points.size() < 1) { if (points.isEmpty()) >> Source/WebKit2/UIProcess/efl/PanGestureHandler.cpp:57 >> + if (points.size() < 1) { > > ==0? == 0 would be against WebKit coding style :P I think this should be simply: if (points.isEmpty())

Mikhail Pozdnyakov

Comment 31 2013-02-18 06:05:35 PST

(In reply to comment #30) > (From update of attachment 184174 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184174&action=review > > > Source/WebKit2/UIProcess/efl/PanGestureHandler.cpp:47 > > + if (points.size() < 1) { > > if (points.isEmpty()) > > >> Source/WebKit2/UIProcess/efl/PanGestureHandler.cpp:57 > >> + if (points.size() < 1) { > > > > ==0? > > == 0 would be against WebKit coding style :P > I think this should be simply: if (points.isEmpty()) Here I meant just that it's better to be compared to '0' (not empty) in principal. But good that you pointed out how to do it according to style guidelines :)

Eunmi Lee

Comment 32 2013-02-18 18:03:12 PST

Comment on attachment 184174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184174&action=review Mikhail and Christophe, thanks for comments :) I will update patch for changed EFL port and comments. >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:76 >> +#if ENABLE(TOUCH_EVENTS) && USE(TILED_BACKING_STORE) > > USE(TILED_BACKING_STORE) is not needed since coord graphics code path is always used in EFL WK2. Yes, I will remove all USE(TILED_BACKING_STORE). >>>>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:150 >>>>> + WebKit::GestureRecognizer* gestureRecognizer() { return m_gestureRecognizer.get(); } >>>> >>>> should be const? >>> >>> Thanks for your review :) >>> I will update this patch for changed Efl port and apply your comments. >> >> Actually, it was discussed on the mailing list that we should not make the getter const if we return a non-const pointer. I think the current patch is correct. > > Agree with Chris, that should not be const indeed. Ok, thanks. >> Source/WebKit2/UIProcess/efl/GestureHandler.h:32 >> +#include <wtf/Vector.h> > > think it could be also forward declared right. >> Source/WebKit2/UIProcess/efl/GestureHandler.h:52 >> + GestureHandler(EwkViewImpl*); > > explicit :) oh, I missed that :) >> Source/WebKit2/UIProcess/efl/GestureRecognizer.cpp:62 >> + for (int i = 0; i < numberOfTouchPoints; ++i) > > you lack {} wrapping 'for' I will add. >> Source/WebKit2/UIProcess/efl/GestureRecognizer.cpp:72 >> + } else if (event.type() == WebEvent::TouchMove) { > > "switch case" would look better imho yes, I agree. >> Source/WebKit2/UIProcess/efl/GestureRecognizer.cpp:107 >> + if (m_currentGesture == TapGesture) > > what about current handler instead? ok, I will think about name again. >> Source/WebKit2/UIProcess/efl/GestureRecognizer.h:55 >> + virtual ~GestureRecognizer() { } > > is anyone inherited from GestureRecognizer? guess it should not need virtual Nobody inherits this class yet. I will remove virtual. >>> Source/WebKit2/UIProcess/efl/GestureRecognizer.h:76 >>> + TapGestureHandler m_tapGestureHandler; >> >> oh, so why than they need common base interface??? if they are all declared and used with their own types > > to be more clear: they all(including Gesture m_currentGesture) could be replaced with OwnPtr<GestureHandler> m_currentHandler; Yes, I will do refactoring all about this. >>> Source/WebKit2/UIProcess/efl/PanGestureHandler.cpp:47 >>> + if (points.size() < 1) { >> >> if (points.isEmpty()) > > Here I meant just that it's better to be compared to '0' (not empty) in principal. But good that you pointed out how to do it according to style guidelines :) oh, I have to use isEmpty() :) >> Source/WebKit2/UIProcess/efl/PanGestureHandler.h:40 >> +class PanGestureHandler : private GestureHandler { > > why do you need private inheritance here? I think I wanted to hide GestureHandler's variables, but I don't have to use private because variables are protected.

Eunmi Lee

Comment 33 2013-02-18 23:23:05 PST

Created attachment 189001 [details] Patch Rebase and update for comments.

Eunmi Lee

Comment 34 2013-03-04 22:17:52 PST

Created attachment 191401 [details] Patch Rebased.

Eunmi Lee

Comment 35 2013-03-15 04:13:34 PDT

Created attachment 193276 [details] Patch Modified gesture recognizer's logic like NIX webkit.

Kenneth Rohde Christiansen

Comment 36 2013-03-18 03:32:07 PDT

Comment on attachment 193276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193276&action=review LGTM > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:56 > + (this->*m_state)(type, WKTouchEventGetTouchPoints(eventRef)); Maybe s/m_state/m_eventHandlerFn/ > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:69 > + static const double s_doubleTapTimeout; > + static const double s_tapAndHoldTimeout; > + static const int s_squaredDoubleTapThreshold = 10000; > + static const int s_squaredPanThreshold = 100; why are some declared here and others not? and what are the units? MS = milli seconds ...TimeoutMillis (or what is common to use in WebCore)

Mikhail Pozdnyakov

Comment 37 2013-03-18 05:29:51 PDT

Comment on attachment 193276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193276&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.h:102 > +class EwkView : public WebKit::GestureRecognizerClient { we usually have clients decoupled from view.. > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:48 > + virtual void handlePinchStarted(const Vector<WebCore::IntPoint>*) { } why pointer and not reference? can it be null? > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:49 > + virtual void handlePinch(const Vector<WebCore::IntPoint>*) { } ditto.

Eunmi Lee

Comment 38 2013-03-19 00:04:59 PDT

Comment on attachment 193276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193276&action=review >> Source/WebKit2/UIProcess/API/efl/EwkView.h:102 >> +class EwkView : public WebKit::GestureRecognizerClient { > > we usually have clients decoupled from view.. Yes, I will separate GestureRecognizerClient :) >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:56 >> + (this->*m_state)(type, WKTouchEventGetTouchPoints(eventRef)); > > Maybe s/m_state/m_eventHandlerFn/ I agree that m_state is ambiguous. how about m_recognizerFunction? because functions are recognizing gestures :) >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:48 >> + virtual void handlePinchStarted(const Vector<WebCore::IntPoint>*) { } > > why pointer and not reference? can it be null? hmm, It should not be null. I will change it to reference. >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:49 >> + virtual void handlePinch(const Vector<WebCore::IntPoint>*) { } > > ditto. ditto. >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:69 >> + static const int s_squaredPanThreshold = 100; > > why are some declared here and others not? and what are the units? MS = milli seconds > > ...TimeoutMillis (or what is common to use in WebCore) It can be confusing because I defined only integer variables here. so, I will modify to declare all static const variables here and define all of them in the cpp file :) and units is seconds, so I will rename to TimeoutInSeconds.

Eunmi Lee

Comment 39 2013-03-19 01:25:29 PDT

Created attachment 193753 [details] Patch Update for comments.

Mikhail Pozdnyakov

Comment 40 2013-03-19 01:41:15 PDT

Comment on attachment 193753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193753&action=review > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:41 > +public: virtual destructor missing.

Eunmi Lee

Comment 41 2013-07-03 22:53:10 PDT

Created attachment 206052 [details] Patch Add virtual destructor for GestureRecognizerClient and rebased.

Eunmi Lee

Comment 42 2013-07-23 18:52:53 PDT

Created attachment 207367 [details] Patch Rebase.

Gyuyoung Kim

Comment 43 2013-07-23 19:21:25 PDT

Comment on attachment 207367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207367&action=review > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:129 > + case kWKEventTypeTouchMove: { It would be good if you keep consistency to use { } for case state. > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:151 > + default: Isn't it use ASSERT_NOT_REACHED() ?

Eunmi Lee

Comment 44 2013-07-25 00:26:25 PDT

Comment on attachment 207367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207367&action=review >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:129 >> + case kWKEventTypeTouchMove: { > > It would be good if you keep consistency to use { } for case state. I think that { } for cast statement is usually used only if it is necessary. (for example, WebPageProxy::didReceiveEvent()) So, I will keep this codes. >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:151 >> + default: > > Isn't it use ASSERT_NOT_REACHED() ? We can reach here if type is kWKEventTypeTouchCancel. So, I will add case for kWKEventTypeTouchCancel and add ASSERT_NOT_REACHED() for default case.

Eunmi Lee

Comment 45 2013-07-25 02:28:38 PDT

Created attachment 207443 [details] Patch Update for comments and modify createVectorWithWKArray to fix the crash.

Gyuyoung Kim

Comment 46 2013-07-25 05:05:35 PDT

Comment on attachment 207443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207443&action=review > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:116 > + default: ASSERT_NOT_REACHED(); ? > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:131 > + if (m_firstPressedPoint.distanceSquaredToPoint(currentPoint) > s_squaredPanThreshold) { Isn't it better to add a function to check threshold as nix port's implementation ? > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:170 > + if (m_firstPressedPoint.distanceSquaredToPoint(toIntPoint(getPointAtIndex(touchPoints, 0))) > s_squaredDoubleTapThreshold) ditto. > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:180 > + if (m_firstPressedPoint.distanceSquaredToPoint(currentPoint) > s_squaredPanThreshold) { ditto. > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:40 > +class GestureRecognizerClient { Should we have virtual class ? Do you have a plan to support other port in efl ?

Eunmi Lee

Comment 47 2013-07-25 20:04:14 PDT

Comment on attachment 207443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207443&action=review >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:116 >> + default: > > ASSERT_NOT_REACHED(); ? Yes, I will check again and add ASSERT_NOT_REACHED() to the whole file. >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:131 >> + if (m_firstPressedPoint.distanceSquaredToPoint(currentPoint) > s_squaredPanThreshold) { > > Isn't it better to add a function to check threshold as nix port's implementation ? ok, I will make separated function for that. >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:40 >> +class GestureRecognizerClient { > > Should we have virtual class ? Do you have a plan to support other port in efl ? Actually no, but I think it can be shared with other port later. anyway, it is not necessary now, so I will remove this virtual class.

Eunmi Lee

Comment 48 2013-07-25 23:11:08 PDT

Created attachment 207509 [details] Patch Update for Gyuyoung's comments.

Ryuan Choi

Comment 49 2013-07-26 00:15:38 PDT

Comment on attachment 207509 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207509&action=review > Source/WebKit2/UIProcess/API/efl/GestureRecognizerClient.h:44 > +class GestureRecognizerClient { I am confusing what this class can do. Will this class have the logic by itself? If this class would just forward the handles to EwkView, we can make this as interface for EwkView to inherit.

Ryuan Choi

Comment 50 2013-07-26 00:34:15 PDT

Gyuyoung Kim

Comment 51 2013-07-26 00:38:42 PDT

(In reply to comment #49) > (From update of attachment 207509 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207509&action=review > > > Source/WebKit2/UIProcess/API/efl/GestureRecognizerClient.h:44 > > +class GestureRecognizerClient { > > I am confusing what this class can do. > Will this class have the logic by itself? > > If this class would just forward the handles to EwkView, we can make this as interface for EwkView to inherit. It looks Eunmi added it to be aligned with nix port as Eunmi said before. So, virtual class was added as well. If there is no need to be aligned with nix port, it looks we don't need to use Client class. Eunmi, is my assumption right? what do you think about it ?

Eunmi Lee

Comment 52 2013-07-26 01:24:28 PDT

Comment on attachment 207509 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207509&action=review >>> Source/WebKit2/UIProcess/API/efl/GestureRecognizerClient.h:44 >>> +class GestureRecognizerClient { >> >> I am confusing what this class can do. >> Will this class have the logic by itself? >> >> If this class would just forward the handles to EwkView, we can make this as interface for EwkView to inherit. > > It looks Eunmi added it to be aligned with nix port as Eunmi said before. So, virtual class was added as well. If there is no need to be aligned with nix port, it looks we don't need to use Client class. Eunmi, is my assumption right? what do you think about it ? Ryuan and Gyuyoung, thanks for comments. As Ryuan's suggestion, GestureRecognizerClient codes can be moved to the EwkView. However, I'm afraid to add many things to the EwkView. Do you think that is ok? If so, I will merge GestureRecognizerClient codes to the EwkView.

Build Bot

Comment 53 2013-07-26 12:43:14 PDT

Comment on attachment 207509 [details] Patch Attachment 207509 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1253285 New failing tests: editing/selection/leak-document-with-selection-inside.html

Build Bot

Comment 54 2013-07-26 12:43:19 PDT

Created attachment 207547 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

Eunmi Lee

Comment 55 2013-07-28 22:07:25 PDT

(In reply to comment #52) > (From update of attachment 207509 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207509&action=review > > >>> Source/WebKit2/UIProcess/API/efl/GestureRecognizerClient.h:44 > >>> +class GestureRecognizerClient { > >> > >> I am confusing what this class can do. > >> Will this class have the logic by itself? > >> > >> If this class would just forward the handles to EwkView, we can make this as interface for EwkView to inherit. > > > > It looks Eunmi added it to be aligned with nix port as Eunmi said before. So, virtual class was added as well. If there is no need to be aligned with nix port, it looks we don't need to use Client class. Eunmi, is my assumption right? what do you think about it ? > > Ryuan and Gyuyoung, thanks for comments. > As Ryuan's suggestion, GestureRecognizerClient codes can be moved to the EwkView. > However, I'm afraid to add many things to the EwkView. Do you think that is ok? If so, I will merge GestureRecognizerClient codes to the EwkView. As I discuss with Ryuan, I will move codes to the EwkView.

Eunmi Lee

Comment 56 2013-07-28 23:11:09 PDT

Created attachment 207619 [details] Patch Move GestureRecognizerClient to the EwkView.

Ryuan Choi

Comment 57 2013-07-29 06:47:32 PDT

Comment on attachment 207619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207619&action=review > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:122 > + ASSERT(m_tapAndHoldTimer); It looks wrong.

Eunmi Lee

Comment 58 2013-07-29 06:50:14 PDT

(In reply to comment #57) > (From update of attachment 207619 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207619&action=review > > > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:122 > > + ASSERT(m_tapAndHoldTimer); > > It looks wrong. My mistake. I will fix that.

Eunmi Lee

Comment 59 2013-07-29 06:55:41 PDT

Created attachment 207650 [details] Patch As discussing with Ryuan, change GestureRecognizerClient to GestureRecognizer::Client and change its functions to pure virtual function.

Mikhail Pozdnyakov

Comment 60 2013-07-29 07:12:26 PDT

Comment on attachment 207650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207650&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1417 > +void EwkView::handlePan(const IntPoint& point) no need for 'point' so far.. > Source/WebKit2/UIProcess/API/efl/EwkView.h:105 > + : public WebKit::GestureRecognizer::Client I don't like this inheritance, clients are usually just owned by view. Can we follow standard pattern? > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:42 > + class Client { nested type.. :/ Why do we need client? what if GestureRecognizer was aware of EwkView instead (like others clients do)? > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:70 > + static const int s_squaredPanThreshold; two variables above can be defined right here.

Eunmi Lee

Comment 61 2013-07-29 19:49:27 PDT

Comment on attachment 207650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207650&action=review >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1417 >> +void EwkView::handlePan(const IntPoint& point) > > no need for 'point' so far.. ok, I will remove point. >> Source/WebKit2/UIProcess/API/efl/EwkView.h:105 >> + : public WebKit::GestureRecognizer::Client > > I don't like this inheritance, clients are usually just owned by view. Can we follow standard pattern? Clients owned by view and GestureRecognizer::Client are different. 1. "Clients owned by view" are WKClient type and they can not be inherited, so EwkView has to own them. 2. "GestureRecognizer::Client" is to get the gestures from GestureRecognizer and the EwkView is the main body to handle each gestures, so it is better to inherit Client directly. We already use this kind of Client in the CoreIPC::Connection::Client, WebPopupMenuProxy::Client, DownloadManager::Client and so on. As a minor reason, GestureRecognizer::Client is not the type of WKClient, so If it is separated Client owned by EwkView, it can be confused with WKClients. So, I want to distinguish GestureRecognizer::Client with WKClients. >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:42 >> + class Client { > > nested type.. :/ Why do we need client? what if GestureRecognizer was aware of EwkView instead (like others clients do)? Why do you hate neted type? we are already using nested type in the webkit, so I 'm curious. Client makes codes structural and easy to understand the connection between EwkView and GestureRecognizer, so I think it is better than calling EwkView's functions directly. >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:70 >> + static const int s_squaredPanThreshold; > > two variables above can be defined right here. I moved them to the cpp file by comment #36 for consistency.

Eunmi Lee

Comment 62 2013-07-29 19:53:38 PDT

Created attachment 207683 [details] Patch Remove parameter name from EwkView::handlePan().

Mikhail Pozdnyakov

Comment 63 2013-07-30 02:15:39 PDT

(In reply to comment #61) > (From update of attachment 207650 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207650&action=review > > >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1417 > >> +void EwkView::handlePan(const IntPoint& point) > > > > no need for 'point' so far.. > > ok, I will remove point. > > >> Source/WebKit2/UIProcess/API/efl/EwkView.h:105 > >> + : public WebKit::GestureRecognizer::Client > > > > I don't like this inheritance, clients are usually just owned by view. Can we follow standard pattern? > > Clients owned by view and GestureRecognizer::Client are different. > 1. "Clients owned by view" are WKClient type and they can not be inherited, so EwkView has to own them. > 2. "GestureRecognizer::Client" is to get the gestures from GestureRecognizer and the EwkView is the main body to handle each gestures, so it is better to inherit Client directly. > We already use this kind of Client in the CoreIPC::Connection::Client, WebPopupMenuProxy::Client, DownloadManager::Client and so on. > As a minor reason, GestureRecognizer::Client is not the type of WKClient, so If it is separated Client owned by EwkView, it can be confused with WKClients. > So, I want to distinguish GestureRecognizer::Client with WKClients. > > >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:42 > >> + class Client { > > > > nested type.. :/ Why do we need client? what if GestureRecognizer was aware of EwkView instead (like others clients do)? > > Why do you hate neted type? we are already using nested type in the webkit, so I 'm curious. No, I don't hate them :) I find however that in this case it just brings complexity and I don't see benefit (so far) in having them. Introducing of a new interface would make sense if we had say several clients, now I see only EwkView.

Eunmi Lee

Comment 64 2013-07-30 02:42:16 PDT

Comment on attachment 207650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207650&action=review >>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1417 >>>> +void EwkView::handlePan(const IntPoint& point) >>> >>> no need for 'point' so far.. >> >> ok, I will remove point. > > No, I don't hate them :) I find however that in this case it just brings complexity and I don't see benefit (so far) in having them. > Introducing of a new interface would make sense if we had say several clients, now I see only EwkView. Actually, I thought GestureRecognizer can be used by other ports later. However, we use that only in the EFL port and it is even in the efl folder now. So, I will modify codes to use EwkView directly now.

Mikhail Pozdnyakov

Comment 65 2013-07-30 02:46:13 PDT

Comment on attachment 207683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207683&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1397 > +void EwkView::handleSingleTap(const IntPoint&) it's hard to say now since all the methods are empty, but is it really necessary to have them in view? I'd prefer having logic encapsulated within a dedicated object than making view do everything.

Ryuan Choi

Comment 66 2013-07-30 03:01:36 PDT

(In reply to comment #65) > (From update of attachment 207683 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207683&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1397 > > +void EwkView::handleSingleTap(const IntPoint&) > > it's hard to say now since all the methods are empty, but is it really necessary to have them in view? I'd prefer having logic encapsulated within a dedicated object than making view do everything. I found GestureHandler until 10th patch, but 11th patch refactored it to match Nix port. (see #35) I also think that dedicated object like GestureHandler (or any better name?) looks better.

Eunmi Lee

Comment 67 2013-07-30 03:02:58 PDT

(In reply to comment #66) > (In reply to comment #65) > > (From update of attachment 207683 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=207683&action=review > > > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1397 > > > +void EwkView::handleSingleTap(const IntPoint&) > > > > it's hard to say now since all the methods are empty, but is it really necessary to have them in view? I'd prefer having logic encapsulated within a dedicated object than making view do everything. > > I found GestureHandler until 10th patch, but 11th patch refactored it to match Nix port. (see #35) > > I also think that dedicated object like GestureHandler (or any better name?) looks better. I agree with you. EwkView is getting bigger, so it is better to have separated object. I will make GestureHandler to delegate function for gestures.

Eunmi Lee

Comment 68 2013-07-30 03:46:25 PDT

Created attachment 207714 [details] Patch Update for comments.

Build Bot

Comment 69 2013-07-30 04:09:55 PDT

Comment on attachment 207714 [details] Patch Attachment 207714 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1288346 New failing tests: editing/selection/leak-document-with-selection-inside.html

Build Bot

Comment 70 2013-07-30 04:10:04 PDT

Created attachment 207717 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

Eunmi Lee

Comment 71 2013-07-30 05:25:45 PDT

Created attachment 207722 [details] Patch Rebase.

Eunmi Lee

Comment 72 2013-07-30 06:56:39 PDT

Created attachment 207732 [details] Patch Fix wrong usage of Vector in the createVectorWithWKArray().

Gyuyoung Kim

Comment 73 2013-07-31 19:14:37 PDT

Comment on attachment 207732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207732&action=review Looks better otherwise. > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:53 > + static const double s_doubleTapTimeoutInSeconds; Is there any reason these static variables should be declared as public ? It looks these variables are being used as private.

Eunmi Lee

Comment 74 2013-08-01 02:33:42 PDT

(In reply to comment #73) > (From update of attachment 207732 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207732&action=review > > Looks better otherwise. > > > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:53 > > + static const double s_doubleTapTimeoutInSeconds; > > Is there any reason these static variables should be declared as public ? It looks these variables are being used as private. No, I will make them private.

Eunmi Lee

Comment 75 2013-08-01 02:39:01 PDT

Created attachment 207909 [details] Patch Make static variables private and do not assert for TouchMove and TouchEnd in the noGesture because they can be called after long press.

Gyuyoung Kim

Comment 76 2013-08-01 02:49:38 PDT

Comment on attachment 207909 [details] Patch LGTM. But, Ryuan, Mikhail or someone else might have a final look before landing.

Mikhail Pozdnyakov

Comment 77 2013-08-01 04:36:42 PDT

(In reply to comment #76) > (From update of attachment 207909 [details]) > LGTM. But, Ryuan, Mikhail or someone else might have a final look before landing. OK, I don't want to prevent from landing but I still have a concern regarding the intended design (maybe it can be addressed later): GestureRecognizer <-> GestureHandler relationships are still unclear to me. So far I see two perspectives: 1) GestureRecognizer is(can be) efl-agnostic and shared between ports, than GestureHandler should be a common interface with port-specific implementation. My question is: is it possible? would qt/nix people like to re-use GestureRecognizer at least in theory? 2) GestureRecognizer is efl specific, than what's the point in having GestureHandler? if the point is only to make GestureRecognizer more readable I would make GestureHandler private and declare it in cpp file.

Eunmi Lee

Comment 78 2013-08-01 19:45:46 PDT

(In reply to comment #77) > (In reply to comment #76) > > (From update of attachment 207909 [details] [details]) > > LGTM. But, Ryuan, Mikhail or someone else might have a final look before landing. > > OK, I don't want to prevent from landing but I still have a concern regarding the intended design (maybe it can be addressed later): > > GestureRecognizer <-> GestureHandler relationships are still unclear to me. > > So far I see two perspectives: > 1) GestureRecognizer is(can be) efl-agnostic and shared between ports, than GestureHandler should be a common interface with port-specific implementation. > My question is: is it possible? would qt/nix people like to re-use GestureRecognizer at least in theory? Yes, GestureRecognizer can be shared if other ports want - especially NIX port because it referenced NIX's logic. And GestureRecognizer should be re-factored when other ports start to use. I can re-refactor that after all gesture codes for EFL are landed. > 2) GestureRecognizer is efl specific, than what's the point in having GestureHandler? if the point is only to make GestureRecognizer more readable I would make GestureHandler private and declare it in cpp file. Yes, at this point, GestureHandler is separated from GestureRecognizer for readability and it can be private. So, I will modify codes for that. Gyuyoung, would you review again after I upload new patch?

Gyuyoung Kim

Comment 79 2013-08-01 21:01:12 PDT

(In reply to comment #78) > (In reply to comment #77) > > (In reply to comment #76) > > > (From update of attachment 207909 [details] [details] [details]) > > > LGTM. But, Ryuan, Mikhail or someone else might have a final look before landing. > > > > OK, I don't want to prevent from landing but I still have a concern regarding the intended design (maybe it can be addressed later): > > > > GestureRecognizer <-> GestureHandler relationships are still unclear to me. > > > > So far I see two perspectives: > > 1) GestureRecognizer is(can be) efl-agnostic and shared between ports, than GestureHandler should be a common interface with port-specific implementation. > > My question is: is it possible? would qt/nix people like to re-use GestureRecognizer at least in theory? > Yes, GestureRecognizer can be shared if other ports want - especially NIX port because it referenced NIX's logic. > And GestureRecognizer should be re-factored when other ports start to use. I wonder when nix port contributor starts to upload their gesture patch. Any nix port folk can say about it ? > > 2) GestureRecognizer is efl specific, than what's the point in having GestureHandler? if the point is only to make GestureRecognizer more readable I would make GestureHandler private and declare it in cpp file. > Yes, at this point, GestureHandler is separated from GestureRecognizer for readability and it can be private. > So, I will modify codes for that. > Gyuyoung, would you review again after I upload new patch? Sure, no problem. Mikhail may want to take a look it as well :)

Eunmi Lee

Comment 80 2013-08-02 04:11:17 PDT

Created attachment 208000 [details] Patch Make GestureClient private.

Mikhail Pozdnyakov

Comment 81 2013-08-02 04:16:10 PDT

Comment on attachment 208000 [details] Patch lgtm

Gyuyoung Kim

Comment 82 2013-08-02 04:54:51 PDT

Comment on attachment 208000 [details] Patch r=me based on informal review.

Lauro Moura Maranhao Neto

Comment 83 2013-08-02 07:17:35 PDT

(In reply to comment #79) > (In reply to comment #78) > > (In reply to comment #77) > > > (In reply to comment #76) > > > > (From update of attachment 207909 [details] [details] [details] [details]) > > > > LGTM. But, Ryuan, Mikhail or someone else might have a final look before landing. > > > > > > OK, I don't want to prevent from landing but I still have a concern regarding the intended design (maybe it can be addressed later): > > > > > > GestureRecognizer <-> GestureHandler relationships are still unclear to me. > > > > > > So far I see two perspectives: > > > 1) GestureRecognizer is(can be) efl-agnostic and shared between ports, than GestureHandler should be a common interface with port-specific implementation. > > > My question is: is it possible? would qt/nix people like to re-use GestureRecognizer at least in theory? > > Yes, GestureRecognizer can be shared if other ports want - especially NIX port because it referenced NIX's logic. > > And GestureRecognizer should be re-factored when other ports start to use. > > I wonder when nix port contributor starts to upload their gesture patch. Any nix port folk can say about it ? > We've already uploaded the WebCore parts in a bunch of bugs but most of them are waiting for review/approval. After that we'd upload the WK2 and Tools with MiniBrowser/GestureRecognizer. metabug Nix: https://bugs.webkit.org/show_bug.cgi?id=117657 metabug WebCore: https://bugs.webkit.org/show_bug.cgi?id=117658 WebCore NIX directives: https://bugs.webkit.org/show_bug.cgi?id=118326 Nix files in Source/Platform: https://bugs.webkit.org/show_bug.cgi?id=118331 Stubs and platform files: https://bugs.webkit.org/show_bug.cgi?id=118358 Build files and scripts: https://bugs.webkit.org/show_bug.cgi?id=118367

Gyuyoung Kim

Comment 84 2013-08-02 23:13:30 PDT

(In reply to comment #83) > We've already uploaded the WebCore parts in a bunch of bugs but most of them are waiting for review/approval. After that we'd upload the WK2 and Tools with MiniBrowser/GestureRecognizer. Will you put GestureRecognizer on MiniBrowser, not port layer ? Why ?

Hugo Parente Lima

Comment 85 2013-08-05 10:15:07 PDT

(In reply to comment #84) > (In reply to comment #83) > > We've already uploaded the WebCore parts in a bunch of bugs but most of them are waiting for review/approval. After that we'd upload the WK2 and Tools with MiniBrowser/GestureRecognizer. > > Will you put GestureRecognizer on MiniBrowser, not port layer ? Why ? Yes, in the Nix philosophy gesture recognizer is an application business and the engine has nothing to do with it. This implies more code on the application side but OTOH the application will have more freedom to have whatever gesture it needs without depend on things implemented on Nix layer.

Eunmi Lee

Comment 86 2013-08-06 05:29:50 PDT

(In reply to comment #85) > (In reply to comment #84) > > (In reply to comment #83) > > > We've already uploaded the WebCore parts in a bunch of bugs but most of them are waiting for review/approval. After that we'd upload the WK2 and Tools with MiniBrowser/GestureRecognizer. > > > > Will you put GestureRecognizer on MiniBrowser, not port layer ? Why ? > > Yes, in the Nix philosophy gesture recognizer is an application business and the engine has nothing to do with it. This implies more code on the application side but OTOH the application will have more freedom to have whatever gesture it needs without depend on things implemented on Nix layer. I understand. Unlike NIX port, WK2 Efl implements default behaviors in the port directory. So, let's think about sharing gesture recognizer later and implement gestures in the Efl port like reviewed patch. Gyuyoung, could you commit the patch?

Gyuyoung Kim

Comment 87 2013-08-06 15:43:57 PDT

(In reply to comment #85) > (In reply to comment #84) > > (In reply to comment #83) > > > We've already uploaded the WebCore parts in a bunch of bugs but most of them are waiting for review/approval. After that we'd upload the WK2 and Tools with MiniBrowser/GestureRecognizer. > > > > Will you put GestureRecognizer on MiniBrowser, not port layer ? Why ? > > Yes, in the Nix philosophy gesture recognizer is an application business and the engine has nothing to do with it. This implies more code on the application side but OTOH the application will have more freedom to have whatever gesture it needs without depend on things implemented on Nix layer. Ok, we have different direction to support gesture recognizer. I land this patch for EFL port at the moment.

WebKit Commit Bot

Comment 88 2013-08-06 16:00:24 PDT

Comment on attachment 208000 [details] Patch Clearing flags on attachment: 208000 Committed r153770: <http://trac.webkit.org/changeset/153770>

WebKit Commit Bot

Comment 89 2013-08-06 16:00:40 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.