Bug 102643

Summary: [EFL][WK2] Implement gesture recognizer.
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit EFLAssignee: 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: P2    
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

Description Eunmi Lee 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.
Comment 1 Eunmi Lee 2012-11-21 23:15:29 PST
Created attachment 175595 [details]
Patch
Comment 2 Eunmi Lee 2012-11-23 00:04:57 PST
Created attachment 175740 [details]
Patch
Comment 3 Gyuyoung Kim 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.
Comment 4 Gyuyoung Kim 2012-12-04 00:26:31 PST
CC'ing Kenneth, Kenneth, how do you think about this patch ?
Comment 5 Kenneth Rohde Christiansen 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
Comment 6 Eunmi Lee 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.
Comment 7 Eunmi Lee 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
Comment 8 Eunmi Lee 2012-12-05 22:21:13 PST
Created attachment 177938 [details]
Patch
Comment 9 Dominik Röttsches (drott) 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"?
Comment 10 Eunmi Lee 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.
Comment 11 Eunmi Lee 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.
Comment 12 Eunmi Lee 2013-01-14 00:51:36 PST
Created attachment 182520 [details]
Patch
Comment 13 Eunmi Lee 2013-01-14 01:21:43 PST
Comment on attachment 182520 [details]
Patch

This patch is in a work-in-process.
Comment 14 Eunmi Lee 2013-01-14 03:16:02 PST
Created attachment 182537 [details]
Patch
Comment 15 Eunmi Lee 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.
Comment 16 Kenneth Rohde Christiansen 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...
Comment 17 Kenneth Rohde Christiansen 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.
Comment 18 Kenneth Rohde Christiansen 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
Comment 19 Eunmi Lee 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. :)
Comment 20 Eunmi Lee 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. :)
Comment 21 Eunmi Lee 2013-01-20 22:56:42 PST
Created attachment 183716 [details]
Patch

Add Nokia in the copyright of gesture files.
Comment 22 Eunmi Lee 2013-01-23 00:51:14 PST
Created attachment 184174 [details]
Patch

Rebase patch
Comment 23 Antonio Gomes 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'
Comment 24 Eunmi Lee 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.
Comment 25 Chris Dumez 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.
Comment 26 Chris Dumez 2013-02-18 00:03:39 PST
I was referring to https://lists.webkit.org/pipermail/webkit-dev/2012-October/022576.html
Comment 27 Mikhail Pozdnyakov 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
Comment 28 Mikhail Pozdnyakov 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
Comment 29 Mikhail Pozdnyakov 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;
Comment 30 Chris Dumez 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())
Comment 31 Mikhail Pozdnyakov 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 :)
Comment 32 Eunmi Lee 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.
Comment 33 Eunmi Lee 2013-02-18 23:23:05 PST
Created attachment 189001 [details]
Patch

Rebase and update for comments.
Comment 34 Eunmi Lee 2013-03-04 22:17:52 PST
Created attachment 191401 [details]
Patch

Rebased.
Comment 35 Eunmi Lee 2013-03-15 04:13:34 PDT
Created attachment 193276 [details]
Patch

Modified gesture recognizer's logic like NIX webkit.
Comment 36 Kenneth Rohde Christiansen 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)
Comment 37 Mikhail Pozdnyakov 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.
Comment 38 Eunmi Lee 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.
Comment 39 Eunmi Lee 2013-03-19 01:25:29 PDT
Created attachment 193753 [details]
Patch

Update for comments.
Comment 40 Mikhail Pozdnyakov 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.
Comment 41 Eunmi Lee 2013-07-03 22:53:10 PDT
Created attachment 206052 [details]
Patch

Add virtual destructor for GestureRecognizerClient and rebased.
Comment 42 Eunmi Lee 2013-07-23 18:52:53 PDT
Created attachment 207367 [details]
Patch

Rebase.
Comment 43 Gyuyoung Kim 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() ?
Comment 44 Eunmi Lee 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.
Comment 45 Eunmi Lee 2013-07-25 02:28:38 PDT
Created attachment 207443 [details]
Patch

Update for comments and modify createVectorWithWKArray to fix the crash.
Comment 46 Gyuyoung Kim 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 ?
Comment 47 Eunmi Lee 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.
Comment 48 Eunmi Lee 2013-07-25 23:11:08 PDT
Created attachment 207509 [details]
Patch

Update for Gyuyoung's comments.
Comment 49 Ryuan Choi 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.
Comment 50 Ryuan Choi 2013-07-26 00:34:15 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/GestureRecognizer.cpp:123
> +        ASSERT(m_tapAndHoldTimer);

ASSERT(!m_tapAndHoldTimer);
Comment 51 Gyuyoung Kim 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 ?
Comment 52 Eunmi Lee 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.
Comment 53 Build Bot 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
Comment 54 Build Bot 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
Comment 55 Eunmi Lee 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.
Comment 56 Eunmi Lee 2013-07-28 23:11:09 PDT
Created attachment 207619 [details]
Patch

Move GestureRecognizerClient to the EwkView.
Comment 57 Ryuan Choi 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.
Comment 58 Eunmi Lee 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.
Comment 59 Eunmi Lee 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.
Comment 60 Mikhail Pozdnyakov 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.
Comment 61 Eunmi Lee 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.
Comment 62 Eunmi Lee 2013-07-29 19:53:38 PDT
Created attachment 207683 [details]
Patch

Remove parameter name from EwkView::handlePan().
Comment 63 Mikhail Pozdnyakov 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.
Comment 64 Eunmi Lee 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.
Comment 65 Mikhail Pozdnyakov 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.
Comment 66 Ryuan Choi 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.
Comment 67 Eunmi Lee 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.
Comment 68 Eunmi Lee 2013-07-30 03:46:25 PDT
Created attachment 207714 [details]
Patch

Update for comments.
Comment 69 Build Bot 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
Comment 70 Build Bot 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
Comment 71 Eunmi Lee 2013-07-30 05:25:45 PDT
Created attachment 207722 [details]
Patch

Rebase.
Comment 72 Eunmi Lee 2013-07-30 06:56:39 PDT
Created attachment 207732 [details]
Patch

Fix wrong usage of Vector in the createVectorWithWKArray().
Comment 73 Gyuyoung Kim 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.
Comment 74 Eunmi Lee 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.
Comment 75 Eunmi Lee 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.
Comment 76 Gyuyoung Kim 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.
Comment 77 Mikhail Pozdnyakov 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.
Comment 78 Eunmi Lee 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?
Comment 79 Gyuyoung Kim 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 :)
Comment 80 Eunmi Lee 2013-08-02 04:11:17 PDT
Created attachment 208000 [details]
Patch

Make GestureClient private.
Comment 81 Mikhail Pozdnyakov 2013-08-02 04:16:10 PDT
Comment on attachment 208000 [details]
Patch

lgtm
Comment 82 Gyuyoung Kim 2013-08-02 04:54:51 PDT
Comment on attachment 208000 [details]
Patch

r=me based on informal review.
Comment 83 Lauro Moura Maranhao Neto 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
Comment 84 Gyuyoung Kim 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 ?
Comment 85 Hugo Parente Lima 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.
Comment 86 Eunmi Lee 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?
Comment 87 Gyuyoung Kim 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.
Comment 88 WebKit Commit Bot 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>
Comment 89 WebKit Commit Bot 2013-08-06 16:00:40 PDT
All reviewed patches have been landed.  Closing bug.