Bug 65044 – [chromium] Layering violations in gesture recognizer

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Robert Kroeger]

The PlatformGestureRecognizer and its Chromium platform implementation violate WebKit layering conventions. Prior to further extending the gesture recognizer, this needs to be corrected.

[Robert Kroeger]

Created attachment 101893 [details]
Patch

[Robert Kroeger]

Adam, please have a look at this patch. I have refactored to correct the layering violation that you pointed out and that we have previously discussed. I think the changes have made for a better API. Note that I have not fixed any of the pre-existing issues in the code -- I'll fix them next if this looks reasonable.

[Robert Kroeger]

Created attachment 101894 [details]
Patch

[Adam Barth]

Comment on attachment 101894 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101894&action=review

This looks good.  Below are a bunch of nit-picky style issues, but I'd like to see this patch one more time before landing.  Thanks for the patch!

> Source/WebCore/ChangeLog:12
> +    Divided the gesture recognizer up to correct a layering
> +    violation by moving gesture implementation from it to
> +    EventHandler::handleGestureEvent so that the gesture recognizer
> +    could simply be an engine for generating gesture events from
> +    touch events.

Look like you might have trouble with tabs / indent.

> Source/WebCore/page/EventHandler.cpp:2225
> +        // FIXME: refactor for hit testing only once having addressed the above fixme.

Can you make this comment a complete sentence?

> Source/WebCore/page/EventHandler.cpp:2234
> +        }
> +        break;

Do we need this break if we always return true?  (Also, the } should be indented one level less.)

> Source/WebCore/page/EventHandler.cpp:2236
> +        // FIXME: interim implementation pending addressing the fixme above.

Comments should be complete sentences.

> Source/WebCore/page/EventHandler.cpp:2237
> +        PlatformWheelEvent syntheticWheelEvent(gestureEvent.position(), gestureEvent.globalPosition(), gestureEvent.deltaX(), gestureEvent.deltaY(), gestureEvent.deltaX() / 120.0f, gestureEvent.deltaY() / 120.0f, ScrollByPixelWheelEvent, /* isAccepted */ false, gestureEvent.shiftKey(), gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey());

Should 120.0f be a named constant?

> Source/WebCore/page/EventHandler.cpp:2241
> +        }
> +        break;

Same comment.

> Source/WebCore/page/EventHandler.cpp:2248
> +        view->handleGestureEvent(gestureEvent);

Strange that this case falls through but the other cases return true explicitly.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:37
>  

Can we remove the EventHandler include now?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:106
> +void InnerGestureRecognizer::constructClickGestureEvent(const PlatformTouchPoint& touchPoint, Gestures* gestures)

construct => append ?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:118
> +    Vector<PlatformGestureEvent>* gestures = new Vector<PlatformGestureEvent>();

OwnPtr pls.  We shouldn't have (hardly) any "naked new" calls.  In this case, new should be wrapped in adoptPtr immediately.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:130
> +void InnerGestureRecognizer::constructScrollGesture(const PlatformTouchPoint& touchPoint, Gestures *gestures)

construct => append as well, probably.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:534
> +// FIXME: Opportunity to refactor this with EventHandler::handleGestureEvent

Another comment that should be a complete sentence.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:547
> +        return true;
> +        }
> +        break;

Same comment.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:555
> +    default:
> +        break;

We usually omit the default case and list any non-handled enums explicitly.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1999
> +        webView->resetGestureRecognizer();

Do you mean to intent this line this far?  If so, you might need { } around the if above.

> Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:145
> +    m_widget->handleGestureEvent((*gestureEvents)[i]);

Bad indent

> Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:268
> +        return TouchEvent(*static_cast<const WebTouchEvent*>(&inputEvent));
> +        break;

Another example of return-followed-by-break.

> Source/WebKit/chromium/src/WebViewImpl.cpp:771
> +    bool defaultPrevented(mainFrameImpl()->frame()->eventHandler()->handleTouchEvent(touchEventBuilder));

Please use the assignment form of the constructor.

> Source/WebKit/chromium/src/WebViewImpl.cpp:776
> +            mainFrameImpl()->frame()->eventHandler()->handleGestureEvent((*gestureEvents)[i]);

Another wild intent.

(*gestureEvents)[i] can be written more nicely as gestureEvents->at(i)

