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.
Created attachment 188981 [details] Patch
Comment on attachment 188981 [details] Patch Attachment 188981 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16610584
Comment on attachment 188981 [details] Patch Attachment 188981 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16610590
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 on attachment 188981 [details] Patch Missing a guard, will resubmit.
Created attachment 189029 [details] Patch
Comment on attachment 189029 [details] Patch Attachment 189029 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16611695
Comment on attachment 189029 [details] Patch Attachment 189029 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16615678
Created attachment 189116 [details] Patch
Created attachment 189118 [details] Patch
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.
(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 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.
> > 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.
Created attachment 189166 [details] Patch
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.
Created attachment 189203 [details] Patch
(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 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.
Created attachment 189205 [details] Patch
(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 on attachment 189205 [details] Patch Clearing flags on attachment: 189205 Committed r143418: <http://trac.webkit.org/changeset/143418>
All reviewed patches have been landed. Closing bug.