Bug 92281 – Propagate gesture events to plugins.

• Component: New Bugs
• Status: RESOLVED FIXED

[Sadrul Habib Chowdhury]

Propagate gesture events to plugins.

[Sadrul Habib Chowdhury]

Created attachment 154420 [details]
Patch

[Adam Barth]

Comment on attachment 154420 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        to Javascript, since there is no spec for that at the moment. Exposing gesture events

Javascript -> JavaScript

> Source/WebCore/ChangeLog:12
> +        No new tests (OOPS!).

This line is going to prevent this patch from landing.  Can we add some tests for this patch?

[Sadrul Habib Chowdhury]

Created attachment 154434 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 154420 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        to Javascript, since there is no spec for that at the moment. Exposing gesture events
> 
> Javascript -> JavaScript

Done.

>> Source/WebCore/ChangeLog:12
>> +        No new tests (OOPS!).
> 
> This line is going to prevent this patch from landing.  Can we add some tests for this patch?

I could not think of a good way for testing this. EventSender has support for generating gesture-events, however, I am not sure how we can test that the gesture reached the plugin. Do you have suggestions? (once the gesture events reach chromium plugins, we will add a browser-test to verify that the gestures reach the plugin, but I am unsure how to test this in a layouttest)

[Early Warning System Bot]

Comment on attachment 154420 [details]
Patch

Attachment 154420 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13344398

[Sadrul Habib Chowdhury]

Created attachment 154449 [details]
Patch

[Sadrul Habib Chowdhury]

Created attachment 154686 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 154686 [details]
Patch

Added an implementation for WebPluginContainerImpl::handleGestureEvent, updated TestWebPlugin to receive events and log the events it receives, and used that to add a new layout test. Please take another look.

[Adam Barth]

Comment on attachment 154686 [details]
Patch

This looks good on a first pass.  Someone (possibly me) will need to review it in more detail.

[Adam Barth]

Comment on attachment 154686 [details]
Patch

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

> Source/WebCore/dom/GestureEvent.h:56
> +    GestureEvent() { }

Should this constructor initialize m_deltaX and m_deltaY ?

[Sadrul Habib Chowdhury]

Created attachment 154721 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 154686 [details]
Patch

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

>> Source/WebCore/dom/GestureEvent.h:56
>> +    GestureEvent() { }
> 
> Should this constructor initialize m_deltaX and m_deltaY ?

Indeed it should. Thanks for catching this!

[WebKit Review Bot]

Comment on attachment 154721 [details]
Patch

Clearing flags on attachment: 154721

Committed r123799: <http://trac.webkit.org/changeset/123799>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Why were no Apple people cc'd on this bug?

[Adam Barth]

> Why were no Apple people cc'd on this bug?

If you'd like to be automatically CCed on patches, you might want to add yourself to one or more watchlists:

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/watchlist

[Adam Barth]

I guess I don't understand your question.  Can you explain more about who you think should be been CCed and why?

[Simon Fraser (smfr)]

It seems like this patch is adding major functionality that people other than Chromium people would be interested in, especially the owners of plug-in code like Anders. His comment was "that code is so broken", so you may benefit from his interest.

[Adam Barth]

I certainly welcome any feedback that you or Anders have on this or any other patch.

[WebKit Review Bot]

Re-opened since this is blocked by 92431

[Adam Barth]

@smfr: Did you have any constructive comments for sadrul?  He need to re-work the patch to compile on apple-mac, and it would be more efficient to incorporate any comments you have in the same iteration.

[Simon Fraser (smfr)]

I'll defer to anders.

[Sadrul Habib Chowdhury]

Created attachment 154984 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 154984 [details]
Patch

andersca@ Hi! Do you have suggestions to make this patch better? Thanks!

[Build Bot]

Comment on attachment 154984 [details]
Patch

Attachment 154984 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13374624

[Sadrul Habib Chowdhury]

Created attachment 155007 [details]
Patch

[Anders Carlsson]

