Bug 102150 – Pass clicks through to the restarted plugin

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jon Lee]

Clicking on the plugin enables snapshotted plugins.

[Radar WebKit Bug Importer]

<rdar://problem/12695575>

[Jon Lee]

Created attachment 174845 [details]
Patch

[Early Warning System Bot]

Comment on attachment 174845 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 174845 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 174845 [details]
Patch

Attachment 174845 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14876563

[WebKit Review Bot]

Comment on attachment 174845 [details]
Patch

Attachment 174845 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14890096

[Build Bot]

Comment on attachment 174845 [details]
Patch

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

[Build Bot]

Comment on attachment 174845 [details]
Patch

Attachment 174845 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14875599

[Peter Beverloo (cr-android ews)]

Comment on attachment 174845 [details]
Patch

Attachment 174845 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14873744

[kov's GTK+ EWS bot]

Comment on attachment 174845 [details]
Patch

Attachment 174845 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14872784

[Darin Adler]

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.

[Darin Adler]

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.

[Jon Lee]

Created attachment 175943 [details]
Patch

[Early Warning System Bot]

Comment on attachment 175943 [details]
Patch

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

[Jon Lee]

Created attachment 175946 [details]
Patch

[Simon Fraser (smfr)]

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* ?

[Jon Lee]

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.

[Jon Lee]

Committed r135767: <http://trac.webkit.org/changeset/135767>