527416 - NPAPI implementation missing from Qt port of Gecko, tested on Fennec, 1.9.2 branch

Status: RESOLVED DUPLICATE of bug 464966

(Core Graveyard :: Plug-ins, defect)

Description

[Juha Seppänen]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3
Build Identifier: 1.9.2

STEPS LEADING TO PROBLEM: 
1. Install a NPAPI plug-in
2. Open up a page that has the plug-in functionality

EXPECTED OUTCOME:
Plug-in functionality works as specified

ACTUAL OUTCOME:
1. No plug-in functionality present in the page
2. Syslog contains the following information:

RESIZING NSWINDOW: 0x363abc80 640 x 385
FIXME:>>>>>>Func:void* nsWindow::SetupPluginPort()::1820

FIXME:>>>>>>Func:void* nsWindow::SetupPluginPort()::1820

FREQUENCY OF OCCURRENCE: 
Always

ADDITIONAL INFORMATION
1. nsWindow::SetupPluginPort function implementation is missing in Mozilla Qt
code, quote from
http://mxr.mozilla.org/mozilla-central/source/widget/src/qt/nsWindow.cpp:

nsWindow::SetupPluginPort(void)
{
     if (!mWidget)
         return nsnull;

     qDebug("FIXME:>>>>>>Func:%s::%d\n", __PRETTY_FUNCTION__, __LINE__);

     return nsnull;
}


Reproducible: Always

Comment 1

[Miika Jarvinen]

Attachment: Work in a progress patch for 1.9.2

This is my first attempt at providing an answer, so please regard it as work in progress.

This should be applied to the 1.9.2 branch, but there are a few things to watch out for. Windowless plugins needs to use QPainter to draw. Otherwise drawn content is lost (quite surely) because of Qt:s doublebuffering. (This can done by searching for widget or Pixmap with QWidget::find or pixmap with QPixmap::fromX11Pixmap) . Composed plugins are under developement, and doesnt work correctly yet. This patch has been tested with Qt build of Firefox and Fennec on desktop.

Comment 2

[Miika Jarvinen]

Attachment: New version of work in a progress patch

New version of work in a progress patch. Appends composited plugin support, and cleans up code. Still for 1.9.2 branch, I'm now going to check changes needed for mozilla-central. 

This patch requires patch on bug 520790 to be applied, because NULL QPaintDevice pointer is occasionally referenced otherwise.

Comment 3

[Juha Seppänen]

There was also this https://bugzilla.mozilla.org/show_bug.cgi?id=464966 where pretty much similar thing was done but not as extensive as Miika's thing.

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch

This patch should support all mandatory things for the Qt case.

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 432104 [details] [diff] [review]
patch

Doug, can you take a look or should we point at Josh?

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 432104 [details] [diff] [review]
patch

i can review first.  Oleg should also look at the next patch

please create a patch against m-c.  much of nsObjectFrame is different.


in nsPluginNativeWindowQt.cpp:


the copyright assignment is wrong.

+#if (MOZ_PLATFORM_MAEMO == 5) || (MOZ_PLATFORM_MAEMO == 6)

#ifdef MOZ_PLATFORM_MAEMO instead?

In this file, would MOZ_COMPOSITED_PLUGINS ever not be set?

+#define DEBUG

drop it

+  /**
+    Creates hidden window for composited plugin. Hides window by moving it to
+   (1000,1000)
+  */


Comment style is funky.  I think you want *'s on each line so that it looks closer to what other people do. (javadoc style)

+// FIXME: leads to a stack overflow for yet unknown reasons (maybe some interaction with QX11EmbedContainer's X11 event filter)

That scares me.


Should xdamage_event_base be a member var?

+  mContainerWidget = NULL;

no need to null stuff in the destructor.  Also, use nsnull instead of NULL.

Comment 7

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #6)
> i can review first.  Oleg should also look at the next patch

Oleg, should know the patch already but he certainly can look at it.
 
> please create a patch against m-c.  much of nsObjectFrame is different.

Hmm, this patch is created against m-c from this morning.

We will address your comments.

Comment 8

[Oleg Romashin (:romaxa)]

Comment on attachment 432104 [details] [diff] [review]
patch


