[webkit-reviews] review denied: [Bug 27850] Panning by emulating Mouse Wheel events breaks Google Maps : [Attachment 35215] Take 3 Addressing Adam's Comments

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 27850: Panning by emulating Mouse Wheel events breaks Google Maps
https://bugs.webkit.org/show_bug.cgi?id=27850

Attachment 35215: Take 3 Addressing Adam's Comments
https://bugs.webkit.org/attachment.cgi?id=35215&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> @@ -319,6 +319,7 @@ WebView::WebView()
>  , m_lastPanY(0)
>  , m_xOverpan(0)
>  , m_yOverpan(0)
> +, m_gestureTargetNode(0)

Since m_gestureTargetNode is a RefPtr, you don't need to explicitly initialize
it. RefPtr's default constructor initializes it to 0.

> +    HitTestRequest request(HitTestRequest::ReadOnly);
> +    for (Frame* childFrame = m_page->mainFrame();
> +	   childFrame && !hitScrollbar && childFrame->document()->renderView()
&& childFrame->document()->renderView()->layer();
> +	   childFrame =
EventHandler::subframeForTargetNode(m_gestureTargetNode.get())) {
> +	   HitTestResult
result(childFrame->view()->screenToContents(gestureBeginPoint));
> +	   childFrame->document()->renderView()->layer()->hitTest(request,
result);
> +	   m_gestureTargetNode = result.innerNode();
> +	   hitScrollbar = result.scrollbar();
> +    }

I think this might be a little clearer if you simplify the loop conditions and
use a few more local variables:

for (Frame* frame = m_page->mainFrame(); frame; frame =
EventHandler::subframeForTargetNode(frame)) {
    FrameView* frameView = frame->view();
    if (!frameView)
	break;
    RenderView* renderView = frame->document()->renderView();
    if (!renderView)
	break;
    RenderLayer* layer = renderView->layer();
    if (!layer)
	break;

    HitTestResult result(frameView->screenToContents(...));
    layer->hitTest(...);

    m_gestureTargetNode = result.innerNode();
    hitScrollbar = result.scrollbar();

    if (hitScrollbar)
	break;
}

> +    // The hit testing above won't detect if we've hit the main frame's
vertical scrollbar. Check that manually now.
> +    RECT webViewRect;
> +    GetWindowRect(m_viewWindow, &webViewRect);
> +    hitScrollbar |= view->verticalScrollbar() && (gestureBeginPoint.x >
(webViewRect.right -
view->verticalScrollbar()->theme()->scrollbarThickness()));  

Seems like we could skip all this if hitScrollbar is already true.

> +    bool canBeScrolled = false;
> +    if (m_gestureTargetNode.get()) {

No need for .get() here.

> +	   if (!m_gestureTargetNode ||
!m_gestureTargetNode->document()->frame()
> +	       || !m_gestureTargetNode->document()->frame()->page())
> +	       return false;
> +
> +	   // We negate here since panning up moves the content up, but moves
the scrollbar down.
> +	  
m_gestureTargetNode->renderer()->enclosingLayer()->scrollByRecursively(-deltaX,
-deltaY);

I guess it would be less roundabout to just null-check
m_gestureTargetNode->renderer() instead of checking that we still have a page.
That should catch all the same conditions, and should also catch cases where
the node is now display:none (in which case it won't have a renderer).

> +	   // FIXME: If the user starts panning down after a window bounce has
started, the window doesn't bounce back 
> +	   // until they release their finger.

Sounds like this deserves a bug report.