Bug 35874

Created attachment 50240 [details] Initial patch

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.

(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.

Created attachment 50247 [details] First-round review changes

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 ;-)

(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.

Created attachment 50262 [details] Second-round review fixes

Comment on attachment 50262 [details] Second-round review fixes Clearing flags on attachment: 50262 Committed r55800: <http://trac.webkit.org/changeset/55800>

All reviewed patches have been landed. Closing bug.