Bug 58065 – WebKit2: Support window bounce when panning

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

[Brian Weinstein]

When panning, MSDN recommends using window bounce when you hit the end of a scrollable area to improve the panning experience.

http://msdn.microsoft.com/en-us/library/dd562167(v=vs.85).aspx

We support this in WebKit1 (vertically, not horizontally).

<rdar://problem/9244367>

[Brian Weinstein]

Created attachment 88667 [details]
[PATCH] Fix v2

[WebKit Review Bot]

Attachment 88667 [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/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:425:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/WebProcess/WebPage/WebPage.h:327:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 8 files


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

[Adam Roben (:aroben)]

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

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

>> Source/WebKit2/ChangeLog:1
>> [phone]  Brian Weinstein  <[e-mail]>
> 
> ChangeLog entry has no bug number  [changelog/bugnumber] [5]

Wha? This just seems completely wrong.

> Source/WebKit2/ChangeLog:12
> +        API to bounce the window to give an indication that you are past the an end

Typo: the an end

> Source/WebKit2/ChangeLog:16
> +        (WebKit::WebPageProxy::gestureDidScroll): Pasa a boolean for the reply, and return it.

Typo: Pasa

> Source/WebKit2/UIProcess/WebPageProxy.cpp:682
> +    bool atBeginningOrEndOfScrollableDocument;
> +    process()->sendSync(Messages::WebPage::GestureDidScroll(size), Messages::WebPage::GestureDidScroll::Reply(atBeginningOrEndOfScrollableDocument), m_pageID);
> +    return atBeginningOrEndOfScrollableDocument;

If sendSync fails (indicated by returning false), atBeginningOrEndOfScrollableDocument will be uninitialized. That's bad! (I think gestureWillBegin has the same problem.)

> Source/WebKit2/UIProcess/win/WebView.cpp:543
>      if (!GetGestureInfoPtr() || !CloseGestureInfoHandlePtr()) {
>          handled = false;
>          return 0;
>      }
>  
> +    if (!UpdatePanningFeedbackPtr() || !BeginPanningFeedbackPtr() || !EndPanningFeedbackPtr()) {
> +        handled = false;
> +        return 0;
> +    }

I'd combine these into a single check.

> Source/WebKit2/UIProcess/win/WebView.cpp:588
> +        // FIXME: Support horizontal window bounce - <rdar://problem/7158348>.
> +        // FIXME: Window Bounce doesnt' undo until user releases their finger - <rdar://problem/7158349>.

Typo: doesnt'

Do we have Bugzilla bugs that you can reference here instead?

>> Source/WebKit2/WebProcess/WebPage/WebPage.h:327
>> +    void gestureDidScroll(const WebCore::IntSize& size, bool& atBeginningOrEndOfDocument);
> 
> The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]

You should fix this.

> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:404
> -void WebPage::gestureDidScroll(const WebCore::IntSize& size)
> +void WebPage::gestureDidScroll(const WebCore::IntSize& size, bool& atBeginningOrEndOfScrollableDocument)

No need for WebCore:: here.

>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:425
>> +    atBeginningOrEndOfScrollableDocument = verticalScrollbar->currentPos() == 0 || verticalScrollbar->currentPos() >= verticalScrollbar->maximum();
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

You should fix this.

[Brian Weinstein]

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

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

>>> Source/WebKit2/ChangeLog:1
>>> [phone]  Brian Weinstein  <[e-mail]>
>> 
>> ChangeLog entry has no bug number  [changelog/bugnumber] [5]
> 
> Wha? This just seems completely wrong.

I typed it by hand, and typed show_big.cgi :-(. It's fixed.

>> Source/WebKit2/ChangeLog:12
>> +        API to bounce the window to give an indication that you are past the an end
> 
> Typo: the an end

Fixed.

>> Source/WebKit2/ChangeLog:16
>> +        (WebKit::WebPageProxy::gestureDidScroll): Pasa a boolean for the reply, and return it.
> 
> Typo: Pasa

Fixed.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:682
>> +    return atBeginningOrEndOfScrollableDocument;
> 
> If sendSync fails (indicated by returning false), atBeginningOrEndOfScrollableDocument will be uninitialized. That's bad! (I think gestureWillBegin has the same problem.)

I will initialize them both to false.

>> Source/WebKit2/UIProcess/win/WebView.cpp:543
>> +    }
> 
> I'd combine these into a single check.

Fixed.

>> Source/WebKit2/UIProcess/win/WebView.cpp:588
>> +        // FIXME: Window Bounce doesnt' undo until user releases their finger - <rdar://problem/7158349>.
> 
> Typo: doesnt'
> 
> Do we have Bugzilla bugs that you can reference here instead?

Fixed. I initially made bugzilla bugs for the WebKit1 equivalent, but the bugs don't seem to exist. I will file Bugzilla bugs.

>>> Source/WebKit2/WebProcess/WebPage/WebPage.h:327
>>> +    void gestureDidScroll(const WebCore::IntSize& size, bool& atBeginningOrEndOfDocument);
>> 
>> The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> You should fix this.

And I did.

>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:404
>> +void WebPage::gestureDidScroll(const WebCore::IntSize& size, bool& atBeginningOrEndOfScrollableDocument)
> 
> No need for WebCore:: here.

Fixed.

>>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:425
>>> +    atBeginningOrEndOfScrollableDocument = verticalScrollbar->currentPos() == 0 || verticalScrollbar->currentPos() >= verticalScrollbar->maximum();
>> 
>> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> 
> You should fix this.

I thought it was more readable this way, but will fix.

[Brian Weinstein]

Landed in r83197.