Last Comment Bug 435041 - implement Cocoa NPAPI event model for Mac OS X
: implement Cocoa NPAPI event model for Mac OS X
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All Mac OS X
: P1 major with 4 votes (vote)
: ---
Assigned To: Josh Aas
:
:
Mentors:
http://wiki.mozilla.org/Mac:NPAPI_Eve...
Depends on: 350109 499921 516602 526276
Blocks: 428973 468509
  Show dependency treegraph
 
Reported: 2008-05-21 09:50 PDT by Josh Aas
Modified: 2010-07-12 21:32 PDT (History)
32 users (show)
mbeltzner: blocking1.9.2-
mbeltzner: wanted1.9.2-
jaas: blocking1.9.1-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
sample plugin v1.0 (from WebKit) (10.35 KB, application/octet-stream)
2008-08-12 13:15 PDT, Josh Aas
no flags Details
fix v0.2 (45.95 KB, patch)
2009-03-24 15:43 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v0.3 (69.84 KB, patch)
2009-03-25 14:12 PDT, Josh Aas
no flags Details | Diff | Splinter Review
Debug Plugin, v1.0 (20.44 KB, application/octet-stream)
2009-03-26 00:08 PDT, Josh Aas
no flags Details
Debug Plugin, v1.1 (14.26 KB, application/octet-stream)
2009-03-26 08:21 PDT, Josh Aas
no flags Details
Debug Plugin, v1.2 (21.01 KB, application/octet-stream)
2009-03-26 13:32 PDT, Josh Aas
no flags Details
fix v0.35 (84.49 KB, patch)
2009-03-26 14:35 PDT, Josh Aas
no flags Details | Diff | Splinter Review
Debug Plugin, v1.3 (21.03 KB, application/octet-stream)
2009-03-26 14:38 PDT, Josh Aas
no flags Details
fix v0.4 (85.56 KB, patch)
2009-03-26 17:50 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v0.41 (82.78 KB, patch)
2009-06-16 20:07 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v0.42 (81.46 KB, patch)
2009-06-17 23:06 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v0.5 (78.16 KB, patch)
2009-07-13 15:12 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v0.51 (88.56 KB, patch)
2009-07-14 14:30 PDT, Josh Aas
no flags Details | Diff | Splinter Review
Debug Plugin, v1.5 (23.23 KB, application/octet-stream)
2009-07-14 14:32 PDT, Josh Aas
no flags Details
fix v0.6 (100.89 KB, patch)
2009-07-15 14:57 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v0.7 (105.39 KB, patch)
2009-07-16 11:26 PDT, Josh Aas
no flags Details | Diff | Splinter Review
Debug Plugin, v1.6 (23.61 KB, application/octet-stream)
2009-07-16 11:45 PDT, Josh Aas
no flags Details
fix v0.8 (109.45 KB, patch)
2009-07-16 14:06 PDT, Josh Aas
no flags Details | Diff | Splinter Review
Fix v0.81 (applies on top of v0.8 and fixes a problem with context menus) (2.49 KB, patch)
2009-07-22 13:45 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Splinter Review
fix v0.8 updated to current trunk (111.38 KB, patch)
2009-07-22 22:08 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.0 (113.48 KB, patch)
2009-07-23 12:13 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.0.1 (113.96 KB, patch)
2009-07-23 13:28 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.0.2 (113.96 KB, patch)
2009-07-23 13:31 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.1 (113.87 KB, patch)
2009-07-30 08:24 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.2 (117.25 KB, patch)
2009-08-13 08:14 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.3 (124.35 KB, patch)
2009-08-14 00:53 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.4 (124.00 KB, patch)
2009-08-14 10:56 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.5 (127.97 KB, patch)
2009-08-26 17:24 PDT, Josh Aas
roc: superreview+
Details | Diff | Splinter Review
Debug Plugin, v1.7 (23.61 KB, application/octet-stream)
2009-10-28 09:47 PDT, Josh Aas
no flags Details

Description Josh Aas 2008-05-21 09:50:57 PDT
We need to implement NPAPI event models for Mac OS X. This will give us a modern event model for plugins on Mac OS X and allow for 64-bit plugins (no Carbon).

