Summary: | Pass clicks through to the restarted plugin | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jon Lee <jonlee> | ||||||||
Component: | WebKit Misc. | Assignee: | Jon Lee <jonlee> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, beidson, bweinstein, dglazkov, eric, gtk-ews, mjs, ojan, peter+ews, webkit-bug-importer, webkit-ews, webkit.review.bot, xan.lopez | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.8 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 98318 | ||||||||||
Attachments: |
|
Description
Jon Lee
2012-11-13 16:19:25 PST
Created attachment 174845 [details]
Patch
Comment on attachment 174845 [details] Patch Attachment 174845 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14888423 Comment on attachment 174845 [details] Patch Attachment 174845 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14888421 Comment on attachment 174845 [details] Patch Attachment 174845 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14876563 Comment on attachment 174845 [details] Patch Attachment 174845 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14890096 Comment on attachment 174845 [details] Patch Attachment 174845 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14868803 Comment on attachment 174845 [details] Patch Attachment 174845 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14875599 Comment on attachment 174845 [details] Patch Attachment 174845 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14873744 Comment on attachment 174845 [details] Patch Attachment 174845 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14872784 Comment on attachment 174845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174845&action=review > Source/WebCore/html/HTMLPlugInImageElement.cpp:297 > + dispatchSimulatedClick(m_pendingClickEventFromSnapshot.get(), true, true, false); EWS says that’s too many booleans there. Comment on attachment 174845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174845&action=review >> Source/WebCore/html/HTMLPlugInImageElement.cpp:297 >> + dispatchSimulatedClick(m_pendingClickEventFromSnapshot.get(), true, true, false); > > EWS says that’s too many booleans there. I see now. This patch depends on that other patch. Created attachment 175943 [details]
Patch
Comment on attachment 175943 [details] Patch Attachment 175943 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14988533 Created attachment 175946 [details]
Patch
Comment on attachment 175946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175946&action=review > Source/WebCore/html/HTMLPlugInImageElement.cpp:278 > + ASSERT_UNUSED(timer, timer == &m_simulatedMouseClickTimer); We don't normally assert this. Why do you think you need to? > Source/WebCore/html/HTMLPlugInImageElement.cpp:282 > + dispatchSimulatedClick(m_pendingClickEventFromSnapshot.get(), SendMouseOverUpDownEvents, DoNotShowPressedLook); What if this event runs JS that destroys |this|? Have you tested that? > Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:928 > + ASSERT_NOT_REACHED(); > + return IntPoint(); Can't you just call the superclass? > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:71 > -static const double pluginSnapshotTimerDelay = 1; > +static const double pluginSnapshotTimerDelay = 1.1; Should you comment here about the dependency with simulatedMouseClickTimerDelay? > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:735 > +PassOwnPtr<WebEvent> PluginView::createWebEvent(MouseEvent* event) const const MouseEvent* ? Comment on attachment 175946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175946&action=review >> Source/WebCore/html/HTMLPlugInImageElement.cpp:278 >> + ASSERT_UNUSED(timer, timer == &m_simulatedMouseClickTimer); > > We don't normally assert this. Why do you think you need to? It is extraneous. I'll remove it. >> Source/WebCore/html/HTMLPlugInImageElement.cpp:282 >> + dispatchSimulatedClick(m_pendingClickEventFromSnapshot.get(), SendMouseOverUpDownEvents, DoNotShowPressedLook); > > What if this event runs JS that destroys |this|? Have you tested that? I have not tested this case. I've been testing with live sites, and have not run into a case where the plugin destroys itself immediately (since |this| would have to be deleted before the 1.1 second timer fired). >> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:928 >> + return IntPoint(); > > Can't you just call the superclass? Superclass uses pure virtual. I can move this up to WebKit::Plugin as a default impl. >> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:71 >> +static const double pluginSnapshotTimerDelay = 1.1; > > Should you comment here about the dependency with simulatedMouseClickTimerDelay? Done. >> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:735 >> +PassOwnPtr<WebEvent> PluginView::createWebEvent(MouseEvent* event) const > > const MouseEvent* ? No. offsetX() and offsetY() are calculated dynamically and then cached, so the event should not be const. Committed r135767: <http://trac.webkit.org/changeset/135767> |