>-#include <QWidget>
>+#include "cairo.h"
> 
> #include "gfxQtNativeRenderer.h"
> #include "gfxContext.h"
> 
>+#undef malloc
>+#undef realloc
>+#include <QWidget>
>+#include <QPainter>

This is fail to compile for me... move this to the top of file


>+    double device_offset_x, device_offset_y;

>+
>+        if ( painterWasActive) {
>+          if (isWidget)
>+            painter->restore();

4 spaces, 2 spaces... fix indentation here  

>+        result = this->NativeDraw(paintDevice,offset_x,offset_y,NULL,0);
........
>+        p.drawPixmap(offset_x/matrix.xx,offset_y/matrix.yy,map);
Here and in many other places put space after comma, like:
+        p.drawPixmap(offset_x / matrix.xx, offset_y / matrix.yy, map);

>+      } else
>+        return result;

No need to else here
> 
>-#ifdef MOZ_X11
>-#ifdef MOZ_WIDGET_QT
>-#include <QWidget>
>-#include <QX11Info>
>-#endif
>-#endif
>-
> #include "nscore.h"
> #include "nsCOMPtr.h"
> #include "nsPresContext.h"
> #include "nsIPresShell.h"
> #include "nsWidgetsCID.h"
> #include "nsIView.h"
> #include "nsIViewManager.h"
> #include "nsIDOMKeyListener.h"
> #include "nsIDOMDragEvent.h"
>+#ifdef MOZ_X11
>+#ifdef MOZ_WIDGET_QT
>+#undef malloc
>+#undef realloc
>+#include <QWidget>
>+#include <QX11Info>
>+#include <QPainter>
>+#include <QX11EmbedContainer>
>+#endif
>+#endif

Why are you moving this in the middle of include list?  also it does not compile for me...