http://wiki.mozilla.org/Mac:NPAPI_Event_Models
Comment 1 Damon Sicore (:damons) 2008-06-04 16:36:54 PDT
marking wanted1.9.1? to get this in the triage queue.  If this needs to be blocking1.9.1?, please mark it as so.
Comment 2 Josh Aas 2008-06-05 23:45:15 PDT
This has to block 1.9.1 because we need to allow plugin developers to use the Cocoa event model ASAP.
Comment 3 Shane Caraveo 2008-08-12 10:01:34 PDT
(In reply to comment #2)
> we need to allow plugin developers to use the
> Cocoa event model ASAP.

Just curious about the "why" part of the above statement.  64bit? or something more?
Comment 4 Josh Aas 2008-08-12 11:17:02 PDT
64-bit is a big part of it, also Apple has been strongly encouraging developers to leave Carbon ASAP. Whether that means they plan to drop support for certain bits of Carbon functionality in 32-bit or not is unclear at this point, at least to me, but in any case moving on quickly is important. We do not want to support Carbon or Quickdraw for any longer than we have to. Since it generally takes a long time to get plugin vendors to make major changes like this, the sooner we allow the process to begin the better.
Comment 5 Josh Aas 2008-08-12 13:15:08 PDT
Created attachment 333450 [details]
sample plugin v1.0 (from WebKit)

This is a release-compiled copy of the Cocoa event model sample plugin from WebKit. This can be used to test.
Comment 6 Josh Aas 2008-08-22 06:09:57 PDT
Not going to make it for 1.9.1.
Comment 7 Josh Aas 2009-03-24 15:43:02 PDT
Created attachment 369169 [details] [diff] [review]
fix v0.2

This loads and runs Cocoa event model plugins, the test plugin becomes a Cocoa event model plugin and all tests pass on it. Many events are missing though and things are crashy. Backing this up.
Comment 8 Josh Aas 2009-03-25 14:12:56 PDT
Created attachment 369359 [details] [diff] [review]
fix v0.3

Backing up further work. Added support for more event types and ported default and basic sample plugins.
Comment 9 Josh Aas 2009-03-26 00:08:50 PDT
Created attachment 369448 [details]
Debug Plugin, v1.0

This is the source code for a Cocoa event model plugin I wrote that will display information about all types of events coming into it. It has modes, you can ask it to show information for different classes of events. I haven't put a lot of effort into making it really user friendly yet.
Comment 10 Josh Aas 2009-03-26 08:21:31 PDT
Created attachment 369507 [details]
Debug Plugin, v1.1

Updated event debug plugin, displays current mode, updated mode changing code.
Comment 11 Josh Aas 2009-03-26 13:32:43 PDT
Created attachment 369553 [details]
Debug Plugin, v1.2

Key handling fixes to the debug plugin.
Comment 12 Josh Aas 2009-03-26 14:35:41 PDT
Created attachment 369569 [details] [diff] [review]
fix v0.35

Mostly updates to key handling.
Comment 13 Josh Aas 2009-03-26 14:38:18 PDT
Created attachment 369570 [details]
Debug Plugin, v1.3

More minor fixes.
Comment 14 Josh Aas 2009-03-26 17:50:18 PDT
Created attachment 369606 [details] [diff] [review]
fix v0.4

More mouse event fixes.
Comment 15 Josh Aas 2009-06-16 20:07:58 PDT
Created attachment 383608 [details] [diff] [review]
fix v0.41
Comment 16 Josh Aas 2009-06-17 23:06:37 PDT
Created attachment 383876 [details] [diff] [review]
fix v0.42
Comment 17 Josh Aas 2009-07-13 15:12:27 PDT
Created attachment 388337 [details] [diff] [review]
fix v0.5
Comment 18 Josh Aas 2009-07-14 14:30:53 PDT
Created attachment 388557 [details] [diff] [review]
fix v0.51
Comment 19 Josh Aas 2009-07-14 14:32:16 PDT
Created attachment 388559 [details]
Debug Plugin, v1.5
Comment 20 Josh Aas 2009-07-15 14:57:38 PDT
Created attachment 388795 [details] [diff] [review]
fix v0.6
Comment 21 Josh Aas 2009-07-16 11:26:37 PDT
Created attachment 388954 [details] [diff] [review]
fix v0.7

Includes a lot of drawing fixes and fixes for drawing-related spec compliance.
Comment 22 Josh Aas 2009-07-16 11:45:08 PDT
Created attachment 388958 [details]
Debug Plugin, v1.6
Comment 23 Josh Aas 2009-07-16 14:06:30 PDT
Created attachment 388997 [details] [diff] [review]
fix v0.8
Comment 24 Steven Michaud [:smichaud] (Retired) 2009-07-22 13:45:02 PDT
Created attachment 390058 [details] [diff] [review]
Fix v0.81 (applies on top of v0.8 and fixes a problem with context menus)

Josh asked me to fix a specific problem with his v0.8 patch --
displaying a plugin's context menu (when the plugin calls
gBrowserFuncs->popupcontextmenu()) causes the plugin's display to
disappear.

