344427 - support Mac OS X NPAPI Drawing Models

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

Proposals for a 64-bit Mac OS X NPAPI have been put forth on the plugin-futures mailing list. We should implement them when approved. I'll post the specs here when they are "finalized."

Comment 1

[Josh Aas]

Since the 64-bit Mac OS X NPAPI spec also solves the more immediate problem of allowing 32-bit plugins to draw with CoreGraphics, it has been renamed Mac OS X NPAPI Drawing Models.

Comment 2

[Josh Aas]

The wiki page that tracks revisions to the spec is here:

http://wiki.mozilla.org/Mac:64-bit_NPAPI

Comment 3

[Josh Aas]

Attachment: fix v0.8

This works fine on Mac OS X, but it needs to be cleaned up for checkin and I highly doubt that it compiles on Linux or Windows. Just putting it here as a backup.

This will negotiate drawing models with plugins and can render using Quickdraw or Quartz.

Comment 4

[Josh Aas]

Attachment: fix v0.81

includes more npapi fixes, linux compile fixes

Comment 5

[Josh Aas]

Starting the review process because v0.81 compiles and runs on all 3 major platforms and I don't think I need to rev the NPAPI version #.

Comment 6

[Simon Fraser [no longer active]]

Comment on attachment 259245 [details] [diff] [review]
fix v0.81

I think it needs a little more thought.

First, I'm not keen on the nsPluginPort -> void* changes; it's a regression in terms of understandability of code, and having two separate structs, one for QD and one for CG add complexity.

Second, the method in nsChildView that gets the current CGContextRef is suspect.

> Index: layout/generic/nsObjectFrame.cpp
> ===================================================================

> +PRBool nsPluginInstanceOwner::UsingQuartzDrawingModel()
> +{
> +  if (!mInstance)
> +    return PR_FALSE;
> +
> +  NPDrawingModel drawingModel = NPDrawingModelQuickDraw;
> +  mInstance->GetValue(nsPluginInstanceVariable_DrawingModel,
> +                      (void *)&drawingModel);
> +  return (drawingModel == NPDrawingModelCoreGraphics);
> +}

Any reason not to call this UsingCoreGraphicsDrawingModel, since that's what the enum value is? "Quartz" is rather an ill-defined term these days.


