RESOLVED FIXED 92281
Propagate gesture events to plugins.
https://bugs.webkit.org/show_bug.cgi?id=92281
Summary Propagate gesture events to plugins.
Sadrul Habib Chowdhury
Reported 2012-07-25 12:52:22 PDT
Propagate gesture events to plugins.
Attachments
Patch (19.74 KB, patch)
2012-07-25 12:58 PDT, Sadrul Habib Chowdhury
no flags
Patch (19.74 KB, patch)
2012-07-25 13:47 PDT, Sadrul Habib Chowdhury
no flags
Patch (21.62 KB, patch)
2012-07-25 14:43 PDT, Sadrul Habib Chowdhury
no flags
Patch (32.80 KB, patch)
2012-07-26 10:44 PDT, Sadrul Habib Chowdhury
no flags
Patch (32.86 KB, patch)
2012-07-26 12:31 PDT, Sadrul Habib Chowdhury
no flags
Patch (37.42 KB, patch)
2012-07-27 10:25 PDT, Sadrul Habib Chowdhury
no flags
Patch (37.42 KB, patch)
2012-07-27 12:03 PDT, Sadrul Habib Chowdhury
no flags
Patch (37.44 KB, patch)
2012-07-27 14:13 PDT, Sadrul Habib Chowdhury
no flags
Sadrul Habib Chowdhury
Comment 1 2012-07-25 12:58:18 PDT
Adam Barth
Comment 2 2012-07-25 13:20:41 PDT
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
Comment 3 2012-07-25 13:47:18 PDT
Sadrul Habib Chowdhury
Comment 4 2012-07-25 13:47:50 PDT
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 5 2012-07-25 14:17:58 PDT
Sadrul Habib Chowdhury
Comment 6 2012-07-25 14:43:56 PDT
Sadrul Habib Chowdhury
Comment 7 2012-07-26 10:44:35 PDT
Sadrul Habib Chowdhury
Comment 8 2012-07-26 10:46:30 PDT
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 9 2012-07-26 11:41:06 PDT
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 10 2012-07-26 12:15:35 PDT
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
Comment 11 2012-07-26 12:31:14 PDT
Sadrul Habib Chowdhury
Comment 12 2012-07-26 12:31:46 PDT
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 13 2012-07-26 15:11:16 PDT
Comment on attachment 154721 [details] Patch Clearing flags on attachment: 154721 Committed r123799: <http://trac.webkit.org/changeset/123799>
WebKit Review Bot
Comment 14 2012-07-26 15:11:23 PDT
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 15 2012-07-26 15:14:36 PDT
Why were no Apple people cc'd on this bug?
Adam Barth
Comment 16 2012-07-26 15:18:19 PDT
> 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
Comment 17 2012-07-26 15:19:34 PDT
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)
Comment 18 2012-07-26 15:21:09 PDT
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
Comment 19 2012-07-26 15:22:22 PDT
I certainly welcome any feedback that you or Anders have on this or any other patch.
WebKit Review Bot
Comment 20 2012-07-26 15:49:01 PDT
Re-opened since this is blocked by 92431
Adam Barth
Comment 21 2012-07-26 17:52:03 PDT
@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)
Comment 22 2012-07-26 17:54:55 PDT
I'll defer to anders.
Sadrul Habib Chowdhury
Comment 23 2012-07-27 10:25:09 PDT
Sadrul Habib Chowdhury
Comment 24 2012-07-27 10:54:39 PDT
Comment on attachment 154984 [details] Patch andersca@ Hi! Do you have suggestions to make this patch better? Thanks!
Build Bot
Comment 25 2012-07-27 11:12:29 PDT
Sadrul Habib Chowdhury
Comment 26 2012-07-27 12:03:03 PDT
Anders Carlsson
Comment 27 2012-07-27 12:14:05 PDT
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 28 2012-07-27 12:38:44 PDT
Adam Barth
Comment 29 2012-07-27 14:02:17 PDT
> 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
Comment 30 2012-07-27 14:09:17 PDT
(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
Comment 31 2012-07-27 14:13:15 PDT
Created attachment 155046 [details] Patch patch with build fix
Adam Barth
Comment 32 2012-07-30 12:02:14 PDT
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 33 2012-07-30 15:13:35 PDT
Comment on attachment 155046 [details] Patch Clearing flags on attachment: 155046 Committed r124098: <http://trac.webkit.org/changeset/124098>
WebKit Review Bot
Comment 34 2012-07-30 15:13:45 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 36 2012-07-30 19:47:55 PDT
Build fix attempt landed in http://trac.webkit.org/changeset/124155.
Ryosuke Niwa
Comment 37 2012-07-30 20:08:28 PDT
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
Comment 38 2012-08-01 08:56:02 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.