Bug 110006 – Allow UI clients to handle vertical wheel events

• Component: UI Events
• Status: RESOLVED FIXED

[Conrad Shultz]

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.

[Conrad Shultz]

Created attachment 188981 [details]
Patch

[Early Warning System Bot]

Comment on attachment 188981 [details]
Patch

Attachment 188981 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16610584

[EFL EWS Bot]

Comment on attachment 188981 [details]
Patch

Attachment 188981 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16610590

[Build Bot]

Comment on attachment 188981 [details]
Patch

Attachment 188981 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16616469

[Conrad Shultz]

Comment on attachment 188981 [details]
Patch

Missing a guard, will resubmit.

[Conrad Shultz]

Created attachment 189029 [details]
Patch

[Early Warning System Bot]

Comment on attachment 189029 [details]
Patch

Attachment 189029 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16611695

[Build Bot]

Comment on attachment 189029 [details]
Patch

Attachment 189029 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16615678

[Conrad Shultz]

Created attachment 189116 [details]
Patch

[Conrad Shultz]

Created attachment 189118 [details]
Patch

[Tim Horton]

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.

[Conrad Shultz]

(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!

[Beth Dakin]

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.

[Conrad Shultz]

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

[Conrad Shultz]

Created attachment 189166 [details]
Patch

[Beth Dakin]

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.

[Conrad Shultz]

Created attachment 189203 [details]
Patch

[Conrad Shultz]

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

[Tim Horton]

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.

[Conrad Shultz]

Created attachment 189205 [details]
Patch

[Conrad Shultz]

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

[WebKit Review Bot]

Comment on attachment 189205 [details]
Patch

Clearing flags on attachment: 189205

Committed r143418: <http://trac.webkit.org/changeset/143418>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.