implement Cocoa NPAPI event model for Mac OS X

RESOLVED FIXED

Status

()

Core
Plug-ins
P1
major
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

({dev-doc-needed})

Trunk
All
Mac OS X
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -
wanted1.9.2 -
blocking1.9.1 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 27 obsolete attachments)

127.97 KB, patch
Details | Diff | Splinter Review
23.61 KB, application/octet-stream
Details
(Assignee)

Description

9 years ago
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+
(Assignee)

Updated

9 years ago
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
(Assignee)

Comment 2

9 years ago
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+
(Assignee)

Updated

9 years ago
Depends on: 350109

Comment 3

9 years ago
(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?
(Assignee)

Comment 4

9 years ago
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.
(Assignee)

Comment 5

9 years ago
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.
(Assignee)

Comment 6

9 years ago
Not going to make it for 1.9.1.
Flags: blocking1.9.1+ → blocking1.9.1-
(Assignee)

Updated

9 years ago
Flags: blocking1.9.2+
(Assignee)

Updated

9 years ago
Hardware: x86 → All
(Assignee)

Updated

9 years ago
Summary: implement NPAPI event models for Mac OS X → implement Cocoa NPAPI event model for Mac OS X
(Assignee)

Comment 7

9 years ago
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.
(Assignee)

Comment 8

9 years ago
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.
Attachment #369169 - Attachment is obsolete: true
(Assignee)

Comment 9

8 years ago
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.
(Assignee)

Comment 10

8 years ago
Created attachment 369507 [details]
Debug Plugin, v1.1

Updated event debug plugin, displays current mode, updated mode changing code.
Attachment #369448 - Attachment is obsolete: true
(Assignee)

Comment 11

8 years ago
Created attachment 369553 [details]
Debug Plugin, v1.2

Key handling fixes to the debug plugin.
Attachment #369507 - Attachment is obsolete: true
(Assignee)

Comment 12

8 years ago
Created attachment 369569 [details] [diff] [review]
fix v0.35

Mostly updates to key handling.
Attachment #369359 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #333450 - Attachment is obsolete: true
(Assignee)

Comment 13

8 years ago
Created attachment 369570 [details]
Debug Plugin, v1.3

More minor fixes.
Attachment #369553 - Attachment is obsolete: true
(Assignee)

Comment 14

8 years ago
Created attachment 369606 [details] [diff] [review]
fix v0.4

More mouse event fixes.
Attachment #369569 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Blocks: 468509
(Assignee)

Comment 15

8 years ago
Created attachment 383608 [details] [diff] [review]
fix v0.41
Attachment #369606 - Attachment is obsolete: true
(Assignee)

Comment 16

8 years ago
Created attachment 383876 [details] [diff] [review]
fix v0.42
Attachment #383608 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Depends on: 499921
(Assignee)

Comment 17

8 years ago
Created attachment 388337 [details] [diff] [review]
fix v0.5
Attachment #383876 - Attachment is obsolete: true
(Assignee)

Comment 18

8 years ago
Created attachment 388557 [details] [diff] [review]
fix v0.51
Attachment #388337 - Attachment is obsolete: true
(Assignee)

Comment 19

8 years ago
Created attachment 388559 [details]
Debug Plugin, v1.5
Attachment #369570 - Attachment is obsolete: true
(Assignee)

Comment 20

8 years ago
Created attachment 388795 [details] [diff] [review]
fix v0.6
Attachment #388557 - Attachment is obsolete: true
(Assignee)

Comment 21

8 years ago
Created attachment 388954 [details] [diff] [review]
fix v0.7

Includes a lot of drawing fixes and fixes for drawing-related spec compliance.
Attachment #388795 - Attachment is obsolete: true
(Assignee)

Comment 22

8 years ago
Created attachment 388958 [details]
Debug Plugin, v1.6
Attachment #388559 - Attachment is obsolete: true
(Assignee)

Comment 23

8 years ago
Created attachment 388997 [details] [diff] [review]
fix v0.8
Attachment #388954 - Attachment is obsolete: true
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.
(Assignee)

Comment 25

8 years ago
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.
Attachment #388997 - Attachment is obsolete: true
(Assignee)

Comment 26

8 years ago
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.
Attachment #390058 - Attachment is obsolete: true
Attachment #390154 - Attachment is obsolete: true
Attachment #390285 - Flags: review?(roc)
(Assignee)

Comment 27

8 years ago
Created attachment 390304 [details] [diff] [review]
fix v1.0.1

Includes compile fixes for other platforms.
Attachment #390285 - Attachment is obsolete: true
Attachment #390304 - Flags: review?(roc)
Attachment #390285 - Flags: review?(roc)
(Assignee)

Comment 28

8 years ago
Created attachment 390306 [details] [diff] [review]
fix v1.0.2

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.
(Assignee)

Comment 30

8 years ago
(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.
(Assignee)

Comment 31

8 years ago
Created attachment 391607 [details] [diff] [review]
fix v1.1
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?
(Assignee)

Updated

8 years ago
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+
(Assignee)

Comment 38

8 years ago
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.
(Assignee)

Comment 39

8 years ago
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.
Attachment #391607 - Attachment is obsolete: true
(Assignee)

Comment 40

8 years ago
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.
Attachment #394274 - Attachment is obsolete: true
(Assignee)

Comment 41

8 years ago
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.
Attachment #394475 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
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.
(Assignee)

Comment 43

8 years ago
(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.
(Assignee)

Comment 44

8 years ago
Created attachment 396918 [details] [diff] [review]
fix v1.5

Address roc's comments.
Attachment #394531 - Attachment is obsolete: true
Attachment #396918 - Flags: superreview?(roc)
Attachment #394531 - Flags: superreview?(roc)
(Assignee)

Comment 45

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 46

8 years ago
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.
(Assignee)

Comment 52

8 years ago
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
(Assignee)

Comment 54

8 years ago
Thanks, fix checked in on mozilla-central.

http://hg.mozilla.org/mozilla-central/rev/914f639bc645
(Assignee)

Comment 55

8 years ago
Created attachment 408862 [details]
 Debug Plugin, v1.7
Attachment #388958 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Depends on: 526276
You need to log in before you can comment on or make changes to this bug.