Bug 58167 – WebKit2: Windows 7 Gestures Window Bounce shouldn't require a sync message

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brian Weinstein]

Handling Window Bounce for Windows 7 gestures shouldn't require a sync message from the UIProcess -> WebProcess.

[Jessie Berlin]

<rdar://problem/9259813>

[Brian Weinstein]

Created attachment 88886 [details]
[PATCH] Fix

[WebKit Review Bot]

Attachment 88886 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/WebPageProxy.messages.in:234:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/UIProcess/WebPageProxy.messages.in:235:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/UIProcess/WebPageProxy.h:282:  The parameter name "limitReached" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/PageClient.h:133:  The parameter name "limitReached" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 88886 [details]
[PATCH] Fix

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:691
> +void WebPageProxy::setGestureScrollingLimitReached(bool limitReached)
> +{
> +    m_pageClient->setGestureScrollingLimitReached(limitReached);
> +}

This and the other Windows-specific functions should really be in WebPageProxyWin.cpp. Can you make a followup patch to move the functions?

> Source/WebKit2/UIProcess/win/WebView.cpp:586
> +        if (m_gestureScrollingLimitReached)

Do we need to reset this when the gesture ends? Otherwise it seems like the wrong thing would happen if you:

1) Gesture-scroll to the end of the document
2) Scroll back up a little by clicking on the scrollbar
3) Gesture-scroll down again

> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:425
> +    bool atScrollingLimitBefore = scrollbarAtTopOfBottomOrDocument(verticalScrollbar);
> +    m_gestureTargetNode->renderer()->enclosingLayer()->scrollByRecursively(size.width(), size.height());
> +    bool atScrollingLimitAfter = scrollbarAtTopOfBottomOrDocument(verticalScrollbar);

Sometimes we use names like wasAtScrollingLimit/isAtScrollingLimit. But I think your names are fine, too.

Using the main frame's scrollbar to answer this question seems wrong. The main frame might be scrolled to the end, but the element you're scrolling (e.g., a <textarea>) might not be. We don't want the window to start bouncing in that case.

I guess the preexisting code had this bug, too. But we should fix it!

[Brian Weinstein]

Comment on attachment 88886 [details]
[PATCH] Fix

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

>> Source/WebKit2/UIProcess/PageClient.h:133
>> +    virtual void setGestureScrollingLimitReached(bool limitReached) = 0;
> 
> The parameter name "limitReached" adds no information, so it should be removed.  [readability/parameter_name] [5]

Removed.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:691
>> +}
> 
> This and the other Windows-specific functions should really be in WebPageProxyWin.cpp. Can you make a followup patch to move the functions?

I sure can. I'll put it as a follow-up and attach it to this bug.

>> Source/WebKit2/UIProcess/WebPageProxy.h:282
>>  #if ENABLE(TILED_BACKING_STORE)
> 
> The parameter name "limitReached" adds no information, so it should be removed.  [readability/parameter_name] [5]

Removed.

>> Source/WebKit2/UIProcess/WebPageProxy.messages.in:234
>> +	SetGestureScrollingLimitReached(bool limitReached)
> 
> Line contains tab character.  [whitespace/tab] [5]

Whoa.

>> Source/WebKit2/UIProcess/WebPageProxy.messages.in:235
>> +#endif
> 
> Line contains tab character.  [whitespace/tab] [5]

Whoa.

>> Source/WebKit2/UIProcess/win/WebView.cpp:586
>> +        if (m_gestureScrollingLimitReached)
> 
> Do we need to reset this when the gesture ends? Otherwise it seems like the wrong thing would happen if you:
> 
> 1) Gesture-scroll to the end of the document
> 2) Scroll back up a little by clicking on the scrollbar
> 3) Gesture-scroll down again

The worry I had when resetting it was the case:

1) Gesture-scroll to end of document
2) End gesture
3) Scroll down from the bottom of the document.

m_gestureScrollingLimitReached will be false, but when the gesture starts again, we will never get the setGestureScrollingLimitReached call (unless I force it to be called by setting a flag in WebPageWin each time we start a new gesture). I'll reset it and then send the message every time in the start of the gesture.

>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:425
>> +    bool atScrollingLimitAfter = scrollbarAtTopOfBottomOrDocument(verticalScrollbar);
> 
> Sometimes we use names like wasAtScrollingLimit/isAtScrollingLimit. But I think your names are fine, too.
> 
> Using the main frame's scrollbar to answer this question seems wrong. The main frame might be scrolled to the end, but the element you're scrolling (e.g., a <textarea>) might not be. We don't want the window to start bouncing in that case.
> 
> I guess the preexisting code had this bug, too. But we should fix it!

I'll switch to wasAtScrollingLimit/isAtScrollingLimit. I like that better.

So, we want to bounce in the case where the element you are scrolling is the main frame, and you've scrolled it to the end. Any ideas in how to accomplish that? The pre-existing code had this bug as well (as did the WK1 code).

[Adam Roben (:aroben)]

Comment on attachment 88886 [details]
[PATCH] Fix

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