>-    virtual nsresult NativeDraw(QWidget * drawable, short offsetX, 
>+    virtual nsresult NativeDraw(QPaintDevice * drawable, short offsetX,
Minor style fix, also in many other places the same:
+    virtual nsresult NativeDraw(QPaintDevice* drawable, short offsetX,


>+#ifdef MOZ_WIDGET_QT
>+  //With Qt test plugin crashes with SIGABORT because window is already gone
>+  //if delayed stop is used. This is caused by ->deleteLater on mozQWidget

No sure, but do we really need deleteLater in mozQWidget? what is problem there?


>-  Renderer renderer(window, mInstance, pluginSize, pluginDirtyRect);
>-  renderer.Draw(aContext, window->width, window->height,
>-                rendererFlags, nsnull);
>+ #ifdef MOZ_WIDGET_QT

start ifdef from 0 position

>+   Renderer renderer(window, mInstance, pluginSize, pluginDirtyRect);
>+   renderer.Draw(aContext, NS_lround(pluginRect.size.width), NS_lround(pluginRect.size.height),
this line > than 80 symbols, move some arguments to the next line


>   mXlibSurfGC = XCreateGC(gdk_x11_get_default_xdisplay(),
>                           mBlitWindow,
>                           0,
>                           0);
>-  if (!mXlibSurfGC)
>+//   if (!mXlibSurfGC)
>     return PR_FALSE;

Don't understand this trick, create ifdef or something, if you don't want to proceed with this code

>+
>+  if (!pixmap)
>+      return NS_ERROR_FAILURE;
>+
>+

Here and in some other places you are adding 2 empty lines... don't do that



(((nsPluginNativeWindow*)mWindow)->mPlugWindow);
>+    if (!plug || !plug->clientWinId()) {
>+        return NS_ERROR_FAILURE;
>+    }

Maybe NS_ENSURE_TRUE(plug && plug->clientWinId(), NS_ERROR_FAILURE)
would be better?

>--- a/modules/plugin/base/src/nsNPAPIPlugin.cpp
> 
>-#if defined(XP_WIN) || defined(XP_OS2) || defined(MOZ_WIDGET_GTK2)
>+#if defined(XP_WIN) || defined(XP_OS2) || defined(MOZ_WIDGET_GTK2)\

Some space is missing here ")\"

>+ || defined(MOZ_WIDGET_QT)
>   case NPNVnetscapeWindow: {
>diff --git a/modules/plugin/test/testplugin/Makefile.in b/modules/plugin/test/testplugin/Makefile.in
>--- a/modules/plugin/test/testplugin/Makefile.in
>+++ b/modules/plugin/test/testplugin/Makefile.in
>@@ -48,17 +48,17 @@ MODULE_NAME  = TestPlugin
> 
> 
> # Need to custom install plugins
> NO_DIST_INSTALL	= 1
> NO_INSTALL = 1
> 
> CPPSRCS   =  \
>   nptest.cpp \
>-  nptest_utils.cpp \
>+  nptest_utils.cpp

Why are you doing this?


>+
>+#if (MOZ_PLATFORM_MAEMO == 5) || (MOZ_PLATFORM_MAEMO == 6)

All latest upstream looks like:
#if (MOZ_PLATFORM_MAEMO == 5) && defined(MOZ_WIDGET_GTK2)
if we are supporting correctly composited plugins, then we should fix back those defines and make sure that it  compiles and somehow works with  ifdef MOZ_PLATFORM_MAEMO

If not, then #if (MOZ_PLATFORM_MAEMO == 5) && defined(MOZ_WIDGET_GTK2) should be also here

>+nsresult PLUG_NewPluginNativeWindow(nsPluginNativeWindow ** aPluginNativeWindow)




>+// FIXME: leads to a stack overflow for yet unknown reasons (maybe some interaction with QX11EmbedContainer's X11 event filter)
>+//     if (msPreviousFilter) {
>+//         return msPreviousFilter(message,result);
>+//     }
Can we kill it?


>+  if(aPluginInstance) {
>+    if (type == NPWindowTypeWindow) {
what is style guideline are you using?

>+      nsresult rv;
>+      if(!mContainerWidget) {
>+        PRBool needXEmbed = PR_FALSE;
>+        if (CanGetValueFromPlugin(aPluginInstance)) {

The same style problem in the rest of the file


>+    Display* display = QX11Info::display();
>+    if (!XDamageQueryExtension (display, &xdamage_event_base, &junk)) {
>+      printf ("This requires the XDamage extension");

Here and in some other places we should not have any printfs I guess... 



>+    //printf("NPERR_OUT_OF_MEMORY_ERROR\n");
>+    return NPERR_OUT_OF_MEMORY_ERROR;

looks not very nice, wrap in NS_ENSURE_TRUE or just remove commented printf, here and in some other places..


> void
> pluginWidgetInit(InstanceData* instanceData, void* oldWindow)
> {
>+  // XXX nothing here yet since we don't support windowed plugins
> }



>@@ -1106,16 +1109,18 @@ nsWindow::OnMoveEvent(QGraphicsSceneHove
> {
>     LOG(("configure event [%p] %d %d\n", (void *)this,
>         aEvent->pos().x(),  aEvent->pos().y()));
> 
>     // can we shortcut?
>     if (!mWidget)
>         return nsEventStatus_eIgnore;
> 
>+
>+

Kill it

Comment 9

[Axel Linder]

O.k. - preparing a revised version of the patch regarding the comments.

Comment 10

[Doug Turner (:dougt)]

Axel, please make sure it compiles against m-c.  No one really cares about Qt on 1.9.2.

Comment 11

[Oleg Romashin (:romaxa)]

Comment on attachment 432104 [details] [diff] [review]
patch

>+
>+        QPixmap map(width,height);

This is will not work in qgraphicssystem raster, because in raster engine QPixmap == QImage, and pixmap->handle() will be invalid.
Create here gfxXlibSurface, and use fromX11Pixmap(xsurface->XDrawable(), QPixmap::ExplicitlyShared);

>+        if((flags & DRAW_IS_OPAQUE) != DRAW_IS_OPAQUE)
>+          map.fill(QColor(255,255,255,0));
This is wrong:
1) if you have default 16bpp depth system, it will drop 16bpp pixmap and create 32bpp pixmap.
2) also most of plugins are not expecting 32bpp blending, because it is not supported by pure Xlib (without extensions...), surface must be opaque with browser content rendered inside, and plugin will paint it's content on top of that surface.


>+        this->NativeDraw(&map,0,0,NULL,0);

Comment 12

[Axel Linder]

Working on a new adapted approach (as mentioned by Oleg - drawing/using content to background instead of creating an alpha channel), because we have detected some issues in this approach. Will upload revised patch this week.