[Robert Kroeger]

Comment on attachment 101894 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101894&action=review

>> Source/WebCore/ChangeLog:12
>> +    touch events.
> 
> Look like you might have trouble with tabs / indent.

apparently my "clever" script to check spacing is buggy. fixed, p3

>> Source/WebCore/page/EventHandler.cpp:2225
>> +        // FIXME: refactor for hit testing only once having addressed the above fixme.
> 
> Can you make this comment a complete sentence?

done, p3

>> Source/WebCore/page/EventHandler.cpp:2234
>> +        break;
> 
> Do we need this break if we always return true?  (Also, the } should be indented one level less.)

done, p3

>> Source/WebCore/page/EventHandler.cpp:2236
>> +        // FIXME: interim implementation pending addressing the fixme above.
> 
> Comments should be complete sentences.

done, p3

>> Source/WebCore/page/EventHandler.cpp:2237
>> +        PlatformWheelEvent syntheticWheelEvent(gestureEvent.position(), gestureEvent.globalPosition(), gestureEvent.deltaX(), gestureEvent.deltaY(), gestureEvent.deltaX() / 120.0f, gestureEvent.deltaY() / 120.0f, ScrollByPixelWheelEvent, /* isAccepted */ false, gestureEvent.shiftKey(), gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey());
> 
> Should 120.0f be a named constant?

Yes. Done. And I tried to make it more obvious where the constant came from.

>> Source/WebCore/page/EventHandler.cpp:2241
>> +        break;
> 
> Same comment.

done, p3

>> Source/WebCore/page/EventHandler.cpp:2248
>> +        view->handleGestureEvent(gestureEvent);
> 
> Strange that this case falls through but the other cases return true explicitly.

I needed one outside the switch to make the compiler happy. Added one inside for consistency.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:37
>>  
> 
> Can we remove the EventHandler include now?

Definitely. Oversight rectified in p3.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:106
>> +void InnerGestureRecognizer::constructClickGestureEvent(const PlatformTouchPoint& touchPoint, Gestures* gestures)
> 
> construct => append ?

done in p3.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:118
>> +    Vector<PlatformGestureEvent>* gestures = new Vector<PlatformGestureEvent>();
> 
> OwnPtr pls.  We shouldn't have (hardly) any "naked new" calls.  In this case, new should be wrapped in adoptPtr immediately.

done in p3.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:130
>> +void InnerGestureRecognizer::constructScrollGesture(const PlatformTouchPoint& touchPoint, Gestures *gestures)
> 
> construct => append as well, probably.

done in p3

>> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:534
>> +// FIXME: Opportunity to refactor this with EventHandler::handleGestureEvent
> 
> Another comment that should be a complete sentence.

done in p3

>> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:547
>> +        break;
> 
> Same comment.

done, p3

>> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:555
>> +        break;
> 
> We usually omit the default case and list any non-handled enums explicitly.

done, p3

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1999
>> +        webView->resetGestureRecognizer();
> 
> Do you mean to intent this line this far?  If so, you might need { } around the if above.

fixed.

>> Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:145
>> +    m_widget->handleGestureEvent((*gestureEvents)[i]);
> 
> Bad indent

fixed, p3

>> Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:268
>> +        break;
> 
> Another example of return-followed-by-break.

also fixed. p3

>> Source/WebKit/chromium/src/WebViewImpl.cpp:771
>> +    bool defaultPrevented(mainFrameImpl()->frame()->eventHandler()->handleTouchEvent(touchEventBuilder));
> 
> Please use the assignment form of the constructor.

done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:776
>> +            mainFrameImpl()->frame()->eventHandler()->handleGestureEvent((*gestureEvents)[i]);
> 
> Another wild intent.
> 
> (*gestureEvents)[i] can be written more nicely as gestureEvents->at(i)

nice. thanks. done in p3

[Robert Kroeger]

Created attachment 102044 [details]
Patch

[Adam Barth]

Comment on attachment 102044 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102044&action=review

This looks great.  The comments below are only about whitespace.  :)

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:52
> +    typedef Vector<PlatformGestureEvent>*  Gestures;

Looks like you've got an extra space on this line.

> Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:269
>      default:
>          break;

I know this was here already, but the default case should be removed.

> Source/WebKit/chromium/src/WebPopupMenuImpl.h:150
> +
> +    

