Bug 110006

Summary: Allow UI clients to handle vertical wheel events
Product: WebKit Reporter: Conrad Shultz <conrad_shultz>
Component: UI EventsAssignee: Conrad Shultz <conrad_shultz>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, bdakin, buildbot, cmarcelo, jamesr, levin+threading, luiz, rniwa, sam, thorton, tonikitoo, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 111001    
Attachments:
Description Flags
Patch
webkit-ews: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Conrad Shultz 2013-02-16 00:38:54 PST
Consistent with existing behavior in the horizontal direction, UI clients should be able to opt-in to handle vertical scroll wheel events that would otherwise trigger rubber banding.

Three pieces are seemingly needed:

1) Selective control over rubber banding in the vertical direction.
2) Extension (to the vertical direction) of the concept of page "pinning."
3) Coupling of #1 and #2 in ScrollingTree::willWheelEventStartSwipeGesture(), allowing the swipe gesture pathway to be activated selectively in the vertical direction, culminating in a didNotHandleWheelEvent() message being sent to the UI client.
Comment 1 Conrad Shultz 2013-02-18 21:13:50 PST
Created attachment 188981 [details]
Patch
Comment 2 Early Warning System Bot 2013-02-18 21:28:06 PST
Comment on attachment 188981 [details]
Patch

Attachment 188981 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16610584
Comment 3 EFL EWS Bot 2013-02-18 21:52:36 PST
Comment on attachment 188981 [details]
Patch

Attachment 188981 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16610590
Comment 4 Build Bot 2013-02-18 22:08:52 PST
Comment on attachment 188981 [details]
Patch

Attachment 188981 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16616469
Comment 5 Conrad Shultz 2013-02-19 00:12:53 PST
Comment on attachment 188981 [details]
Patch

Missing a guard, will resubmit.
Comment 6 Conrad Shultz 2013-02-19 02:05:06 PST
Created attachment 189029 [details]
Patch
Comment 7 Early Warning System Bot 2013-02-19 02:16:46 PST
Comment on attachment 189029 [details]
Patch

Attachment 189029 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16611695
Comment 8 Build Bot 2013-02-19 04:19:21 PST
Comment on attachment 189029 [details]
Patch

Attachment 189029 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16615678
Comment 9 Conrad Shultz 2013-02-19 11:02:27 PST
Created attachment 189116 [details]
Patch
Comment 10 Conrad Shultz 2013-02-19 11:09:24 PST
Created attachment 189118 [details]
Patch
Comment 11 Tim Horton 2013-02-19 13:18:43 PST
Comment on attachment 189118 [details]
Patch

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

> Source/WebCore/page/Page.h:279
> +#if ENABLE(THREADED_SCROLLING)
> +    bool rubberBandsAtBottom() const;
> +    void setRubberBandsAtBottom(bool);
> +    bool rubberBandsAtTop() const;
> +    void setRubberBandsAtTop(bool);
> +#endif

Vaguely interesting that you're adding these for the vertical case but parallels don't exist for the horizontal case.

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:410
> +    return toImpl(pageRef)->setRubberBandsAtBottom(rubberBandsAtBottom);

return with a value in a void function?

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:426
> +    return toImpl(pageRef)->setRubberBandsAtTop(rubberBandsAtTop);

