Bug 107101 – [EFL][WK2] Implement pan and flick gesture.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Eunmi Lee]

Implement pan gesture to scroll page during touch point is moving on the page,
and flick gesture to scroll page continuously and stop smoothly after pan gesture ends.

[Eunmi Lee]

Created attachment 183136 [details]
Patch

[Eunmi Lee]

Created attachment 191934 [details]
Patch

Rebased.

[Kenneth Rohde Christiansen]

Comment on attachment 191934 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EasingUtilities.cpp:2
> +/*
> + * Copyright (C) 2001 Robert Penner All rights reserved.

EasingUtilities? why not EasingCurves?

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:969
> +    FloatPoint oldPagePosition = m_pagePosition;
> +    FloatPoint newPagePosition = boundContentsPosition(m_pagePosition + offset);

why not just currentPosition and newPosition

this could sounds as 'new page' position instead of new 'page position'

[Eunmi Lee]

Comment on attachment 191934 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/EasingUtilities.cpp:2
>> + * Copyright (C) 2001 Robert Penner All rights reserved.
> 
> EasingUtilities? why not EasingCurves?

I agree that EasingCurves is more accurate than EasingUtilities. I will change the name :)

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:969
>> +    FloatPoint newPagePosition = boundContentsPosition(m_pagePosition + offset);
> 
> why not just currentPosition and newPosition
> 
> this could sounds as 'new page' position instead of new 'page position'

Yes, it can be confused with new 'page position', I will change the name of variables.

[Eunmi Lee]

Created attachment 193534 [details]
Patch

Rebase and Update for comments.

[Eunmi Lee]

Created attachment 193780 [details]
Patch

Rebase.

[Eunmi Lee]

Created attachment 206058 [details]
Patch

Rebased.

[Eunmi Lee]

Created attachment 207620 [details]
Patch

Rebase.

[Eunmi Lee]

Created attachment 207646 [details]
Patch

Modify scrollBy() to not use webview().

[Eunmi Lee]

Created attachment 208330 [details]
Patch

Rebase.

[Eunmi Lee]

Created attachment 208388 [details]
Patch

Rebase.

[Eunmi Lee]

Created attachment 208389 [details]
Patch

Rebase.

[Gyuyoung Kim]

Comment on attachment 208389 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:160
> +    HistoryItem item = {m_currentPoint, ecore_time_get()};

{ m_currentPoint, ecore_time_get() }; ?

> Source/WebKit2/UIProcess/efl/EasingCurves.cpp:2
> + * Copyright (C) 2001 Robert Penner All rights reserved.

2001 Robert Penner. ? Missing . after Robert Penner.

> Source/WebKit2/UIProcess/efl/EasingCurves.h:33
> +float easeInOutQuad(float time, const float base, const float constant, const unsigned duration);

Could you explain why you need to use this function ?

[Eunmi Lee]

Comment on attachment 208389 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:160
>> +    HistoryItem item = {m_currentPoint, ecore_time_get()};
> 
> { m_currentPoint, ecore_time_get() }; ?

Yes, I will fix.

>> Source/WebKit2/UIProcess/efl/EasingCurves.cpp:2
>> + * Copyright (C) 2001 Robert Penner All rights reserved.
> 
> 2001 Robert Penner. ? Missing . after Robert Penner.

Thanks, I will add.

>> Source/WebKit2/UIProcess/efl/EasingCurves.h:33
>> +float easeInOutQuad(float time, const float base, const float constant, const unsigned duration);
> 
> Could you explain why you need to use this function ?

The easeInOutQuad function is used for flick.
I want to stop scrolling smoothly - that is flick, and flick with easeInOutQuad graph is looks good than linear graph, so I use that function.

[Eunmi Lee]

Created attachment 209181 [details]
Patch

Update for comment.

[Mikhail Pozdnyakov]

Comment on attachment 209181 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
> +        return false;

return (oldPosition != position);

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:79
> +    typedef struct {

why not just "struct HistoryItem"?

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:107
> +        ecore_animator_del(m_panAnimator);

looks like it would be nice to have OwnPtr<Ecore_Animator>

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:165
> +        if (m_firstHistoryItemIndex == m_history.capacity())

how this m_history is supposed to work? from what I see we're always working with m_history[m_firstHistoryItemIndex] only, how other items are used?

[Eunmi Lee]

Comment on attachment 209181 [details]
Patch

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

Thanks for comments.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
>> +        return false;
> 
> return (oldPosition != position);

That's simple.

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:79
>> +    typedef struct {
> 
> why not just "struct HistoryItem"?

OK, I will modify to "struct HistoryItem".

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:107
>> +        ecore_animator_del(m_panAnimator);
> 
> looks like it would be nice to have OwnPtr<Ecore_Animator>

Yes, that's good idea. I will make OwnPtr<Ecore_Animator> as a separated patch. :)

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:165
>> +        if (m_firstHistoryItemIndex == m_history.capacity())
> 
> how this m_history is supposed to work? from what I see we're always working with m_history[m_firstHistoryItemIndex] only, how other items are used?

Yes, m_firstHistoryItemIndex is only used and others are not used.
I want to get oldest item from the m_history (at most 5 points ago) and calculate difference between oldest item and current item, so m_history array is needed.

When I started to implement flick, I thought that I could get more correct velocity if I use older point than point right before current point.
I've tested using point right before current point after getting your comment and I could get the reasonable velocity, so I don't have to maintain the history array.
I will modify codes for that.

[Eunmi Lee]

Created attachment 209251 [details]
Patch

Update for comments.

[Eunmi Lee]

Created attachment 209252 [details]
Patch

Remove s_historyCapacity.

[Gyuyoung Kim]

Comment on attachment 209252 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:117
> +void WebView::setContentPosition(const WebCore::FloatPoint& position)

I think we need to get an agreement from coordinatedgraphics folks.

[Eunmi Lee]

Comment on attachment 209181 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:107
>>> +        ecore_animator_del(m_panAnimator);
>> 
>> looks like it would be nice to have OwnPtr<Ecore_Animator>
> 
> Yes, that's good idea. I will make OwnPtr<Ecore_Animator> as a separated patch. :)

Dear Mikhail,

I've found patch to stop using smart pointers for Ecore_Timer - https://bugs.webkit.org/show_bug.cgi?id=109409.
Like Ecore_Timer, Ecore_Animator also can become invalid when callback returns ECORE_CALLBACK_CANCEL and crash will occur when OwnPtr calls ecore_animator_del() on an invalid handle.
So, I decide to not make OwnPtr for Ecore_Animator.

[Eunmi Lee]

Comment on attachment 209181 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:165
>>> +        if (m_firstHistoryItemIndex == m_history.capacity())
>> 
>> how this m_history is supposed to work? from what I see we're always working with m_history[m_firstHistoryItemIndex] only, how other items are used?
> 
> Yes, m_firstHistoryItemIndex is only used and others are not used.
> I want to get oldest item from the m_history (at most 5 points ago) and calculate difference between oldest item and current item, so m_history array is needed.
> 
> When I started to implement flick, I thought that I could get more correct velocity if I use older point than point right before current point.
> I've tested using point right before current point after getting your comment and I could get the reasonable velocity, so I don't have to maintain the history array.
> I will modify codes for that.

Dear Mikhail,
I'm sorry but I have to modify patch to use history again.
I've tested on target device, the flick is sometimes too slow on target even though it is OK in the MiniBrowser.
I think that's because target is slower than PC, so last two touched point can be got at a time and they are not reliable.

[Eunmi Lee]

Created attachment 209325 [details]
Patch

Modify codes to use history again.

[Eunmi Lee]

Comment on attachment 209252 [details]
Patch

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

>> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:117
>> +void WebView::setContentPosition(const WebCore::FloatPoint& position)
> 
> I think we need to get an agreement from coordinatedgraphics folks.

Dear Luciano Wolf and Marcelo Lira,
Could you look at this change?
I modified UIProcess/CoordinatedGraphics/WebView.cpp's setContentPosition() function in order to check boundary of position.

[Luciano Wolf]

(In reply to comment #24)
> (From update of attachment 209252 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209252&action=review
> 
> >> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:117
> >> +void WebView::setContentPosition(const WebCore::FloatPoint& position)
> > 
> > I think we need to get an agreement from coordinatedgraphics folks.
> 
> Dear Luciano Wolf and Marcelo Lira,
> Could you look at this change?
> I modified UIProcess/CoordinatedGraphics/WebView.cpp's setContentPosition() function in order to check boundary of position.

Hi. My concern is about panning. Our MiniBrowser allows the user to freely move and only applies the check for boundaries on handlePanningFinished. It relies on WKViewSetContentPosition to do the panning, so it would be restricted by boundaries check during the panning.

[Eunmi Lee]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 209252 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=209252&action=review
> > 
> > >> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:117
> > >> +void WebView::setContentPosition(const WebCore::FloatPoint& position)
> > > 
> > > I think we need to get an agreement from coordinatedgraphics folks.
> > 
> > Dear Luciano Wolf and Marcelo Lira,
> > Could you look at this change?
> > I modified UIProcess/CoordinatedGraphics/WebView.cpp's setContentPosition() function in order to check boundary of position.
> 
> Hi. My concern is about panning. Our MiniBrowser allows the user to freely move and only applies the check for boundaries on handlePanningFinished. It relies on WKViewSetContentPosition to do the panning, so it would be restricted by boundaries check during the panning.

Thanks for your answer :)
I understand what you mean.
In the EFL, I want to restrict boundary for panning so, it is better to move boundary checking codes to the EwkView, not WebView.
I will update patch with that.

[Eunmi Lee]

Created attachment 209443 [details]
Patch

Move boundary checking code to the EwkView from WebView.

[Gyuyoung Kim]

Comment on attachment 209443 [details]
Patch

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

It looks this patch need to be done refactoring a little further.

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:138
> +    gestureHandler->m_ewkView->scrollBy(gestureHandler->m_lastPoint - gestureHandler->m_currentPoint);

It would be good if you make view() to return m_ewkView.

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:151
> +    m_history.resize(0);

Can't we reset these variables in handlePanFinished() ? I don't think we need to keep it until panning is started again.

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:164
> +        m_history[m_firstHistoryItemIndex++] = item;

I wonder if we should use m_firstHistoryItemIndex memeber variable. Can't we simplify this as below ?

if (m_history.size() < m_history.capacity())
    m_history.append(item)
else
    m_history.clear();

Then, find first item.

const HistoryItem& firstHistoryItem = m_history[m_firstHistoryItemIndex];
=> const HistoryItem& firstHistoryItem = m_history.first();

[Eunmi Lee]

Comment on attachment 209443 [details]
Patch

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

Thanks for review.

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:138
>> +    gestureHandler->m_ewkView->scrollBy(gestureHandler->m_lastPoint - gestureHandler->m_currentPoint);
> 
> It would be good if you make view() to return m_ewkView.

OK, I will make that.

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:151
>> +    m_history.resize(0);
> 
> Can't we reset these variables in handlePanFinished() ? I don't think we need to keep it until panning is started again.

Yes, it can be moved to the PanFinished().

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:164
>> +        m_history[m_firstHistoryItemIndex++] = item;
> 
> I wonder if we should use m_firstHistoryItemIndex memeber variable. Can't we simplify this as below ?
> 
> if (m_history.size() < m_history.capacity())
>     m_history.append(item)
> else
>     m_history.clear();
> 
> Then, find first item.
> 
> const HistoryItem& firstHistoryItem = m_history[m_firstHistoryItemIndex];
> => const HistoryItem& firstHistoryItem = m_history.first();

The m_firstHistoryItemIndex is needed because I want to get the oldest point in the history.
That means, if the history grows as follows:
[10] -> [10, 20] -> [10, 20, 30] -> [10, 20, 30, 40] -> [10, 20, 30, 40, 50].
Until this moment, the m_firstHistoryItemIndex is 0, and I can get "10".
"60" is added to the history, the m_history can be [60, 20, 30, 40, 50], and m_firstHistoryItemIndex is 1 and the vaule is "20".

I think the variable name "m_firstHistoryItemIndex" can be confused, so I will change that to "m_oldestHistoryItemIndex".

[Eunmi Lee]

Created attachment 211409 [details]
Patch

Update for Gyuyoung's comment.

[Chris Dumez]

Comment on attachment 211409 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1400
> +    FloatPoint newPosition = FloatPoint(oldPosition.x + offset.width(), oldPosition.y + offset.height());

FloatPoint newPosition(oldPosition.x + offset.width(), oldPosition.y + offset.height());

This is not the same thing.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1412
> +    // If the page position is not changed, notify that using returning value.

// If the page position has not changed, notify the caller using the return value.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1413
> +    return !(oldPosition == position);

return oldPosition != position;

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:58
> +    void handleFlick(const WebCore::IntSize&);

useless WebCore::

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:72
> +    WebCore::IntPoint m_lastPoint;

Useless WebCore::

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:73
> +    WebCore::IntPoint m_currentPoint;

Ditto.

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:75
> +    Ecore_Animator* m_panAnimator;

Would be nice to use WTF::OwnPtr for this (adding the right specialization if needed)

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:76
> +    Ecore_Animator* m_flickAnimator;

Ditto.

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:77
> +    WebCore::IntSize m_flickOffset;

useless WebCore::

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:82
> +        WebCore::IntPoint point;

useless WebCore::

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:86
> +    unsigned m_oldestHistoryItemIndex;

size_t

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:98
> +    m_history.reserveCapacity(s_historyCapacity);

reserveInitialCapacity()

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:106
> +void GestureHandler::reset()

Can be cleaned up once we use smart pointers

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:205
> +        return false;

Would be nicer to use the EINA equavalent in callback (e.g. *_RENEW)

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:208
> +    return true;

Ditto.

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:214
> +    m_flickIndex = m_flickDuration = 1 / ecore_animator_frametime_get();

m_flickIndex and m_flickDuration are integers, 1/ecore_animator_frametime_get() returns a floating point value. It is likely m_flickIndex and m_flickDuration are always 0, right?

[Chris Dumez]

(In reply to comment #31)
> (From update of attachment 211409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211409&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1400
> > +    FloatPoint newPosition = FloatPoint(oldPosition.x + offset.width(), oldPosition.y + offset.height());
> 
> FloatPoint newPosition(oldPosition.x + offset.width(), oldPosition.y + offset.height());
> 
> This is not the same thing.
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1412
> > +    // If the page position is not changed, notify that using returning value.
> 
> // If the page position has not changed, notify the caller using the return value.
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1413
> > +    return !(oldPosition == position);
> 
> return oldPosition != position;
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:58
> > +    void handleFlick(const WebCore::IntSize&);
> 
> useless WebCore::
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:72
> > +    WebCore::IntPoint m_lastPoint;
> 
> Useless WebCore::
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:73
> > +    WebCore::IntPoint m_currentPoint;
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:75
> > +    Ecore_Animator* m_panAnimator;
> 
> Would be nice to use WTF::OwnPtr for this (adding the right specialization if needed)
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:76
> > +    Ecore_Animator* m_flickAnimator;
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:77
> > +    WebCore::IntSize m_flickOffset;
> 
> useless WebCore::
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:82
> > +        WebCore::IntPoint point;
> 
> useless WebCore::
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:86
> > +    unsigned m_oldestHistoryItemIndex;
> 
> size_t
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:98
> > +    m_history.reserveCapacity(s_historyCapacity);
> 
> reserveInitialCapacity()
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:106
> > +void GestureHandler::reset()
> 
> Can be cleaned up once we use smart pointers
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:205
> > +        return false;
> 
> Would be nicer to use the EINA equavalent in callback (e.g. *_RENEW)
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:208
> > +    return true;
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:214
> > +    m_flickIndex = m_flickDuration = 1 / ecore_animator_frametime_get();
> 
> m_flickIndex and m_flickDuration are integers, 1/ecore_animator_frametime_get() returns a floating point value. It is likely m_flickIndex and m_flickDuration are always 0, right?

I published by mistake, sorry. I am still reviewing.

[Chris Dumez]

Comment on attachment 211409 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:75
>> +    Ecore_Animator* m_panAnimator;
> 
> Would be nice to use WTF::OwnPtr for this (adding the right specialization if needed)

Nevermind this comment. Since they get destroyed when the callback returns false, using smart pointers is bug prone.

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:106
>> +void GestureHandler::reset()
> 
> Can be cleaned up once we use smart pointers

ignore.

> Source/WebKit2/UIProcess/efl/EasingCurves.h:33
> +float easeInOutQuad(float time, const float base, const float constant, const unsigned duration);

No namespace?

[Eunmi Lee]

Comment on attachment 211409 [details]
Patch

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

Thanks for review :)

>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1400
>>> +    FloatPoint newPosition = FloatPoint(oldPosition.x + offset.width(), oldPosition.y + offset.height());
>> 
>> FloatPoint newPosition(oldPosition.x + offset.width(), oldPosition.y + offset.height());
>> 
>> This is not the same thing.
> 
> I published by mistake, sorry. I am still reviewing.

Yes, right. I will modify the codes.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1412
>> +    // If the page position is not changed, notify that using returning value.
> 
> // If the page position has not changed, notify the caller using the return value.

Thanks for correction.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1413
>> +    return !(oldPosition == position);
> 
> return oldPosition != position;

I use !(oldPosition == position) because there is no "!=" operator for WKPoint.
Is it better to add "!=" operator to the WKPoint?

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:58
>> +    void handleFlick(const WebCore::IntSize&);
> 
> useless WebCore::

Yes, I will remove all "WebCore::".

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:86
>> +    unsigned m_oldestHistoryItemIndex;
> 
> size_t

I will change that.

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:98
>> +    m_history.reserveCapacity(s_historyCapacity);
> 
> reserveInitialCapacity()

Thanks, I will use that.

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:205
>> +        return false;
> 
> Would be nicer to use the EINA equavalent in callback (e.g. *_RENEW)

Right. I will use ECORE_CALLBACK_{RENEW, CANCEL}.

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:214
>> +    m_flickIndex = m_flickDuration = 1 / ecore_animator_frametime_get();
> 
> m_flickIndex and m_flickDuration are integers, 1/ecore_animator_frametime_get() returns a floating point value. It is likely m_flickIndex and m_flickDuration are always 0, right?

If the FPS is 60, the ecore_animator_frametime_get() returns 1/60 (= 0.016..), so I can get the FPS from "1/ecore_animator_frametime_get()" and convert that value to integer.

>> Source/WebKit2/UIProcess/efl/EasingCurves.h:33
>> +float easeInOutQuad(float time, const float base, const float constant, const unsigned duration);
> 
> No namespace?

I need namespace WebKit :)

[Eunmi Lee]

Created attachment 211524 [details]
Patch

Modify for Christophe's comments.

[Chris Dumez]

Comment on attachment 211409 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1413
>>> +    return !(oldPosition == position);
>> 
>> return oldPosition != position;
> 
> I use !(oldPosition == position) because there is no "!=" operator for WKPoint.
> Is it better to add "!=" operator to the WKPoint?

Ok, I thought those were FloatPoint. Nevermind then.

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:189
> +    m_history.resize(0);

Shouldn't this be in the if()? If m_history.isEmpty() then this call is useless.

>>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:214
>>> +    m_flickIndex = m_flickDuration = 1 / ecore_animator_frametime_get();
>> 
>> m_flickIndex and m_flickDuration are integers, 1/ecore_animator_frametime_get() returns a floating point value. It is likely m_flickIndex and m_flickDuration are always 0, right?
> 
> If the FPS is 60, the ecore_animator_frametime_get() returns 1/60 (= 0.016..), so I can get the FPS from "1/ecore_animator_frametime_get()" and convert that value to integer.

Ok, thanks for clarifying.

[Chris Dumez]

Comment on attachment 211524 [details]
Patch

No big issue for me anymore but I'll let Gyuyoung review since he started.

[Gyuyoung Kim]

Comment on attachment 211524 [details]
Patch

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

It looks there is no big issue as well.

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:181
> +        const HistoryItem& firstHistoryItem = m_history[m_oldestHistoryItemIndex];

> Until this moment, the m_firstHistoryItemIndex is 0, and I can get "10". "60" is added to the history, the m_history can be [60, 20, 30, 40, 50], and m_firstHistoryItemIndex is 1 and the vaule is "20".

Thank you for your explanation. Don't you need to change *firstHistoryItem* with *oldestHistoryItem* as well?

[Eunmi Lee]

Christophe and Gyuyoung, thanks for review :)

I will upload new patch after fixing as follows:
1. check history size before resize.
2. change name first -> oldest.

[Eunmi Lee]

Created attachment 211751 [details]
Patch

Update for Christophe and Gyuyoung's comment.

[WebKit Commit Bot]

Comment on attachment 211751 [details]
Patch

Clearing flags on attachment: 211751

Committed r155852: <http://trac.webkit.org/changeset/155852>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.