Here's a patch that does the job.  It applies on top of Josh's v0.8
patch after that's been updated to current trunk code.

This patch is a bit of a hack.  It might be better to go back to
preventing nsChildView::StartDrawPlugin() from being re-entered only
in QuickDraw drawing mode.  As best I can tell, doing this doesn't
regress bug 409615, probably because the current Flash plugin (10.0
r22) fixes the underlying cause of bug 409615.  But I've only done
minimal testing (on OS X 10.4.11).

But my current v0.81 patch is probably safer.
Comment 25 Josh Aas 2009-07-22 22:08:38 PDT
Created attachment 390154 [details] [diff] [review]
fix v0.8 updated to current trunk

This is fix v0.8 updated to current trunk, it does not contain the context menu fix from Steven.
Comment 26 Josh Aas 2009-07-23 12:13:44 PDT
Created attachment 390285 [details] [diff] [review]
fix v1.0

This implementation is ready for review, though I am still doing some testing. The only thing it lacks in terms of Cocoa NPAPI support is complex text events. I'd like to work on adding support for that in a followup bug.

I chose not to convert our test plugin to the Cocoa NPAPI. We have no real Cocoa NPAPI consumers yet so I don't think it is necessary to resolve a testing strategy immediately, I'll work on that in a followup bug. For the moment it is most important that we make sure this implementation does not cause regressions in the carbon event model.
Comment 27 Josh Aas 2009-07-23 13:28:54 PDT
Created attachment 390304 [details] [diff] [review]
fix v1.0.1

Includes compile fixes for other platforms.
Comment 28 Josh Aas 2009-07-23 13:31:18 PDT
Created attachment 390306 [details] [diff] [review]
fix v1.0.2

Sorry, that bustage fix was incomplete.
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-28 21:19:18 PDT
We'll definitely need test plugin support before we ship a release with this. We don't want to poison the well with a crippled implementation.

+  // Convert coordinates to native view placement.
+  nsIntRect rect;
+  mWidget->GetBounds(rect);

Unused

+  // don't bother doing anything if there is no native msg for the plugin
+  if (!anEvent.nativeMsg)
+    return nsEventStatus_eIgnore;

This will conflict with my patch to enable testing of plugin mouse events.

-      mInstanceOwner->Paint();
+      mInstanceOwner->Paint(nativeClipRect);

I believe this should be nativeClipRect - offset.

How does the CGContext returned from gfxQuartzNativeDrawing become the current CGContext?

I kinda wonder why NPCocoaEvent doesn't contain a reference to an actual Cocoa event instead of replicating structure definitions, but whatever.

Seems like the mouse event conversion code should be shared. We appear to have 7 copies of it.