ditto.
Comment 12 Conrad Shultz 2013-02-19 13:25:06 PST
(In reply to comment #11)
> (From update of attachment 189118 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189118&action=review
> 
> > Source/WebCore/page/Page.h:279
> > +#if ENABLE(THREADED_SCROLLING)
> > +    bool rubberBandsAtBottom() const;
> > +    void setRubberBandsAtBottom(bool);
> > +    bool rubberBandsAtTop() const;
> > +    void setRubberBandsAtTop(bool);
> > +#endif
> 
> Vaguely interesting that you're adding these for the vertical case but parallels don't exist for the horizontal case.

The horizontal case isn't as important since WebKit will already send back didNotHandleWheelEvent() messages in that case when appropriate.

> 
> > Source/WebKit2/UIProcess/API/C/WKPage.cpp:410
> > +    return toImpl(pageRef)->setRubberBandsAtBottom(rubberBandsAtBottom);
> 
> return with a value in a void function?
> 
> > Source/WebKit2/UIProcess/API/C/WKPage.cpp:426
> > +    return toImpl(pageRef)->setRubberBandsAtTop(rubberBandsAtTop);
> 
> ditto.

I am surprised that these compiled without complaint; fixed. Thanks!
Comment 13 Beth Dakin 2013-02-19 13:31:59 PST
Comment on attachment 189118 [details]
Patch

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

> Source/WebCore/page/Page.cpp:70
> +#include "ScrollingTree.h"

I think we should avoid including ScrollingTree from within Page. See comments below.

> Source/WebCore/page/Page.cpp:822
> +    return m_scrollingCoordinator->scrollingTree()->rubberBandsAtBottom();

Even though it's more overheard, you should add a function to ScrollingCoordinator to do this. Right now ScrollingCoordinator is the only thing that interfaces with the ScrollingTree and the scrolling thread, and I think we want to keep it that way. For example, see ScrollingCoordinatorMac::isRubberBandInProgress()… ScrollingCoordinator interfaces with ScrollingTree and then the rest of the code in WebCore just interfaces with ScrollingCoordinator. Then you won't need to put this inside and ifdef either since you can put a dummy implementation in the cross-platform ScrollingCoordinator.h and then override it in ScrollingCoordinatorMac.

> Source/WebCore/page/Page.cpp:827
> +    m_scrollingCoordinator->scrollingTree()->setRubberBandsAtBottom(rubberBandsAtBottom);

Same here.

> Source/WebCore/page/Page.cpp:832
> +    return m_scrollingCoordinator->scrollingTree()->rubberBandsAtTop();

Same here.

> Source/WebCore/page/Page.cpp:837
> +    m_scrollingCoordinator->scrollingTree()->setRubberBandsAtTop(rubberBandsAtTop);

Same here.
Comment 14 Conrad Shultz 2013-02-19 13:37:57 PST
> > Source/WebCore/page/Page.cpp:70
> > +#include "ScrollingTree.h"
> 
> I think we should avoid including ScrollingTree from within Page. See comments below.
> 
> > Source/WebCore/page/Page.cpp:822
> > +    return m_scrollingCoordinator->scrollingTree()->rubberBandsAtBottom();
> 
> Even though it's more overheard, you should add a function to ScrollingCoordinator to do this. Right now ScrollingCoordinator is the only thing that interfaces with the ScrollingTree and the scrolling thread, and I think we want to keep it that way. For example, see ScrollingCoordinatorMac::isRubberBandInProgress()… ScrollingCoordinator interfaces with ScrollingTree and then the rest of the code in WebCore just interfaces with ScrollingCoordinator. Then you won't need to put this inside and ifdef either since you can put a dummy implementation in the cross-platform ScrollingCoordinator.h and then override it in ScrollingCoordinatorMac.

That sounds much better, will change.
Comment 15 Conrad Shultz 2013-02-19 14:36:24 PST
Created attachment 189166 [details]
Patch
Comment 16 Beth Dakin 2013-02-19 14:44:49 PST
Comment on attachment 189166 [details]
Patch

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

This is very close! My only comment this time is that you don't need to use the threaded-scrolling ifdef so much now that you have done the nice ScrollingCoordinator/ScrollingCoordinatorMac implementations of the rubberbanding functions. Eventually I stopped commenting on individual spots, because I think you don't need the ifdefs anywhere, actually.

> Source/WebCore/page/Page.cpp:818
> +#if ENABLE(THREADED_SCROLLING)

Don't need this ifdef.

> Source/WebCore/page/Page.h:274
> +#if ENABLE(THREADED_SCROLLING)

Nor this one.

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:400
> +#if ENABLE(THREADED_SCROLLING)

I don't think you need these ones in WKPage either. The API is a no-op non-threaded-scrolling mode, but that doesn't mean it has to be ifdef'ed out.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:229
> +#if ENABLE(THREADED_SCROLLING)

And same here.
Comment 17 Conrad Shultz 2013-02-19 16:44:11 PST
Created attachment 189203 [details]
Patch
Comment 18 Conrad Shultz 2013-02-19 16:45:49 PST
(In reply to comment #16)
> (From update of attachment 189166 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189166&action=review
> 
> This is very close! My only comment this time is that you don't need to use the threaded-scrolling ifdef so much now that you have done the nice ScrollingCoordinator/ScrollingCoordinatorMac implementations of the rubberbanding functions. Eventually I stopped commenting on individual spots, because I think you don't need the ifdefs anywhere, actually.

Thanks, I posted a revised patch.
Comment 19 Tim Horton 2013-02-19 16:47:43 PST
Comment on attachment 189203 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:145
> +    SetRubberBandsAtBottom(bool rubberBandsAtTop)

Typo in argument name.
Comment 20 Conrad Shultz 2013-02-19 16:54:52 PST
Created attachment 189205 [details]
Patch
Comment 21 Conrad Shultz 2013-02-19 16:56:03 PST
(In reply to comment #19)
> (From update of attachment 189203 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189203&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:145
> > +    SetRubberBandsAtBottom(bool rubberBandsAtTop)
> 
> Typo in argument name.

Oops, fixed.
Comment 22 WebKit Review Bot 2013-02-19 17:44:29 PST
Comment on attachment 189205 [details]
Patch

Clearing flags on attachment: 189205

Committed r143418: <http://trac.webkit.org/changeset/143418>
Comment 23 WebKit Review Bot 2013-02-19 17:44:34 PST
All reviewed patches have been landed.  Closing bug.