Bug 81895

Summary: [Qt] Don't resume the suspended page if the user is continuously flicking.
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: New BugsAssignee: Jocelyn Turcotte <jturcotte>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, hausmann, kenneth, menard, schenney, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch kenneth: review+

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. ***