568513 - [OOPP] Implement NPN_PopUpContextMenu

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benoit Girard (:BenWa)]

Right-click flash does not work because 'NPN_PopUpContextMenu' is not implemented. See http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginModuleChild.cpp#1495.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Is this related to bug 561477?  (Also, I sort-of thought marcia had a trunk-only Flash context menu bug somewhere, but I can't find it now.)

Comment 2

[Benoit Girard (:BenWa)]

(In reply to comment #1)
> Is this related to bug 561477?  (Also, I sort-of thought marcia had a
> trunk-only Flash context menu bug somewhere, but I can't find it now.)

Thanks, these two bugs are similar so we should fix them together.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Implement Context Menu, missing position information

The following patch implements context menu for OOPP. I'm just waiting on a response for the best way to get the plug-in's coordinate to position the context menu. If there is no easy way I will use an IPC message to retrieve it.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: Implement Context Menu, working+no hang

Comment 5

[Josh Aas]

Comment on attachment 450814 [details] [diff] [review]
Implement Context Menu, working+no hang

>diff --git a/dom/plugins/PluginModuleChild.cpp b/dom/plugins/PluginModuleChild.cpp
...
>+    if (!currentEvent || 
>+        currentEvent->type != NPCocoaEventMouseDown    &&
>+        currentEvent->type != NPCocoaEventMouseUp      &&
>+        currentEvent->type != NPCocoaEventMouseMoved   &&
>+        currentEvent->type != NPCocoaEventMouseEntered &&
>+        currentEvent->type != NPCocoaEventMouseExited  &&
>+        currentEvent->type != NPCocoaEventMouseDragged
>+       ) {
>+        return NPERR_GENERIC_ERROR;
>+    }

The ") {" goes on the same line as the last boolean statement, maybe make "!currentEvent" its own check to avoid operator precedence confusion and too many parens.

>+    NPBool result = _convertpoint(instance, 

Nit, maybe rename this to "success" to reflect the type of the result.

>+  * Portions created by the Initial Developer are Copyright (C) 2008

2008->2010, in both new files.

>diff --git a/dom/plugins/PluginUtilsOSX.mm b/dom/plugins/PluginUtilsOSX.mm

Does the code in this file need some exception guards?

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Implement Context Menu, review

Comment 7

[Benoit Girard (:BenWa)]

So the problem causing this error:
uginUtilsOSX.pp -fobjc-exceptions /Users/mozilla/mozilla/mozilla-central/dom/plugins/PluginUtilsOSX.mm
/Users/mozilla/mozilla/mozilla-central/dom/plugins/PluginUtilsOSX.mm:84: error: stray ‘@’ in program
/Users/mozilla/mozilla/mozilla-central/dom/plugins/PluginUtilsOSX.mm:86: error: stray ‘@’ in program
/Users/mozilla/mozilla/mozilla-central/dom/plugins/PluginUtilsOSX.mm:96: error: stray ‘@’ in program
/Users/mozilla/mozilla/mozilla-central/dom/plugins/PluginUtilsOSX.mm:98: error: stray ‘@’ in program

is that we are compiling with both -fobjc-exceptions and -fno-exceptions. This file (/usr/include/c++/4.2.1/exception_defines.h) gets included containing this snippet that is interfering with nsObjCExp:

#ifndef __EXCEPTIONS
// Iff -fno-exceptions, transform error handling code to work without it.
# define try      if (true)
# define catch(X) if (false)
# define __throw_exception_again
#else
// Else proceed normally.
# define __throw_exception_again throw
#endif

I think the bottom line is we shouldn't be using objective-c exceptions with exceptions disabled (or if we do want we need to tweak nsObjCExceptions.h to deal with these #define try/catch statements). I'm going to patch a patch to detect if exceptions are disabled when including nsObjCExceptions.h to give a clear error when this happens again.

As far as handling exceptions in PluginUtilsOSX.h goes we can either have it compiled with exception handling or not catch them. I'm not aware of the code throwing exceptions ATM.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: nsObjCExceptions.h with -fno-exceptions

Detect if we are including nsObjCExceptions when compiling with -fno-exceptions. This is bad because of #define conflicts mentioned above.

Comment 9

[Josh Aas]

Comment on attachment 451207 [details] [diff] [review]
Implement Context Menu, review

Please try to use a stack-based class for nulling out mCurrentEvent.

Try to improve on the "ProcessNativeEvents" naming situation like we talked about.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: Implement Context Menu, using AutoRelease

cjones pointed me to AutoRelease which does what we need.

Comment 11

[Benoit Girard (:BenWa)]

When a right click menu is created drawing now stops. I remember the drawing working when I posted on 2010-06-11 but I'm not certain anymore.

This check is preventing drawing from happening when a context menu is active:
http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsChildView.mm#1247

The comment explicitly states:
But in CoreGraphics drawing mode only do this if the current plugin event isn't an update/paint event.  This allows popupcontextmenu() to work properly from a plugin that supports the Cocoa event model [...]

However the current event IS a paint event. Is a paint event supposed to imply (mView == [NSView focusView])?

Comment 12

[Benoit Girard (:BenWa)]

Attachment: Context menu (Added namespace according to IRC review)

Let's land this without the code to prevent hangs and we can file another bug to fix that issue later. It's more useful to have the browser provide a context menu and pause then ignoring the event.

Comment 13

[Josh Aas]

Comment on attachment 459712 [details] [diff] [review]
Context menu (Added namespace according to IRC review)

+NPError mozilla::plugins::PluginUtilsOSX::ShowCocoaContextMenu(void* aMenu, int aX, int aY, void* pluginModule, RemoteProcessEvents remoteEvent)

Is it really necessary to have the entire namespace chain written out here?

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Any reason why this hasn't landed? I'd rather we get nightly coverage before code freeze whenever possible.

Comment 15

[Benoit Girard (:BenWa)]

No technical reason, I had a few end of term assignments. I wanted to clarify the approval rule but from what I gather Blocking2.0 does not require approval.

I will land this tonight.

Comment 16

[Benoit Girard (:BenWa)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/b34ec5a10d8f