Bug 63243 – PluginView::dispatchNPEvent is deceptive

• Component: Plug-ins
• Status: RESOLVED FIXED

[Balazs Kelemen]

This is true for the Qt, Gtk, and Windows implementations. This is the Qt one, the other two are almost the same:

bool PluginView::dispatchNPEvent(NPEvent& event)
{
    ...
    bool accepted = m_plugin->pluginFuncs()->event(m_instance, &event);
    ...
    return acccepted;
}

callers are using it this way:
if (!dispatchNPEvent(npEvent))
    event->setDefaultHandled();

This works because the plugin function returns with 0 if the plugin handled the event but it's clear that it is messed up.
dispatchNPEvent should return with the inverted value and the caller should call setDefaultHandled if the return value is true.

[Balazs Kelemen]

(In reply to comment #0)
> This is true for the Qt, Gtk, and Windows implementations. This is the Qt one, the other two are almost the same:
 
Symbian is also suffering from this. According to the test plugin (TestNetscapePlugin) on Mac carbon plugins are also returning with 0 if the event has been handled but I won't fix that case in this patch.

[Balazs Kelemen]

(In reply to comment #1)
> (In reply to comment #0)
> > This is true for the Qt, Gtk, and Windows implementations. This is the Qt one, the other two are almost the same:
> 
> Symbian is also suffering from this. According to the test plugin (TestNetscapePlugin) on Mac carbon plugins are also returning with 0 if the event has been handled but I won't fix that case in this patch.

Forget to mention: Cocoa plugins return with 1 if the event has been handled (according to the test plugin).

[Balazs Kelemen]

Created attachment 98334 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 98334 [details]
Patch

It should be possible to write a regression test for this, right?

[Balazs Kelemen]

(In reply to comment #4)
> (From update of attachment 98334 [details])
> It should be possible to write a regression test for this, right?

I cannot imagine any way to test this. The behavior is the same, it is actually  a refactoring to make the code readable. Neither the plugin nor WebCore see any effect of this change.

[Adam Roben (:aroben)]

(In reply to comment #0)
> This works because the plugin function returns with 0 if the plugin handled the event but it's clear that it is messed up.

http://devedge-temp.mozilla.org/library/manuals/2002/plugin/1.0/npp_api6.html says:

If the plug-in handles the event, [NPP_HandleEvent] should return true.
If the plug-in ignores the event, [NPP_HandleEvent] returns false.

This doesn't seem to match what you're saying.

[Balazs Kelemen]

(In reply to comment #6)
> (In reply to comment #0)
> > This works because the plugin function returns with 0 if the plugin handled the event but it's clear that it is messed up.
> 
> http://devedge-temp.mozilla.org/library/manuals/2002/plugin/1.0/npp_api6.html says:
> 
> If the plug-in handles the event, [NPP_HandleEvent] should return true.
> If the plug-in ignores the event, [NPP_HandleEvent] returns false.
> 
> This doesn't seem to match what you're saying.

Unfortunately only Cocoa plugins work that way.

[WebKit Review Bot]

Comment on attachment 98334 [details]
Patch

Clearing flags on attachment: 98334

Committed r89576: <http://trac.webkit.org/changeset/89576>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #7)
> > 
> > If the plug-in handles the event, [NPP_HandleEvent] should return true.
> > If the plug-in ignores the event, [NPP_HandleEvent] returns false.
> > 
> > This doesn't seem to match what you're saying.
> 
> Unfortunately only Cocoa plugins work that way.

Really? The following seems to CARBON if I'm not mistaken ...
http://trac.webkit.org/browser/trunk/Source/WebCore/plugins/mac/PluginViewMac.mm?rev=89576#L602

[Balazs Kelemen]

(In reply to comment #10)
> (In reply to comment #7)
> > > 
> > > If the plug-in handles the event, [NPP_HandleEvent] should return true.
> > > If the plug-in ignores the event, [NPP_HandleEvent] returns false.
> > > 
> > > This doesn't seem to match what you're saying.
> > 
> > Unfortunately only Cocoa plugins work that way.
> 
> Really? The following seems to CARBON if I'm not mistaken ...
> http://trac.webkit.org/browser/trunk/Source/WebCore/plugins/mac/PluginViewMac.mm?rev=89576#L602

Yes it is. And it says:
   if (!dispatchNPEvent(record))
LOG(Events, "PluginView::handleKeyboardEvent(): Keyboard event type %d not accepted", record.what);
683	    else
684	        event->setDefaultHandled();

[Balazs Kelemen]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > > 
> > > > If the plug-in handles the event, [NPP_HandleEvent] should return true.
> > > > If the plug-in ignores the event, [NPP_HandleEvent] returns false.
> > > > 
> > > > This doesn't seem to match what you're saying.
> > > 
> > > Unfortunately only Cocoa plugins work that way.
> > 
> > Really? The following seems to CARBON if I'm not mistaken ...
> > http://trac.webkit.org/browser/trunk/Source/WebCore/plugins/mac/PluginViewMac.mm?rev=89576#L602
> 

Again. Last time I was committing the text accidentally, sorry.
So, it says:
   if (!dispatchNPEvent(record))
        LOG(Events, "PluginView::handleKeyboardEvent(): Keyboard event type %d not accepted", record.what);
    else
        event->setDefaultHandled();

which means the event has been handled if NPP_HandleEvent returned with 0 a.k.a false.