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)
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
|
roc
:
superreview+
|
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+
Comment 1•16 years ago
|
||
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+
Comment 3•16 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?
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.
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-
Summary: implement NPAPI event models for Mac OS X → implement Cocoa NPAPI event model for Mac OS X
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.
Backing up further work. Added support for more event types and ported default and basic sample plugins.
Attachment #369169 -
Attachment is obsolete: true
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•15 years ago
|
||
Updated event debug plugin, displays current mode, updated mode changing code.
Attachment #369448 -
Attachment is obsolete: true
Assignee | ||
Comment 11•15 years ago
|
||
Key handling fixes to the debug plugin.
Attachment #369507 -
Attachment is obsolete: true
Assignee | ||
Comment 12•15 years ago
|
||
Mostly updates to key handling.
Attachment #369359 -
Attachment is obsolete: true
Attachment #333450 -
Attachment is obsolete: true
Assignee | ||
Comment 13•15 years ago
|
||
More minor fixes.
Attachment #369553 -
Attachment is obsolete: true
Assignee | ||
Comment 14•15 years ago
|
||
More mouse event fixes.
Attachment #369569 -
Attachment is obsolete: true
Assignee | ||
Comment 15•15 years ago
|
||
Attachment #369606 -
Attachment is obsolete: true
Assignee | ||
Comment 16•15 years ago
|
||
Attachment #383608 -
Attachment is obsolete: true
Assignee | ||
Comment 17•15 years ago
|
||
Attachment #383876 -
Attachment is obsolete: true
Assignee | ||
Comment 18•15 years ago
|
||
Attachment #388337 -
Attachment is obsolete: true
Assignee | ||
Comment 19•15 years ago
|
||
Attachment #369570 -
Attachment is obsolete: true
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #388557 -
Attachment is obsolete: true
Assignee | ||
Comment 21•15 years ago
|
||
Includes a lot of drawing fixes and fixes for drawing-related spec compliance.
Attachment #388795 -
Attachment is obsolete: true
Assignee | ||
Comment 22•15 years ago
|
||
Attachment #388559 -
Attachment is obsolete: true
Assignee | ||
Comment 23•15 years ago
|
||
Attachment #388954 -
Attachment is obsolete: true
Comment 24•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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•15 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•15 years ago
|
||
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)
Comment 35•15 years ago
|
||
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
Comment 37•15 years ago
|
||
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•15 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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 43•15 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•15 years ago
|
||
Address roc's comments.
Attachment #394531 -
Attachment is obsolete: true
Attachment #396918 -
Flags: superreview?(roc)
Attachment #394531 -
Flags: superreview?(roc)
Assignee | ||
Comment 45•15 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
Closed: 15 years ago
Resolution: --- → FIXED
Comment 46•15 years ago
|
||
Josh filed bug 512886 on complex text input support.
Comment 47•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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
Comment 50•15 years ago
|
||
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•15 years ago
|
||
By the way, we still support the Carbon event model ... and will continue to do so for a while.
Assignee | ||
Comment 52•15 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+
Comment 53•15 years ago
|
||
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•15 years ago
|
||
Thanks, fix checked in on mozilla-central. http://hg.mozilla.org/mozilla-central/rev/914f639bc645
Assignee | ||
Comment 55•15 years ago
|
||
Attachment #388958 -
Attachment is obsolete: true
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•