544250 - Implement different rendering backends for Qt mozilla port

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Oleg Romashin (:romaxa)]

Original idea is to provide way to render Qt Port with gfxImageSurface/pixman.
Also provide option to render Qt Port with XLibSurface backend.
Make possibility to switch between rendering backends without recompiling whole stuff, and choose best option for rendering on specific platform.

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: WIP1 version

Patch is providing buffered pixmap rendering similar to bug 450916.
Also it is allowing to compile Qt port with system cairo (which does not have qpainter surface)
ENV, and pref option to switch between Image/Xlib/Qpainter rendering.
Also initial implementation for gfxSharedImageSurface class

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 425201 [details] [diff] [review]
WIP1 version

karl, would you be the right person to take a first pass on this patch?  It isn't completely, but it would be good to get your eyes on it.

oleg, patch isn't applying cleanly on m-c.  Two small problem (MOZ_TREE_CAIRO, and something in nsWindow.cpp).

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 425201 [details] [diff] [review]
WIP1 version

s/karl/jeff

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: WIP2 A bit updated version

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: WIP3 update

Fixed Native Qt theme rendering.
Removed 16bpp part.
Fixed better implementation for color conversion stuff (16bpp target destination rendering).
Moved functionality from gfxSharedImageSurface.h to gfxSharedImageSurface.cpp

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 426233 [details] [diff] [review]
WIP3 update

