WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(19.74 KB, patch)
2012-07-25 13:47 PDT
,
Sadrul Habib Chowdhury
no flags
Details
Formatted Diff
Diff
Patch
(21.62 KB, patch)
2012-07-25 14:43 PDT
,
Sadrul Habib Chowdhury
no flags
Details
Formatted Diff
Diff
Patch
(32.80 KB, patch)
2012-07-26 10:44 PDT
,
Sadrul Habib Chowdhury
no flags
Details
Formatted Diff
Diff
Patch
(32.86 KB, patch)
2012-07-26 12:31 PDT
,
Sadrul Habib Chowdhury
no flags
Details
Formatted Diff
Diff
Patch
(37.42 KB, patch)
2012-07-27 10:25 PDT
,
Sadrul Habib Chowdhury
no flags
Details
Formatted Diff
Diff
Patch
(37.42 KB, patch)
2012-07-27 12:03 PDT
,
Sadrul Habib Chowdhury
no flags
Details
Formatted Diff
Diff
Patch
(37.44 KB, patch)
2012-07-27 14:13 PDT
,
Sadrul Habib Chowdhury
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Sadrul Habib Chowdhury
Comment 1
2012-07-25 12:58:18 PDT
Created
attachment 154420
[details]
Patch
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
Created
attachment 154434
[details]
Patch
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
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
Comment 6
2012-07-25 14:43:56 PDT
Created
attachment 154449
[details]
Patch
Sadrul Habib Chowdhury
Comment 7
2012-07-26 10:44:35 PDT
Created
attachment 154686
[details]
Patch
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
Created
attachment 154721
[details]
Patch
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
Created
attachment 154984
[details]
Patch
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
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
Comment 26
2012-07-27 12:03:03 PDT
Created
attachment 155007
[details]
Patch
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
Comment on
attachment 155007
[details]
Patch
Attachment 155007
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/13371623
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 35
2012-07-30 19:47:38 PDT
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
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.
Top of Page
Format For Printing
XML
Clone This Bug