Closed Bug 435041 Opened 16 years ago Closed 15 years ago

implement Cocoa NPAPI event model for Mac OS X

Categories

(Core Graveyard :: Plug-ins, defect, P1)

All
macOS

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 27 obsolete files)

127.97 KB, patch
Details | Diff | Splinter Review
23.61 KB, application/octet-stream
Details
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
Flags: wanted-next+
Blocks: 428973
marking wanted1.9.1? to get this in the triage queue.  If this needs to be blocking1.9.1?, please mark it as so.
Flags: wanted1.9.1?
Priority: -- → P1
This has to block 1.9.1 because we need to allow plugin developers to use the Cocoa event model ASAP.
Flags: wanted1.9.1?
Flags: wanted-next+
Flags: blocking1.9.1+
Depends on: 350109
(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?
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.
Attached file sample plugin v1.0 (from WebKit) (obsolete) —
This is a release-compiled copy of the Cocoa event model sample plugin from WebKit. This can be used to test.
Not going to make it for 1.9.1.
Flags: blocking1.9.1+ → blocking1.9.1-
Flags: blocking1.9.2+
Hardware: x86 → All
Summary: implement NPAPI event models for Mac OS X → implement Cocoa NPAPI event model for Mac OS X
Attached patch fix v0.2 (obsolete) — Splinter Review
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.
Attached patch fix v0.3 (obsolete) — Splinter Review
Backing up further work. Added support for more event types and ported default and basic sample plugins.
Attachment #369169 - Attachment is obsolete: true
Attached file Debug Plugin, v1.0 (obsolete) —
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.
Attached file Debug Plugin, v1.1 (obsolete) —
Updated event debug plugin, displays current mode, updated mode changing code.
Attachment #369448 - Attachment is obsolete: true
Attached file Debug Plugin, v1.2 (obsolete) —
Key handling fixes to the debug plugin.
Attachment #369507 - Attachment is obsolete: true
Attached patch fix v0.35 (obsolete) — Splinter Review
Mostly updates to key handling.
Attachment #369359 - Attachment is obsolete: true
Attachment #333450 - Attachment is obsolete: true
Attached file Debug Plugin, v1.3 (obsolete) —
More minor fixes.
Attachment #369553 - Attachment is obsolete: true
Attached patch fix v0.4 (obsolete) — Splinter Review
More mouse event fixes.
Attachment #369569 - Attachment is obsolete: true
Blocks: 468509
Attached patch fix v0.41 (obsolete) — Splinter Review
Attachment #369606 - Attachment is obsolete: true
Attached patch fix v0.42 (obsolete) — Splinter Review
Attachment #383608 - Attachment is obsolete: true
Depends on: 499921
Attached patch fix v0.5 (obsolete) — Splinter Review
Attachment #383876 - Attachment is obsolete: true
Attached patch fix v0.51 (obsolete) — Splinter Review
Attachment #388337 - Attachment is obsolete: true
Attached file Debug Plugin, v1.5 (obsolete) —
Attachment #369570 - Attachment is obsolete: true
Attached patch fix v0.6 (obsolete) — Splinter Review
Attachment #388557 - Attachment is obsolete: true
Attached patch fix v0.7 (obsolete) — Splinter Review
Includes a lot of drawing fixes and fixes for drawing-related spec compliance.
Attachment #388795 - Attachment is obsolete: true
Attached file Debug Plugin, v1.6 (obsolete) —
Attachment #388559 - Attachment is obsolete: true
Attached patch fix v0.8 (obsolete) — Splinter Review
Attachment #388954 - Attachment is obsolete: true
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.
This is fix v0.8 updated to current trunk, it does not contain the context menu fix from Steven.
Attachment #388997 - Attachment is obsolete: true
Attached patch fix v1.0 (obsolete) — Splinter Review
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.
Attachment #390058 - Attachment is obsolete: true
Attachment #390154 - Attachment is obsolete: true
Attachment #390285 - Flags: review?(roc)
Attached patch fix v1.0.1 (obsolete) — Splinter Review
Includes compile fixes for other platforms.
Attachment #390285 - Attachment is obsolete: true
Attachment #390304 - Flags: review?(roc)
Attachment #390285 - Flags: review?(roc)
Attached patch fix v1.0.2 (obsolete) — Splinter Review
Sorry, that bustage fix was incomplete.
Attachment #390304 - Attachment is obsolete: true
Attachment #390306 - Flags: review?(roc)
Attachment #390304 - Flags: review?(roc)
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.
(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.
Attached patch fix v1.1 (obsolete) — Splinter Review
Attachment #390306 - Attachment is obsolete: true
Attachment #391607 - Flags: review?(roc)
Attachment #390306 - Flags: review?(roc)
(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.
(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.
(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?
Attachment #391607 - Flags: review?(roc)
This is a P1 1.9.2 blocker, thus blocking Firefox 3.6a1 - any ETA on landing?
Josh is on holiday this week, I believe
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)
Flags: wanted1.9.2-
Flags: blocking1.9.2-
Flags: blocking1.9.2+
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.
Attached patch fix v1.2 (obsolete) — Splinter Review
Addresses some of rocs comments, includes a bunch of 64-bit fixes, and updated to current trunk. Still not ready yet.
Attachment #391607 - Attachment is obsolete: true
Attached patch fix v1.3 (obsolete) — Splinter Review
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.
Attachment #394274 - Attachment is obsolete: true
Attached patch fix v1.4 (obsolete) — Splinter Review
Fixes drawing issues and gets reftests passing. The only remaining issues are complex text input and plugin event creation refactoring.
Attachment #394475 - Attachment is obsolete: true
Attachment #394531 - Flags: superreview?(roc)
+// 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.
(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.
Attached patch fix v1.5Splinter Review
Address roc's comments.
Attachment #394531 - Attachment is obsolete: true
Attachment #396918 - Flags: superreview?(roc)
Attachment #394531 - Flags: superreview?(roc)
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.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Josh filed bug 512886 on complex text input support.
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.
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.
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.
Keywords: dev-doc-needed
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).
By the way, we still support the Carbon event model ... and will continue to do so for a while.
There are 3 Cocoa plugins in mozilla-central, part of the patch here:

- basic sample plugin
- default plugin
- test plugin
Attachment #396918 - Flags: superreview?(roc) → superreview+
Depends on: 516602
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
Thanks, fix checked in on mozilla-central.

http://hg.mozilla.org/mozilla-central/rev/914f639bc645
Attached file Debug Plugin, v1.7
Attachment #388958 - Attachment is obsolete: true
Depends on: 526276
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.