[webkit-reviews] review denied: [Bug 27850] Panning by emulating Mouse Wheel events breaks Google Maps : [Attachment 35207] Take 2

[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 35207: Take 2
https://bugs.webkit.org/attachment.cgi?id=35207&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +	   One downside of this fix is that panning will not jump out of frames
anymore (like pan-scrolling),

It isn't clear to me whether this is saying that pan-scrolling *does* jump out
of frames, or doesn't.

> +    HitTestResult
result(m_page->mainFrame()->view()->screenToContents(gestureBeginPoint));

Do you know whether view() can ever return null? I don't

> +    m_page->mainFrame()->document()->renderView()->layer()->hitTest(request,
result);

Do you know whether renderView() or layer() can ever return null? I don't.

It seems like this code could move down into the loop. It's exactly the same as
the loop code, except it uses mainFrame() instead of childFrame. A little
refactoring would reduce the duplication.

> +    m_gestureTargetNode = result.innerNode();
> +
> +    bool hitScrollbar = !!result.scrollbar();

No need for the !! here.

> +	   HitTestResult
result(childFrame->view()->screenToContents(gestureBeginPoint));
> +
> +	   childFrame->document()->renderView()->layer()->hitTest(request,
result);

Same questions here about what can and can't be null.

> +	   m_gestureTargetNode = result.innerNode();
> +	   bool hitScrollbar = !!result.scrollbar();

I don't think you meant to redeclare hitScrollbar. This declaration shadows the
one outside the loop.

> +    // If we hit a scrollbar, we won't be panning, so we don't want to set
> +    // m_gestureTarget node to prevent additional unncessary calculations.
> +    if (hitScrollbar)
> +	   m_gestureTargetNode = 0;

I think you should move all the code to retrieve m_gestureTargetNode into a
separate function.

> +    if (hitScrollbar || !canBeScrolled || (view->verticalScrollbar() &&
gestureBeginPoint.x > (webViewRect.right -
view->verticalScrollbar()->theme()->scrollbarThickness()))) {

I think it would be a little clearer to put the last part of this expression
into its own variable. I think you could even combine it with hitScrollbar:

// The hit-testing above won't detect if we've hit the main frame's vertical
scrollbar. Check that manually now.
if (view->verticalScrollbar() && gestureBeginPoint.x > (...))
    hitScrollbar = true;

if (hitScrollbar || !canBeScrolled) {
...

> +	   // If we didn't hit a scrollbar in our hit testing, the only other
scrollbar it would be 
> +	   // possible to hit would be the WebView scrollbar, so we can check
if our x coordinate is
> +	   // outside of the webview, where the scollbar vertical is.
> +	   // In this case, we are either over a scrollbar, or we can't scroll,
so we want to disable
> +	   // single finger panning.
>	   dwPanWant = GC_PAN  | GC_PAN_WITH_INERTIA | GC_PAN_WITH_GUTTER;
>	   dwPanBlock = GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY |
GC_PAN_WITH_SINGLE_FINGER_VERTICALLY;
>      } else {
> +	   // Someone in our DOM Hierarchy can scroll, and we're not on a
scrollbar, let them pan.
>	   dwPanWant = GC_PAN  | GC_PAN_WITH_SINGLE_FINGER_VERTICALLY |
GC_PAN_WITH_INERTIA | GC_PAN_WITH_GUTTER;
>	   dwPanBlock = GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY;
>      }

I think it would be clearer to combine these a little more, like this:

// We always allow two-fingered panning with inertia and a gutter (which limits
movement to one
// direction in most cases).
DWORD dwPanWant = GC_PAN | GC_PAN_WITH_INERTIA | GC_PAN_WITH_GUTTER;
// We never allow single-fingered horizontal panning. That gesture is reserved
for creating text
// selections. This matches IE.
DWORD dwPanBlock = GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY;

if (!canBeScrolled || hitScrollbar) {
    // The part of the page under the gesture can't be scrolled, or the gesture
is on a scrollbar.
    // Disallow single-fingered vertical panning in this case, too, so we'll
fall back to the default
    // behavior (which allows the scrollbar thumb to be dragged, text
selections to be made, etc.).
    dwPanBlock |= GC_PAN_WITH_SINGLE_FINGER_VERTICALLY;
}

> -    GESTURECONFIG gc = { GID_PAN, dwPanWant , dwPanBlock } ;
> -    return SetGestureConfigPtr()(m_viewWindow, 0, 1, &gc,
sizeof(GESTURECONFIG));
> +    GESTURECONFIG gc = { GID_PAN, dwPanWant, dwPanBlock };
> +    return SetGestureConfigPtr()(gn->hwndTarget, 0, 1, &gc,
sizeof(GESTURECONFIG));
>  }

Was it a bug that we didn't use gn->hwndTarget before? This change should be
mentioned in the ChangeLog. If this change is not directly related to fixing
this particular bug, maybe it should be done in its own separate patch. (Ditto
for similar changes in WebView::gesture.)

> @@ -1396,20 +1432,23 @@ bool WebView::gesture(WPARAM wParam, LPA
>	       CloseGestureInfoHandlePtr()(gestureHandle);
>	       return false;
>	   }
> -	   // Represent the pan gesture as a mouse wheel event
> -	   PlatformWheelEvent wheelEvent(m_viewWindow, FloatSize(deltaX,
deltaY), FloatPoint(currentX, currentY));
> -	   coreFrame->eventHandler()->handleWheelEvent(wheelEvent);
>  
> +	   if (!m_gestureTargetNode)
> +	       return false;

Don't we need to call CloseGestureInfoHandle here?

> +	   // We negate here since panning up moves the screen down, and vice
versa.

I'd say that panning up moves the content up, but moves the scrollbar down.

>      default:
> -	   // We have encountered an unknown gesture - return false to pass it
to DefWindowProc
> -	   CloseGestureInfoHandlePtr()(gestureHandle);
>	   break;
>      }
>  
> -    return true;
> +    return false;

Don't we still need to call CloseGestureInfoHandle in the default case?

> +    WebCore::Node* m_gestureTargetNode;

What guarantees that m_gestureTargetNode has not been destroyed by the time you
use it in WebView::gesture?