[webkit-reviews] review granted: [Bug 58167] WebKit2: Windows 7 Gestures Window Bounce shouldn't require a sync message : [Attachment 88900] [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 58167: WebKit2: Windows 7 Gestures Window Bounce shouldn't require a sync
message
https://bugs.webkit.org/show_bug.cgi?id=58167

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

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
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.