[webkit-reviews] review granted: [Bug 58065] WebKit2: Support window bounce when panning : [Attachment 88667] [PATCH] Fix v2

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 58065: WebKit2: Support window bounce when panning
https://bugs.webkit.org/show_bug.cgi?id=58065

Attachment 88667: [PATCH] Fix v2
https://bugs.webkit.org/attachment.cgi?id=88667&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=88667&action=review

>> Source/WebKit2/ChangeLog:1
>> +2011-04-07	Brian Weinstein  <bweinstein at apple.com>
> 
> 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.