Bug 81895 - [Qt] Don't resume the suspended page if the user is continuously flicking.
Summary: [Qt] Don't resume the suspended page if the user is continuously flicking.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jocelyn Turcotte
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-22 05:21 PDT by Jocelyn Turcotte
Modified: 2012-03-26 11:01 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.82 KB, patch)
2012-03-22 05:24 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (4.78 KB, patch)
2012-03-22 05:53 PDT, Jocelyn Turcotte
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2012-03-22 05:21:53 PDT
[Qt] Don't resume the suspended page if the user is continuously flicking.
Comment 1 Jocelyn Turcotte 2012-03-22 05:24:48 PDT
Created attachment 133231 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-03-22 05:30:11 PDT
Comment on attachment 133231 [details]
Patch

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

Seems like an ok idea, though it doesnt fit so well with the current code.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:301
> +void QtViewportInteractionEngine::notifyTouchBegin()

I am not sure how well this naming with together with what we already have in the class

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:304
> +    if (scrollAnimationActive())

This only works with the animation and not the "dragging"

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h:153
>      OwnPtr<ViewportUpdateDeferrer> m_scaleUpdateDeferrer;
>      OwnPtr<ViewportUpdateDeferrer> m_scrollUpdateDeferrer;
> +    OwnPtr<ViewportUpdateDeferrer> m_panUpdateDeferrer;

These are not actually per gesture recognizer, so scroll vs pan is confusing here.
Comment 3 Jocelyn Turcotte 2012-03-22 05:53:40 PDT
Created attachment 133237 [details]
Patch
Comment 4 Jocelyn Turcotte 2012-03-22 05:55:28 PDT
(In reply to comment #2)
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:304
> > +    if (scrollAnimationActive())
> 
> This only works with the animation and not the "dragging"
> 
m_scrollUpdateDeferrer handles the dragging, flickable tells us that it's dragging through movementStarted/Ended.

Fixed the rest, tell me what you think about it.
Comment 5 Kenneth Rohde Christiansen 2012-03-22 05:57:54 PDT
Comment on attachment 133237 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:304
> +    if (scrollAnimationActive())

I still think this might need to check isMoving instead of isFlicking.
Comment 6 Kenneth Rohde Christiansen 2012-03-22 05:58:37 PDT
(In reply to comment #4)
> (In reply to comment #2)
> > > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:304
> > > +    if (scrollAnimationActive())
> > 
> > This only works with the animation and not the "dragging"
> > 
> m_scrollUpdateDeferrer handles the dragging, flickable tells us that it's dragging through movementStarted/Ended.
> 
> Fixed the rest, tell me what you think about it.

cant we merge them then?
Comment 7 Jocelyn Turcotte 2012-03-22 06:38:53 PDT
(In reply to comment #6)
> cant we merge them then?

I think it's pretty simple like this, the suspend count logic makes it all work.
The other way would be to have some heavy boolean logic to decide when to clear the deferrer.
Comment 8 Jocelyn Turcotte 2012-03-22 08:24:53 PDT
Committed r111705: <http://trac.webkit.org/changeset/111705>
Comment 9 Stephen Chenney 2012-03-26 11:01:06 PDT
*** Bug 81691 has been marked as a duplicate of this bug. ***