These lines are extraneous.

> Source/WebKit/chromium/src/WebViewImpl.cpp:776
> +            mainFrameImpl()->frame()->eventHandler()->handleGestureEvent(gestureEvents->at(i));

Too much indent.

[Robert Kroeger]

Comment on attachment 102044 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102044&action=review

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:52
>> +    typedef Vector<PlatformGestureEvent>*  Gestures;
> 
> Looks like you've got an extra space on this line.

done, p4

>> Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:269
>>          break;
> 
> I know this was here already, but the default case should be removed.

done, p4. Thanks for encouraging always making it better.

>> Source/WebKit/chromium/src/WebPopupMenuImpl.h:150
>> +    
> 
> These lines are extraneous.

done, p4

>> Source/WebKit/chromium/src/WebViewImpl.cpp:776
>> +            mainFrameImpl()->frame()->eventHandler()->handleGestureEvent(gestureEvents->at(i));
> 
> Too much indent.

also done, p4

[Robert Kroeger]

Created attachment 102061 [details]
Patch

[Robert Kroeger]

Created attachment 102064 [details]
Patch

[Adam Barth]

Comment on attachment 102064 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 102064 [details]
Patch

Clearing flags on attachment: 102064

Committed r91809: <http://trac.webkit.org/changeset/91809>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

This patch had some trouble building on Chromium-Win.  I'm not entirely sure why:

http://build.webkit.org/builders/Chromium%20Win%20Release/builds/28808/steps/compile-webkit/logs/stdio

[Robert Kroeger]

Created attachment 102381 [details]
Patch

[Robert Kroeger]

Adam: please take another look. Fixed the build on windows.

[WebKit Review Bot]

Comment on attachment 102381 [details]
Patch

Clearing flags on attachment: 102381

Committed r92011: <http://trac.webkit.org/changeset/92011>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Zhenyao Mo]

OK, I rolled out this patch due to Chrome Win Link failure.

http://trac.webkit.org/changeset/92032

webkit-patch rollout didn't finish on my machine so I manually reopen this bug.

[Zhenyao Mo]

error LNK2019: unresolved external symbol "public: void __thiscall WebCore::ScrollableArea::handleGestureEvent(class WebCore::PlatformGestureEvent const &)" (?handleGestureEvent@ScrollableArea@WebCore@@QAEXABVPlatformGestureEvent@2@@Z) referenced in function "public: bool __thiscall WebCore::EventHandler::handleGestureEvent(class WebCore::PlatformGestureEvent const &)"

[Adam Barth]

I looked at trying to fix this before the rollout and I couldn't figure it out.

[Robert Kroeger]

