Closed Bug 568513 Opened 11 years ago Closed 11 years ago

[OOPP] Implement NPN_PopUpContextMenu


(Core :: Plug-ins, defect)

Not set



Tracking Status
blocking2.0 --- beta3+


(Reporter: BenWa, Assigned: BenWa)




(1 file, 5 obsolete files)

Right-click flash does not work because 'NPN_PopUpContextMenu' is not implemented. See
Blocks: 567265
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.)
Depends on: 561477
(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.
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.
Attachment #450434 - Attachment is obsolete: true
Attachment #450814 - Flags: review?(joshmoz)
Assignee: nobody → b56girard
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/ b/dom/plugins/

Does the code in this file need some exception guards?
Attached patch Implement Context Menu, review (obsolete) — Splinter Review
Attachment #450814 - Attachment is obsolete: true
Attachment #451207 - Flags: review?(joshmoz)
Attachment #450814 - Flags: review?(joshmoz)
So the problem causing this error:
uginUtilsOSX.pp -fobjc-exceptions /Users/mozilla/mozilla/mozilla-central/dom/plugins/
/Users/mozilla/mozilla/mozilla-central/dom/plugins/ error: stray ‘@’ in program
/Users/mozilla/mozilla/mozilla-central/dom/plugins/ error: stray ‘@’ in program
/Users/mozilla/mozilla/mozilla-central/dom/plugins/ error: stray ‘@’ in program
/Users/mozilla/mozilla/mozilla-central/dom/plugins/ 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 proceed normally.
# define __throw_exception_again throw

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.
Detect if we are including nsObjCExceptions when compiling with -fno-exceptions. This is bad because of #define conflicts mentioned above.
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.
Attachment #451207 - Flags: review?(joshmoz)
Attachment #452435 - Flags: review+
Attachment #452435 - Attachment is obsolete: true
cjones pointed me to AutoRelease which does what we need.
Attachment #451207 - Attachment is obsolete: true
Attachment #457649 - Flags: review?
Attachment #457649 - Flags: review? → review?(joshmoz)
Attachment #457649 - Attachment is patch: true
Attachment #457649 - Attachment mime type: application/octet-stream → text/plain
Attachment #457649 - Flags: review?(joshmoz)
blocking2.0: --- → beta3+
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:

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])?
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.
Attachment #457649 - Attachment is obsolete: true
Attachment #459712 - Flags: review?(joshmoz)
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?
Attachment #459712 - Flags: review?(joshmoz) → review+
Any reason why this hasn't landed? I'd rather we get nightly coverage before code freeze whenever possible.
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.
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.