> -            nsPluginPort* pluginPort = FixUpPluginWindow(ePluginPaintDisable);
> -            if (pluginPort) {
> -              nsPluginEvent pluginEvent = { &scrollEvent, nsPluginPlatformWindowRef(GetWindowFromPort(pluginPort->port)) };
> +            WindowRef window = nsnull;
> +            if (UsingQuartzDrawingModel()) {
> +              nsPluginPortQuartz* pluginPort = (nsPluginPortQuartz*)FixUpPluginWindow(ePluginPaintDisable);
> +              if (pluginPort)
> +                window = pluginPort->window;
> +            }
> +            else {
> +              nsPluginPortQD* pluginPort = (nsPluginPortQD*)FixUpPluginWindow(ePluginPaintDisable);
> +              if (pluginPort)
> +                window = ::GetWindowFromPort(pluginPort->port);
> +            }

This pattern shows up serveral times. Maybe factor it into a helper method? Or have FixUpPluginWindow return the WindowRef?

> -  return pluginPort;
> +  if (pluginPortQuartz)
> +    return pluginPortQuartz;
> +  
> +  return pluginPortQD;

These are really the same pointer, so the if() test makes it a little confusing.

> Index: modules/plugin/base/public/npapi.h
> ===================================================================

>  #ifdef XP_MACOSX
> -	#include <Quickdraw.h>
> -	#include <Events.h>
> +#include <Quickdraw.h>
> +#include <Events.h>

This should just include <Carbon/Carbon.h> these days.

>  /*----------------------------------------------------------------------*/
> @@ -420,17 +432,22 @@ typedef enum {
>    NPNVDOMWindow      = (12 | NP_ABI_MASK),
>    NPNVToolkit        = (13 | NP_ABI_MASK),
>    NPNVSupportsXEmbedBool = 14,
>  
>    /* Get the NPObject wrapper for the browser window. */
>    NPNVWindowNPObject = 15,
>  
>    /* Get the NPObject wrapper for the plugins DOM element. */
> -  NPNVPluginElementNPObject = 16
> +  NPNVPluginElementNPObject = 16,
> +  
> +  /* Used for negotiating drawing models, right now on Mac OS X only */
> +  NPNVpluginDrawingModel = 1000,
> +  NPNVsupportsQuickDrawBool = 2000,
> +  NPNVsupportsCoreGraphicsBool = 2001

NPNVPluginDrawingModel, NPNVSupportsQuickDrawBool, NPNVSupportsCoreGraphicsBool I think.

> Index: modules/plugin/base/public/nsplugindefs.h
> ===================================================================

> -struct nsPluginPort {
> -    CGrafPtr     port;   /* Grafport */
> +struct nsPluginPortQD {
> +    CGrafPtr    port;   /* Grafport */
>      PRInt32     portx;  /* position inside the topmost window */
>      PRInt32     porty;
>  };
> +struct nsPluginPortQuartz {
> +  CGContextRef context;
> +  WindowRef window;
> +};
>  typedef RgnHandle       nsPluginRegion;
>  typedef WindowRef       nsPluginPlatformWindowRef;

Add some whitespace around the struct.

>  struct nsPluginWindow {
> -    nsPluginPort* window;       /* Platform specific window handle */
> +    void* window;               /* Platform specific window handle */

So rather than change nsPluginPort* to a void* everywhere (which reduces semantic value), could you use a union for Mac, or just typedef nsPluginPortRef to void* or something? Reading code full of void*s is just hard.

>      PRInt32       x;            /* Position of top left corner relative */
> -    PRInt32       y;            /*	to a netscape page.					*/
> +    PRInt32       y;            /* to a netscape page.					*/

I think the indentation was intentional.

> Index: modules/plugin/base/src/ns4xPluginInstance.h
> ===================================================================
> RCS file: /cvsroot/mozilla/modules/plugin/base/src/ns4xPluginInstance.h,v
> retrieving revision 1.37
> diff -U8 -p -r1.37 ns4xPluginInstance.h
> --- modules/plugin/base/src/ns4xPluginInstance.h	20 Mar 2007 02:35:56 -0000	1.37
> +++ modules/plugin/base/src/ns4xPluginInstance.h	21 Mar 2007 19:38:03 -0000
> @@ -117,16 +117,21 @@ public:
>       * Return the callbacks for the plugin instance.
>       */
>      nsresult GetCallbacks(const NPPluginFuncs ** aCallbacks);
>  
>      NPError SetWindowless(PRBool aWindowless);
>  
>      NPError SetTransparent(PRBool aTransparent);
>  
> +#ifdef XP_MACOSX
> +    void SetDrawingModel(NPDrawingModel aModel);
> +    NPDrawingModel GetDrawingModel();
> +#endif

Method should be |const|

> Index: widget/public/nsIWidget.h
> ===================================================================
> RCS file: /cvsroot/mozilla/widget/public/nsIWidget.h,v
> retrieving revision 3.152
> diff -U8 -p -r3.152 nsIWidget.h
> --- widget/public/nsIWidget.h	7 Feb 2007 07:46:43 -0000	3.152
> +++ widget/public/nsIWidget.h	21 Mar 2007 19:38:14 -0000
> @@ -89,16 +89,21 @@ typedef nsEventStatus (*PR_CALLBACK EVEN
>  #define NS_NATIVE_DISPLAY     4
>  #define NS_NATIVE_REGION      5
>  #define NS_NATIVE_OFFSETX     6
>  #define NS_NATIVE_OFFSETY     7
>  #define NS_NATIVE_PLUGIN_PORT 8
>  #define NS_NATIVE_SCREEN      9
>  #define NS_NATIVE_SHELLWIDGET 10      // Get the shell GtkWidget
>  
> +#ifdef XP_MACOSX
> +#define NS_NATIVE_PLUGIN_PORT_QUICKDRAW 100
> +#define NS_NATIVE_PLUGIN_PORT_QUARTZ    101
> +#endif

Do these really need to pollute nsIWidget.h?

> Index: widget/src/cocoa/nsChildView.h
> ===================================================================
> RCS file: /cvsroot/mozilla/widget/src/cocoa/nsChildView.h,v
> retrieving revision 1.59
> diff -U8 -p -r1.59 nsChildView.h
> --- widget/src/cocoa/nsChildView.h	12 Mar 2007 16:04:40 -0000	1.59
> +++ widget/src/cocoa/nsChildView.h	21 Mar 2007 19:38:14 -0000
> @@ -66,17 +66,18 @@
>  #include "nsplugindefs.h"
>  #import <Quickdraw.h>
>  
>  class gfxASurface;
>  
>  #define NSRGB_2_COLOREF(color) \
>              RGB(NS_GET_R(color),NS_GET_G(color),NS_GET_B(color))
>  
> -struct nsPluginPort;
> +struct nsPluginPortQD;
> +struct nsPluginPortQuartz;
>  
>  #undef DARWIN
>  #import <Cocoa/Cocoa.h>
>  
>  class nsChildView;
>  
>  @interface ChildView : NSView<
>  #ifdef ACCESSIBILITY
> @@ -319,14 +320,15 @@ protected:
>    PRPackedBool          mVisible;
>  
>    PRPackedBool          mDrawing;
>      
>    PRPackedBool          mAcceptFocusOnClick;
>    PRPackedBool          mLiveResizeInProgress;
>    PRPackedBool          mPluginDrawing;
>    
> -  nsPluginPort*         mPluginPort;
> +  nsPluginPortQD*       mPluginPortQD;
> +  nsPluginPortQuartz*   mPluginPortQuartz;

If you used a union, then you'd only need one pointer (save 4 bytes per widget!)

> @@ -562,42 +564,69 @@ void* nsChildView::GetNativeData(PRUint3
>        retVal = 0;
>        break;
>  
>      case NS_NATIVE_OFFSETY:
>        retVal = 0;
>        break;
>  
>      case NS_NATIVE_PLUGIN_PORT:
> +    case NS_NATIVE_PLUGIN_PORT_QUICKDRAW:
>      {
> +      delete mPluginPortQuartz;
> +      mPluginPortQuartz = nsnull;

What if other code still has references to the mPluginPortQuartz pointer?
This might be another reason to use a union.

> +      mPluginPortQuartz->context = (CGContextRef)[[NSGraphicsContext currentContext] graphicsPort];

Hm, I'm not at all sure that [NSGraphicsContext currentContext] is valid outside of drawRect:.
Can you guarantee that this is only called from drawing code? If not, you might be grabbing some random CGContextRef from whatever happens to be drawing right now.

Comment 7

[Josh Aas]

> +  /* Used for negotiating drawing models, right now on Mac OS X only */
> +  NPNVpluginDrawingModel = 1000,
> +  NPNVsupportsQuickDrawBool = 2000,
> +  NPNVsupportsCoreGraphicsBool = 2001

As for the capitalization, yeah, that part of the patch is from before we had the spec written out formally.

> -  return pluginPort;
> +  if (pluginPortQuartz)
> +    return pluginPortQuartz;
> +  
> +  return pluginPortQD;

"These are really the same pointer, so the if() test makes it a little
confusing."

They are different pointers.

"So rather than change nsPluginPort* to a void* everywhere (which reduces
semantic value), could you use a union for Mac, or just typedef nsPluginPortRef
to void* or something? Reading code full of void*s is just hard."

I considered using a union, that is actually what WebKit does. However, I've been brought up to stay away from unions so I decided to try this first. I'll use a union in my next patch.

Thanks for the review!

Comment 8

[Josh Aas]

> +  NPNVpluginDrawingModel = 1000,
> +  NPNVsupportsQuickDrawBool = 2000,
> +  NPNVsupportsCoreGraphicsBool = 2001

Actually, those are correct. They are that way in the final spec and that is also what WebKit has implemented. There are a bunch of other NPNVariables that have that capitalization convention.

Comment 9

[Josh Aas]

Attachment: fix v0.9

This fixes a few bugs and addresses most of smfr's comments. Not requesting review because there are a couple more things I want to do. This is basically a rewrite of the whole patch, much better I think.

Comment 10

[Josh Aas]

Attachment: fix v1.0

Comment 11

[Josh Aas]

As for the validity of [NSGraphicsContext currentContext] at any given time, it looks like the port is occasionally grabbed outside of drawRect: but usually drawRect: is on the stack. I have never seen a rendering glitch that looks like it could be caused by drawRect: not being on the stack when the port is grabbed. I think when it is being grabbed without drawRect: on the stack it is the side effect of another operation and thus it doesn't really matter.

If this does turn out to be an issue I suspect it will become apparent pretty quickly with more widespread testing.

Comment 12

[Simon Fraser [no longer active]]

Comment on attachment 259439 [details] [diff] [review]
fix v1.0

I like this a lot better.

Comment 13

[Simon Fraser [no longer active]]

(In reply to comment #11)
> As for the validity of [NSGraphicsContext currentContext] at any given time, it
> looks like the port is occasionally grabbed outside of drawRect: but usually
> drawRect: is on the stack. I have never seen a rendering glitch that looks like
> it could be caused by drawRect: not being on the stack when the port is
> grabbed. I think when it is being grabbed without drawRect: on the stack it is
> the side effect of another operation and thus it doesn't really matter.
> 
> If this does turn out to be an issue I suspect it will become apparent pretty
> quickly with more widespread testing.

On the contrary, this could make for some very hard to track down and timing-dependent drawing or crashing bugs, related to the timing of opening new windows, closing windows, or creating, destroying, or switching tabs. I really think we need to absolutely sure about the lifetime of the CGContextRef that we're handing off to a plugin. That may require that we fetch it once early on, and keep a reference to it for the life of the plugin.

Unfortunately the nature of the CGContextRef that you get from the NSGraphicsContext is obscured by the details of AppKit. It would be interesting to see what WebKit does here.

So where are the other browser vendors on CG and plugins? Are other browsers offering a CG drawing context to NPAPI-based plugins at this point?

Comment 14

[Simon Fraser [no longer active]]

(In reply to comment #13)

> On the contrary, this could make for some very hard to track down and
> timing-dependent drawing or crashing bugs, related to the timing of opening new
> windows, closing windows, or creating, destroying, or switching tabs. I really
> think we need to absolutely sure about the lifetime of the CGContextRef that
> we're handing off to a plugin. That may require that we fetch it once early on,
> and keep a reference to it for the life of the plugin.

Having said that, perhaps we should fetch the CGContext for each draw, otherwise you can't draw the entire page to a different destination (e.g. when printing, or creating an image of the page) and expect plugins to draw correctly. So maybe we should only return a CGContextRef if focus is locked on the view.

Comment 15

[Josh Aas]

From the WebKit impl where it grabs the port for plugins:

=================================================
case NPDrawingModelCoreGraphics:
{
// A CoreGraphics plugin's window may only be set while the plugin view is being updated
ASSERT(forUpdate && [NSView focusView] == self);
...
=================================================

Good guess Simon :) I'm pretty sure the one caller that calls without drawRect: on the stack is in our nsObjectFrame code, and it doesn't matter if we set the port to null in that case. I'll post a new patch with a similar check and maybe get rid of the bogus object frame caller if it is a reasonable thing to do.

Comment 16

[Josh Aas]

Attachment: fix v1.0.1

This sets the cgcontext to null if the plugin view isn't the focus view.

Comment 17

[Josh Aas]

Landed on trunk.