Wouldn't it make more sense to move the creation of the plugin event record up to nsPluginUtilsOSX? It doesn't really belong in widget.
Comment 30 Josh Aas 2009-07-29 21:56:26 PDT
(In reply to comment #29)
> How does the CGContext returned from gfxQuartzNativeDrawing become the current
> CGContext?

We can just use the current context because that event only happens when the current context is correctly focused (within a native widget's drawRect: method).

> I kinda wonder why NPCocoaEvent doesn't contain a reference to an actual Cocoa
> event instead of replicating structure definitions, but whatever.

This API decision was made because we wanted to avoid the problem we have now with Carbon events - lack of flexibility has forced us onto another event structure.

> Wouldn't it make more sense to move the creation of the plugin event record up
> to nsPluginUtilsOSX? It doesn't really belong in widget.

I tried to do it that way at first, for a number of reasons it doesn't work well. I tried that so long ago though that I don't remember the reasons off the top of my head.
Comment 31 Josh Aas 2009-07-30 08:24:31 PDT
Created attachment 391607 [details] [diff] [review]
fix v1.1
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-30 15:14:23 PDT
(In reply to comment #30)
> (In reply to comment #29)
> > How does the CGContext returned from gfxQuartzNativeDrawing become the current
> > CGContext?
> 
> We can just use the current context because that event only happens when the
> current context is correctly focused (within a native widget's drawRect:
> method).

This code can be called from places like canvas.drawWindow and printing where drawRect: isn't even on the stack.

> > Wouldn't it make more sense to move the creation of the plugin event record up
> > to nsPluginUtilsOSX? It doesn't really belong in widget.
> 
> I tried to do it that way at first, for a number of reasons it doesn't work
> well. I tried that so long ago though that I don't remember the reasons off the
> top of my head.

Can you investigate it again? It still seems like the right thing to me.
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-30 15:15:48 PDT
(In reply to comment #32)
> This code can be called from places like canvas.drawWindow and printing where
> drawRect: isn't even on the stack.

BTW if you added Cocoa event model support to our test plugin you would have caught this, I think, since our windowless plugin reftests would have failed.
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-30 15:17:57 PDT
(In reply to comment #29)
> -      mInstanceOwner->Paint();
> +      mInstanceOwner->Paint(nativeClipRect);
> 
> I believe this should be nativeClipRect - offset.

You seem to have ignored this comment?

> Seems like the mouse event conversion code should be shared. We appear to have
> 7 copies of it.

And this comment?

Did you upload the wrong patch?
Comment 35 Mike Beltzner [:beltzner, not reading bugmail] 2009-08-04 04:43:00 PDT
This is a P1 1.9.2 blocker, thus blocking Firefox 3.6a1 - any ETA on landing?
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-04 10:55:12 PDT
Josh is on holiday this week, I believe
Comment 37 Mike Beltzner [:beltzner, not reading bugmail] 2009-08-04 12:11:53 PDT
This is going to miss branch date, and so it's going to miss 1.9.2 at this point and isn't blocking. I would also assert that it's probably too big a change to put in outside of the alpha cycle, so we likely want to wait on this for 1.9.3.

If you disagree, please let me know (here and perhaps on IM/IRC)
Comment 38 Josh Aas 2009-08-04 13:19:21 PDT
I agree. I'm on holiday this week and I have perhaps another week of work to do. Also I made the blocking decision before our much shorter release cycle was announced, and this is less important if we're not going to have 64-bit builds.
Comment 39 Josh Aas 2009-08-13 08:14:14 PDT
Created attachment 394274 [details] [diff] [review]
fix v1.2

Addresses some of rocs comments, includes a bunch of 64-bit fixes, and updated to current trunk. Still not ready yet.
Comment 40 Josh Aas 2009-08-14 00:53:45 PDT
Created attachment 394475 [details] [diff] [review]
fix v1.3

A bunch more fixes including a test plugin port to Cocoa NPAPI and 64-bit. In order to do so I had to add a Core Text drawing alternative to our current ATSUI code. In 64-bit builds the test plugin will use Cocoa NPAPI.
Comment 41 Josh Aas 2009-08-14 10:56:59 PDT
Created attachment 394531 [details] [diff] [review]
fix v1.4

Fixes drawing issues and gets reftests passing. The only remaining issues are complex text input and plugin event creation refactoring.
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-24 18:44:42 PDT
+// Get the rect for an entire top-level Carbon window in screen coords.
+void NS_NPAPI_CarbonWindowFrame(WindowRef aWindow, nsRect& outRect);
+
+// Get the rect for an entire top-level Cocoa window in screen coords.
+void NS_NPAPI_CocoaWindowFrame(void* aWindow, nsRect& outRect);

The comments are incorrect, right? Both functions give us the rect for our toplevel window, which is a Cocoa window, the difference is the coordinate system we return it in. Right?

+  // Create an NSEvent we can use to show the context menu. Only the location
+  // is important here so just simulate a right mouse down. The coordinates
+  // must be in top-level window terms.

Don't we need to pass the right modifier flags?

+  // We need a view.
+  if (!inView)
+    return PR_FALSE;
+
+  // Caller has to want a result.
+  if (!destX && !destY)
+    return PR_FALSE;

Shouldn't these just be assertions?

+  if (browser->getvalue(instance, NPNVsupportsCoreGraphicsBool, &supportsCoreGraphics) == NPERR_NO_ERROR && supportsCoreGraphics) {
+    browser->setvalue(instance, NPPVpluginDrawingModel, (void*)NPDrawingModelCoreGraphics);

Line too long! Happens again many times below.
Comment 43 Josh Aas 2009-08-24 22:06:17 PDT
(In reply to comment #42)
> +// Get the rect for an entire top-level Carbon window in screen coords.
> +void NS_NPAPI_CarbonWindowFrame(WindowRef aWindow, nsRect& outRect);
> +
> +// Get the rect for an entire top-level Cocoa window in screen coords.
> +void NS_NPAPI_CocoaWindowFrame(void* aWindow, nsRect& outRect);
> 
> The comments are incorrect, right? Both functions give us the rect for our
> toplevel window, which is a Cocoa window, the difference is the coordinate
> system we return it in. Right?

I think the comments are correct. The difference is that one takes a Carbon WindowRef and the other takes a Cocoa NSWindow. The reason the Cocoa version has a void* type is because if the argument was NSWindow* then you wouldn't be able to include the header in c++-only files, like nsObjectFrame.cpp.
Comment 44 Josh Aas 2009-08-26 17:24:48 PDT
Created attachment 396918 [details] [diff] [review]
fix v1.5

Address roc's comments.
Comment 45 Josh Aas 2009-08-26 21:09:57 PDT
pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/ebb78acb0087

bustage fix
http://hg.mozilla.org/mozilla-central/rev/814a26052bf5

Filing another bug on complex text input support.
Comment 46 Jesse Ruderman 2009-08-27 22:11:54 PDT
Josh filed bug 512886 on complex text input support.
Comment 47 Eric Shepherd [:sheppy] 2009-08-28 07:36:10 PDT
Does this particular bug directly affect how developers will create plugins in any way? I know lots is being done in terms of plugins on Mac OS X in particular, and want to be sure I catch the stuff that will need to be documented. I'm not sure after a quick looking over if this bug is implementation side or something plugins will directly have to worry about.
Comment 48 Steven Michaud [:smichaud] (Retired) 2009-08-28 09:35:53 PDT
Plugins will definitely need to be rewritten to support the NPAPI
Cocoa event model.

There's already some documentation at the link mentioned in comment #0
(https://wiki.mozilla.org/Mac:NPAPI_Event_Models).  Current WebKit
code also includes at least one example plugin that uses this model
(I'll post more information in a later comment).

Josh told me he'd converted one of our example plugins to use the
Cocoa event model ... but I can't find it just now.
Comment 49 Eric Shepherd [:sheppy] 2009-08-28 09:37:20 PDT
OK, if anyone can find that example plugin that would be a huge help. This will likely be one of the larger bits of documentation work for Firefox 3.6.
Comment 50 Steven Michaud [:smichaud] (Retired) 2009-08-28 10:41:39 PDT
There are four example plugins in the WebKitExamplePlugins directory
of the WebKit source distro (available at
http://webkit.org/building/checkout.html):

NetscapeCocoaPlugin
NetscapeCoreAnimationMoviePlugin
NetscapeCoreAnimationPlugin
NetscapeInputMethodPlugin

All support the NPAPI Cocoa events model, and in fact require it (they
don't support the Carbon events model).
Comment 51 Steven Michaud [:smichaud] (Retired) 2009-08-28 10:44:27 PDT
By the way, we still support the Carbon event model ... and will continue to do so for a while.
Comment 52 Josh Aas 2009-08-28 15:10:16 PDT
There are 3 Cocoa plugins in mozilla-central, part of the patch here:

- basic sample plugin
- default plugin
- test plugin
Comment 53 Daniel Holbert [:dholbert] 2009-10-18 22:50:51 PDT
Comment on attachment 396918 [details] [diff] [review]
fix v1.5

>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
[SNIP]
>+NPError
>+nsDummyJavaPluginOwner::ShowNativeContextMenu(NPMenu* menu, nsPluginEvent* event)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}

The above-quoted change from this bug's patch introduced this compile warning:
>   nsGlobalWindow.cpp:514: warning: overflow in implicit constant conversion

We get this warning because NS_ERROR_NOT_IMPLEMENTED is a large 32-bit number[1], and we're trying to cram this value into this function's return type, NPError (which is 16-bit [2]).

I'm not sure how the overflowed version of NS_ERROR_NOT_IMPLEMENTED behaves as a NPError, but I doubt it gives the behavior we want. (Or if it does, it's only by coincidence. :))

Should this function be returning NPERR_GENERIC_ERROR instead?  (or one of the other error codes from npapi.h? [3])

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsError.h#180
[2] http://mxr.mozilla.org/mozilla-central/source/modules/plugin/base/public/npapi.h#133
[3] http://mxr.mozilla.org/mozilla-central/source/modules/plugin/base/public/npapi.h#622
Comment 54 Josh Aas 2009-10-19 09:49:23 PDT
Thanks, fix checked in on mozilla-central.

http://hg.mozilla.org/mozilla-central/rev/914f639bc645
Comment 55 Josh Aas 2009-10-28 09:47:38 PDT
Created attachment 408862 [details]
 Debug Plugin, v1.7

Note You need to log in before you can comment on or make changes to this bug.