>diff -r a99321b82eae gfx/thebes/public/Makefile.in
>--- a/gfx/thebes/public/Makefile.in	Wed Feb 10 09:04:39 2010 +0200
>+++ b/gfx/thebes/public/Makefile.in	Wed Feb 10 15:04:35 2010 +0200
>@@ -32,6 +32,10 @@ EXPORTS		= 	gfxASurface.h \
> 			gfxUserFontSet.h \
> 			$(NULL)
> 
>+EXPORTS		+= \
>+			gfxSharedImageSurface.h \
>+			$(NULL)
>+
> EXPORTS += gfxFontTest.h
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
>@@ -75,7 +79,10 @@ EXPORTS += gfxFT2FontBase.h
> endif
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),qt)
>-EXPORTS += gfxQtPlatform.h gfxQPainterSurface.h
>+EXPORTS += gfxQtPlatform.h
>+ifdef MOZ_TREE_CAIRO
>+EXPORTS += gfxQPainterSurface.h
>+endif
> EXPORTS += gfxXlibSurface.h gfxQtNativeRenderer.h
> EXPORTS += gfxFT2Fonts.h
> EXPORTS += gfxFT2FontBase.h
>diff -r a99321b82eae gfx/thebes/public/gfxSharedImageSurface.h
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/gfx/thebes/public/gfxSharedImageSurface.h	Wed Feb 10 15:04:35 2010 +0200
>@@ -0,0 +1,104 @@
>+/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ * ***** 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 Nokia Corporation code.
>+ *
>+ * The Initial Developer of the Original Code is Nokia Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2005
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Oleg Romashin <[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 ***** */
>+
>+#ifndef GFX_SHARED_IMAGESURFACE_H
>+#define GFX_SHARED_IMAGESURFACE_H
>+
>+#ifdef HAVE_XSHM
>+
>+#include "gfxASurface.h"
>+#include "gfxImageSurface.h"
>+#include "gfxPoint.h"
>+
>+#include <stdlib.h>
>+#include <string.h>
>+
>+#ifdef MOZ_WIDGET_QT
>+#include <QX11Info>
>+#endif
>+
>+#ifdef MOZ_WIDGET_GTK2
>+#include <gtk/gtk.h>
>+#include <gdk/gdk.h>
>+#include <gdk/gdkx.h>
>+#endif
>+
>+#ifdef MOZ_X11
>+#include <X11/Xlib.h>
>+#include <X11/Xutil.h>
>+#include <X11/extensions/XShm.h>
>+#endif
>+
>+#include <sys/ipc.h>
>+#include <sys/shm.h>
>+#include <cairo.h>
>+#include <cairo-deprecated.h>
>+
>+class THEBES_API gfxSharedImageSurface : public gfxASurface {
>+public:
>+  gfxSharedImageSurface();
>+  ~gfxSharedImageSurface();
>+  int shmid() { return mShmId; }
>+  int width() { return mWidth; }
>+  int height() { return mHeight; }
>+  unsigned int depth() { return mDepth; }
>+  unsigned int bpl() { return mBytesPerLine; }
>+  char* data() { return mDataAddr; }
>+  XImage *image() { return mXShmImage; }
>+  cairo_surface_t* cairo_img_surface();
>+  int CreateXShmImage(int aWidth, int aHeight, int aDepth = -1, int aShmId = 0, Display *display = NULL);
>+  void DestroyXShmImage();
>+
>+private:
>+  int CreateShm(int aShmid = 0);
>+  void DestroyShm();
>+
>+  XShmSegmentInfo  mShmInfo;
>+  XImage          *mXShmImage;
>+  int              mWidth;
>+  int              mHeight;
>+  unsigned int     mDepth;
>+  unsigned int     mBytesPerLine;
>+  char            *mDataAddr;
>+  int              mShmId;
>+  bool             mFromExternalShm;
>+  bool             mCreatedImage;
>+  Display *mDisp;
>+};
>+
>+#endif /* HAVE_SHM */
>+#endif /* GFX_SHARED_IMAGESURFACE_H */
>diff -r a99321b82eae gfx/thebes/src/Makefile.in
>--- a/gfx/thebes/src/Makefile.in	Wed Feb 10 09:04:39 2010 +0200
>+++ b/gfx/thebes/src/Makefile.in	Wed Feb 10 15:04:35 2010 +0200
>@@ -34,6 +34,10 @@ CPPSRCS	= \
> 	gfxUserFontSet.cpp \
> 	$(NULL)
> 
>+CPPSRCS += \
>+	gfxSharedImageSurface.cpp \
>+	$(NULL)
>+
> EXTRA_DSO_LDOPTS += \
> 	$(MOZ_CAIRO_LIBS) \
> 	$(MOZ_UNICHARUTIL_LIBS) \
>@@ -121,7 +125,10 @@ CPPSRCS += gfxDirectFBSurface.cpp
> endif
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),qt)
>-CPPSRCS += gfxQtPlatform.cpp gfxQPainterSurface.cpp
>+CPPSRCS += gfxQtPlatform.cpp
>+ifdef MOZ_TREE_CAIRO
>+CPPSRCS += gfxQPainterSurface.cpp
>+endif
> CPPSRCS += gfxXlibSurface.cpp gfxQtNativeRenderer.cpp
> CPPSRCS +=	gfxFT2Fonts.cpp
> CPPSRCS +=	gfxFT2FontBase.cpp
>diff -r a99321b82eae gfx/thebes/src/gfxQtPlatform.cpp
>--- a/gfx/thebes/src/gfxQtPlatform.cpp	Wed Feb 10 09:04:39 2010 +0200
>+++ b/gfx/thebes/src/gfxQtPlatform.cpp	Wed Feb 10 15:04:35 2010 +0200
>@@ -48,7 +48,9 @@
> #include "cairo.h"
> 
> #include "gfxImageSurface.h"
>+#ifdef MOZ_TREE_CAIRO
> #include "gfxQPainterSurface.h"
>+#endif
> 
> #include "gfxFT2Fonts.h"
> 
>@@ -58,12 +60,17 @@
> 
> #include "nsMathUtils.h"
> #include "nsTArray.h"
>+#ifdef MOZ_X11
>+#include "gfxXlibSurface.h"
>+#endif
> 
> #include "qcms.h"
> 
> #include <ft2build.h>
> #include FT_FREETYPE_H
> 
>+extern PRInt32 gQWidgetRenderingSurfaceType;
>+
> gfxFontconfigUtils *gfxQtPlatform::sFontconfigUtils = nsnull;
> static cairo_user_data_key_t cairo_qt_pixmap_key;
> static void do_qt_pixmap_unref (void *data)
>@@ -134,8 +141,59 @@ already_AddRefed<gfxASurface>
> gfxQtPlatform::CreateOffscreenSurface(const gfxIntSize& size,
>                                       gfxASurface::gfxImageFormat imageFormat)
> {
>-    nsRefPtr<gfxASurface> newSurface =
>-        new gfxQPainterSurface (size, gfxASurface::ContentFromFormat(imageFormat));
>+    nsRefPtr<gfxASurface> newSurface = nsnull;
>+
>+#ifdef MOZ_TREE_CAIRO
>+    if (gQWidgetRenderingSurfaceType == 0) {
>+      newSurface = new gfxQPainterSurface(size, gfxASurface::ContentFromFormat(imageFormat));
>+      return newSurface.forget();
>+    }
>+#endif
>+    if (gQWidgetRenderingSurfaceType == 2) {
>+      newSurface = new gfxImageSurface(size, imageFormat);
>+      return newSurface.forget();
>+    }

surface type should probably have names.

>+
>+#ifdef MOZ_X11
>+    int glitzf;
>+    int xrenderFormatID = -1;
>+    switch (imageFormat) {
>+        case gfxASurface::ImageFormatARGB32:
>+            glitzf = 0; // GLITZ_STANDARD_ARGB32;
>+            xrenderFormatID = PictStandardARGB32;
>+            break;
>+        case gfxASurface::ImageFormatRGB24:
>+            glitzf = 1; // GLITZ_STANDARD_RGB24;
>+            xrenderFormatID = PictStandardRGB24;
>+            break;
>+        case gfxASurface::ImageFormatA8:
>+            glitzf = 2; // GLITZ_STANDARD_A8;
>+            xrenderFormatID = PictStandardA8;
>+            break;
>+        case gfxASurface::ImageFormatA1:
>+            glitzf = 3; // GLITZ_STANDARD_A1;
>+            xrenderFormatID = PictStandardA1;
>+            break;
>+        default:
>+            return nsnull;
>+    }

What's glitzf?

>+
>+    // XXX we really need a different interface here, something that passes
>+    // in more context, including the display and/or target surface type that
>+    // we should try to match
>+    XRenderPictFormat* xrenderFormat =
>+        XRenderFindStandardFormat(QX11Info().display(), xrenderFormatID);
>+
>+    newSurface = new gfxXlibSurface((Display*)QX11Info().display(),
>+                                    xrenderFormat,
>+                                    size);
>+#endif
>+
>+    if (newSurface) {
>+        gfxContext tmpCtx(newSurface);
>+        tmpCtx.SetOperator(gfxContext::OPERATOR_CLEAR);
>+        tmpCtx.Paint();
>+    }

calling it ctx is probably fine, no need to add 'tmp'

> 
>     return newSurface.forget();
> }
>diff -r a99321b82eae gfx/thebes/src/gfxSharedImageSurface.cpp
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/gfx/thebes/src/gfxSharedImageSurface.cpp	Wed Feb 10 15:04:35 2010 +0200
>@@ -0,0 +1,198 @@
>+/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ * ***** 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 Nokia Corporation code.
>+ *
>+ * The Initial Developer of the Original Code is Nokia Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2005
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Oleg Romashin <[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 "gfxSharedImageSurface.h"
>+
>+#ifdef HAVE_XSHM
>+
>+gfxSharedImageSurface::gfxSharedImageSurface()
>+ : mXShmImage(NULL),
>+   mWidth(0),
>+   mHeight(0),
>+   mDepth(0),
>+   mBytesPerLine(0),
>+   mDataAddr(NULL),
>+   mShmId(-1),
>+   mFromExternalShm(false),
>+   mCreatedImage(false)
>+{
>+    mShmInfo.shmaddr = NULL;
>+    mShmInfo.shmid = -1;
>+    mShmInfo.readOnly = false;
>+    mShmInfo.shmseg = 0;
>+}
>+
>+gfxSharedImageSurface::~gfxSharedImageSurface()
>+{
>+    if (mCreatedImage)
>+        DestroyXShmImage();
>+}
>+
>+
>+cairo_surface_t*
>+gfxSharedImageSurface::cairo_img_surface()
>+{
>+    NS_ENSURE_TRUE(mXShmImage, NULL);
>+    cairo_format_t format = CAIRO_FORMAT_RGB24;
>+#ifndef CAIRO_FORMAT_RGB16_565
>+    if (mDepth == 16)
>+        format = CAIRO_FORMAT_RGB16_565;
>+#endif

We don't have CAIRO_FORMAT_RGB16_565...?

>+    if (mDepth == 32)
>+        format = CAIRO_FORMAT_ARGB32;
>+
>+    cairo_surface_t *result =
>+        cairo_image_surface_create_for_data((unsigned char*)mXShmImage->data,
>+                                            format,
>+                                            mWidth,
>+                                            mHeight,
>+                                            mXShmImage->bytes_per_line);
>+    return result;
>+}
>+
>+int
>+gfxSharedImageSurface::CreateXShmImage(int aWidth,
>+                                       int aHeight,
>+                                       int aDepth,
>+                                       int aShmId,
>+                                       Display *display)
>+{
>+    if (mCreatedImage)
>+        DestroyXShmImage();
>+
>+    mWidth  = aWidth;
>+    mHeight = aHeight;
>+
>+    if (!aDepth)
>+#ifdef MOZ_WIDGET_QT
>+        mDepth = QX11Info::appDepth();
>+#elif defined(MOZ_WIDGET_GTK2)
>+        mDepth = gdk_visual_get_system()->depth;
>+#endif
>+    else
>+        mDepth = aDepth;
>+
>+    mDisp = display ? display :
>+#ifdef MOZ_WIDGET_QT
>+            QX11Info::display();
>+#elif defined(MOZ_WIDGET_GTK2)
>+            GDK_DISPLAY();
>+#endif

It would be nicer if we didn't have to have #ifdef noise here.
Perhaps, a common helper function that can contain the #ifdef would be better.

>+
>+    if (CreateShm(aShmId) > 0)
>+        mCreatedImage = true;
>+
>+    return mCreatedImage ? 1 : -1;
>+}

Should this function return a bool?

>+
>+void
>+gfxSharedImageSurface::DestroyXShmImage()
>+{
>+    if (!mCreatedImage)
>+        return;
>+
>+    DestroyShm();
>+    mWidth = mHeight = mDepth = mBytesPerLine = 0;
>+    mCreatedImage = false;
>+}
>+
>+int
>+gfxSharedImageSurface::CreateShm(int aShmid)
>+{
>+    memset(&mShmInfo, 0, sizeof(mShmInfo));
>+
>+    mXShmImage = XShmCreateImage(mDisp, NULL,
>+                                 mDepth, (mDepth == 1) ? XYBitmap : ZPixmap, NULL, &mShmInfo, mWidth, mHeight);
>+    if (!mXShmImage) return -1;
>+
>+    mBytesPerLine = mXShmImage->bytes_per_line;
>+
>+    if (aShmid > 0)
>+    {
>+        mShmInfo.shmid = aShmid;
>+        mFromExternalShm = true;
>+    }
>+    else
>+        mShmInfo.shmid = shmget(IPC_PRIVATE, mXShmImage->bytes_per_line * mHeight, IPC_CREAT | 0777);
>+
>+    if (mShmInfo.shmid < 0)
>+    {
>+        XDestroyImage(mXShmImage);
>+        return -1;
>+    }
>+
>+    mShmInfo.shmaddr = mDataAddr = mXShmImage->data = (char *)shmat(mShmInfo.shmid, 0, 0);
>+
>+    if (!mShmInfo.shmaddr)
>+    {
>+        shmctl(mShmInfo.shmid, IPC_RMID, 0);
>+        mShmInfo.shmid = -1;
>+        XDestroyImage(mXShmImage);
>+        return -1;
>+    }
>+
>+    mShmInfo.readOnly = False;
>+    if (!XShmAttach(mDisp, &mShmInfo))
>+    {
>+        shmctl(mShmInfo.shmid, IPC_RMID, 0);
>+        shmdt(mShmInfo.shmaddr);
>+        mShmInfo.shmaddr = mDataAddr = NULL;
>+        mShmInfo.shmid = mShmId = -1;
>+        XDestroyImage(mXShmImage);
>+        return -1;
>+    }
>+    XSync(mDisp, False);
>+    shmctl(mShmInfo.shmid, IPC_RMID, 0);
>+    mShmId = mShmInfo.shmid;
>+    return 1;
>+}

Should this function return a bool. Also, the error handling would avoid all of the code duplication if it was structured to use goto's.

>+
>+void
>+gfxSharedImageSurface::DestroyShm()
>+{
>+    if (!mDataAddr)
>+        return;
>+
>+    XSync(mDisp, False);
>+    XShmDetach(mDisp, &mShmInfo);
>+    XDestroyImage(mXShmImage);
>+    shmdt(mShmInfo.shmaddr);
>+    mShmInfo.shmaddr = mDataAddr = NULL;
>+    mShmInfo.shmid = mShmId = -1;
>+}
>+
>+#endif /* HAVE_XSHM */

>diff -r a99321b82eae widget/src/qt/nsNativeThemeQt.cpp
>--- a/widget/src/qt/nsNativeThemeQt.cpp	Wed Feb 10 09:04:39 2010 +0200
>+++ b/widget/src/qt/nsNativeThemeQt.cpp	Wed Feb 10 15:04:35 2010 +0200
>@@ -69,7 +69,12 @@
> 
> #include "gfxASurface.h"
> #include "gfxContext.h"
>+#ifdef MOZ_TREE_CAIRO
> #include "gfxQPainterSurface.h"
>+#endif
>+#ifdef MOZ_X11
>+#include "gfxXlibSurface.h"
>+#endif
> #include "nsIRenderingContext.h"
> 
> nsNativeThemeQt::nsNativeThemeQt()
>@@ -93,6 +98,29 @@ static inline QRect qRectInPixels(const 
>                  NSAppUnitsToIntPixels(aRect.height, p2a));
> }
> 
>+static inline QImage::Format
>+_qimage_from_gfximage_format (gfxASurface::gfxImageFormat aFormat)
>+{
>+    switch (aFormat) {
>+    case gfxASurface::ImageFormatARGB32:
>+        return QImage::Format_ARGB32_Premultiplied;
>+    case gfxASurface::ImageFormatRGB24:
>+        return QImage::Format_RGB32;
>+    case gfxASurface::ImageFormatA8:
>+        return QImage::Format_Indexed8;
>+    case gfxASurface::ImageFormatA1:
>+#ifdef WORDS_BIGENDIAN
>+        return QImage::Format_Mono;
>+#else
>+        return QImage::Format_MonoLSB;
>+#endif
>+    default:
>+        return QImage::Format_Invalid;
>+    }
>+
>+    return QImage::Format_Mono;
>+}
>+
> NS_IMETHODIMP
> nsNativeThemeQt::DrawWidgetBackground(nsIRenderingContext* aContext,
>                                       nsIFrame* aFrame,
>@@ -103,19 +131,58 @@ nsNativeThemeQt::DrawWidgetBackground(ns
>     gfxContext* context = aContext->ThebesContext();
>     nsRefPtr<gfxASurface> surface = context->CurrentSurface();
> 
>+#ifdef MOZ_TREE_CAIRO
>+    if (surface->GetType() == gfxASurface::SurfaceTypeQPainter) {
>+        gfxQPainterSurface* qSurface = (gfxQPainterSurface*) (surface.get());
>+        QPainter *painter = qSurface->GetQPainter();
>+        NS_ASSERTION(painter, "Where'd my QPainter go?");
>+        if (!painter) 
>+            return NS_ERROR_FAILURE;
>+        return DrawWidgetBackground(painter, aContext,
>+                                    aFrame, aWidgetType,
>+                                    aRect, aClipRect);
>+    } else
>+#endif
>+    if (surface->GetType() == gfxASurface::SurfaceTypeImage) {
>+        gfxImageSurface* qSurface = (gfxImageSurface*) (surface.get());
>+        QImage tempQImage(qSurface->Data(),
>+                          qSurface->Width(),
>+                          qSurface->Height(),
>+                          qSurface->Stride(),
>+                          _qimage_from_gfximage_format(qSurface->Format()));
>+        QPainter painter(&tempQImage);
>+        return DrawWidgetBackground(&painter, aContext,
>+                                    aFrame, aWidgetType,
>+                                    aRect, aClipRect);
>+    }
>+#ifdef MOZ_X11
>+    else if (surface->GetType() == gfxASurface::SurfaceTypeXlib) {
>+        gfxXlibSurface* qSurface = (gfxXlibSurface*) (surface.get());
>+        QPixmap pixmap(QPixmap::fromX11Pixmap(qSurface->XDrawable()));
>+        QPainter painter(&pixmap);
>+        return DrawWidgetBackground(&painter, aContext,
>+                                    aFrame, aWidgetType,
>+                                    aRect, aClipRect);
>+    }
>+#endif
>+
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+}

This feels like 3 different functions wrapped into one.

>+
>+nsresult
>+nsNativeThemeQt::DrawWidgetBackground(QPainter *qPainter,
>+                                      nsIRenderingContext* aContext,
>+                                      nsIFrame* aFrame,
>+                                      PRUint8 aWidgetType,
>+                                      const nsRect& aRect,
>+                                      const nsRect& aClipRect)
>+
>+{
>+    gfxContext* context = aContext->ThebesContext();
>+    nsRefPtr<gfxASurface> surface = context->CurrentSurface();
>+
>     context->UpdateSurfaceClip();
> 
>-    if (surface->GetType() != gfxASurface::SurfaceTypeQPainter)
>-        return NS_ERROR_NOT_IMPLEMENTED;
>-
>-    gfxQPainterSurface* qSurface = (gfxQPainterSurface*) (surface.get());
>-    QPainter* qPainter = qSurface->GetQPainter();
>-
>-    NS_ASSERTION(qPainter, "Where'd my QPainter go?");
>-
>-    if (qPainter == nsnull)
>-        return NS_OK;
>-
>     QStyle* style = qApp->style();
> 
>     qPainter->save();
>@@ -225,7 +292,7 @@ nsNativeThemeQt::DrawWidgetBackground(ns
>     }
>     case NS_THEME_DROPDOWN: {
>         QStyleOptionComboBox comboOpt;
>-        
>+
>         InitComboStyle(aWidgetType, aFrame, r, comboOpt);
> 
>         style->drawComplexControl(QStyle::CC_ComboBox, &comboOpt, qPainter);
>@@ -246,7 +313,7 @@ nsNativeThemeQt::DrawWidgetBackground(ns
>     case NS_THEME_TEXTFIELD_MULTILINE:
>     case NS_THEME_LISTBOX: {
>         QStyleOptionFrameV2 frameOpt;
>-        
>+
>         if (!IsDisabled(aFrame))
>             frameOpt.state |= QStyle::State_Enabled;
> 
>@@ -258,7 +325,7 @@ nsNativeThemeQt::DrawWidgetBackground(ns
>             contentRect.adjust(mFrameWidth, mFrameWidth, -mFrameWidth, -mFrameWidth);
>             qPainter->fillRect(contentRect, QBrush(Qt::white));
>         }
>-        
>+
>         frameOpt.palette = mNoBackgroundPalette;
>         style->drawPrimitive(QStyle::PE_FrameLineEdit, &frameOpt, qPainter, NULL);
>         break;

Whitespace noise.


>diff -r a99321b82eae widget/src/qt/nsWindow.cpp
>--- a/widget/src/qt/nsWindow.cpp	Wed Feb 10 09:04:39 2010 +0200
>+++ b/widget/src/qt/nsWindow.cpp	Wed Feb 10 15:04:35 2010 +0200
>@@ -87,9 +87,27 @@
> 
> #include "gfxQtPlatform.h"
> #include "gfxXlibSurface.h"
>+#ifdef MOZ_TREE_CAIRO
> #include "gfxQPainterSurface.h"
>+#endif
> #include "gfxContext.h"
>+#include "gfxSharedImageSurface.h"
> 
>+// Buffered Pixmap stuff
>+// 0 - default gfxQPainterSurface
>+// 1 - gfxXlibSurface
>+// 2 - gfxImageSurface
>+PRInt32 gQWidgetRenderingSurfaceType = 2;
>+static QPixmap *gBufferPixmap = nsnull;
>+static int gBufferPixmapUsageCount = 0;
>+
>+// Buffered shared image + pixmap
>+static gfxSharedImageSurface *gBufferImage = nsnull;
>+static gfxSharedImageSurface *gBufferImageTemp = nsnull;
>+PRBool gNeedColorConversion = PR_FALSE;
>+extern "C" {
>+#include "pixman.h"
>+}
> 
> /* For PrepareNativeWidget */
> static NS_DEFINE_IID(kDeviceContextCID, NS_DEVICE_CONTEXT_CID);
>@@ -175,6 +193,69 @@ nsWindow::nsWindow()
>     mIsTransparent = PR_FALSE;
> 
>     mCursor = eCursor_standard;
>+
>+    gBufferPixmapUsageCount++;
>+}
>+
>+static inline gfxASurface::gfxImageFormat
>+_depth_to_gfximage_format(PRInt32 aDepth)
>+{
>+    switch (aDepth) {
>+    case 32:
>+        return gfxASurface::ImageFormatARGB32;
>+    case 24:
>+        return gfxASurface::ImageFormatRGB24;
>+    default:
>+        return gfxASurface::ImageFormatUnknown;
>+    }
>+}
>+
>+void
>+nsWindow::UpdateOffScreenBuffers(QSize aSize, int aDepth)
>+{
>+    gfxIntSize size(aSize.width(), aSize.height());
>+    if (gBufferPixmap) {
>+        if (gBufferPixmap->size().width() < size.width ||
>+            gBufferPixmap->size().height() < size.height) {
>+            delete gBufferPixmap;
>+            gBufferPixmap = nsnull;
>+        }
>+    }
>+    if (gBufferPixmap)
>+        return;
>+
>+    gBufferPixmap = new QPixmap(size.width, size.height);
>+    if (!gBufferPixmap)
>+        return;
>+
>+    if (gQWidgetRenderingSurfaceType < 2)
>+        return;
>+
>+    gNeedColorConversion = _depth_to_gfximage_format(gBufferPixmap->x11Info().depth()) == gfxASurface::ImageFormatUnknown;
>+
>+    if (!gBufferImage)
>+        gBufferImage = new gfxSharedImageSurface();
>+    if (!gBufferImage)
>+        return;
>+
>+    gBufferImage->CreateXShmImage(gBufferPixmap->size().width(),
>+                                  gBufferPixmap->size().height(),
>+                                  gBufferPixmap->x11Info().depth(),
>+                                  0, gBufferPixmap->x11Info().display());
>+
>+    // Current cairo does not have 16bpp image surface
>+    // we have to paint it with temp color conversion
>+    if (!gNeedColorConversion) return;
>+
>+    if (!gBufferImageTemp)
>+        gBufferImageTemp = new gfxSharedImageSurface();
>+    if (!gBufferImageTemp)
>+        return;
>+
>+    gBufferImageTemp->CreateXShmImage(gBufferPixmap->size().width(),
>+                                      gBufferPixmap->size().height(),
>+                                      24,
>+                                      0, gBufferPixmap->x11Info().display());
> }
> 
> nsWindow::~nsWindow()
>@@ -221,6 +302,21 @@ nsWindow::Destroy(void)
>     LOG(("nsWindow::Destroy [%p]\n", (void *)this));
>     mIsDestroyed = PR_TRUE;
> 
>+    if (gBufferPixmapUsageCount &&
>+        --gBufferPixmapUsageCount == 0) {
>+
>+        if (gBufferPixmap)
>+            delete gBufferPixmap;
>+        gBufferPixmap = nsnull;
>+
>+        if (!gBufferImage)
>+            delete gBufferImage;
>+        gBufferImage = nsnull;
>+        if (!gBufferImageTemp)
>+            delete gBufferImageTemp;
>+        gBufferImageTemp = nsnull;
>+    }
>+

No need to check for null before deleting.

>     nsCOMPtr<nsIWidget> rollupWidget = do_QueryReferent(gRollupWindow);
>     if (static_cast<nsIWidget *>(this) == rollupWidget.get()) {
>         if (gRollupListener)
>@@ -830,6 +926,36 @@ nsWindow::GetAttention(PRInt32 aCycleCou
>     return NS_ERROR_NOT_IMPLEMENTED;
> }
> 
>+#ifdef MOZ_X11
>+static already_AddRefed<gfxASurface>
>+GetSurfaceForQWidget(QPixmap* aDrawable)
>+{
>+    gfxASurface* result =
>+        new gfxXlibSurface(aDrawable->x11Info().display(),
>+                           aDrawable->handle(),
>+                           (Visual*)aDrawable->x11Info().visual(),
>+                           gfxIntSize(aDrawable->size().width(), aDrawable->size().height()));
>+    NS_IF_ADDREF(result);
>+    return result;
>+}
>+#endif
>+
>+static already_AddRefed<gfxASurface>
>+GetSurfaceForSharedImage(gfxSharedImageSurface* aImage)
>+{
>+    gfxASurface::gfxImageFormat imageFormat = gfxASurface::ImageFormatRGB24;
>+
>+    // Adjust Image Format
>+    if (aImage->depth() == 32) imageFormat = gfxASurface::ImageFormatARGB32;
>+
>+    gfxASurface* result =
>+        new gfxImageSurface((unsigned char *)aImage->data(),
>+                            gfxIntSize(aImage->width(), aImage->height()),
>+                            aImage->bpl(), imageFormat);

Maybe aImage->data() should return the type we expect here?

>+    NS_IF_ADDREF(result);
>+    return result;
>+}
>+
> nsEventStatus
> nsWindow::DoPaint(QPainter* aPainter, const QStyleOptionGraphicsItem* aOption)
> {
>@@ -857,7 +983,25 @@ nsWindow::DoPaint(QPainter* aPainter, co
> 
>     updateRegion->SetTo( (int)r.x(), (int)r.y(), (int)r.width(), (int)r.height() );
> 
>-    nsRefPtr<gfxASurface> targetSurface = new gfxQPainterSurface(aPainter);
>+    // Make sure we won't create something that will overload the X server
>+    if (gQWidgetRenderingSurfaceType)
>+        UpdateOffScreenBuffers(r.size().toSize(), QX11Info().depth());

I don't understand how that comment applies to the surface type test. Further we should
compare against the actual type instead of 0

>+
>+    nsRefPtr<gfxASurface> targetSurface = nsnull;
>+    if (gQWidgetRenderingSurfaceType == 1) {
>+        targetSurface = GetSurfaceForQWidget(gBufferPixmap);
>+    } else if (gQWidgetRenderingSurfaceType == 2) {
>+        targetSurface = GetSurfaceForSharedImage(gNeedColorConversion ? gBufferImageTemp : gBufferImage);
>+    } else
>+#ifdef MOZ_TREE_CAIRO
>+        targetSurface = new gfxQPainterSurface(aPainter);
>+#else
>+      return nsEventStatus_eIgnore;
>+#endif
>+

There are a lot of MOZ_TREE_CAIRO's added. It would be nice if there we're fewer.

Comment 7

[Oleg Romashin (:romaxa)]

Attachment: Updated with jeff comments

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 426382 [details] [diff] [review]
Updated with jeff comments

Overall, there are still a lot of #ifdefs. If we can eliminate some of these without much additional duplication that would be nice. This kind of complexity is also present with the rendermode variable.
Though I don't know how easy it is to split it out or make the code easier to follow.

>diff -r 9cd6e595e47e gfx/thebes/public/gfxQtPlatform.h
>--- a/gfx/thebes/public/gfxQtPlatform.h	Thu Feb 11 00:03:51 2010 +0200
>+++ b/gfx/thebes/public/gfxQtPlatform.h	Thu Feb 11 01:53:07 2010 +0200
>@@ -38,24 +38,40 @@
> 
> #ifndef GFX_PLATFORM_QT_H
> #define GFX_PLATFORM_QT_H
> 
> #include "gfxPlatform.h"
> #include "nsDataHashtable.h"
> #include "nsTArray.h"
> 
>+#ifdef MOZ_TREE_CAIRO
>+#define HAS_QPAINTER_SURFACE 1
>+#endif
>+
> typedef struct FT_LibraryRec_ *FT_Library;
> 
> class gfxFontconfigUtils;
> class FontFamily;
> class FontEntry;
> 
> class THEBES_API gfxQtPlatform : public gfxPlatform {
> public:
>+
>+    enum RenderMode {
>+        /* Use QPainter surfaces */
>+        RENDER_QPAINTER = 0,
>+        /* Use Xlib surface to render and wrap as QPixmap */
>+        RENDER_XLIB,
>+        /* Use 24bpp image surfaces, XShmPutImage to QPixmap */
>+        RENDER_SHARED_IMAGE,
>+        /* max */
>+        RENDER_MODE_MAX
>+    };
>+
>     gfxQtPlatform();
>     virtual ~gfxQtPlatform();
> 
>     static gfxQtPlatform *GetPlatform() {
>         return (gfxQtPlatform*) gfxPlatform::GetPlatform();
>     }
> 
>     already_AddRefed<gfxASurface> CreateOffscreenSurface(const gfxIntSize& size,
>@@ -82,21 +98,25 @@ public:
>     already_AddRefed<gfxFont> FindFontForChar(PRUint32 aCh, gfxFont *aFont);
>     PRBool GetPrefFontEntries(const nsCString& aLangGroup, nsTArray<nsRefPtr<gfxFontEntry> > *aFontEntryList);
>     void SetPrefFontEntries(const nsCString& aLangGroup, nsTArray<nsRefPtr<gfxFontEntry> >& aFontEntryList);
> 
>     void ClearPrefFonts() { mPrefFonts.Clear(); }
> 
>     FT_Library GetFTLibrary();
> 
>+    RenderMode GetRenderMode() { return mRenderMode; }
>+    void SetRenderMode(RenderMode rmode) { mRenderMode = rmode; }
>+
> protected:
>     static gfxFontconfigUtils *sFontconfigUtils;
> 
> private:
>     virtual qcms_profile *GetPlatformCMSOutputProfile();
> 
>     // TODO: unify this with mPrefFonts (NB: holds families, not fonts) in gfxPlatformFontList
>     nsDataHashtable<nsCStringHashKey, nsTArray<nsRefPtr<gfxFontEntry> > > mPrefFonts;
> 
>+    RenderMode mRenderMode;
> };
> 
> #endif /* GFX_PLATFORM_QT_H */
> 
>diff -r 9cd6e595e47e gfx/thebes/public/gfxSharedImageSurface.h
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/gfx/thebes/public/gfxSharedImageSurface.h	Thu Feb 11 01:53:07 2010 +0200
>@@ -0,0 +1,105 @@
[snip]
>+#ifndef GFX_SHARED_IMAGESURFACE_H
>+#define GFX_SHARED_IMAGESURFACE_H
>+
>+#ifdef HAVE_XSHM

is this file worth building if we don't have xshm?

>+#include "gfxASurface.h"
>+#include "gfxImageSurface.h"
>+#include "gfxPoint.h"
>+
>+#include <stdlib.h>
>+#include <string.h>
>+
>+#ifdef MOZ_WIDGET_QT
>+#include <QX11Info>
>+#endif
>+
>+#ifdef MOZ_WIDGET_GTK2
>+#include <gtk/gtk.h>
>+#include <gdk/gdk.h>
>+#include <gdk/gdkx.h>
>+#endif
>+
>+#ifdef MOZ_X11
>+#include <X11/Xlib.h>
>+#include <X11/Xutil.h>
>+#include <X11/extensions/XShm.h>
>+#endif
>+
>+#include <sys/ipc.h>
>+#include <sys/shm.h>
>+#include <cairo.h>
>+#include <cairo-deprecated.h>

What's cairo-deprecated.h for?

>+
>+class THEBES_API gfxSharedImageSurface : public gfxASurface {
>+public:
>+  gfxSharedImageSurface();
>+  ~gfxSharedImageSurface();
>+  int shmid() { return mShmId; }
>+  int width() { return mWidth; }
>+  int height() { return mHeight; }
>+  unsigned int depth() { return mDepth; }

I didn't see any users for some of these functions (shmid(), depth()).

>+  unsigned int bpl() { return mBytesPerLine; }
>+  unsigned char* data() { return mDataAddr; }
>+  XImage *image() { return mXShmImage; }
>+  cairo_surface_t* cairo_img_surface();
>+  already_AddRefed<gfxASurface> getASurface(void);
>+  bool CreateXShmImage(int aWidth, int aHeight, int aDepth = -1, int aShmId = 0, Display *display = NULL);

All of the callers seem to have aShmId as 0, what do you anticipate it being used for?

>+  void DestroyXShmImage();
>+
>+private:
>+  bool CreateShm(int aShmid = 0);
>+  void DestroyShm();
>+
>+  XShmSegmentInfo  mShmInfo;
>+  XImage          *mXShmImage;
>+  int              mWidth;
>+  int              mHeight;
>+  unsigned int     mDepth;
>+  unsigned int     mBytesPerLine;
>+  unsigned char   *mDataAddr;
>+  int              mShmId;
>+  bool             mFromExternalShm;

likewise mFromExternlShm isn't really used.

>+  bool             mCreatedImage;
>+  Display *mDisp;
>+};
>+
>+#endif /* HAVE_SHM */

should be HAVE_XSHM

>+#endif /* GFX_SHARED_IMAGESURFACE_H */
>diff -r 9cd6e595e47e gfx/thebes/src/gfxQtPlatform.cpp
>--- a/gfx/thebes/src/gfxQtPlatform.cpp	Thu Feb 11 00:03:51 2010 +0200
>+++ b/gfx/thebes/src/gfxQtPlatform.cpp	Thu Feb 11 01:53:07 2010 +0200
>@@ -43,32 +43,41 @@
> 
> #include "gfxQtPlatform.h"
> 
> #include "gfxFontconfigUtils.h"
> 
> #include "cairo.h"
> 
> #include "gfxImageSurface.h"
>+#ifdef HAS_QPAINTER_SURFACE
> #include "gfxQPainterSurface.h"
>+#endif
> 
> #include "gfxFT2Fonts.h"
> 
> #include "nsUnicharUtils.h"
> 
> #include <fontconfig/fontconfig.h>
> 
> #include "nsMathUtils.h"
> #include "nsTArray.h"
>+#ifdef MOZ_X11
>+#include "gfxXlibSurface.h"
>+#endif
> 
> #include "qcms.h"
> 
> #include <ft2build.h>
> #include FT_FREETYPE_H
> 
>+#include "nsIPrefBranch.h"
>+#include "nsIPrefService.h"
>+#define DEFAULT_RENDER_MODE 2

It would be nice if we could specify the default render mode by name instead of number.

>+
> gfxFontconfigUtils *gfxQtPlatform::sFontconfigUtils = nsnull;
> static cairo_user_data_key_t cairo_qt_pixmap_key;
> static void do_qt_pixmap_unref (void *data)
> {
>     QPixmap *pmap = (QPixmap*)data;
>     delete pmap;
> }
> 
>@@ -94,16 +103,46 @@ gfxQtPlatform::gfxQtPlatform()
>     gPlatformFonts = new FontTable();
>     gPlatformFonts->Init(100);
>     gPlatformFontAliases = new FontTable();
>     gPlatformFontAliases->Init(100);
>     gPrefFonts = new PrefFontTable();
>     gPrefFonts->Init(100);
>     gCodepointsWithNoFonts = new gfxSparseBitSet();
>     UpdateFontList();
>+
>+    nsresult rv;
>+    PRInt32 ival;
>+    // 0 - default gfxQPainterSurface
>+    // 1 - gfxXlibSurface
>+    // 2 - gfxImageSurface
>+    nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+    if (prefs) {
>+      rv = prefs->GetIntPref("mozilla.widget-qt.render-mode", &ival);
>+      if (NS_FAILED(rv))
>+          ival = DEFAULT_RENDER_MODE;
>+    }
>+
>+    const char *envTypeOverride = getenv("MOZ_QT_RENDER_TYPE");
>+    if (envTypeOverride)
>+        ival = atoi(envTypeOverride);

Do we need a pref and an environment variable?
What kind of performance difference do the different modes have?

>+
>+    switch (ival) {
>+        case 0:
>+            mRenderMode = RENDER_QPAINTER;
>+            break;
>+        case 1:
>+            mRenderMode = RENDER_XLIB;
>+            break;
>+        case 2:
>+            mRenderMode = RENDER_SHARED_IMAGE;
>+            break;
>+        default:
>+            mRenderMode = RENDER_QPAINTER;
>+    }
> }
> 
>diff -r 9cd6e595e47e gfx/thebes/src/gfxSharedImageSurface.cpp
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/gfx/thebes/src/gfxSharedImageSurface.cpp	Thu Feb 11 01:53:07 2010 +0200
>@@ -0,0 +1,225 @@
[snip]

>+#include "gfxSharedImageSurface.h"
>+
>+#ifdef HAVE_XSHM
>+
>+gfxSharedImageSurface::gfxSharedImageSurface()
>+ : mWidth(0),
>+   mHeight(0),
>+   mDepth(0),
>+   mBytesPerLine(0),
>+   mDataAddr(NULL),
>+   mShmId(-1),
>+   mFromExternalShm(false),
>+   mCreatedImage(false),
>+   mXShmImage(NULL)
>+{
>+    mShmInfo.shmaddr = NULL;
>+    mShmInfo.shmid = -1;
>+    mShmInfo.readOnly = false;
>+    mShmInfo.shmseg = 0;
>+}
>+
>+gfxSharedImageSurface::~gfxSharedImageSurface()
>+{
>+    if (mCreatedImage)
>+        DestroyXShmImage();

I'm not sure I understand the motivation for having mCreatedImage be switchable instead having
this class just have a constructor, an initialization function that can fail and a destructor. It seems
like that would simplify the code. Also this class seems to have a lot of state which makes following
the code flow a bit trickier and harder to ensure that all the state stays consistent.

>+}
>+
>+
>+cairo_surface_t*
>+gfxSharedImageSurface::cairo_img_surface(void)
>+{
>+    NS_ENSURE_TRUE(mXShmImage, NULL);
>+    cairo_format_t format = CAIRO_FORMAT_RGB24;
>+    if (mDepth == 32)
>+        format = CAIRO_FORMAT_ARGB32;
>+
>+    cairo_surface_t *result =
>+        cairo_image_surface_create_for_data((unsigned char*)mXShmImage->data,
>+                                            format,
>+                                            mWidth,
>+                                            mHeight,
>+                                            mXShmImage->bytes_per_line);
>+    return result;
>+}
>+

What is this method for?

>+already_AddRefed<gfxASurface>
>+gfxSharedImageSurface::getASurface(void)
>+{
>+    NS_ENSURE_TRUE(mXShmImage, NULL);
>+
>+    gfxASurface::gfxImageFormat imageFormat = gfxASurface::ImageFormatRGB24;
>+    if (mDepth == 32)
>+        imageFormat = gfxASurface::ImageFormatARGB32;
>+
>+    gfxASurface* result =
>+        new gfxImageSurface(data(),
>+                            gfxIntSize(width(), height()),
>+                            bpl(), imageFormat);
>+    NS_IF_ADDREF(result);
>+    return result;
>+}
>+
>+static nsresult
>+getSystemDepthAndDisplay(PRInt32 &aDepth, Display **aOutDisplay)
>+{
>+    NS_ENSURE_ARG_POINTER(aOutDisplay);
>+    nsresult rv(NS_OK);
>+#ifdef MOZ_WIDGET_QT
>+    aDepth = QX11Info::appDepth();
>+    *aOutDisplay = QX11Info::display();
>+#elif defined(MOZ_WIDGET_GTK2)
>+    aDepth = gdk_visual_get_system()->depth;
>+    *aOutDisplay = GDK_DISPLAY();
>+#else
>+    rv = NS_ERROR_NOT_IMPLEMENTED;
>+#endif
>+    return rv;
>+}

I don't see a need for adding the NS_ERROR_NOT_IMPLEMENTED. Is there a situation where we want to
build this file without having support for getting the depth and display. That will let us get rid
of all the error handling code.

There's also no need to have these in the same function. Here's how I would do it:

#ifdef MOZ_WIDGET_QT
static unsigned int
getSystemDepth()
{
    return QX11Info::appDepth();
}

static Display*
getSystemDisplay()
{
    return QX11Info::display();
}
#endif
#ifdef MOZ_WIDGET_GTK2
static unsigned int
getSystemDepth()
{
    return gdk_visual_get_system()->depth;
}

static Display*
getSystemDisplay()
{
    return GDK_DISPLAY();
}
#endif


>+
>+bool
>+gfxSharedImageSurface::CreateXShmImage(int aWidth,
>+                                       int aHeight,
>+                                       int aDepth,
>+                                       int aShmId,
>+                                       Display *display)
>+{
>+    if (mCreatedImage)
>+        DestroyXShmImage();
>+
>+    mWidth  = aWidth;
>+    mHeight = aHeight;
>+
>+    int sysDepth = 0;
>+    Display *sysDisplay = nsnull;
>+    nsresult rv = getSystemDepthAndDisplay(sysDepth, &sysDisplay);
>+    NS_ENSURE_SUCCESS(rv, false);
>+
>+    mDepth = aDepth ? aDepth : sysDepth;
>+    mDisp = display ? display : sysDisplay;

It looks like all the callers of CreateXShmImage pass in depth and display. Under what circumstances
do we expect depth and display to be unset?

>+
>+    mCreatedImage = CreateShm(aShmId);
>+
>+    if (mShmId > 0)
>+      shmctl(mShmId, IPC_RMID, 0);
>+
>+    if (!mCreatedImage) {
>+        mShmInfo.shmid = mShmId = -1;
>+        if (mShmInfo.shmaddr)
>+            shmdt(mShmInfo.shmaddr);
>+        mShmInfo.shmaddr = NULL;
>+        mDataAddr = NULL;
>+        if (mXShmImage)
>+            XDestroyImage(mXShmImage);
>+        mXShmImage = NULL;
>+    }
>+
>+    XSync(mDisp, False);

The XSync should have a comment explaining why it's necessary.

>+
>+    return mCreatedImage;
>+}
>+
>+void
>+gfxSharedImageSurface::DestroyXShmImage()
>+{
>+    if (!mCreatedImage)
>+        return;
>+
>+    DestroyShm();
>+    mWidth = mHeight = mDepth = mBytesPerLine = 0;
>+    mCreatedImage = false;
>+}
>+
>+bool
>+gfxSharedImageSurface::CreateShm(int aShmid)
>+{
>+    memset(&mShmInfo, 0, sizeof(mShmInfo));
>+
>+    mXShmImage = XShmCreateImage(mDisp, NULL,
>+                                 mDepth, (mDepth == 1) ? XYBitmap : ZPixmap,
>+                                 NULL, &mShmInfo, mWidth, mHeight);

Do we really handle the mDepth == 1 case in the rest of the code?

>+    if (!mXShmImage)
>+        return false;
>+
>+    mBytesPerLine = mXShmImage->bytes_per_line;
>+
>+    if (aShmid > 0)
>+    {
>+        mShmInfo.shmid = aShmid;
>+        mFromExternalShm = true;
>+    }
>+    else
>+        mShmInfo.shmid = shmget(IPC_PRIVATE, mXShmImage->bytes_per_line * mHeight, IPC_CREAT | 0777);
>+

The bracketing style above is inconsistent with the rest of the file.

>+    mShmId = mShmInfo.shmid;
>+    if (mShmInfo.shmid < 0)
>+        return false;
>+
>+    mShmInfo.shmaddr = mXShmImage->data = (char *)shmat(mShmInfo.shmid, 0, 0);
>+    mDataAddr = (unsigned char*)mXShmImage->data;

Do we need to keep this address in three places?
>+
>+    if (!mShmInfo.shmaddr)
>+        return false;
>+
>+    mShmInfo.readOnly = False;
>+    if (!XShmAttach(mDisp, &mShmInfo))
>+        return false;
>+
>+    return true;
>+}
>+
>+void
>+gfxSharedImageSurface::DestroyShm()
>+{
>+    if (!mDataAddr)
>+        return;
>+
>+    XSync(mDisp, False);

// what's the xsync here for?

>+    XShmDetach(mDisp, &mShmInfo);
>+    if (mXShmImage)
>+      XDestroyImage(mXShmImage);
>+    mXShmImage = NULL;
>+    shmdt(mShmInfo.shmaddr);
>+    mShmInfo.shmaddr = NULL;
>+    mDataAddr = NULL;
>+    mShmInfo.shmid = mShmId = -1;
>+}
>+
>+#endif /* HAVE_XSHM */
>diff -r 9cd6e595e47e toolkit/library/Makefile.in
>--- a/toolkit/library/Makefile.in	Thu Feb 11 00:03:51 2010 +0200
>+++ b/toolkit/library/Makefile.in	Thu Feb 11 01:53:07 2010 +0200
>@@ -252,17 +252,17 @@ OS_LIBS += $(call EXPAND_LIBNAME, libGLE
> endif
> endif
> 
> ifdef MOZ_JPROF
> EXTRA_DSO_LDOPTS += -ljprof
> endif
> 
> ifdef MOZ_ENABLE_QT
>-EXTRA_DSO_LDOPTS += $(MOZ_QT_LDFLAGS)
>+EXTRA_DSO_LDOPTS += $(MOZ_QT_LDFLAGS) $(XEXT_LIBS)
> endif
> 
> include $(topsrcdir)/config/rules.mk
> 
> export:: $(RDF_UTIL_SRC_CPPSRCS) $(INTL_UNICHARUTIL_UTIL_CPPSRCS)
> 	$(INSTALL) $^ .
> 
> ifdef MOZ_ENABLE_LIBXUL
>diff -r 9cd6e595e47e widget/src/qt/nsNativeThemeQt.cpp
>--- a/widget/src/qt/nsNativeThemeQt.cpp	Thu Feb 11 00:03:51 2010 +0200
>+++ b/widget/src/qt/nsNativeThemeQt.cpp	Thu Feb 11 01:53:07 2010 +0200
>@@ -64,17 +64,23 @@
> #include "nsIServiceManager.h"
> #include "nsIEventStateManager.h"
> #include "nsIDOMHTMLInputElement.h"
> #include <malloc.h>
> 
> 
> #include "gfxASurface.h"
> #include "gfxContext.h"
>+#include "gfxQtPlatform.h"
>+#ifdef HAS_QPAINTER_SURFACE
> #include "gfxQPainterSurface.h"
>+#endif
>+#ifdef MOZ_X11
>+#include "gfxXlibSurface.h"
>+#endif
> #include "nsIRenderingContext.h"
> 
> nsNativeThemeQt::nsNativeThemeQt()
> {
>     mNoBackgroundPalette.setColor(QPalette::Window, Qt::transparent);
>     ThemeChanged();
> }
> 
>@@ -88,39 +94,101 @@ static inline QRect qRectInPixels(const 
>                                   const PRInt32 p2a)
> {
>     return QRect(NSAppUnitsToIntPixels(aRect.x, p2a),
>                  NSAppUnitsToIntPixels(aRect.y, p2a),
>                  NSAppUnitsToIntPixels(aRect.width, p2a),
>                  NSAppUnitsToIntPixels(aRect.height, p2a));
> }
> 
>+static inline QImage::Format
>+_qimage_from_gfximage_format (gfxASurface::gfxImageFormat aFormat)
>+{
>+    switch (aFormat) {
>+    case gfxASurface::ImageFormatARGB32:
>+        return QImage::Format_ARGB32_Premultiplied;
>+    case gfxASurface::ImageFormatRGB24:
>+        return QImage::Format_RGB32;
>+    case gfxASurface::ImageFormatA8:
>+        return QImage::Format_Indexed8;
>+    case gfxASurface::ImageFormatA1:
>+#ifdef WORDS_BIGENDIAN
>+        return QImage::Format_Mono;
>+#else
>+        return QImage::Format_MonoLSB;
>+#endif
>+    default:
>+        return QImage::Format_Invalid;
>+    }
>+
>+    return QImage::Format_Mono;
>+}
>+
> NS_IMETHODIMP
> nsNativeThemeQt::DrawWidgetBackground(nsIRenderingContext* aContext,
>                                       nsIFrame* aFrame,
>                                       PRUint8 aWidgetType,
>                                       const nsRect& aRect,
>                                       const nsRect& aClipRect)
> {
>     gfxContext* context = aContext->ThebesContext();
>     nsRefPtr<gfxASurface> surface = context->CurrentSurface();
> 
>+#ifdef HAS_QPAINTER_SURFACE
>+    if (surface->GetType() == gfxASurface::SurfaceTypeQPainter) {
>+        gfxQPainterSurface* qSurface = (gfxQPainterSurface*) (surface.get());
>+        QPainter *painter = qSurface->GetQPainter();
>+        NS_ASSERTION(painter, "Where'd my QPainter go?");
>+        if (!painter)
>+            return NS_ERROR_FAILURE;
>+        return DrawWidgetBackground(painter, aContext,
>+                                    aFrame, aWidgetType,
>+                                    aRect, aClipRect);
>+    } else
>+#endif
>+    if (surface->GetType() == gfxASurface::SurfaceTypeImage) {
>+        gfxImageSurface* qSurface = (gfxImageSurface*) (surface.get());
>+        QImage tempQImage(qSurface->Data(),
>+                          qSurface->Width(),
>+                          qSurface->Height(),
>+                          qSurface->Stride(),
>+                          _qimage_from_gfximage_format(qSurface->Format()));
>+        QPainter painter(&tempQImage);
>+        return DrawWidgetBackground(&painter, aContext,
>+                                    aFrame, aWidgetType,
>+                                    aRect, aClipRect);
>+    }
>+#ifdef MOZ_X11
>+    else if (surface->GetType() == gfxASurface::SurfaceTypeXlib) {
>+        gfxXlibSurface* qSurface = (gfxXlibSurface*) (surface.get());
>+        QPixmap pixmap(QPixmap::fromX11Pixmap(qSurface->XDrawable()));
>+        QPainter painter(&pixmap);
>+        return DrawWidgetBackground(&painter, aContext,
>+                                    aFrame, aWidgetType,
>+                                    aRect, aClipRect);
>+    }
>+#endif
>+
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+}
>+
>+nsresult
>+nsNativeThemeQt::DrawWidgetBackground(QPainter *qPainter,
>+                                      nsIRenderingContext* aContext,
>+                                      nsIFrame* aFrame,
>+                                      PRUint8 aWidgetType,
>+                                      const nsRect& aRect,
>+                                      const nsRect& aClipRect)
>+
>+{
>+    gfxContext* context = aContext->ThebesContext();
>+    nsRefPtr<gfxASurface> surface = context->CurrentSurface();
>+
>     context->UpdateSurfaceClip();
> 
>-    if (surface->GetType() != gfxASurface::SurfaceTypeQPainter)
>-        return NS_ERROR_NOT_IMPLEMENTED;
>-
>-    gfxQPainterSurface* qSurface = (gfxQPainterSurface*) (surface.get());
>-    QPainter* qPainter = qSurface->GetQPainter();
>-
>-    NS_ASSERTION(qPainter, "Where'd my QPainter go?");
>-
>-    if (qPainter == nsnull)
>-        return NS_OK;
>-
>     QStyle* style = qApp->style();
> 
>     qPainter->save();
> 
>     gfxPoint offs = surface->GetDeviceOffset();
>     qPainter->translate(offs.x, offs.y);
> 
>     gfxMatrix ctm = context->CurrentMatrix();
>@@ -220,17 +288,17 @@ nsNativeThemeQt::DrawWidgetBackground(ns
>         QStyleOptionSlider option;
>         InitPlainStyle(aWidgetType, aFrame, r, (QStyleOption&)option, extraFlags);
>         option.orientation = Qt::Vertical;
>         style->drawControl(QStyle::CE_ScrollBarSlider, &option, qPainter, NULL);
>         break;
>     }
>     case NS_THEME_DROPDOWN: {
>         QStyleOptionComboBox comboOpt;
>-        
>+

white space churn

>         InitComboStyle(aWidgetType, aFrame, r, comboOpt);
> 
>         style->drawComplexControl(QStyle::CC_ComboBox, &comboOpt, qPainter);
>         break;
>     }
>     case NS_THEME_DROPDOWN_BUTTON: {
>         QStyleOptionComboBox option;
> 
>@@ -241,29 +309,29 @@ nsNativeThemeQt::DrawWidgetBackground(ns
>         break;
>     }
>     case NS_THEME_DROPDOWN_TEXT:
>     case NS_THEME_DROPDOWN_TEXTFIELD:
>     case NS_THEME_TEXTFIELD:
>     case NS_THEME_TEXTFIELD_MULTILINE:
>     case NS_THEME_LISTBOX: {
>         QStyleOptionFrameV2 frameOpt;
>-        
>+

white space churn

>         if (!IsDisabled(aFrame))
>             frameOpt.state |= QStyle::State_Enabled;
> 
>         frameOpt.rect = r;
>         frameOpt.features = QStyleOptionFrameV2::Flat;
> 
>         if (aWidgetType == NS_THEME_TEXTFIELD || aWidgetType == NS_THEME_TEXTFIELD_MULTILINE) {
>             QRect contentRect = style->subElementRect(QStyle::SE_LineEditContents, &frameOpt);
>             contentRect.adjust(mFrameWidth, mFrameWidth, -mFrameWidth, -mFrameWidth);
>             qPainter->fillRect(contentRect, QBrush(Qt::white));
>         }
>-        
>+

again

>         frameOpt.palette = mNoBackgroundPalette;
>         style->drawPrimitive(QStyle::PE_FrameLineEdit, &frameOpt, qPainter, NULL);
>         break;
>     }
>     case NS_THEME_MENUPOPUP: {
>         QStyleOptionMenuItem option;
>         InitPlainStyle(aWidgetType, aFrame, r, (QStyleOption&)option, extraFlags);
>         style->drawPrimitive(QStyle::PE_FrameMenu, &option, qPainter, NULL);
>diff -r 9cd6e595e47e widget/src/qt/nsNativeThemeQt.h
>--- a/widget/src/qt/nsNativeThemeQt.h	Thu Feb 11 00:03:51 2010 +0200
>+++ b/widget/src/qt/nsNativeThemeQt.h	Thu Feb 11 01:53:07 2010 +0200
>@@ -101,16 +101,23 @@ public:
> 
>   virtual nsTransparencyMode GetWidgetTransparency(PRUint8 aWidgetType);
> 
>   nsNativeThemeQt();
>   virtual ~nsNativeThemeQt();
> 
> private:
> 
>+  inline nsresult DrawWidgetBackground(QPainter *qPainter,
>+                                       nsIRenderingContext* aContext,
>+                                       nsIFrame* aFrame,
>+                                       PRUint8 aWidgetType,
>+                                       const nsRect& aRect,
>+                                       const nsRect& aClipRect);
>+
>   inline PRInt32 GetAppUnitsPerDevPixel(nsIRenderingContext* aContext){
>     nsCOMPtr<nsIDeviceContext> dctx = nsnull;
>     aContext->GetDeviceContext(*getter_AddRefs(dctx));
>     return dctx->AppUnitsPerDevPixel();
>   }
> 
>   void InitButtonStyle(PRUint8 widgetType,
>                        nsIFrame* aFrame,
>diff -r 9cd6e595e47e widget/src/qt/nsWindow.cpp
>--- a/widget/src/qt/nsWindow.cpp	Thu Feb 11 00:03:51 2010 +0200
>+++ b/widget/src/qt/nsWindow.cpp	Thu Feb 11 01:53:07 2010 +0200
>@@ -82,19 +82,33 @@
> #include <QtGui/QStyleOptionGraphicsItem>
> 
> #include <QtCore/QDebug>
> #include <QtCore/QEvent>
> #include <QtCore/QVariant>
> 
> #include "gfxQtPlatform.h"
> #include "gfxXlibSurface.h"
>+#ifdef HAS_QPAINTER_SURFACE
> #include "gfxQPainterSurface.h"
>+#endif
> #include "gfxContext.h"
>+#include "gfxSharedImageSurface.h"
> 
>+// Buffered Pixmap stuff
>+static QPixmap *gBufferPixmap = nsnull;
>+static int gBufferPixmapUsageCount = 0;
>+
>+// Buffered shared image + pixmap
>+static gfxSharedImageSurface *gBufferImage = nsnull;
>+static gfxSharedImageSurface *gBufferImageTemp = nsnull;
>+PRBool gNeedColorConversion = PR_FALSE;
>+extern "C" {
>+#include "pixman.h"
>+}
> 
> /* For PrepareNativeWidget */
> static NS_DEFINE_IID(kDeviceContextCID, NS_DEVICE_CONTEXT_CID);
> 
> // initialization static functions 
> static nsresult    initialize_prefs        (void);
> 
> static NS_DEFINE_IID(kCDragServiceCID,  NS_DRAGSERVICE_CID);
>@@ -170,16 +184,79 @@ nsWindow::nsWindow()
>         initialize_prefs();
>     }
> 
>     memset(mKeyDownFlags, 0, sizeof(mKeyDownFlags));
> 
>     mIsTransparent = PR_FALSE;
> 
>     mCursor = eCursor_standard;
>+
>+    gBufferPixmapUsageCount++;
>+}
>+
>+static inline gfxASurface::gfxImageFormat
>+_depth_to_gfximage_format(PRInt32 aDepth)
>+{
>+    switch (aDepth) {
>+    case 32:
>+        return gfxASurface::ImageFormatARGB32;
>+    case 24:
>+        return gfxASurface::ImageFormatRGB24;
>+    default:
>+        return gfxASurface::ImageFormatUnknown;
>+    }
>+}
>+
>+void
>+nsWindow::UpdateOffScreenBuffers(QSize aSize, int aDepth)
>+{
>+    gfxIntSize size(aSize.width(), aSize.height());
>+    if (gBufferPixmap) {
>+        if (gBufferPixmap->size().width() < size.width ||
>+            gBufferPixmap->size().height() < size.height) {
>+            delete gBufferPixmap;
>+            gBufferPixmap = nsnull;
>+        }
>+    }
>+    if (gBufferPixmap)
>+        return;
>+
>+    gBufferPixmap = new QPixmap(size.width, size.height);
>+    if (!gBufferPixmap)
>+        return;
>+
>+    if (gfxQtPlatform::GetPlatform()->GetRenderMode() == gfxQtPlatform::RENDER_XLIB)
>+        return;
>+
>+    gNeedColorConversion = _depth_to_gfximage_format(gBufferPixmap->x11Info().depth()) == gfxASurface::ImageFormatUnknown;
>+
>+    if (!gBufferImage)
>+        gBufferImage = new gfxSharedImageSurface();
>+    if (!gBufferImage)
>+        return;
>+
>+    gBufferImage->CreateXShmImage(gBufferPixmap->size().width(),
>+                                  gBufferPixmap->size().height(),
>+                                  gBufferPixmap->x11Info().depth(),
>+                                  0, gBufferPixmap->x11Info().display());
>+
>+    // Current cairo does not have 16bpp image surface
>+    // we have to paint it with temp color conversion
>+    if (!gNeedColorConversion) return;
>+
>+    if (!gBufferImageTemp)
>+        gBufferImageTemp = new gfxSharedImageSurface();
>+    if (!gBufferImageTemp)
>+        return;
>+
>+    gBufferImageTemp->CreateXShmImage(gBufferPixmap->size().width(),
>+                                      gBufferPixmap->size().height(),
>+                                      24,
>+                                      0, gBufferPixmap->x11Info().display());

It's not clear to me how the failure of CreateXShmImage is handled...

> }
> 
> nsWindow::~nsWindow()
> {
>     LOG(("%s [%p]\n", __PRETTY_FUNCTION__, (void *)this));
> 
>     Destroy();
> }
>@@ -216,16 +293,27 @@ NS_IMETHODIMP
> nsWindow::Destroy(void)
> {
>     if (mIsDestroyed || !mWidget)
>         return NS_OK;
> 
>     LOG(("nsWindow::Destroy [%p]\n", (void *)this));
>     mIsDestroyed = PR_TRUE;
> 
>+    if (gBufferPixmapUsageCount &&
>+        --gBufferPixmapUsageCount == 0) {

I found this condition confusing the first time I read it. It seemed contradictory. On a second
look it made more sense to me. I guess these needs to have 3 states (>1, 1 and 0)?

>+
>+        delete gBufferPixmap;
>+        delete gBufferImage;
>+        delete gBufferImageTemp;
>+        gBufferPixmap = nsnull;
>+        gBufferImage = nsnull;
>+        gBufferImageTemp = nsnull;
>+    }
>+
>     nsCOMPtr<nsIWidget> rollupWidget = do_QueryReferent(gRollupWindow);
>     if (static_cast<nsIWidget *>(this) == rollupWidget.get()) {
>         if (gRollupListener)
>             gRollupListener->Rollup(nsnull, nsnull);
>         gRollupWindow = nsnull;
>         gRollupListener = nsnull;
>         NS_IF_RELEASE(gMenuRollup);
>     }
>@@ -825,16 +913,30 @@ is_mouse_in_window (MozQWidget* aWindow,
> 
> NS_IMETHODIMP
> nsWindow::GetAttention(PRInt32 aCycleCount)
> {
>     LOG(("nsWindow::GetAttention [%p]\n", (void *)this));
>     return NS_ERROR_NOT_IMPLEMENTED;
> }
> 
>+#ifdef MOZ_X11
>+static already_AddRefed<gfxASurface>
>+GetSurfaceForQWidget(QPixmap* aDrawable)
>+{
>+    gfxASurface* result =
>+        new gfxXlibSurface(aDrawable->x11Info().display(),
>+                           aDrawable->handle(),
>+                           (Visual*)aDrawable->x11Info().visual(),
>+                           gfxIntSize(aDrawable->size().width(), aDrawable->size().height()));
>+    NS_IF_ADDREF(result);
>+    return result;
>+}
>+#endif
>+
> nsEventStatus
> nsWindow::DoPaint(QPainter* aPainter, const QStyleOptionGraphicsItem* aOption)
> {
>     if (mIsDestroyed) {
>         LOG(("Expose event on destroyed window [%p] window %p\n",
>              (void *)this, mWidget));
>         return nsEventStatus_eIgnore;
>     }
>@@ -852,17 +954,40 @@ nsWindow::DoPaint(QPainter* aPainter, co
> 
>     if (aOption)
>         r = aOption->exposedRect;
>     else
>         r = mWidget->boundingRect();
> 
>     updateRegion->SetTo( (int)r.x(), (int)r.y(), (int)r.width(), (int)r.height() );
> 
>-    nsRefPtr<gfxASurface> targetSurface = new gfxQPainterSurface(aPainter);
>+    // Make sure we won't create something that will overload the X server

I still don't understand the comment above.

>+    gfxQtPlatform::RenderMode renderMode = gfxQtPlatform::GetPlatform()->GetRenderMode();
>+    // Prepare offscreen buffers if RenderMode Xlib or Image
>+    if (renderMode != gfxQtPlatform::RENDER_QPAINTER)
>+        UpdateOffScreenBuffers(r.size().toSize(), QX11Info().depth());
>+
>@@ -1799,18 +1964,35 @@ nsWindow::createQWidget(MozQWidget *pare
> 
> // return the gfxASurface for rendering to this widget
> gfxASurface*
> nsWindow::GetThebesSurface()
> {
>     /* This is really a dummy surface; this is only used when doing reflow, because
>      * we need a RenderingContext to measure text against.
>      */
>-    if (!mThebesSurface)
>+    if (mThebesSurface)
>+        return mThebesSurface;
>+
>+    gfxQtPlatform::RenderMode renderMode = gfxQtPlatform::GetPlatform()->GetRenderMode();
>+    if (renderMode == gfxQtPlatform::RENDER_QPAINTER) {
>+#ifdef HAS_QPAINTER_SURFACE
>         mThebesSurface = new gfxQPainterSurface(gfxIntSize(5,5), gfxASurface::CONTENT_COLOR);
>+#else
>+        NS_WARNING(mThebesSurface, "gfxQPainterSurface is not available");
>+#endif
>+    } else if (renderMode == gfxQtPlatform::RENDER_XLIB) {
>+        mThebesSurface = new gfxXlibSurface(QX11Info().display(),
>+                                            (Visual*)QX11Info().visual(),
>+                                            gfxIntSize(5,5), QX11Info().depth());
>+    }
>+    if (!mThebesSurface) {
>+        gfxASurface::gfxImageFormat imageFormat = gfxASurface::ImageFormatRGB24;
>+        mThebesSurface = new gfxImageSurface(gfxIntSize(5,5), imageFormat);
>+    }

Why does this method choose a surface of size 5x5? is 1x1 no good?

Comment 9

[Oleg Romashin (:romaxa)]

> >+#ifdef HAVE_XSHM
> 
> is this file worth building if we don't have xshm?

configure.in is only providing HAVE_XSHM define, also I'm not sure that if SHM available on other systems... that why I'm using just existing XSHM define

> >+#include <cairo.h>
> >+#include <cairo-deprecated.h>
> 
> What's cairo-deprecated.h for?

That was used to detect availability on 16bpp  image surface support in cairo

> >+  unsigned int depth() { return mDepth; }
> 
> I didn't see any users for some of these functions (shmid(), depth()).

yep, I think we don't need it here.

> >+  bool CreateXShmImage(int aWidth, int aHeight, int aDepth = -1, int aShmId = 0, Display *display = NULL);
> 
> All of the callers seem to have aShmId as 0, what do you anticipate it being
> used for?

Idea here is to create SHM image with internal memory allocation by default, and if aShmId specified, then allow to create SHMImage wrapper which is using outside created Shared memory
> 
> likewise mFromExternlShm isn't really used.

See comment about aShmId

> >+#define DEFAULT_RENDER_MODE 2
> 
> It would be nice if we could specify the default render mode by name instead of
> number.

Oh, sure

> >+    const char *envTypeOverride = getenv("MOZ_QT_RENDER_TYPE");
> >+    if (envTypeOverride)
> >+        ival = atoi(envTypeOverride);
> 
> Do we need a pref and an environment variable?

We need this env until QPainter backend not very stable...
Also it will help check some rendering and performance problems without recompilation

> What kind of performance difference do the different modes have?

Image surface as fast as pixman neon optimized
XSurface - is possible solution but not for all platforms, but it tested for a long time and can be used as good reference to find problems in QPainter surface.
QPainter - is something that should be used by default, be it cannot, because still problems with fonts API (we are trying to push fonts API into QPainter API now),
    and some other performance problems (GL backends e.t.c.)


> >+
> >+gfxSharedImageSurface::~gfxSharedImageSurface()
> >+{
> >+    if (mCreatedImage)
> >+        DestroyXShmImage();
> 

> I'm not sure I understand the motivation for having mCreatedImage be switchable
> instead having
> this class just have a constructor, an initialization function that can fail
> and a destructor. It seems
> like that would simplify the code. Also this class seems to have a lot of state
> which makes following
> the code flow a bit trickier and harder to ensure that all the state stays
> consistent.

> >+cairo_surface_t*
> >+gfxSharedImageSurface::cairo_img_surface(void)
> What is this method for?

Original class came from another place, and I did not change it a lot,
Seems I need to make it more close to mozilla style now... to pass this review

> > 
> >+    if (gBufferPixmapUsageCount &&
> >+        --gBufferPixmapUsageCount == 0) {

> I found this condition confusing the first time I read it. It seemed
> contradictory. On a second
> look it made more sense to me. I guess these needs to have 3 states (>1, 1 and
> 0)?

This came from similar patch on GTK code
http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsWindow.cpp#745

> 
> Why does this method choose a surface of size 5x5? is 1x1 no good?

No idea, I think it was Copy-Pasted from some other place or widget backend.

Comment 10

[Oleg Romashin (:romaxa)]

>+void
>+gfxSharedImageSurface::DestroyShm()
>+{
>+    if (!mDataAddr)
>+        return;
>+
>+    XSync(mDisp, False);

// what's the xsync here for?

To finish all pending operations on X-Server, and avoid access to destroyed data...

Comment 11

[Oleg Romashin (:romaxa)]

Attachment: Fixed almost all comments

Comment 12

[Oleg Romashin (:romaxa)]

Attachment: More cleanup, checks, tested with different system bpp values

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 426686 [details] [diff] [review]
More cleanup, checks, tested with different system bpp values

>+bool
>+gfxSharedImageSurface::CreateInternal(int aShmid)
>+{
>+    memset(&mShmInfo, 0, sizeof(mShmInfo));
>+
>+    mStride = ComputeStride();
>+
>+    if (aShmid != -1) {
>+        mShmId = aShmid;
>+        mOwnsData = false;
>+    } else
>+        mShmId = shmget(IPC_PRIVATE, GetDataSize(), IPC_CREAT | 0777);
>+
>+    if (mShmId == -1)
>+        return false;
>+
>+    char *data = (char*)shmat(mShmId, 0, 0);
>+    shmctl(mShmId, IPC_RMID, 0);
>+    if (data == (void *)-1)
>+        return false;
>+    mData = (unsigned char *)data;

There's a lot of casting going on here, plus comparing 'data' with (void *)-1 seems like it will cause a warning.

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Cast fixes, to push

Comment 15

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/73f49689630a

Comment 16

[Doug Turner (:dougt)]

missing files:
http://hg.mozilla.org/mozilla-central/rev/68aa31088a1d

Comment 17

[Doug Turner (:dougt)]

busted mac:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1266345347.1266345427.8660.gz

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: Added check for MOZ_X11.

Patch is building now on tryServer

Comment 19

[Oleg Romashin (:romaxa)]

Ok, seems Darwin boxes are green with updated patch on MozillaTry "bug544250-SHM"

Comment 20

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/092ee825f583

Comment 21

[Doug Turner (:dougt)]

this patch caused the seamonkey builds to turn run:

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1266638882.1266638574.28818.gz

Maybe the builder is misconfigured since we are wrapping all of those XSHM calls with HAVE_XSHM?

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> this patch caused the seamonkey builds to turn run:

Thunderbird too.

> Maybe the builder is misconfigured since we are wrapping all of those XSHM
> calls with HAVE_XSHM?

Fwiw, (comm-central) XShm support looked unused and was removed in
http://hg.mozilla.org/comm-central/rev/40193c79a97d

Maybe we now need to restore (part of) it?

Comment 23

[Doug Turner (:dougt)]

yes, probably restoring it will fix this build bustage.

Comment 24

[Mark Banner (:standard8)]

I've just checked in a possible bustage fix for the Thunderbird/SeaMonkey builds:

http://hg.mozilla.org/mozilla-central/rev/777fac9d915c

I very much doubt it is the fact that XShm support was removed previously - the detection should still take place in the mozilla-central configure file that we run, and set everything correctly, and the error is in the mozilla-central part of the build.

The two builds that were failing on Thunderbird were both non-libxul, shared builds. This would mean that they were creating a shared lib in gfx/thebes/src. This would therefore need the XEXT_LIBS adding to the LDOPTS as was done in the original patch in the toolkit/library/Makefile.in.

Hopefully this will fix the issue, if it doesn't I'll back out again.

Comment 25

[Mark Banner (:standard8)]

(In reply to comment #24)
> Hopefully this will fix the issue, if it doesn't I'll back out again.

The Thunderbird builds are now green again, and one of the SeaMonkey builds is also green. The other two are still cycling, but I think there should be no issues there.