522299 - Electrolysis: Get windowless plugins drawing on win32

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

I'm not sure the exact approach that we should take here. Whether it should be synchronous or asynchronous painting.

Comment 1

[Benjamin Smedberg]

Synchronously forwarding paints (WM_PAINT) is the easy first step to getting something up. Then we can measure performance and worry about asynchronous painting which will require more thinking.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Does anyone know if different processes can share the same device context (DC) on windows?

Comment 3

[Benjamin Smedberg]

I doubt it. Why? Do we apply clipping or something to the DC when plugins are painting?

Comment 4

[Jim Mathies [:jimm]]

(In reply to comment #2)
> Does anyone know if different processes can share the same device context (DC)
> on windows?

Those are thread local. If you have the HWND, you can request a device context for the window, or just call BeginPaint, paint, and then EndPaint. If you're dealing with a window, I assume you have a windows procedure in the local process? In which case maybe hand a buffer in which gets painted to the window and force a local wm_paint message? (some context might help.)

Comment 5

[Jim Mathies [:jimm]]

Since these are windowless, why not have them paint into a buffer, hand that back, and paint that to the plugin space within the window?

Comment 6

[Benjamin Smedberg]

Maybe I don't understand the problem, but can't you just pass WM_PAINT to the plugin (sychronously), have the plugin call BeginPaint/EndPaint as it normally would, acting on the live HWND, and then continue the painting cycle? The advantage of the extra buffer that Chromium uses is that it can do the paintaing asynchronously, but let's do the easy thing first.

Comment 7

[Karl Tomlinson (:karlt)]

The plugin's drawing needs to end up on the surface for the gfxContext (not an HWND).

The plugin expects an HDC:
https://developer.mozilla.org/index.php?title=en/NPWindow#Fields
https://developer.mozilla.org/en/Gecko_Plugin_API_Reference:Drawing_and_Event_Handling#Receiving_a_Paint_Message

If there is a way that a surface can be set up that can be shared across process and from which an HDC reference can be generated, then that may be worth investigating.

Comment 8

[Jim Mathies [:jimm]]

(In reply to comment #7)
> The plugin's drawing needs to end up on the surface for the gfxContext (not an
> HWND).
> 
> The plugin expects an HDC:
> https://developer.mozilla.org/index.php?title=en/NPWindow#Fields
> https://developer.mozilla.org/en/Gecko_Plugin_API_Reference:Drawing_and_Event_Handling#Receiving_a_Paint_Message
> 
> If there is a way that a surface can be set up that can be shared across
> process and from which an HDC reference can be generated, then that may be
> worth investigating.

You could create and cache a global buffer the size of the drawing area, and share that with the shim on init. The plugin draws to that, and the browser reads from it after a paint. The one thing I'm not sure of, to paint that into an hdc (the window's hdc in this case), you would probably have to have a dib for painting, in which case there may be an added copy of the cached buffer into a dib every frame. (Might be a way around that though.) At most one copy and one bitblt.

Comment 9

[Karl Tomlinson (:karlt)]

It seems that the surfaces used in both browser and plugin should be in shared memory.  AFAICS a DIB seems necessary for this.  (It would seem better if the graphics system could choose where to put the memory.)

If all of the browser's surfaces were in shared memory then the copying into and out of a shared memory DIB for plugin rendering would not be necessary (in a sync painting model).  I don't know what cost would be involved by using shared memory DIBs for all drawing.

http://www.google.com/codesearch/p?hl=en&sa=N&cd=2&ct=rc#h0RrPvyPu-c/skia/ext/bitmap_platform_device_win.cc&q=skia%20PlatformCanvas&l=172
http://msdn.microsoft.com/en-us/library/dd183494%28VS.85%29.aspx

Comment 10

[Karl Tomlinson (:karlt)]

http://code.google.com/p/chromium/wiki/BitmapPipeline

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #9)
> It seems that the surfaces used in both browser and plugin should be in shared
> memory.  AFAICS a DIB seems necessary for this.  (It would seem better if the
> graphics system could choose where to put the memory.)

