Make GestureRecognizerChromium aware of Frame

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Varun Jain]

Make GestureRecognizerChromium aware of Frame

[Varun Jain]

Created attachment 100696 [details]
Patch

[Varun Jain]

Make GestureRecognizerChromium aware of Frame instead of EventHandler. This is needed as we may need access to the dom to process certain gestures.

[Adam Barth]

Comment on attachment 100696 [details]
Patch

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

> Source/WebCore/platform/PlatformGestureRecognizer.h:59
> -    virtual bool processTouchEventForGesture(const PlatformTouchEvent&, EventHandler*, bool handled) = 0;
> +    virtual bool processTouchEventForGesture(const PlatformTouchEvent&, Frame*, bool handled) = 0;

Code in WebCore/platform cannot be aware of anything in WebCore outside WebCore/platform.  That means both EventHandler and Frame are forbidden.

[Varun Jain]

(In reply to comment #3)
> (From update of attachment 100696 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100696&action=review
> 
> > Source/WebCore/platform/PlatformGestureRecognizer.h:59
> > -    virtual bool processTouchEventForGesture(const PlatformTouchEvent&, EventHandler*, bool handled) = 0;
> > +    virtual bool processTouchEventForGesture(const PlatformTouchEvent&, Frame*, bool handled) = 0;
> 
> Code in WebCore/platform cannot be aware of anything in WebCore outside WebCore/platform.  That means both EventHandler and Frame are forbidden.

Hi Adam, I am not sure I understand.. there are already plenty of classes in WebCore/platform using Frame (See WebCore/platform/DragData.h, DragImage.h, PasteBoard.h for example). Are all these incorrect?

[Adam Barth]

> Are all these incorrect?

Yes.  :(

[Varun Jain]

(In reply to comment #5)
> > Are all these incorrect?
> 
> Yes.  :(

So is there a bug filed to fix these? In any case, what would be the correct way to do this then? Can you point me to an example?

[Adam Barth]

If GestureRecognizerChromium needs to interact with the DOM, it should move out of the platform directory.  Another option is to refactor the dependency so that it can interact with the DOM via some API.

Do the other GestureRecognizers need to interact with the DOM?

[Varun Jain]

(In reply to comment #7)
> If GestureRecognizerChromium needs to interact with the DOM, it should move out of the platform directory.  Another option is to refactor the dependency so that it can interact with the DOM via some API.
> 
> Do the other GestureRecognizers need to interact with the DOM?

There is only one GR for now and yes it does need to interact with the DOM.
I would like to revisit the dependency restriction you have mentioned though. On further looking up, I found many many references inside platform to non-platform stuff such as FrameView, Page, Document, Node, Range etc. This makes me wonder if there is a restriction at all. Can you point me to the relevant doc/webpage where this rule might be listed?

Regarding your suggestions:
I believe when rjkroege submitted the first version of the GestureRecognizer, there was extensive debate about where it should go and its current place was found to be the most agreeable. Moving it outside platform may not be an option (rjkroege can comment more on this)

That leaves us with the second option of creating a new API to interact with the DOM. Is there an example of this somewhere?

[Adam Barth]

I'm not sure where it's documented, but it's true.  However, we often screw it up because there's nothing forcing us to comply with the layering.  At some point, we'll move Platform out to be a top-level project, at which point we'll need to pay for all these layering violation sins.

[Adam Barth]

All that being said, you can probably find someone who's willing to review this patch with the layering violation, but it's not going to be me.

I don't really understand the constraints involved, which is making it hard for me to recommend the best course of action.  I don't really understand why a gesture recognizer should need to interact with the DOM.  Shouldn't it just recognize gestures?

[Varun Jain]

(In reply to comment #10)
> All that being said, you can probably find someone who's willing to review this patch with the layering violation, but it's not going to be me.
> 
> I don't really understand the constraints involved, which is making it hard for me to recommend the best course of action.  I don't really understand why a gesture recognizer should need to interact with the DOM.  Shouldn't it just recognize gestures?

For the most basic case, the GR will recognize gestures and do what?.. it will at the very least need to pass it to the event handler to process the gesture (EventHandler dependency). Also, parameters for some gestures are dependent on the DOM. Take for example, a double-tap-to-zoom gesture. The parameters such as how much to zoom and what to bring to focus can only be obtained from the DOM. This information is also platform specific because different platforms may want to have different interpretation of the gesture.

[Adam Barth]

I'm very ignorant in this area, but I would have expected a design something like the following:

1) The event system gets a bunch of low-level touch events (or whatever).
2) The event system asks the gesture recognizer to recognize them, getting back some high-level description of what sort of gesture it was.
3) The event system interacts with the DOM to perform the gesture.

You should think of WebCore/platform as a library that's useful for, but not necessarily coupled to, a web rendering engine.

In any case, I'll think it's best if I step back now and let some other reviewer handle this patch.  It's not really related to anything I have specific expertise about.

[Robert Kroeger]

Darin: Sorry to bother you but could you suggest another reviewer?

Adam & Darin: I'll take a bug to move the GR code to any location that is reviewer-approved but unless the separation of the Platform code is happening shortly, I'd like to get the GR code at least mostly complete first (including some gestures that do depend very much on having DOM access for their identification.)

Thoughts?

And (fwiw) I'm good with Varun's change after he makes it work on win.

[Varun Jain]

(In reply to comment #13)
> Darin: Sorry to bother you but could you suggest another reviewer?
> 
> Adam & Darin: I'll take a bug to move the GR code to any location that is reviewer-approved but unless the separation of the Platform code is happening shortly, I'd like to get the GR code at least mostly complete first (including some gestures that do depend very much on having DOM access for their identification.)
> 
> Thoughts?
> 
> And (fwiw) I'm good with Varun's change after he makes it work on win.

I think the error on win queue is un-related

[Robert Kroeger]

This ought to be closed as won't fix yes?

[Varun Jain]

Yes