Bug 35874 – [Chromium] Adding touch event routing

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Garret Kelly]

WebKit-side support for routing WebTouchEvents into the event handler.

[Garret Kelly]

Created attachment 50240 [details]
Initial patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 50240 [details]
Initial patch

> +++ b/WebKit/chromium/src/ChromeClientImpl.h
...
> +#if ENABLE(TOUCH_EVENTS)
> +    virtual void needTouchEvents(bool needTouchEvents) { }
> +#endif

^^^ is there more to be done here?  should there be a FIXME comment?


> +++ b/WebKit/chromium/src/WebViewImpl.cpp
> @@ -609,6 +609,22 @@ bool WebViewImpl::charEvent(const WebKeyboardEvent& event)
>      return true;
>  }
>  
> +#if ENABLE(TOUCH_EVENTS)
> +bool WebViewImpl::touchEvent(const WebTouchEvent& event)
> +{
> +    Frame* focusedFrame = focusedWebCoreFrame();
> +    if (!focusedFrame)
> +        return false;
> +
> +    EventHandler* eventHandler = focusedFrame->eventHandler();
> +    if (!eventHandler)
> +        return false;
> +
> +    PlatformTouchEventBuilder touchEventBuilder(mainFrameImpl()->frameView(), event);
> +    return eventHandler->handleTouchEvent(touchEventBuilder);
> +}

Are you sure that calling handleTouchEvent on the focused frame is correct?
For mouseDown, etc., we use the EventHandler of the main frame.

It looks like an EventHandler for a subframe will still perform a hit test
on the topmost frame if the subframe does not contain the point, so it may
not matter what EventHandler you use.  I was just wondering why you chose
the EventHandler of the focused frame.

[Garret Kelly]

(In reply to comment #2)
> (From update of attachment 50240 [details])
> > +++ b/WebKit/chromium/src/ChromeClientImpl.h
> ...
> > +#if ENABLE(TOUCH_EVENTS)
> > +    virtual void needTouchEvents(bool needTouchEvents) { }
> > +#endif
> 
> ^^^ is there more to be done here?  should there be a FIXME comment?

Quite right. FIXME added.
 
> > +++ b/WebKit/chromium/src/WebViewImpl.cpp
> > @@ -609,6 +609,22 @@ bool WebViewImpl::charEvent(const WebKeyboardEvent& event)
> >      return true;
> >  }
> >  
> > +#if ENABLE(TOUCH_EVENTS)
> > +bool WebViewImpl::touchEvent(const WebTouchEvent& event)
> > +{
> > +    Frame* focusedFrame = focusedWebCoreFrame();
> > +    if (!focusedFrame)
> > +        return false;
> > +
> > +    EventHandler* eventHandler = focusedFrame->eventHandler();
> > +    if (!eventHandler)
> > +        return false;
> > +
> > +    PlatformTouchEventBuilder touchEventBuilder(mainFrameImpl()->frameView(), event);
> > +    return eventHandler->handleTouchEvent(touchEventBuilder);
> > +}
> 
> Are you sure that calling handleTouchEvent on the focused frame is correct?
> For mouseDown, etc., we use the EventHandler of the main frame.
> 
> It looks like an EventHandler for a subframe will still perform a hit test
> on the topmost frame if the subframe does not contain the point, so it may
> not matter what EventHandler you use.  I was just wondering why you chose
> the EventHandler of the focused frame.

Your right, the hit test shouldn't start in the focused frame but in the topmost. Done.

[Garret Kelly]

Created attachment 50247 [details]
First-round review changes

[Darin Fisher (:fishd, Google)]

Comment on attachment 50247 [details]
First-round review changes

> +++ b/WebKit/chromium/src/WebViewImpl.cpp
...
> +#if ENABLE(TOUCH_EVENTS)
> +bool WebViewImpl::touchEvent(const WebTouchEvent& event)
> +{
> +    if (!mainFrameImpl() || !mainFrameImpl()->frameView())
> +        return false;
> +
> +    PlatformTouchEventBuilder touchEventBuilder(
> +        mainFrameImpl()->frameView(), event);
> +
> +    EventHandler* eventHandler = mainFrameImpl()->frame()->eventHandler();
> +    return eventHandler->handleTouchEvent(touchEventBuilder);

nit: it is more common in webkit code to just write this w/o the temporary:

  return mainFrameImpl()->frame()->eventHandler()->handleTouchEvent(touchEventBuilder);

it is OK that lines exceed 80 chars ;-)

[Garret Kelly]

(In reply to comment #5)
> (From update of attachment 50247 [details])
> > +++ b/WebKit/chromium/src/WebViewImpl.cpp
> ...
> > +#if ENABLE(TOUCH_EVENTS)
> > +bool WebViewImpl::touchEvent(const WebTouchEvent& event)
> > +{
> > +    if (!mainFrameImpl() || !mainFrameImpl()->frameView())
> > +        return false;
> > +
> > +    PlatformTouchEventBuilder touchEventBuilder(
> > +        mainFrameImpl()->frameView(), event);
> > +
> > +    EventHandler* eventHandler = mainFrameImpl()->frame()->eventHandler();
> > +    return eventHandler->handleTouchEvent(touchEventBuilder);
> 
> nit: it is more common in webkit code to just write this w/o the temporary:
> 
>   return
> mainFrameImpl()->frame()->eventHandler()->handleTouchEvent(touchEventBuilder);
> 
> it is OK that lines exceed 80 chars ;-)

Ah, my mistake. Force of habit. ;) Done.

[Garret Kelly]

Created attachment 50262 [details]
Second-round review fixes

[WebKit Commit Bot]

Comment on attachment 50262 [details]
Second-round review fixes

Clearing flags on attachment: 50262

Committed r55800: <http://trac.webkit.org/changeset/55800>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.