Yes, this looks like what we should be doing. I didn't know that CreateDIBSection() could take a handle for sharing the dibsection across processes. Since it does, that looks like the best/only way forward.

Comment 12

[Masatoshi Kimura [:emk]]

When HDC is associated to a window, it may be possible to:
1. get HWND from HDC by WindowFromDC() in parent process,
2. pass HWND to child process via IPC,
3. get HDC back from HWND by GetDC() in child process.
It is perfectly legal to get a DC from a window which is owned by an other process. If WindowFromDC() returns NULL, we can fallback to a DIB section.

Comment 13

[Jim Mathies [:jimm]]

What's needed to start looking at this? Looks like a set of the patches in bug 522122 would need to be applied first?

Comment 14

[Benjamin Smedberg]

I'm pretty sure all of those landed last night.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #13)
> What's needed to start looking at this? Looks like a set of the patches in bug
> 522122 would need to be applied first?

We should decide on an approach. We could try the approach suggested in comment 12 (pass the window handle and get a DC for it), or we could follow the approach that chrome uses and use a shared DIBSection.

Passing the window handle might be easier to get working, but the DIBSection approach feels like it might work out better in the long run.

What do others think?

Comment 16

[Benjamin Smedberg]

According to IRC conversation, I thought that GFX was drawing to a DIB, not directly to a window. In order for transparency and whatnot to work, you have to draw to the same surface GFX is using, so I think DIBSection is the only available option.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Most of the time we draw directly to the window. However, there are occasional time when we do draw to a dibsection (partially transparent divs). But yes, those situations are probably reason enough to go with the DIBSection approach.

Comment 18

[Jim Mathies [:jimm]]

Attachment: wip v.1

Work remaining:

1) split the shared dib code out into something stand alone and reusable.
2) hook shared dib code up to cjones work on shared mem support.
3) tests for the above.

Comment 19

[Jim Mathies [:jimm]]

I guess in addition, there's the possibility of doing async drawing. That really shouldn't be too hard to implement here. Child can keep a surface of its own to paint to, and the parent can request that whenever it needs it. Parent could also force a sync redraw whenever it needs it. Not sure where we are on implementing this generally though. 

The other thing I was wondering about was memory usage and setup on the mapping file. There is some overhead in that so possibly we should be looking for a way to pool those allocations.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #18)
> Created an attachment (id=410383) [details]
> wip v.1
> 
> Work remaining:
> 
> 1) split the shared dib code out into something stand alone and reusable.

Any reason we can't just use the transport_dib section abstraction from the included chrome code?

Comment 21

[Jim Mathies [:jimm]]

Attachment: wip v.2

There's one remaining issue with this patch - in cases where thebes is painting a windowless plugin that sits in a transparent div, it does a double pass render on the plugin using two memory surfaces and then extracts alpha from the results. In cases where there is heavy animation going on (like video) the delay between the two render passes is sufficiently long enough to result in frame changes in the animation which results in tearing.

The information I need in the shim to compensate for this is available up in nsObjectFrame on the Paint call. I'm still searching for a way to get that.

This last issue is a corner case, if we are desperate to get this in before the merge next week I think this could land as-is and I could work on a follow up patch.

Comment 22

[Jim Mathies [:jimm]]

Attachment: wip v.2

w/addremove!

Comment 23

[Jim Mathies [:jimm]]

Attachment: windowless patch v.1

Fixed the problem using a custom event sent from nsObjectFrame to the shim.

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 414457 [details] [diff] [review]
windowless patch v.1

bent -> for PluginInstanceParent/PluginInstanceChild changes.

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 414457 [details] [diff] [review]
windowless patch v.1