(In reply to comment #21)
> I looked at trying to fix this before the rollout and I couldn't figure it out.

I'm ashamed that my theoretically corrected patch had the same problem. I'm also most perplexed: the patch passed on the windows layout try bots.

[Adam Barth]

Maybe we should try landing the patch in small pieces to narrow down the problem?

[Robert Kroeger]

I think I understand. The build problem with this patch seems to an example of http://code.google.com/p/chromium/issues/detail?id=85789 -- I changed features.gypi and some .o files which ought to have have been recompiled remained un-clobbered.

My try jobs passed because I had commented out experimental code in the files causing the link errors. This had the side-effect of accidentally forcing sufficient re-compilations. I removed the comments while preparing my final patch and assumed naively that it would not alter the build result. Shucks.

[Adam Barth]

So, we should re-land and clobber?  Maybe we should put the comments back in so they'll trigger the re-build?

[Robert Kroeger]

(In reply to comment #25)
> So, we should re-land and clobber?  Maybe we should put the comments back in so they'll trigger the re-build?

Clobbering would seem to be the reliable recommended course based on an irc discussion. I will upload a new patch tomorrow that compiles cleanly on windows and email the gardeners.

[Robert Kroeger]

Created attachment 102676 [details]
Patch

[Adam Barth]

Comment on attachment 102676 [details]
Patch

Is this patch going to require a clobber?

[Robert Kroeger]

(In reply to comment #28)
> (From update of attachment 102676 [details])
> Is this patch going to require a clobber?

yes. I sent email to the gardeners about this.

[Adam Barth]

Comment on attachment 102676 [details]
Patch

Ok.  Let's rock and roll.

[WebKit Review Bot]

Comment on attachment 102676 [details]
Patch

Clearing flags on attachment: 102676

Committed r92234: <http://trac.webkit.org/changeset/92234>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

Could you explain a bit what layering violation this is fixing?



On an unrelated note, I am really, really not a fan of PlatformGestureEvent::TapType.

The reason is the events you added with this patch are fundamentally different from the other GestureEvent. The native gesture and gesture from touch are orthogonal concepts.

On Windows, you get the native gesture events only if you do not handle touch events. So previously the PlatformGestureEvent had nothing to do with the touch integration.

You can quickly get into tricky issues. For example a PlatformGestureEvent for tap and hold would behave differently if it is the real platform event or a fake event originating from touch (one case cancel the touch event, the other does not).

I think we should not mix native gesture events and the one originating from full touch support.

[Robert Kroeger]

(In reply to comment #33)
> Could you explain a bit what layering violation this is fixing?


The previous version of the gesture recognizer code took a reference to a WebCore::EventHandler object that it used to inject synthetic events. Adam Barth told me that this violated a WebKit convention that WebCore objects (EventHandler) should never get passed to the platform layer (gesture recognizer). Dividing the code into gesture recognition creating gesture events and event handling implementation for gesture events resolved this violation.

> 
> 
> 
> On an unrelated note, I am really, really not a fan of PlatformGestureEvent::TapType.
> 
> The reason is the events you added with this patch are fundamentally different from the other GestureEvent. The native gesture and gesture from touch are orthogonal concepts.
> 
> On Windows, you get the native gesture events only if you do not handle touch events. So previously the PlatformGestureEvent had nothing to do with the touch integration.
> 
> You can quickly get into tricky issues. For example a PlatformGestureEvent for tap and hold would behave differently if it is the real platform event or a fake event originating from touch (one case cancel the touch event, the other does not).
> 
> I think we should not mix native gesture events and the one originating from full touch support.

Shucks. I'm happy to try to fix this. 

To preserve the layering separation from this patch, I need to have some kind of synthetic event that is constructed by the gesture recognizer and then handle these synthetic events.

So, what if I reverted the changes to PlatformGestureEvent and instead have the gesture recognizer create a completely separate alternative kind of event? I could call these "PlatformSyntheticEvents" maybe?

[Benjamin Poulain]

(In reply to comment #34)
> (In reply to comment #33)
> > Could you explain a bit what layering violation this is fixing?
> 
> 
> The previous version of the gesture recognizer code took a reference to a WebCore::EventHandler object that it used to inject synthetic events. Adam Barth told me that this violated a WebKit convention that WebCore objects (EventHandler) should never get passed to the platform layer (gesture recognizer). Dividing the code into gesture recognition creating gesture events and event handling implementation for gesture events resolved this violation.

Ok, I did not see that at all like that.

For me GestureManager was a WebCore object and happen to have some port specific part to it. I was hoping the code would evolve in such a way that the gesture recognition would become common to the ports.

This patch changed the GestureManager to be a Chromium specificity with a weird handling faking native gestures.

> To preserve the layering separation from this patch, I need to have some kind of synthetic event that is constructed by the gesture recognizer and then handle these synthetic events.
> 
> So, what if I reverted the changes to PlatformGestureEvent and instead have the gesture recognizer create a completely separate alternative kind of event? I could call these "PlatformSyntheticEvents" maybe?

What I was doing for WebKit2 is exposing new functions to event handler (in our case the gesture recognition happen on the UIProcess side): https://bugs.webkit.org/show_bug.cgi?id=65545

Scrolling and zooming are handled differently in some case on mobile (the page is suspended during the scrolling, and gets only one event when the scrolling ends).

I think PlatformSyntheticEvents can do the trick but I do not see the value over having specific functions in EventHandler. For touch interface, we generally also call functions prior to the events in order to highlight what is under the finger. This does not map easily into events.

[Benjamin Poulain]

And some background info about the layer violation problem :)

I am unhappy about the current state of gesture on the web because there are as many implementation as there are browsers which make the life impossible for content authors.

So what happen today is every author target the iOS implementation.

I was hopping to have something in common some day so we can end this mess (following iOS for compatibility).

[Robert Kroeger]

(In reply to comment #33)
> Could you explain a bit what layering violation this is fixing?
> 
> 
> 
> On an unrelated note, I am really, really not a fan of PlatformGestureEvent::TapType.
> 
> The reason is the events you added with this patch are fundamentally different from the other GestureEvent. The native gesture and gesture from touch are orthogonal concepts.

I definitely agree that TapType is different. The logical fix would be to create a different object type.

I persuaded myself that using PlatformGestureEvent was OK because PlatformGestureEvent::ScrollStart and ::ScrollEnd align really well with how they're used for touch-initiated scroll.


> 
> On Windows, you get the native gesture events only if you do not handle touch events. So previously the PlatformGestureEvent had nothing to do with the touch integration.
> 
> You can quickly get into tricky issues. For example a PlatformGestureEvent for tap and hold would behave differently if it is the real platform event or a fake event originating from touch (one case cancel the touch event, the other does not).
> 
> I think we should not mix native gesture events and the one originating from full touch support.

OK. This sounds reasonable. I'll be sending you a CL that creates a different event-like object. I'll call it a SyntheticGestureEvent.

I imagine the following mapping:

SGE::OneTouchDown follows a touchPressed. Its position determines the target element for the remaining one-finger gestures. Link highlighting would be initiated here.

SGE::TapRelease dispatches the click after the TouchReleased

SGE::ScrollStart -> as PGE::ScrollStart after a TouchMoved
SGE::ScrollUpdate -> PGE::wheelEvent (updates velocity, etc.) after a TouchMoved
SGE::ScrollStop -> as PGE::ScrollStop (finger has left screen, flick in progress if velocity is sufficient.)after the TouchReleased

[Robert Kroeger]

(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > Could you explain a bit what layering violation this is fixing?
> > 
> > 
> > The previous version of the gesture recognizer code took a reference to a WebCore::EventHandler object that it used to inject synthetic events. Adam Barth told me that this violated a WebKit convention that WebCore objects (EventHandler) should never get passed to the platform layer (gesture recognizer). Dividing the code into gesture recognition creating gesture events and event handling implementation for gesture events resolved this violation.
> 
> Ok, I did not see that at all like that.
> 
> For me GestureManager was a WebCore object and happen to have some port specific part to it. I was hoping the code would evolve in such a way that the gesture recognition would become common to the ports.
> 
> This patch changed the GestureManager to be a Chromium specificity with a weird handling faking native gestures.
> 
> > To preserve the layering separation from this patch, I need to have some kind of synthetic event that is constructed by the gesture recognizer and then handle these synthetic events.
> > 
> > So, what if I reverted the changes to PlatformGestureEvent and instead have the gesture recognizer create a completely separate alternative kind of event? I could call these "PlatformSyntheticEvents" maybe?
> 
> What I was doing for WebKit2 is exposing new functions to event handler (in our case the gesture recognition happen on the UIProcess side): https://bugs.webkit.org/show_bug.cgi?id=65545
> 
> Scrolling and zooming are handled differently in some case on mobile (the page is suspended during the scrolling, and gets only one event when the scrolling ends).
> 
> I think PlatformSyntheticEvents can do the trick but I do not see the value over having specific functions in EventHandler. 

I think that the Synthetic objects enforce a separation between the recognition of gestures and the implementation of the gestures.

> For touch interface, we generally also call functions prior to the events in order to highlight what is under the finger. This does not map easily into events.

Per previous comment: this an be done with the two-phase event sequence. Highlight could be added to handleTouchEvent but doing via a synthetic event can keep the highlight out of the handleTouchEvent code.

[Robert Kroeger]

(In reply to comment #36)
> And some background info about the layer violation problem :)
> 
> I am unhappy about the current state of gesture on the web because there are as many implementation as there are browsers which make the life impossible for content authors.
> 
> So what happen today is every author target the iOS implementation.
> 
> I was hopping to have something in common some day so we can end this mess (following iOS for compatibility).

I am planning on adding support to the gesture recognizer to recognize touch sequences that should generate GestureEvents per http://developer.apple.com/library/safari/#documentation/UserExperience/Reference/GestureEventClassReference/GestureEvent/GestureEvent.html#//apple_ref/javascript/cl/GestureEvent

and using these to drive zoom animation.

I can easily modify the gesture recognizer to make it possible to have this feature separate from the existing scroll recognition. Thoughts?