Bug 63243 - PluginView::dispatchNPEvent is deceptive
Summary: PluginView::dispatchNPEvent is deceptive
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-23 04:17 PDT by Balazs Kelemen
Modified: 2011-06-30 08:57 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.45 KB, patch)
2011-06-23 04:34 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2011-06-23 04:17:41 PDT
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.
Comment 1 Balazs Kelemen 2011-06-23 04:24:32 PDT
(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.
Comment 2 Balazs Kelemen 2011-06-23 04:25:34 PDT
(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).
Comment 3 Balazs Kelemen 2011-06-23 04:34:44 PDT
Created attachment 98334 [details]
Patch
Comment 4 Adam Roben (:aroben) 2011-06-23 06:36:24 PDT
Comment on attachment 98334 [details]
Patch

It should be possible to write a regression test for this, right?
Comment 5 Balazs Kelemen 2011-06-23 07:11:13 PDT
(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.
Comment 6 Adam Roben (:aroben) 2011-06-23 07:20:46 PDT
(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.
Comment 7 Balazs Kelemen 2011-06-23 08:02:38 PDT
(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.
Comment 8 WebKit Review Bot 2011-06-23 08:53:16 PDT
Comment on attachment 98334 [details]
Patch

Clearing flags on attachment: 98334

Committed r89576: <http://trac.webkit.org/changeset/89576>
Comment 9 WebKit Review Bot 2011-06-23 08:53:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 noel gordon 2011-06-30 08:29:10 PDT
(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
Comment 11 Balazs Kelemen 2011-06-30 08:51:56 PDT
(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();
Comment 12 Balazs Kelemen 2011-06-30 08:57:46 PDT
(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.