> diff --git a/dom/plugins/PluginInstanceChild.cpp b/dom/plugins/ 
> PluginInstanceChild.cpp
> --- a/dom/plugins/PluginInstanceChild.cpp
> +++ b/dom/plugins/PluginInstanceChild.cpp
> @@ -16,16 +16,17 @@
> @@ -323,18 +324,24 @@ PluginInstanceChild::AnswerNPP_HandleEve
>      AssertPluginThread();
>
>  #if defined(OS_LINUX) && defined(DEBUG)
>      if (GraphicsExpose == event.event.type)
>          printf("  received drawable 0x%lx\n",
>                 event.event.xgraphicsexpose.drawable);
>  #endif
>
> -    // plugins might be fooling with these, make a copy
> +    // make a copy

Why the comment change?

>      NPEvent evcopy = event.event;
> +
> +#ifdef OS_WIN
> +    if (mWindow.type == NPWindowTypeDrawable && WM_PAINT ==  
> event.event.event)
> +        SharedSurfaceBeforePaint(evcopy);
> +#endif
> +

event.event.event looks dumb, but that's partly my fault. Maybe use  
evcopy?

>      *handled = mPluginIface->event(&mData,  
> reinterpret_cast<void*>(&evcopy));
>
>  #ifdef MOZ_X11
>      if (GraphicsExpose == event.event.type) {
>          // Make sure the X server completes the drawing before the  
> parent
>          // draws on top and destroys the Drawable.
>          //
>          // XSync() waits for the X server to complete.  Really this  
> child
> @@ -403,55 +410,68 @@ PluginInstanceChild::AnswerNPP_SetWindow
>      mWsInfo.colormap = aWindow.colormap;
>      if (!XVisualIDToInfo(mWsInfo.display, aWindow.visualID,
>                           &mWsInfo.visual, &mWsInfo.depth))
>          return false;
>
>      *rv = mPluginIface->setwindow(&mData, &mWindow);
>
>  #elif defined(OS_WIN)
> -    ReparentPluginWindow((HWND)aWindow.window);
> -    SizePluginWindow(aWindow.width, aWindow.height);
> +    switch (aWindow.type) {
> +      case NPWindowTypeWindow:
> +      {
> +          if (!CreatePluginWindow())
> +              return false;

Why do we call CreatePluginWindow() when we didn't before? Because of  
the change in PluginInstanceChild::Initialize()?


> +                  mPluginWndProc = reinterpret_cast<WNDPROC>(
> +                      SetWindowLongPtr(mPluginWindowHWND,  
> GWLP_WNDPROC,
> +                                        
> reinterpret_cast<LONG>(PluginWindowProc)));
> +              }
> +          }
> +      }
> +      break;
> +
> +      case NPWindowTypeDrawable:
> +          return SharedSurfaceSetup(aWindow, rv);
> +      break;
> +
> +      default:
> +          // Woah!

Maybe we want to assert here?

> +          return false;
> +      break;
>      }



> @@ -463,16 +483,20 @@ PluginInstanceChild::Destroy()
>      }
> +
> +#if defined(OS_WIN)
> +    SharedSurfaceFree();
> +#endif
>  }

It's ok to call SharedSurfaceFree() even if we didn't do  
SharedSurfaceSetup()?