>>> Source/WebKit2/UIProcess/win/WebView.cpp:586
>>> +        if (m_gestureScrollingLimitReached)
>> 
>> Do we need to reset this when the gesture ends? Otherwise it seems like the wrong thing would happen if you:
>> 
>> 1) Gesture-scroll to the end of the document
>> 2) Scroll back up a little by clicking on the scrollbar
>> 3) Gesture-scroll down again
> 
> The worry I had when resetting it was the case:
> 
> 1) Gesture-scroll to end of document
> 2) End gesture
> 3) Scroll down from the bottom of the document.
> 
> m_gestureScrollingLimitReached will be false, but when the gesture starts again, we will never get the setGestureScrollingLimitReached call (unless I force it to be called by setting a flag in WebPageWin each time we start a new gesture). I'll reset it and then send the message every time in the start of the gesture.

That sounds good to me.

>>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:425
>>> +    bool atScrollingLimitAfter = scrollbarAtTopOfBottomOrDocument(verticalScrollbar);
>> 
>> Sometimes we use names like wasAtScrollingLimit/isAtScrollingLimit. But I think your names are fine, too.
>> 
>> Using the main frame's scrollbar to answer this question seems wrong. The main frame might be scrolled to the end, but the element you're scrolling (e.g., a <textarea>) might not be. We don't want the window to start bouncing in that case.
>> 
>> I guess the preexisting code had this bug, too. But we should fix it!
> 
> I'll switch to wasAtScrollingLimit/isAtScrollingLimit. I like that better.
> 
> So, we want to bounce in the case where the element you are scrolling is the main frame, and you've scrolled it to the end. Any ideas in how to accomplish that? The pre-existing code had this bug as well (as did the WK1 code).

Maybe we need scrollByRecursively to tell us what happened? Layout folks (Simon, Dan, Hyatt) might have ideas.

[Brian Weinstein]

Created attachment 88900 [details]
[PATCH] Fix v2

[Adam Roben (:aroben)]

Comment on attachment 88900 [details]
[PATCH] Fix v2

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

"GestureReachedScrollingLimit" might be slightly better than "GestureScrollingLimitReached". The former name implies to me that a particular gesture reached the limit, which is I think what we want to convey. It also seems more parallel with GestureWillBegin, GestureDidScroll, GestureDidEnd, etc.

> Source/WebKit2/ChangeLog:33
> +            and send a message if the avlue changed.

Typo: avlue

> Source/WebKit2/UIProcess/win/WebView.cpp:581
>          } else if (gi.dwFlags & GF_END) {
>              EndPanningFeedbackPtr()(m_window, true);
> +            m_gestureScrollingLimitReached = false;

This probably isn't needed, since you always set the flag to false when you get GF_BEGIN.

> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:410
> +static bool scrollbarAtTopOfBottomOrDocument(Scrollbar* scrollbar)
>  {
> -    atBeginningOrEndOfScrollableDocument = false;
> +    if (!scrollbar)
> +        return false;

Don't we want to return true when there's no scrollbar? If the document isn't scrollable, then clearly we're at both the beginning *and* the end.

> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:423
> +    if (Frame* frame = m_page->mainFrame())
> +        if (ScrollView* view = frame->view())
> +            verticalScrollbar = view->verticalScrollbar();

The outer if needs braces around its body. (I'm surprised the style bot didn't catch this.)

> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:439
>  void WebPage::gestureDidEnd()
>  {
> +    m_gestureScrollingLimitReached = false;

This isn't really needed, since you reset the flag whenever you get gestureWillBegin.

[Brian Weinstein]

Comment on attachment 88900 [details]
[PATCH] Fix v2

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

>> Source/WebKit2/ChangeLog:33
>> +            and send a message if the avlue changed.
> 
> Typo: avlue

Fixed.

>> Source/WebKit2/UIProcess/win/WebView.cpp:581
>> +            m_gestureScrollingLimitReached = false;
> 
> This probably isn't needed, since you always set the flag to false when you get GF_BEGIN.

Removed.

>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:410
>> +        return false;
> 
> Don't we want to return true when there's no scrollbar? If the document isn't scrollable, then clearly we're at both the beginning *and* the end.

Well, if the user scrolled a scrollable div or text field in a document without a main frame scrollbar, we don't want the window to bounce when the user scrolls that scrollable div.

I'm going to change the caller to:

scrollbar && scrollbarAtTopOrBottomOfDocument, and ASSERT_ARG(scrollbar) here.

Plus I'll fix the typo in the function name :-).

>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:423
>> +            verticalScrollbar = view->verticalScrollbar();
> 
> The outer if needs braces around its body. (I'm surprised the style bot didn't catch this.)

Fixed.

>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:439
>> +    m_gestureScrollingLimitReached = false;
> 
> This isn't really needed, since you reset the flag whenever you get gestureWillBegin.

Removed.

[Brian Weinstein]

Created attachment 89031 [details]
[PATCH] Fix v3

[Adam Roben (:aroben)]

Comment on attachment 89031 [details]
[PATCH] Fix v3

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

> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:433
> +    if (gestureReachedScrollingLimit != m_gestureReachedScrollingLimit) {

You could make this an early return.

[Brian Weinstein]

Landed in r83460.