Is GestureEvent a real HTML5 DOM event? If not, it's very weird that we're adding it to dom for the purpose of "wrapping" a PlatformGestureEvent. If it is, then it should really be added as a separate patch.

To expand on my "this code is so broken" comment - there's a serious layering violation in this code in that Widget::handleEvent takes a DOM event. (This has been the case ever since this code was added by me over 5 years ago).

What I'd really like to see is a way for widgets to receive PlatformEvents directly and get rid of this mess where we convert back and forth between DOM events.

(In fact, in WebKit2's plug-in implementation we don't even look at the passed in event - we just keep track of the current PlatformEvent event being handled and send that directly to the plug-in).

[Build Bot]

Comment on attachment 155007 [details]
Patch

Attachment 155007 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13371623

[Adam Barth]

> Is GestureEvent a real HTML5 DOM event?

Not yet, but given that Microsoft has implemented gesture events (i.e., http://msdn.microsoft.com/en-us/library/ie/hh968249(v=vs.85).aspx) and strong interest from web developers, I would expect them to make their way in to DOM events in the near term.

> To expand on my "this code is so broken" comment - there's a serious layering violation in this code in that Widget::handleEvent takes a DOM event. (This has been the case ever since this code was added by me over 5 years ago).
> 
> What I'd really like to see is a way for widgets to receive PlatformEvents directly and get rid of this mess where we convert back and forth between DOM events.

That make sense.  I haven't looked at what's involved in that refactoring.  Do you think we should try to make that happen before adding this feature?

[Sadrul Habib Chowdhury]

(In reply to comment #29)
> > Is GestureEvent a real HTML5 DOM event?
> 
> Not yet, but given that Microsoft has implemented gesture events (i.e., http://msdn.microsoft.com/en-us/library/ie/hh968249(v=vs.85).aspx) and strong interest from web developers, I would expect them to make their way in to DOM events in the near term.
> 
> > To expand on my "this code is so broken" comment - there's a serious layering violation in this code in that Widget::handleEvent takes a DOM event. (This has been the case ever since this code was added by me over 5 years ago).
> > 
> > What I'd really like to see is a way for widgets to receive PlatformEvents directly and get rid of this mess where we convert back and forth between DOM events.

If I understand you correctly, you think this should happen for all kinds of platform-events, and not just gesture events?

> 
> That make sense.  I haven't looked at what's involved in that refactoring.  Do you think we should try to make that happen before adding this feature?

Right now, plugins can preventDefault() an event from the handleEvent callback. There is no corresponding notion for platform-events. So when this changes, I suspect there will need to be a way to notify the event was prevent-defaulted by the plugins? How does WebKit2 does this?

[Sadrul Habib Chowdhury]

Created attachment 155046 [details]
Patch

patch with build fix

[Adam Barth]

Comment on attachment 155046 [details]
Patch

After the discussion above, my sense is that this is the right approach given that we expect to expose gesture events to JavaScript once we get a spec we're happy with.  As for refactoring plugins to use PlatformEvents directly, I don't have much of an opinion on that topic, but I don't think it's something that should block this patch.

[WebKit Review Bot]

Comment on attachment 155046 [details]
Patch

Clearing flags on attachment: 155046

Committed r124098: <http://trac.webkit.org/changeset/124098>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch broke Qt Windows build:
http://build.webkit.org/builders/Qt%20Windows%2032-bit%20Debug/builds/47298
http://build.webkit.org/builders/Qt%20Windows%2032-bit%20Debug/builds/47298/steps/compile-webkit/logs/stdio

[Ryosuke Niwa]

Build fix attempt landed in http://trac.webkit.org/changeset/124155.

[Ryosuke Niwa]

Another build fix attempt landed in http://trac.webkit.org/changeset/124159.

"interface" is a keyword. Don't use it as a variable name.

[Sadrul Habib Chowdhury]

(In reply to comment #37)
> Another build fix attempt landed in http://trac.webkit.org/changeset/124159.
> 
> "interface" is a keyword. Don't use it as a variable name.

Oops. My bad. Thank you very much for taking care of the build breakages.