>
>  bool
>  PluginInstanceChild::CreatePluginWindow()
>  {
> +    // already initialized
> +    if (mPluginWindowHWND)
> +        return true;
> +
>      if (!RegisterWindowClass())
>          return false;
>
> -    if (!mPluginWindowHWND) {
> -        mPluginWindowHWND =
> -            CreateWindowEx(WS_EX_LEFT | WS_EX_LTRREADING |
> -                           WS_EX_NOPARENTNOTIFY | // XXXbent Get  
> rid of this!
> -                           WS_EX_RIGHTSCROLLBAR,
> -                           kWindowClassName, 0,
> -                           WS_POPUP | WS_CLIPCHILDREN |  
> WS_CLIPSIBLINGS, 0, 0,
> -                           0, 0, NULL, 0, GetModuleHandle(NULL), 0);
> -        if (!mPluginWindowHWND)
> -            return false;
> -        if (!SetProp(mPluginWindowHWND,  
> kPluginInstanceChildProperty, this))
> -            return false;
> +    mPluginWindowHWND =
> +        CreateWindowEx(WS_EX_LEFT | WS_EX_LTRREADING |
> +                       WS_EX_NOPARENTNOTIFY | // XXXbent Get rid of  
> this!
> +                       WS_EX_RIGHTSCROLLBAR,
> +                       kWindowClassName, 0,
> +                       WS_POPUP | WS_CLIPCHILDREN |  
> WS_CLIPSIBLINGS, 0, 0,
> +                       0, 0, NULL, 0, GetModuleHandle(NULL), 0);
> +    if (!mPluginWindowHWND)
> +        return false;
> +    if (!SetProp(mPluginWindowHWND, kPluginInstanceChildProperty,  
> this))
> +        return false;
>
> -        // Apparently some plugins require an ASCII WndProc.
> -        SetWindowLongPtrA(mPluginWindowHWND, GWLP_WNDPROC,
> -                          reinterpret_cast<LONG>(DefWindowProcA));
> -    }
> +    // Apparently some plugins require an ASCII WndProc.
> +    SetWindowLongPtrA(mPluginWindowHWND, GWLP_WNDPROC,
> +                      reinterpret_cast<LONG>(DefWindowProcA));
>
>      return true;
>  }

Is the change to CreatePluginWindow() just a reorg? if it is it should  
probably be a separate patch.




> +        mWindow.height = aWindow.height;
> +        mWindow.type   = aWindow.type;
> +        mWindow.window = (void*)mSharedSurfaceDib.GetHDC();
It would be nice if we had some type safety here...
> +
> +        *rv = mPluginIface->setwindow(&mData, &mWindow);
> +
> +        return true;
> +    }
> +
> +    // NPRemoteWindow's origin is the origin of our shared dib.
> +    mWindow.x      = 0;
> +    mWindow.y      = 0;
> +    mWindow.width  = aWindow.width;
> +    mWindow.height = aWindow.height;
> +    mWindow.type   = aWindow.type;
> +
> +    // Attach to the new shared surface parent handed us.
> +    if  
> (NS_FAILED 
> (mSharedSurfaceDib 
> .Attach((mozilla::gfx::SharedDIBWin::Handle)aWindow.surfaceHandle,
> +                                           aWindow.width,  
> aWindow.height, 32)))

mozilla::gfx::SharedDIBWin::Handle is a pretty long type name. I don't  
really like the idea of having really deep namespaces, but I'm not  
sure what other people think. There is certainly a precedent for this  
type of thing.

> +      return false;
> +
> +    mWindow.window = (void*)mSharedSurfaceDib.GetHDC();
> +    *rv = mPluginIface->setwindow(&mData, &mWindow);

This function seems to duplicate quite a bit (mWindow.x  == 0... and  
the GetHDC, setwindow stuff)

