Propagate gesture events to plugins.
Created attachment 154420 [details] Patch
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?
Created attachment 154434 [details] Patch
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)
Comment on attachment 154420 [details] Patch Attachment 154420 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13344398
Created attachment 154449 [details] Patch
Created attachment 154686 [details] Patch
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.
Comment on attachment 154686 [details] Patch This looks good on a first pass. Someone (possibly me) will need to review it in more detail.
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 ?
Created attachment 154721 [details] Patch
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!
Comment on attachment 154721 [details] Patch Clearing flags on attachment: 154721 Committed r123799: <http://trac.webkit.org/changeset/123799>
All reviewed patches have been landed. Closing bug.
Why were no Apple people cc'd on this bug?
> 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
I guess I don't understand your question. Can you explain more about who you think should be been CCed and why?
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.
I certainly welcome any feedback that you or Anders have on this or any other patch.
Re-opened since this is blocked by 92431
@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.
I'll defer to anders.
Created attachment 154984 [details] Patch
Comment on attachment 154984 [details] Patch andersca@ Hi! Do you have suggestions to make this patch better? Thanks!
Comment on attachment 154984 [details] Patch Attachment 154984 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13374624
Created attachment 155007 [details] Patch
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).
Comment on attachment 155007 [details] Patch Attachment 155007 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13371623
> 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?
(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?
Created attachment 155046 [details] Patch patch with build fix
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.
Comment on attachment 155046 [details] Patch Clearing flags on attachment: 155046 Committed r124098: <http://trac.webkit.org/changeset/124098>
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
Build fix attempt landed in http://trac.webkit.org/changeset/124155.
Another build fix attempt landed in http://trac.webkit.org/changeset/124159. "interface" is a keyword. Don't use it as a variable name.
(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.