> +
> +    return true;
> +}
> +
> +void
> +PluginInstanceChild::SharedSurfaceFree()
> +{
> +    mSharedSurfaceDib.Close();
> +}
> +
> +void
> +PluginInstanceChild::SharedSurfaceBeforePaint(NPEvent& evcopy)
> +{

This name's not really the best. It describes when to call it, but not  
really what it does. Maybe that's ok..




>    : mParent(parent),
>      mNPP(npp),
> -    mNPNIface(npniface)
> +    mNPNIface(npniface),
> +    mWindowType(NPWindowTypeWindow)
>  {
> +#if defined(OS_WIN)
> +  mDoublePassEvent  
> = ::RegisterWindowMessage(NS_OOPP_DOUBLEPASS_MSGID);
> +  mLocalCopyRender = false;
> +#endif

Do we need to use ::RegisterWindowMessage? We're not sending the  
events through typical means, just over our IPC connection right?


> +
> +bool
> +PluginInstanceParent::SharedSurfaceReset(const NPWindow* aWindow,  
> NPRemoteWindow& aRemoteWindow)
> +{

I don't really like the name of this method




> diff --git a/gfx/ipc/SharedDIB.cpp b/gfx/ipc/SharedDIB.cpp
> new file mode 100644
> --- /dev/null

I wonder if it's worth having SharedDIB? It seems like a pretty thin  
wrapper around base::SharedMemory and has only one user...


> diff --git a/gfx/ipc/SharedDIBWin.cpp b/gfx/ipc/SharedDIBWin.cpp
> new file mode 100644
> --- /dev/null
> +++ b/gfx/ipc/SharedDIBWin.cpp
> @@ -0,0 +1,161 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic- 
> offset: 2 -*- */
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> + *
> + * The contents of this file are subject to the Mozilla Public  
> License Version
> + * 1.1 (the "License"); you may not use this file except in  
> compliance with
> + * the License. You may obtain a copy of the License at
> + * http://www.mozilla.org/MPL/
> + *
> + * Software distributed under the License is distributed on an "AS  
> IS" basis,
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the  
> License
> + * for the specific language governing rights and limitations under  
> the
> + * License.
> + *
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + * Mozilla Foundation.
> + * Portions 'd by the Initial Developer are Copyright (C) 2009
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Jim Mathies <[e-mail]>
> + *
> + * Alternatively, the contents of this file may be used under the  
> terms of
> + * either the GNU General Public License Version 2 or later (the  
> "GPL"), or
> + * the GNU Lesser General Public License Version 2.1 or later (the  
> "LGPL"),
> + * in which case the provisions of the GPL or the LGPL are  
> applicable instead
> + * of those above. If you wish to allow use of your version of this  
> file only
> + * under the terms of either the GPL or the LGPL, and not to allow  
> others to
> + * use your version of this file under the terms of the MPL,  
> indicate your
> + * decision by deleting the provisions above and replace them with  
> the notice
> + * and other provisions required by the GPL or the LGPL. If you do  
> not delete
> + * the provisions above, a recipient may use your version of this  
> file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#include "SharedDIBWin.h"
> +#include "nsMathUtils.h"
> +#include "nsDebug.h"
> +
> +namespace mozilla {
> +namespace gfx {
> +
> +SharedDIBWin::SharedDIBWin() :
> +    mSharedHdc(nsnull)
> +  , mSharedBmp(nsnull)
> +  , mOldObj(nsnull)
> +{
> +}
> +
> +SharedDIBWin::~SharedDIBWin()
> +{
> +  Close();
> +}
> +
> +nsresult
> +SharedDIBWin::Close()
> +{
> +  if (mSharedHdc && mOldObj)
> +    ::SelectObject(mSharedHdc, mOldObj);
> +
> +  if (mSharedHdc)
> +    ::DeleteObject(mSharedHdc);
> +
> +  if (mSharedBmp)
> +    ::DeleteObject(mSharedBmp);
> +
> +  mSharedHdc = NULL;
> +  mOldObj = mSharedBmp = NULL;
> +
> +  SharedDIB::Close();
> +
> +  return NS_OK;
> +}
> +
> +nsresult
> +SharedDIBWin::Create(HDC aHdc, PRUint32 aWidth, PRUint32 aHeight,  
> PRUint32 aDepth)
> +{
> +  Close();
> +
> +  // create the offscreen shared dib
> +  BITMAPV5HEADER bmih;
> +  PRUint32 size = SetupBitmapHeader(aWidth, aHeight, aDepth, &bmih);
> +
> +  nsresult rv = SharedDIB::Create(size);
> +  if (NS_FAILED(rv))
> +    return rv;
> +
> +  if (NS_FAILED(SetupSurface(aHdc, &bmih))) {
> +    Close();
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  return NS_OK;
> +}
> +
> +nsresult
> +SharedDIBWin::Attach(Handle aHandle, PRUint32 aWidth, PRUint32  
> aHeight, PRUint32 aDepth)
> +{
> +  Close();
> +
> +  BITMAPV5HEADER bmih;
> +  SetupBitmapHeader(aWidth, aHeight, aDepth, &bmih);
> +
> +  nsresult rv = SharedDIB::Attach(aHandle, 0);
> +  if (NS_FAILED(rv))
> +    return rv;
> +
> +  if (NS_FAILED(SetupSurface(NULL, &bmih))) {
> +    Close();
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  return NS_OK;
> +}
> +
> +PRUint32
> +SharedDIBWin::SetupBitmapHeader(PRUint32 aWidth, PRUint32 aHeight,  
> PRUint32 aDepth, BITMAPV5HEADER *aHeader)
> +{
> +  NS_ASSERTION(aDepth == 32, "Invalid SharedDIBWin depth");
> +
> +  memset((void*)aHeader, 0, sizeof(BITMAPV5HEADER));
> +  aHeader->bV5Size        = sizeof(BITMAPV5HEADER);
> +  aHeader->bV5Width       = aWidth;
> +  aHeader->bV5Height      = aHeight;
> +  aHeader->bV5Planes      = 1;
> +  aHeader->bV5BitCount    = aDepth;
> +  aHeader->bV5Compression = BI_RGB;
> +
> +  // deal better with varying depths. (we currently only ask for 32  
> bit)
> +  return (sizeof(BITMAPV5HEADER) + (aHeader->bV5Height * aHeader- 
> >bV5Width * (PRUint32)NS_ceil(aDepth/8)));

Why do we use BITMAPV5HEADER instead of BITMAPINFOHEADER?

Comment 26

[Jim Mathies [:jimm]]

(In reply to comment #25)
> > -    // plugins might be fooling with these, make a copy
> > +    // make a copy
> 
> Why the comment change?

PluginInstanceChild now makes changes so we have to make a local copy.

> 
> >      NPEvent evcopy = event.event;
> > +
> > +#ifdef OS_WIN
> > +    if (mWindow.type == NPWindowTypeDrawable && WM_PAINT ==  
> > event.event.event)
> > +        SharedSurfaceBeforePaint(evcopy);
> > +#endif
> > +
> 
> event.event.event looks dumb, but that's partly my fault. Maybe use  
> evcopy?

updated.

> 
> >      *handled = mPluginIface->event(&mData,  
> > reinterpret_cast<void*>(&evcopy));
> >
> >  #ifdef MOZ_X11
> >      if (GraphicsExpose == event.event.type) {
> >          // Make sure the X server completes the drawing before the  
> > parent
> >          // draws on top and destroys the Drawable.
> >          //
> >          // XSync() waits for the X server to complete.  Really this  
> > child
> > @@ -403,55 +410,68 @@ PluginInstanceChild::AnswerNPP_SetWindow
> >      mWsInfo.colormap = aWindow.colormap;
> >      if (!XVisualIDToInfo(mWsInfo.display, aWindow.visualID,
> >                           &mWsInfo.visual, &mWsInfo.depth))
> >          return false;
> >
> >      *rv = mPluginIface->setwindow(&mData, &mWindow);
> >
> >  #elif defined(OS_WIN)
> > -    ReparentPluginWindow((HWND)aWindow.window);
> > -    SizePluginWindow(aWindow.width, aWindow.height);
> > +    switch (aWindow.type) {
> > +      case NPWindowTypeWindow:
> > +      {
> > +          if (!CreatePluginWindow())
> > +              return false;
> 
> Why do we call CreatePluginWindow() when we didn't before? Because of  
> the change in PluginInstanceChild::Initialize()?

PluginInstanceChild was setup to only handle windowed plugins - creating a window in initialize. We don't know what kind of plugin we'll be working with until setwindow, so the re-org was needed.

> > +          // Woah!
> 
> Maybe we want to assert here?

added.

> > @@ -463,16 +483,20 @@ PluginInstanceChild::Destroy()
> >      }
> > +
> > +#if defined(OS_WIN)
> > +    SharedSurfaceFree();
> > +#endif
> >  }
> 
> It's ok to call SharedSurfaceFree() even if we didn't do  
> SharedSurfaceSetup()?
> 

Yep, SharedDIB anticipates such things.

> > +        mWindow.height = aWindow.height;
> > +        mWindow.type   = aWindow.type;
> > +        mWindow.window = (void*)mSharedSurfaceDib.GetHDC();
> It would be nice if we had some type safety here...
> > +
> > +        *rv = mPluginIface->setwindow(&mData, &mWindow);
> > +
> > +        return true;
> > +    }
> > +
> > +    // NPRemoteWindow's origin is the origin of our shared dib.
> > +    mWindow.x      = 0;
> > +    mWindow.y      = 0;
> > +    mWindow.width  = aWindow.width;
> > +    mWindow.height = aWindow.height;
> > +    mWindow.type   = aWindow.type;
> > +
> > +    // Attach to the new shared surface parent handed us.
> > +    if  
> > (NS_FAILED 
> > (mSharedSurfaceDib 
> > .Attach((mozilla::gfx::SharedDIBWin::Handle)aWindow.surfaceHandle,
> > +                                           aWindow.width,  
> > aWindow.height, 32)))
> 
> mozilla::gfx::SharedDIBWin::Handle is a pretty long type name. I don't  
> really like the idea of having really deep namespaces, but I'm not  
> sure what other people think. There is certainly a precedent for this  
> type of thing.

I'll simplify this.

> 
> > +      return false;
> > +
> > +    mWindow.window = (void*)mSharedSurfaceDib.GetHDC();
> > +    *rv = mPluginIface->setwindow(&mData, &mWindow);
> 
> This function seems to duplicate quite a bit (mWindow.x  == 0... and  
> the GetHDC, setwindow stuff)

I'll streamline it a bit.

> > +void
> > +PluginInstanceChild::SharedSurfaceFree()
> > +void
> > +PluginInstanceChild::SharedSurfaceBeforePaint(NPEvent& evcopy)
> > +{
> 
> This name's not really the best. It describes when to call it, but not  
> really what it does. Maybe that's ok..

I think the windowless code should be opaque generally. "You need to do this here" type of thing.

We could update these to "WindowlessBeforePaint", etc. that might make it clearer.

> > +#if defined(OS_WIN)
> > +  mDoublePassEvent  
> > = ::RegisterWindowMessage(NS_OOPP_DOUBLEPASS_MSGID);
> > +  mLocalCopyRender = false;
> > +#endif
> 
> Do we need to use ::RegisterWindowMessage? We're not sending the  
> events through typical means, just over our IPC connection right?

The message goes from nsObjectFrame to PluginInstanceParent, it doesn't go over the wire. We could chose a hard coded value like WM_APP+1 since we filter windows events for windowless plugins pretty heavily. But RegisterWindowMessage insures we'll not conflict, ever. This seemed like the best approach.

> > +
> > +bool
> > +PluginInstanceParent::SharedSurfaceReset(const NPWindow* aWindow,  
> > NPRemoteWindow& aRemoteWindow)
> > +{
> 
> I don't really like the name of this method

I'll revamp naming of these per comments above.


> > diff --git a/gfx/ipc/SharedDIB.cpp b/gfx/ipc/SharedDIB.cpp
> > new file mode 100644
> > --- /dev/null
> 
> I wonder if it's worth having SharedDIB? It seems like a pretty thin  
> wrapper around base::SharedMemory and has only one user...

I started with a win32 shared dib, then split it out thinking we might need this on other platforms. I'll bring it up on irc.

> Why do we use BITMAPV5HEADER instead of BITMAPINFOHEADER?

It's the latest definition and is defined on all the sdks we support. Didn't see anything wrong with using it?

Comment 27

[Jim Mathies [:jimm]]

Attachment: windowless patch v.2

Update to comments.

Comment 28

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 415146 [details] [diff] [review]
windowless patch v.2


>+
>+  void* ppvBits = nsnull;
>+  mSharedBmp = ::CreateDIBSection(mSharedHdc,
>+                                  (BITMAPINFO*)aHdr,
>+                                  DIB_RGB_COLORS,
>+                                  (void**)&ppvBits,
>+                                  mShMem->handle(),
>+                                  (unsigned long)sizeof(BITMAPV5HEADER));

I suggested using a BITMAPINFOHEADER because I thought it would avoid the need to cast here. Looks like CreateDIBSection actually takes a 'BITMAPINFO*' so we could use that instead of BITMAPINFOHEADER. Also the sizeof(BITMAPV5HEADER) probably isn't what you want even if it does still work.

Comment 29

[Jim Mathies [:jimm]]

(In reply to comment #28)
> (From update of attachment 415146 [details] [diff] [review])
> 
> >+
> >+  void* ppvBits = nsnull;
> >+  mSharedBmp = ::CreateDIBSection(mSharedHdc,
> >+                                  (BITMAPINFO*)aHdr,
> >+                                  DIB_RGB_COLORS,
> >+                                  (void**)&ppvBits,
> >+                                  mShMem->handle(),
> >+                                  (unsigned long)sizeof(BITMAPV5HEADER));
> 
> I suggested using a BITMAPINFOHEADER because I thought it would avoid the need
> to cast here. Looks like CreateDIBSection actually takes a 'BITMAPINFO*' so we

BITMAPINFO contains a BITMAPINFOHEADER plus pal info. Casting is ok as long as you don't need the color array. I looked at ciaro code and we always use the base BITMAPINFOHEADER definition so I went ahead and ditched 'V5' struct use.

> could use that instead of BITMAPINFOHEADER. Also the sizeof(BITMAPV5HEADER)
> probably isn't what you want even if it does still work.

Oops, will fix.

Comment 30

[Jeff Muizelaar [:jrmuizel]]

> > Do we need to use ::RegisterWindowMessage? We're not sending the  
> > events through typical means, just over our IPC connection right?
> 
> The message goes from nsObjectFrame to PluginInstanceParent, it doesn't go over
> the wire. We could chose a hard coded value like WM_APP+1 since we filter
> windows events for windowless plugins pretty heavily. But RegisterWindowMessage
> insures we'll not conflict, ever. This seemed like the best approach.

How about adding a comment to that effect?

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 415146 [details] [diff] [review]
windowless patch v.2


>+
>+    // setup the translated dirty rect we'll send to the child
>+    rect.left   = dirtyRect.x;
>+    rect.top    = dirtyRect.y;
>+    rect.right  = dirtyRect.width;
>+    rect.bottom = dirtyRect.height;
>+
>+    npremoteevent.event.wParam = WPARAM(0);
>+    npremoteevent.event.lParam = LPARAM(dr);
>+
>+    // Send the event to the plugin
>+    return true;

npremoteevent.event.lParam is getting reassigned with the same value?

Comment 32

[Jim Mathies [:jimm]]

(In reply to comment #31)
> (From update of attachment 415146 [details] [diff] [review])
> 
> >+
> >+    // setup the translated dirty rect we'll send to the child
> >+    rect.left   = dirtyRect.x;
> >+    rect.top    = dirtyRect.y;
> >+    rect.right  = dirtyRect.width;
> >+    rect.bottom = dirtyRect.height;
> >+
> >+    npremoteevent.event.wParam = WPARAM(0);
> >+    npremoteevent.event.lParam = LPARAM(dr);
> >+
> >+    // Send the event to the plugin
> >+    return true;
> 
> npremoteevent.event.lParam is getting reassigned with the same value?

That should be &rect, I made a change from the previous patch to fix an out of scope rect pointer.

Comment 33

[Jim Mathies [:jimm]]

Attachment: windowless patch v.3

Updated to comments.

Comment 34

[Jim Mathies [:jimm]]

Attachment: winless distortion test case