Bug 33514 – [Qt] Implement GraphicsLayer for accelerated layer compositing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Noam Rosenthal]

Webkit contains an abstraction that allows platform-specific acceleration of CSS opacity and transform animations. This abstraction, however, is not implemented in QtWebkit.

I've created a WIP repo. on Gitorious where this is implemented nearly to the full extent using QGraphicsItems - it needs a lot of testing but already works pretty well with the LayoutTests from the repo.

Tracking on the QtWebkit side:
see http://bugreports.qt.nokia.com/browse/QTWEBKIT-15

WIP: 
http://gitorious.org/~noamr/webkit/noamrs-webkit/commits/accel
- [path-to-]/QGVLauncher http://webkit.org/blog-files/leaves/ --accel -graphicssystem raster

Note: This might share some commonalities with what's needed for webkit tiling, such as painting the contents and the scrollbar separately.

Right now I could use more eyes to look at the code to find architectural issues, before I go to heavy debugging mode.
Note that right now it can only work with QGraphicsWebView and not with QWebView: mainly because for implementing it with QWebView I'd need my own QGraphicsScene rendering inside QWebView, and I'm not sure performance would be that great.

[Noam Rosenthal]

Created attachment 46752 [details]
compositing layers for Qt 

Since there's no way I can think of to chop this feature into small commits, I'm posting it now as one patch. 

Composite layers for CSS animations seem to be pretty solid. Some further testing needs to be done, but so far all the LayoutTests work, and I don't foresee any big architectural changes apart from inevitable bug-fixes.

Currently the compositing-layers feature only works in QGraphicsWebView, and only if QWebSettings::AcceleratedCompositingEnabled is true.

I tried to get all the style issues done in advance but of course I might have overlooked a few things. Hopefully reviewing this won't be a hassle :)
In my view this could maybe go in and marked as an "experimental" feature and we can make it more solid as we go along - since there's no new API and it's an optional feature with QWebSettings, the risk is low. The main changes that can affect other parts are in qgraphicswebview.cpp, and those should probably be reviewed more carefully.

[Kenneth Rohde Christiansen]

Comment on attachment 46752 [details]
compositing layers for Qt 

Great stuff Noam! It is a huge patch and it will take some time to review. There are lots of style errors etc, so please look at my comments below, and then fix this so it will be easier doing a review of the actual contents of the patch.

> Index: WebKit/qt/QGVLauncher/main.cpp
> ===================================================================
> --- WebKit/qt/QGVLauncher/main.cpp	(revision 53357)
> +++ WebKit/qt/QGVLauncher/main.cpp	(working copy)
> @@ -129,6 +129,7 @@
>  
>          setFrameShape(QFrame::NoFrame);
>          setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Expanding);
> +        setViewportUpdateMode(BoundingRectViewportUpdate);

Would be nice to make this passable as an argument for testing purposes

>      }
>  
>      void setMainWidget(QGraphicsWidget* widget)
> @@ -455,17 +456,24 @@
>      QWebSettings::setMaximumPagesInCache(4);
>      QWebSettings::globalSettings()->setAttribute(QWebSettings::PluginsEnabled, true);
>      QWebSettings::globalSettings()->setAttribute(QWebSettings::DeveloperExtrasEnabled, true);
> +    QWebSettings::globalSettings()->setAttribute(QWebSettings::LocalContentCanAccessRemoteUrls, true);
>      QWebSettings::enablePersistentStorage();
>  
>      const QStringList args = app.arguments();
> -    if (args.count() > 1)
> +    const int idxOfUrl = args.indexOf("--url");

No need to abbreviate index to idx

> +    if (idxOfUrl > 0 && idxOfUrl < args.count()-1)

spacing: add spaces between () and - and 1

> +        url = args.at(idxOfUrl+1);

Same thing

> +    else if (args.count() > 1)
>          url = args.at(1);
> +    if (args.contains("--accel"))

Better write accel out, I guess

> +        QWebSettings::globalSettings()->setAttribute(QWebSettings::AcceleratedCompositingEnabled, true);
>  
>      MainWindow* window = new MainWindow;
>      window->load(url);
>  
>      for (int i = 2; i < args.count(); i++)
> -        window->newWindow(args.at(i));
> +        if (!args.at(i).startsWith("-") && !args.at(i-1).startsWith("-"))

Spacing. Doesn't Qt have anything to deal with command line options? 

> +            window->newWindow(args.at(i));
>  
>      window->show();
>      return app.exec();
> Index: WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
> ===================================================================
> --- WebKit/qt/WebCoreSupport/ChromeClientQt.cpp	(revision 53357)
> +++ WebKit/qt/WebCoreSupport/ChromeClientQt.cpp	(working copy)
> @@ -42,6 +42,10 @@
>  #include "QWebPageClient.h"
>  #include "SecurityOrigin.h"
>  
> +#include <qdebug.h>
> +#include <qtextdocument.h>
> +#include <qtooltip.h>
> +
>  #include "qwebpage.h"
>  #include "qwebpage_p.h"
>  #include "qwebframe_p.h"
> @@ -49,9 +53,10 @@
>  #include "qwebsecurityorigin_p.h"
>  #include "qwebview.h"
>  
> -#include <qtooltip.h>
> -#include <qtextdocument.h>
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +#include "GraphicsLayerQt.h"
> +#endif
>  namespace WebCore

Pls run check-webkit-style to make sure your reordering is correct.

>  {
>  
> @@ -465,6 +470,30 @@
>      // See the comment in WebCore/page/ChromeClient.h
>      notImplemented();
>  }
> +#if USE(ACCELERATED_COMPOSITING)

Please add an empty line before that

> +// Pass 0 as the GraphicsLayer to detatch the root layer.

Detach 

> +void ChromeClientQt::attachRootGraphicsLayer(Frame* frame, GraphicsLayer* graphicsLayer)
> +{
> +    if (platformPageClient())
> +        platformPageClient()->setRootGraphicsLayer(graphicsLayer?graphicsLayer->nativeLayer():0);

Spacing again, you should really run that check-webkit-style script.

> +}

There should be an empty line after this

> +// Sets a flag to specify that the next time content is drawn to the window,
> +// the changes appear on the screen in synchrony with updates to GraphicsLayers.

We normally add this documentation (in WebCore at least) to the header file. Maybe it would be good to duplicate it there.

> +void ChromeClientQt::setNeedsOneShotDrawingSynchronization()
> +{
> +    if (platformPageClient()) {
> +        platformPageClient()->markForSync(false);
> +        platformPageClient()->update(QRect(QPoint(0, 0), m_webPage->viewportSize()));
> +    }
> +}
> +// Sets a flag to specify that the view needs to be updated, so we need
> +// to do an eager layout before the drawing.
> +void ChromeClientQt::scheduleCompositingLayerSync()
> +{
> +    if (platformPageClient())
> +        platformPageClient()->markForSync(true);
> +}
> +#endif
>  
>  QtAbstractWebPopup* ChromeClientQt::createPopup()
>  {
> Index: WebKit/qt/WebCoreSupport/ChromeClientQt.h
> ===================================================================
> --- WebKit/qt/WebCoreSupport/ChromeClientQt.h	(revision 53357)
> +++ WebKit/qt/WebCoreSupport/ChromeClientQt.h	(working copy)
> @@ -145,6 +145,11 @@
>          bool toolBarsVisible;
>          bool statusBarVisible;
>          bool menuBarVisible;
> +#if USE(ACCELERATED_COMPOSITING)
> +        virtual void attachRootGraphicsLayer(Frame*, GraphicsLayer*);
> +        virtual void setNeedsOneShotDrawingSynchronization();
> +        virtual void scheduleCompositingLayerSync();
> +#endif

Are these the same as used by other ports? I'm not sure you have added these the most logical place in the header file

>      };
>  }
>  
> Index: WebKit/qt/Api/qgraphicswebview.cpp
> ===================================================================
> --- WebKit/qt/Api/qgraphicswebview.cpp	(revision 53357)
> +++ WebKit/qt/Api/qgraphicswebview.cpp	(working copy)
> @@ -22,24 +22,64 @@
>  #include "qgraphicswebview.h"
>  
>  #include "qwebframe.h"
> +#include "qwebframe_p.h"
>  #include "qwebpage.h"
>  #include "qwebpage_p.h"
>  #include "QWebPageClient.h"
> -#include <QtGui/QGraphicsScene>
> -#include <QtGui/QGraphicsView>
> +#include <FrameView.h>
> +#include <QtCore/qsharedpointer.h>
> +#include <QtCore/qtimer.h>
>  #include <QtGui/qapplication.h>
> +#include <QtGui/qgraphicsscene.h>
>  #include <QtGui/qgraphicssceneevent.h>
> +#include <QtGui/qgraphicsview.h>
> +#include <QtGui/qpixmapcache.h>
>  #include <QtGui/qstyleoption.h>
>  #if defined(Q_WS_X11)
>  #include <QX11Info>
>  #endif
> +#include <Settings.h>
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +class QGraphicsWebViewOverlay : public QGraphicsItem {
> +    friend class QGraphicsWebView;
> +    QGraphicsWebView* q;

These should go at the bottom of the class definition

> +
> +    public:

We normally wouldn't have spaces before the public here.

> +    QGraphicsWebViewOverlay(QGraphicsWebView* wv)
> +            :QGraphicsItem(wv),

, should be on next line, spacing issue as well

> +            q(wv)

I think view is more explanatory than wv.

> +    {
> +    }
> +
> +    QRectF boundingRect() const
> +    {
> +        return q->boundingRect();
> +    }
> +
> +    void paint(QPainter* painter, const QStyleOptionGraphicsItem* opt, QWidget*)

write opt out (options)

> +    {
> +        q->page()->mainFrame()->render(painter, (QWebFrame::RenderLayer)(QWebFrame::AllLayers&(~QWebFrame::ContentsLayer)), opt->exposedRect.toRect());

Remember (you probably did) that opt->exposedRect is the bounding rect unless you set some graphics view flag.

No C-casts please

> +    }
> +};

Missing new line after this

> +#endif
>  class QGraphicsWebViewPrivate : public QWebPageClient {
>  public:
>      QGraphicsWebViewPrivate(QGraphicsWebView* parent)
>          : q(parent)
>          , page(0)
> -    {}
> +#if USE(ACCELERATED_COMPOSITING)
> +        , rootGraphicsLayer(0),
> +        overlay(new QGraphicsWebViewOverlay(parent)),

Where are you freeing this?

> +        markedForSync(true)
> +#endif
> +    {
> +#if USE(ACCELERATED_COMPOSITING)
> +        overlay->setZValue(2);
> +        overlay->setPos(0, 0);
> +        q->setCacheMode(QGraphicsItem::ItemCoordinateCache);
> +#endif
> +    }
>  
>      virtual ~QGraphicsWebViewPrivate();
>      virtual void scroll(int dx, int dy, const QRect&);
> @@ -60,17 +100,47 @@
>      virtual QWidget* ownerWidget() const;
>  
>      virtual QObject* pluginParent() const;
> -
> +#if USE(ACCELERATED_COMPOSITING)
> +    virtual void setRootGraphicsLayer(QGraphicsItem* layer);
> +    virtual void markForSync(bool now);
> +#endif
>      void _q_doLoadFinished(bool success);
>  
>      QGraphicsWebView* q;
>      QWebPage* page;
> +#if USE(ACCELERATED_COMPOSITING)
> +    QGraphicsItem* rootGraphicsLayer;
> +    QGraphicsWebViewOverlay* overlay;
> +    bool markedForSync;

shouldSync? Could you explain the cases where you need to synchronize and when not?

> +    QPoint scrollDelta;
> +#endif
>  };
>  
>  QGraphicsWebViewPrivate::~QGraphicsWebViewPrivate()
>  {
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (rootGraphicsLayer) {
> +        rootGraphicsLayer->setParentItem(0);
> +        q->scene()->removeItem(rootGraphicsLayer);
> +    }

Should you free the overlay here?

> +#endif
>  }
> -
> +#if USE(ACCELERATED_COMPOSITING)
> +void QGraphicsWebViewPrivate::setRootGraphicsLayer(QGraphicsItem* layer)
> +{
> +    if (rootGraphicsLayer) {
> +        q->scene()->removeItem(rootGraphicsLayer);
> +        QWebFramePrivate::core(q->page()->mainFrame())->view()->syncCompositingStateRecursive();
> +    }
> +    rootGraphicsLayer = layer;
> +    if (layer) {
> +        layer->setZValue(1);

Maybe define an enum for these two Zvalues 1 and 2, to make it clear what is what

> +        layer->setParentItem(q);
> +        layer->moveBy(scrollDelta.x(), scrollDelta.y());
> +        scrollDelta = QPoint();
> +    }
> +}
> +#endif
>  void QGraphicsWebViewPrivate::_q_doLoadFinished(bool success)
>  {
>      // If the page had no title, still make sure it gets the signal
> @@ -83,13 +153,32 @@
>  void QGraphicsWebViewPrivate::scroll(int dx, int dy, const QRect& rectToScroll)
>  {
>      q->scroll(qreal(dx), qreal(dy), QRectF(rectToScroll));
> +#if USE(ACCELERATED_COMPOSITING)
> +
> +    if (rootGraphicsLayer)
> +        rootGraphicsLayer->moveBy(dx, dy);
> +    else
> +        scrollDelta += QPoint(dx, dy);

Hard to see frome the contents, but where do you use this scrollDelta?

> +
> +    overlay->scroll(dx, dy, rectToScroll);
> +#endif
>  }
>  
>  void QGraphicsWebViewPrivate::update(const QRect& dirtyRect)
>  {
>      q->update(QRectF(dirtyRect));
>  }
> +#if USE(ACCELERATED_COMPOSITING)
> +void QGraphicsWebViewPrivate::markForSync(bool now)

bool now seems strange

> +{
> +    if (now && !markedForSync) {
> +        q->syncLayers();
>  
> +        QTimer::singleShot(0, q, SLOT(syncLayers()));
> +    }
> +    markedForSync = true;
> +}
> +#endif
>  
>  void QGraphicsWebViewPrivate::setInputMethodEnabled(bool enable)
>  {
> @@ -248,6 +337,7 @@
>      setAcceptTouchEvents(true);
>  #endif
>      setFocusPolicy(Qt::StrongFocus);
> +    setFlag(QGraphicsItem::ItemClipsChildrenToShape, true);
>  }
>  
>  /*!
> @@ -293,11 +383,27 @@
>      return d->page;
>  }
>  
> +void QGraphicsWebView::syncLayers()

I don't think we want this public. And if so, we would need API review and documention

> +{
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (d->markedForSync) {
> +        QWebFramePrivate::core(page()->mainFrame())->view()->syncCompositingStateRecursive();
> +        d->markedForSync = false;
> +    }
> +
> +#endif
> +}
> +
>  /*! \reimp
>  */
>  void QGraphicsWebView::paint(QPainter* painter, const QStyleOptionGraphicsItem* option, QWidget*)
>  {
> -    page()->mainFrame()->render(painter, option->exposedRect.toRect());
> +#if USE(ACCELERATED_COMPOSITING)
> +    syncLayers();
> +    page()->mainFrame()->render(painter, QWebFrame::ContentsLayer, option->exposedRect.toRect());
> +#else
> +    page()->mainFrame()->render(painter, QWebFrame::AllLayers, option->exposedRect.toRect());
> +#endif

Whoo! Someone is using my new API :-)

>  }
>  
>  /*! \reimp
> @@ -425,6 +531,9 @@
>      d->page = page;
>      if (!d->page)
>          return;
> +#if USE(ACCELERATED_COMPOSITING)
> +    d->overlay->prepareGeometryChange();
> +#endif
>      d->page->d->client = d; // set the page client
>  
>      QSize size = geometry().size().toSize();
> @@ -528,6 +637,9 @@
>  */
>  void QGraphicsWebView::updateGeometry()
>  {
> +#if USE(ACCELERATED_COMPOSITING)
> +    d->overlay->prepareGeometryChange();
> +#endif
>      QGraphicsWidget::updateGeometry();
>  
>      if (!d->page)
> @@ -542,7 +654,9 @@
>  void QGraphicsWebView::setGeometry(const QRectF& rect)
>  {
>      QGraphicsWidget::setGeometry(rect);
> -
> +#if USE(ACCELERATED_COMPOSITING)
> +    d->overlay->prepareGeometryChange();
> +#endif
>      if (!d->page)
>          return;
>  
> Index: WebKit/qt/Api/qwebsettings.h
> ===================================================================
> --- WebKit/qt/Api/qwebsettings.h	(revision 53357)
> +++ WebKit/qt/Api/qwebsettings.h	(working copy)
> @@ -68,7 +68,8 @@
>  #endif
>          LocalContentCanAccessRemoteUrls,
>          DnsPrefetchEnabled,
> -        XSSAuditorEnabled
> +        XSSAuditorEnabled,
> +        AcceleratedCompositingEnabled
>      };
>      enum WebGraphic {
>          MissingImageGraphic,
> Index: WebKit/qt/Api/qwebsettings.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebsettings.cpp	(revision 53357)
> +++ WebKit/qt/Api/qwebsettings.cpp	(working copy)
> @@ -152,7 +152,12 @@
>          value = attributes.value(QWebSettings::JavascriptEnabled,
>                                        global->attributes.value(QWebSettings::JavascriptEnabled));
>          settings->setJavaScriptEnabled(value);
> +#if USE(ACCELERATED_COMPOSITING)
> +        value = attributes.value(QWebSettings::AcceleratedCompositingEnabled,
> +                                      global->attributes.value(QWebSettings::AcceleratedCompositingEnabled));
>  
> +        settings->setAcceleratedCompositingEnabled(value);
> +#endif
>          value = attributes.value(QWebSettings::JavascriptCanOpenWindows,
>                                        global->attributes.value(QWebSettings::JavascriptCanOpenWindows));
>          settings->setJavaScriptCanOpenWindowsAutomatically(value);
> @@ -389,6 +394,7 @@
>      d->attributes.insert(QWebSettings::OfflineWebApplicationCacheEnabled, false);
>      d->attributes.insert(QWebSettings::LocalStorageEnabled, false);
>      d->attributes.insert(QWebSettings::LocalContentCanAccessRemoteUrls, false);
> +    d->attributes.insert(QWebSettings::AcceleratedCompositingEnabled, false);
>      d->offlineStorageDefaultQuota = 5 * 1024 * 1024;
>      d->defaultTextEncoding = QLatin1String("iso-8859-1");
>  }
> Index: WebKit/qt/Api/qgraphicswebview.h
> ===================================================================
> --- WebKit/qt/Api/qgraphicswebview.h	(revision 53357)
> +++ WebKit/qt/Api/qgraphicswebview.h	(working copy)
> @@ -105,7 +105,8 @@
>      void iconChanged();
>      void statusBarMessage(const QString& message);
>      void linkClicked(const QUrl&);
> -
> +protected slots:
> +    void syncLayers();
>  protected:
>      virtual void mousePressEvent(QGraphicsSceneMouseEvent*);
>      virtual void mouseDoubleClickEvent(QGraphicsSceneMouseEvent*);
> Index: WebKit/qt/Api/qwebview.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebview.cpp	(revision 53357)
> +++ WebKit/qt/Api/qwebview.cpp	(working copy)
> @@ -22,10 +22,11 @@
>  #include "config.h"
>  #include "qwebview.h"
>  
> +#include "Page.h"
>  #include "QWebPageClient.h"
> +#include "Settings.h"
>  #include "qwebframe.h"
>  #include "qwebpage_p.h"
> -
>  #include "qbitmap.h"
>  #include "qevent.h"
>  #include "qpainter.h"
> @@ -247,6 +248,9 @@
>                  this, SLOT(_q_pageDestroyed()));
>      }
>      setAttribute(Qt::WA_OpaquePaintEvent, d->page);
> +#if USE(ACCELERATED_COMPOSITING)
> +    d->page->d->page->settings()->setAcceleratedCompositingEnabled(false);
> +#endif
>      update();
>  }
>  
> Index: WebCore/plugins/qt/PluginViewQt.cpp
> ===================================================================
> --- WebCore/plugins/qt/PluginViewQt.cpp	(revision 53357)
> +++ WebCore/plugins/qt/PluginViewQt.cpp	(working copy)
> @@ -53,6 +53,7 @@
>  #include "PluginContainerQt.h"
>  #include "PluginDebug.h"
>  #include "PluginPackage.h"
> +#include "PluginWidget.h"
>  #include "PluginMainThreadScheduler.h"
>  #include "RenderLayer.h"
>  #include "ScriptController.h"
> @@ -844,4 +845,5 @@
>  {
>  }
>  
> +
>  } // namespace WebCore
> Index: WebCore/WebCore.pro
> ===================================================================
> --- WebCore/WebCore.pro	(revision 53357)
> +++ WebCore/WebCore.pro	(working copy)
> @@ -2707,6 +2707,19 @@
>              plugins/win/PaintHooks.asm
>      }
>  }
> +contains(DEFINES, WTF_USE_ACCELERATED_COMPOSITING) {
> +HEADERS += \
> +    rendering/RenderLayerBacking.h \
> +    rendering/RenderLayerCompositor.h \
> +    platform/graphics/GraphicsLayer.h \
> +    platform/graphics/GraphicsLayerClient.h \
> +    platform/graphics/qt/GraphicsLayerQt.h
> +SOURCES += \
> +    platform/graphics/GraphicsLayer.cpp \
> +    platform/graphics/qt/GraphicsLayerQt.cpp \
> +    rendering/RenderLayerBacking.cpp \
> +    rendering/RenderLayerCompositor.cpp
> +}
>  
>  symbian {
>      shared {
> Index: WebCore/platform/qt/QWebPageClient.h
> ===================================================================
> --- WebCore/platform/qt/QWebPageClient.h	(revision 53357)
> +++ WebCore/platform/qt/QWebPageClient.h	(working copy)
> @@ -30,7 +30,7 @@
>  #include <QCursor>
>  #endif
>  #include <QRect>
> -
> +class QGraphicsItem;
>  class QWebPageClient {
>  public:
>      virtual ~QWebPageClient() { }
> @@ -39,6 +39,11 @@
>      virtual void update(const QRect&) = 0;
>      virtual void setInputMethodEnabled(bool enable) = 0;
>      virtual bool inputMethodEnabled() const = 0;
> +#if USE(ACCELERATED_COMPOSITING)
> +    virtual void setRootGraphicsLayer(QGraphicsItem* layer) {}
> +    virtual void markForSync(bool now = false) {}

These API's here will need some review. We much keep QWebPageClient.h API very clear and under control.

> +#endif
> +
>  #if QT_VERSION >= 0x040600
>      virtual void setInputMethodHint(Qt::InputMethodHint hint, bool enable) = 0;
>  #endif
> Index: WebCore/platform/graphics/qt/GraphicsLayerQt.cpp
> ===================================================================
> --- WebCore/platform/graphics/qt/GraphicsLayerQt.cpp	(revision 0)
> +++ WebCore/platform/graphics/qt/GraphicsLayerQt.cpp	(revision 0)
> @@ -0,0 +1,976 @@
> +/*
> +    Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies)
> +
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public
> +    License as published by the Free Software Foundation; either
> +    version 2 of the License, or (at your option) any later version.
> +
> +    This library is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +    Library General Public License for more details.
> +
> +    You should have received a copy of the GNU Library General Public License
> +    along with this library; see the file COPYING.LIB.  If not, write to
> +    the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> +    Boston, MA 02110-1301, USA.
> +*/
> +
> +#include "config.h"
> +#include "GraphicsLayerQt.h"
> +
> +#include "CurrentTime.h"
> +#include "FloatRect.h"
> +#include "GraphicsContext.h"
> +#include "Image.h"
> +#include "RefCounted.h"
> +#include "TranslateTransformOperation.h"
> +#include "UnitBezier.h"
> +#include <QtCore/qabstractanimation.h>
> +#include <QtCore/qdebug.h>
> +#include <QtCore/qset.h>
> +#include <QtCore/qtimer.h>
> +#include <QtGui/qbitmap.h>
> +#include <QtGui/qcolor.h>
> +#include <QtGui/qgraphicseffect.h>
> +#include <QtGui/qgraphicsitem.h>
> +#include <QtGui/qgraphicsscene.h>
> +#include <QtGui/qmatrix4x4.h>
> +#include <QtGui/qpainter.h>
> +#include <QtGui/qpalette.h>
> +#include <QtGui/qpixmap.h>
> +#include <QtGui/qstyleoption.h>
> +
> +namespace {
> +    enum StaticContentType { NoContentType, PixmapContentType, ColorContentType};
> +
> +}
> +
> +namespace WebCore {
> +
> +class GraphicsLayerQtPrivate : public QGraphicsObject {
> +    Q_OBJECT


All the friends and the fields should be at the bottom of the class definition.

> +    friend class GraphicsLayerQt;
> +    friend class TransformAnimationQt;
> +    friend class OpacityAnimationQt;
> +    GraphicsLayerQt* layer;
> +    GraphicsLayerClient* client;
> +    QTransform bTransform;
> +    bool transformAnimationRunning, opacityAnimationRunning;
> +    struct ContentData {
> +        QPixmap pixmap;
> +        QRegion regionToUpdate;
> +        bool updateAll, bgcolorValid;

Please declare on separate lines

> +        QColor contentsBgColor, bgcolor;

write out background 

> +        StaticContentType contentType;
> +        float opacity;
> +        ContentData()
> +                :updateAll(false)
> +                , bgcolorValid(false)
> +                , contentType(NoContentType)
> +                , opacity(1)
> +        {
> +        }
> +    } pendingContent, curContent;

current

[Noam Rosenthal]

Created attachment 46808 [details]
accelerated compositing in Qt: with massive style improvements and in-code comments

I've tried to get the style right this time. 
There's probably a few substance issues (architecture choices) to review here - is the style at a good enough level to start reviewing those? I'm sure things are not yet perfect but I want to do what's necessary to get this in soon so we can fill in the gap QtWebkit has with CSS animations.

[WebKit Review Bot]

Attachment 46808 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring WebKit/qt/Api/qwebsettings.cpp; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebview.cpp; This file is exempt from the style guide.
WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1116:  Alphabetical sorting problem.  [build/include_order] [4]
Ignoring WebKit/qt/Api/qgraphicswebview.cpp; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebsettings.h; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qgraphicswebview.h; This file is exempt from the style guide.
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> Attachment 46808 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> Ignoring WebKit/qt/Api/qwebsettings.cpp; This file is exempt from the style
> guide.
> Ignoring WebKit/qt/Api/qwebview.cpp; This file is exempt from the style guide.
> WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1116:  Alphabetical sorting
> problem.  [build/include_order] [4]
> Ignoring WebKit/qt/Api/qgraphicswebview.cpp; This file is exempt from the style
> guide.
> Ignoring WebKit/qt/Api/qwebsettings.h; This file is exempt from the style
> guide.
> Ignoring WebKit/qt/Api/qgraphicswebview.h; This file is exempt from the style
> guide.
> Total errors found: 1

Why are these exempted from the style guide? We don't use Qt style in them, but WebKit style

[Noam Rosenthal]

This is definitely not something I deliberately asked for... the whole webkit contribution machinery is pure magic to me :)

[Kenneth Rohde Christiansen]

Comment on attachment 46808 [details]
accelerated compositing in Qt: with massive style improvements and in-code comments


> Index: WebKit/qt/QGVLauncher/main.cpp
> ===================================================================
> --- WebKit/qt/QGVLauncher/main.cpp	(revision 53364)
> +++ WebKit/qt/QGVLauncher/main.cpp	(working copy)
> @@ -458,14 +458,20 @@
>      QWebSettings::enablePersistentStorage();
>  
>      const QStringList args = app.arguments();
> -    if (args.count() > 1)
> +    const int indexOfUrl = args.indexOf("--url");
> +    if (indexOfUrl > 0 && indexOfUrl < args.count() - 1)
> +        url = args.at(indexOfUrl+1);

spacing issue

> +    else if (args.count() > 1)
>          url = args.at(1);
> +    if (args.contains("--compositing"))
> +        QWebSettings::globalSettings()->setAttribute(QWebSettings::AcceleratedCompositingEnabled, true);
>  
>      MainWindow* window = new MainWindow;
>      window->load(url);
>  
> -    for (int i = 2; i < args.count(); i++)
> -        window->newWindow(args.at(i));
> +    for (int i = 2; i < args.count(); ++i)
> +        if (!args.at(i).startsWith("-") && !args.at(i - 1).startsWith("-"))
> +            window->newWindow(args.at(i));
>  
>      window->show();
>      return app.exec();
> Index: WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
> ===================================================================
> --- WebKit/qt/WebCoreSupport/ChromeClientQt.cpp	(revision 53364)
> +++ WebKit/qt/WebCoreSupport/ChromeClientQt.cpp	(working copy)
> @@ -25,6 +25,7 @@
>   * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
> +
>  #include "config.h"
>  #include "ChromeClientQt.h"
>  
> @@ -42,6 +43,10 @@
>  #include "QWebPageClient.h"
>  #include "SecurityOrigin.h"
>  
> +#include <qdebug.h>
> +#include <qtextdocument.h>
> +#include <qtooltip.h>
> +
>  #include "qwebpage.h"
>  #include "qwebpage_p.h"
>  #include "qwebframe_p.h"
> @@ -49,13 +54,12 @@
>  #include "qwebsecurityorigin_p.h"
>  #include "qwebview.h"
>  
> -#include <qtooltip.h>
> -#include <qtextdocument.h>
> +#if USE(ACCELERATED_COMPOSITING)
> +#include "GraphicsLayerQt.h"
> +#endif
>  
> -namespace WebCore
> -{
> +namespace WebCore {
>  
> -
>  ChromeClientQt::ChromeClientQt(QWebPage* webPage)
>      : m_webPage(webPage)
>  {
> @@ -466,6 +470,28 @@
>      notImplemented();
>  }
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +void ChromeClientQt::attachRootGraphicsLayer(Frame* frame, GraphicsLayer* graphicsLayer)
> +{    
> +    if (platformPageClient())
> +        platformPageClient()->setRootGraphicsLayer(graphicsLayer ? graphicsLayer->nativeLayer() : 0);
> +}
> +
> +void ChromeClientQt::setNeedsOneShotDrawingSynchronization()
> +{
> +    // we want the layers to synchronize next time we update the screen anyway
> +    if (platformPageClient())
> +        platformPageClient()->markForSync(false);

I would suggest setNeedsSynchronization, to keep it similar with setNeedsLayout()

> +}
> +
> +void ChromeClientQt::scheduleCompositingLayerSync()
> +{
> +    // we want the layers to synchronize ASAP

Explain why

> +    if (platformPageClient())
> +        platformPageClient()->markForSync(true);
> +}
> +#endif
> +
>  QtAbstractWebPopup* ChromeClientQt::createPopup()
>  {
>      return new QtFallbackWebPopup;
> Index: WebKit/qt/WebCoreSupport/ChromeClientQt.h
> ===================================================================
> --- WebKit/qt/WebCoreSupport/ChromeClientQt.h	(revision 53364)
> +++ WebKit/qt/WebCoreSupport/ChromeClientQt.h	(working copy)
> @@ -123,6 +123,15 @@
>  #if ENABLE(OFFLINE_WEB_APPLICATIONS)
>          virtual void reachedMaxAppCacheSize(int64_t spaceNeeded);
>  #endif
> +
> +#if USE(ACCELERATED_COMPOSITING)
> +        // see ChromeClient.h
> +        // this is a hook for WebCore to tell us what we need to do with the GraphicsLayers
> +        virtual void attachRootGraphicsLayer(Frame*, GraphicsLayer*);
> +        virtual void setNeedsOneShotDrawingSynchronization();
> +        virtual void scheduleCompositingLayerSync();
> +#endif
> +
>          virtual void runOpenPanel(Frame*, PassRefPtr<FileChooser>);
>  
>          virtual void formStateDidChange(const Node*) { }
> Index: WebKit/qt/Api/qgraphicswebview.cpp
> ===================================================================
> --- WebKit/qt/Api/qgraphicswebview.cpp	(revision 53364)
> +++ WebKit/qt/Api/qgraphicswebview.cpp	(working copy)
> @@ -22,24 +22,73 @@
>  #include "qgraphicswebview.h"
>  
>  #include "qwebframe.h"
> +#include "qwebframe_p.h"
>  #include "qwebpage.h"
>  #include "qwebpage_p.h"
>  #include "QWebPageClient.h"
> -#include <QtGui/QGraphicsScene>
> -#include <QtGui/QGraphicsView>
> +#include <FrameView.h>
> +#include <QtCore/qsharedpointer.h>
> +#include <QtCore/qtimer.h>
>  #include <QtGui/qapplication.h>
> +#include <QtGui/qgraphicsscene.h>
>  #include <QtGui/qgraphicssceneevent.h>
> +#include <QtGui/qgraphicsview.h>
> +#include <QtGui/qpixmapcache.h>
>  #include <QtGui/qstyleoption.h>
>  #if defined(Q_WS_X11)
>  #include <QX11Info>
>  #endif
> +#include <Settings.h>
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +
> +// the overlay is here for one reason only: to have the scroll-bars and other

scrollbars

> +// extra UI elements appear on top of any QGraphicsItems created by CSS compositing layers
> +class QGraphicsWebViewOverlay : public QGraphicsItem {
> +    public:

No space before public: please

> +    QGraphicsWebViewOverlay(QGraphicsWebView* view)
> +            :QGraphicsItem(view)

missing space after :

> +            , q(view)
> +    {
> +        setPos(0, 0);
> +        setFlag(QGraphicsItem::ItemUsesExtendedStyleOption, true);
> +        setCacheMode(QGraphicsItem::DeviceCoordinateCache);

Would be good to add a comment why these are set

> +    }
> +
> +    QRectF boundingRect() const
> +    {
> +        return q->boundingRect();
> +    }
> +
> +    void paint(QPainter* painter, const QStyleOptionGraphicsItem* options, QWidget*)
> +    {
> +        q->page()->mainFrame()->render(painter, static_cast<QWebFrame::RenderLayer>(QWebFrame::AllLayers&(~QWebFrame::ContentsLayer)), options->exposedRect.toRect());
> +    }
> +
> +    friend class QGraphicsWebView;
> +    QGraphicsWebView* q;
> +};
> +
> +#endif
> +
>  class QGraphicsWebViewPrivate : public QWebPageClient {
>  public:
>      QGraphicsWebViewPrivate(QGraphicsWebView* parent)
> -        : q(parent)
> -        , page(0)
> -    {}
> +        : q(parent),
> +        page(0),
> +#if USE(ACCELERATED_COMPOSITING)
> +        rootGraphicsLayer(0),
> +        overlay(new QGraphicsWebViewOverlay(parent)),
> +        shouldSync(true)
> +#endif
> +    {
> +#if USE(ACCELERATED_COMPOSITING)
> +        // the overlay and stays alive for the lifetime of
> +        // this QGraphicsWebView as the scrollbars are needed when there's no compositing
> +        overlay->setZValue(OverlayZValue);
> +        q->setFlag(QGraphicsItem::ItemUsesExtendedStyleOption);
> +#endif
> +    }
>  
>      virtual ~QGraphicsWebViewPrivate();
>      virtual void scroll(int dx, int dy, const QRect&);
> @@ -61,16 +110,90 @@
>  
>      virtual QObject* pluginParent() const;
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +    virtual void setRootGraphicsLayer(QGraphicsItem* layer);
> +    virtual void markForSync(bool scheduleSync);
> +    void updateCompositingScrollPosition();

updateComposedScroll.. ? the method name isn't so clear

updateOverlayScrollPosition ?

> +#endif
> +
> +    void syncLayers();
>      void _q_doLoadFinished(bool success);
>  
>      QGraphicsWebView* q;
>      QWebPage* page;
> +#if USE(ACCELERATED_COMPOSITING)
> +    QGraphicsItem* rootGraphicsLayer;
> +    QGraphicsWebViewOverlay* overlay;
> +
> +    // we need to sync the layers if we get a special call from the WebCore
> +    // compositor telling us to do so. We'll get that call from ChromeClientQt
> +    bool shouldSync;

syncNeeded ?

> +
> +    // we need to put the root graphics layer behind the overlay (which contains the scrollbar)
> +    enum { RootGraphicsLayerZValue, OverlayZValue };
> +#endif
>  };
>  
>  QGraphicsWebViewPrivate::~QGraphicsWebViewPrivate()
>  {
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (rootGraphicsLayer) {
> +        // we don't need to delete the root graphics layer
> +        // The lifecycle is managed in GraphicsLayerQt.cpp
> +        rootGraphicsLayer->setParentItem(0);
> +        q->scene()->removeItem(rootGraphicsLayer);
> +    }
> +#endif
>  }
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +void QGraphicsWebViewPrivate::setRootGraphicsLayer(QGraphicsItem* layer)
> +{
> +    if (rootGraphicsLayer) {
> +        rootGraphicsLayer->setParentItem(0);
> +        q->scene()->removeItem(rootGraphicsLayer);
> +        QWebFramePrivate::core(q->page()->mainFrame())->view()->syncCompositingStateRecursive();
> +    }
> +
> +    rootGraphicsLayer = layer;
> +
> +    if (layer) {
> +        layer->setFlag(QGraphicsItem::ItemClipsChildrenToShape, true);
> +        layer->setParentItem(q);
> +        layer->setZValue(RootGraphicsLayerZValue);
> +        updateCompositingScrollPosition();
> +    }
> +}
> +
> +void QGraphicsWebViewPrivate::markForSync(bool scheduleSync)
> +{
> +    shouldSync = true;
> +    if (scheduleSync)
> +        QTimer::singleShot(0, q, SLOT(syncLayers()));
> +}
> +
> +void QGraphicsWebViewPrivate::updateCompositingScrollPosition()
> +{
> +    if (rootGraphicsLayer && q->page() && q->page()->mainFrame()) {
> +        const QPoint scrollPosition = q->page()->mainFrame()->scrollPosition();
> +        rootGraphicsLayer->setPos(-scrollPosition);
> +    }
> +}
> +
> +#endif
> +
> +
> +void QGraphicsWebViewPrivate::syncLayers()
> +{
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (shouldSync) {
> +        QWebFramePrivate::core(q->page()->mainFrame())->view()->syncCompositingStateRecursive();
> +        shouldSync = false;
> +    }
> +#endif
> +}
> +
> +
>  void QGraphicsWebViewPrivate::_q_doLoadFinished(bool success)
>  {
>      // If the page had no title, still make sure it gets the signal
> @@ -83,11 +206,18 @@
>  void QGraphicsWebViewPrivate::scroll(int dx, int dy, const QRect& rectToScroll)
>  {
>      q->scroll(qreal(dx), qreal(dy), QRectF(rectToScroll));
> +#if USE(ACCELERATED_COMPOSITING)
> +    updateCompositingScrollPosition();
> +//    QApplication::instance()->sendPostedEvents();
> +#endif
>  }
>  
>  void QGraphicsWebViewPrivate::update(const QRect & dirtyRect)
>  {
>      q->update(QRectF(dirtyRect));
> +#if USE(ACCELERATED_COMPOSITING)
> +    overlay->update(QRectF(dirtyRect));
> +#endif
>  }
>  
>  
> @@ -248,6 +378,7 @@
>      setAcceptTouchEvents(true);
>  #endif
>      setFocusPolicy(Qt::StrongFocus);
> +    setFlag(QGraphicsItem::ItemClipsChildrenToShape, true);
>  }
>  
>  /*!
> @@ -293,11 +424,17 @@
>      return d->page;
>  }
>  
> +
>  /*! \reimp
>  */
>  void QGraphicsWebView::paint(QPainter* painter, const QStyleOptionGraphicsItem* option, QWidget*)
>  {
> -    page()->mainFrame()->render(painter, option->exposedRect.toRect());
> +#if USE(ACCELERATED_COMPOSITING)
> +    page()->mainFrame()->render(painter, QWebFrame::ContentsLayer, option->exposedRect.toRect());
> +    d->syncLayers();
> +#else
> +    page()->mainFrame()->render(painter, QWebFrame::AllLayers, option->exposedRect.toRect());
> +#endif
>  }
>  
>  /*! \reimp
> @@ -425,6 +562,9 @@
>      d->page = page;
>      if (!d->page)
>          return;
> +#if USE(ACCELERATED_COMPOSITING)
> +    d->overlay->prepareGeometryChange();
> +#endif
>      d->page->d->client = d; // set the page client
>  
>      QSize size = geometry().size().toSize();
> @@ -528,6 +668,11 @@
>  */
>  void QGraphicsWebView::updateGeometry()
>  {
> +
> +#if USE(ACCELERATED_COMPOSITING)
> +    d->overlay->prepareGeometryChange();
> +#endif
> +
>      QGraphicsWidget::updateGeometry();
>  
>      if (!d->page)
> @@ -543,6 +688,10 @@
>  {
>      QGraphicsWidget::setGeometry(rect);
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +    d->overlay->prepareGeometryChange();
> +#endif
> +
>      if (!d->page)
>          return;
>  
> Index: WebKit/qt/Api/qwebsettings.h
> ===================================================================
> --- WebKit/qt/Api/qwebsettings.h	(revision 53364)
> +++ WebKit/qt/Api/qwebsettings.h	(working copy)
> @@ -68,7 +68,8 @@
>  #endif
>          LocalContentCanAccessRemoteUrls,
>          DnsPrefetchEnabled,
> -        XSSAuditorEnabled
> +        XSSAuditorEnabled,
> +        AcceleratedCompositingEnabled
>      };
>      enum WebGraphic {
>          MissingImageGraphic,
> Index: WebKit/qt/Api/qwebsettings.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebsettings.cpp	(revision 53364)
> +++ WebKit/qt/Api/qwebsettings.cpp	(working copy)
> @@ -152,7 +152,12 @@
>          value = attributes.value(QWebSettings::JavascriptEnabled,
>                                        global->attributes.value(QWebSettings::JavascriptEnabled));
>          settings->setJavaScriptEnabled(value);
> +#if USE(ACCELERATED_COMPOSITING)
> +        value = attributes.value(QWebSettings::AcceleratedCompositingEnabled,
> +                                      global->attributes.value(QWebSettings::AcceleratedCompositingEnabled));
>  
> +        settings->setAcceleratedCompositingEnabled(value);
> +#endif
>          value = attributes.value(QWebSettings::JavascriptCanOpenWindows,
>                                        global->attributes.value(QWebSettings::JavascriptCanOpenWindows));
>          settings->setJavaScriptCanOpenWindowsAutomatically(value);
> @@ -389,6 +394,7 @@
>      d->attributes.insert(QWebSettings::OfflineWebApplicationCacheEnabled, false);
>      d->attributes.insert(QWebSettings::LocalStorageEnabled, false);
>      d->attributes.insert(QWebSettings::LocalContentCanAccessRemoteUrls, false);
> +    d->attributes.insert(QWebSettings::AcceleratedCompositingEnabled, false);
>      d->offlineStorageDefaultQuota = 5 * 1024 * 1024;
>      d->defaultTextEncoding = QLatin1String("iso-8859-1");
>  }
> Index: WebKit/qt/Api/qgraphicswebview.h
> ===================================================================
> --- WebKit/qt/Api/qgraphicswebview.h	(revision 53364)
> +++ WebKit/qt/Api/qgraphicswebview.h	(working copy)
> @@ -105,7 +105,6 @@
>      void iconChanged();
>      void statusBarMessage(const QString& message);
>      void linkClicked(const QUrl&);
> -
>  protected:
>      virtual void mousePressEvent(QGraphicsSceneMouseEvent*);
>      virtual void mouseDoubleClickEvent(QGraphicsSceneMouseEvent*);
> @@ -134,7 +133,9 @@
>  
>  private:
>      Q_PRIVATE_SLOT(d, void _q_doLoadFinished(bool success))
> -
> +    // we don't want to change the moc based on USE() macro, so this function is here
> +    // but will be empty if ACCLERATED_COMPOSITING is disabled
> +    Q_PRIVATE_SLOT(d, void syncLayers())
>      QGraphicsWebViewPrivate* const d;
>      friend class QGraphicsWebViewPrivate;
>  };
> Index: WebKit/qt/Api/qwebview.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebview.cpp	(revision 53364)
> +++ WebKit/qt/Api/qwebview.cpp	(working copy)
> @@ -22,10 +22,11 @@
>  #include "config.h"
>  #include "qwebview.h"
>  
> +#include "Page.h"
>  #include "QWebPageClient.h"
> +#include "Settings.h"
>  #include "qwebframe.h"
>  #include "qwebpage_p.h"
> -
>  #include "qbitmap.h"
>  #include "qevent.h"
>  #include "qpainter.h"
> @@ -247,6 +248,9 @@
>                  this, SLOT(_q_pageDestroyed()));
>      }
>      setAttribute(Qt::WA_OpaquePaintEvent, d->page);
> +#if USE(ACCELERATED_COMPOSITING)
> +    d->page->d->page->settings()->setAcceleratedCompositingEnabled(false);

This seems like a hack. It should just ignore it nicely. And documentation should say that it only works for the GraphicsView

> +#endif
>      update();
>  }
>  
> Index: WebCore/WebCore.pro
> ===================================================================
> --- WebCore/WebCore.pro	(revision 53364)
> +++ WebCore/WebCore.pro	(working copy)
> @@ -2707,6 +2707,19 @@
>              plugins/win/PaintHooks.asm
>      }
>  }
> +contains(DEFINES, WTF_USE_ACCELERATED_COMPOSITING) {
> +HEADERS += \
> +    rendering/RenderLayerBacking.h \
> +    rendering/RenderLayerCompositor.h \
> +    platform/graphics/GraphicsLayer.h \
> +    platform/graphics/GraphicsLayerClient.h \
> +    platform/graphics/qt/GraphicsLayerQt.h
> +SOURCES += \
> +    platform/graphics/GraphicsLayer.cpp \
> +    platform/graphics/qt/GraphicsLayerQt.cpp \
> +    rendering/RenderLayerBacking.cpp \
> +    rendering/RenderLayerCompositor.cpp
> +}
>  
>  symbian {
>      shared {
> Index: WebCore/platform/qt/QWebPageClient.h
> ===================================================================
> --- WebCore/platform/qt/QWebPageClient.h	(revision 53364)
> +++ WebCore/platform/qt/QWebPageClient.h	(working copy)
> @@ -30,7 +30,7 @@
>  #include <QCursor>
>  #endif
>  #include <QRect>
> -
> +class QGraphicsItem;
>  class QWebPageClient {
>  public:
>      virtual ~QWebPageClient() { }
> @@ -39,6 +39,16 @@
>      virtual void update(const QRect&) = 0;
>      virtual void setInputMethodEnabled(bool enable) = 0;
>      virtual bool inputMethodEnabled() const = 0;
> +#if USE(ACCELERATED_COMPOSITING)
> +    // this gets called when we start/stop compositing.
> +    virtual void setRootGraphicsLayer(QGraphicsItem* layer) {}
> +
> +    // this gets called when the compositor wants us to sync the layers
> +    // if scheduleSync is true, we schedule a sync ourselves. otherwise,
> +    // we wait for the next update and sync the layers then.
> +    virtual void markForSync(bool scheduleSync = false) {}
> +#endif
> +
>  #if QT_VERSION >= 0x040600
>      virtual void setInputMethodHint(Qt::InputMethodHint hint, bool enable) = 0;
>  #endif
> Index: WebCore/platform/graphics/qt/GraphicsLayerQt.cpp
> ===================================================================
> --- WebCore/platform/graphics/qt/GraphicsLayerQt.cpp	(revision 0)
> +++ WebCore/platform/graphics/qt/GraphicsLayerQt.cpp	(revision 0)
> @@ -0,0 +1,1116 @@
> +/*
> +    Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies)
> +
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public
> +    License as published by the Free Software Foundation; either
> +    version 2 of the License, or (at your option) any later version.
> +
> +    This library is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +    Library General Public License for more details.
> +
> +    You should have received a copy of the GNU Library General Public License
> +    along with this library; see the file COPYING.LIB.  If not, write to
> +    the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> +    Boston, MA 02110-1301, USA.
> +*/
> +
> +#include "config.h"
> +#include "GraphicsLayerQt.h"
> +
> +#include "CurrentTime.h"
> +#include "FloatRect.h"
> +#include "GraphicsContext.h"
> +#include "Image.h"
> +#include "RefCounted.h"
> +#include "TranslateTransformOperation.h"
> +#include "UnitBezier.h"
> +#include <QtCore/qabstractanimation.h>
> +#include <QtCore/qdebug.h>
> +#include <QtCore/qset.h>
> +#include <QtCore/qtimer.h>
> +#include <QtGui/qapplication.h>
> +#include <QtGui/qbitmap.h>
> +#include <QtGui/qcolor.h>
> +#include <QtGui/qgraphicseffect.h>
> +#include <QtGui/qgraphicsitem.h>
> +#include <QtGui/qgraphicsscene.h>
> +#include <QtGui/qmatrix4x4.h>
> +#include <QtGui/qpainter.h>
> +#include <QtGui/qpalette.h>
> +#include <QtGui/qpixmap.h>
> +#include <QtGui/qstyleoption.h>
> +
> +namespace WebCore {
> +
> +
> +class GraphicsLayerQtPrivate : public QGraphicsObject {
> +    Q_OBJECT
> +
> +public:
> +    // this set of flags help us defer which properties of the layer have been
> +    // modified by the compositor, so we can know what to look for in the next flush
> +    enum PossibleChange {
> +        NoChanges       =           0,
> +        ChildrenChange  =           (1L << 1),
> +        MaskLayerChange =           (1L << 2),
> +        PositionChange  =           (1L << 3),
> +        AnchorPointChange =         (1L << 4),
> +        SizeChange      =           (1L << 5),
> +        TransformChange =           (1L << 6),
> +        ContentChange   =           (1L << 7),
> +        GeometryOrientationChange = (1L << 8),
> +        ContentsOrientationChange = (1L << 9),
> +        OpacityChange   =           (1L << 10),
> +        ContentsRectChange =        (1L << 11),
> +        Preserves3DChange =         (1L << 12),
> +        MasksToBoundsChange =       (1L << 13),
> +        DrawsContentChange =        (1L << 14),
> +        ContentsOpaqueChange =      (1L << 15),
> +        BackfaceVisibilityChange =  (1L << 16),
> +        ChildrenTransformChange =   (1L << 17),
> +        DisplayChange =             (1L << 18),
> +        BackgroundColorChange =     (1L << 19),
> +        ParentChange =              (1L << 20),
> +        DistributesOpacityChange =  (1L << 21)
> +    };
> 

where does these come from?

> +    // the compositor lets us special-case images and colors, so we try to do so
> +    enum StaticContentType { HTMLContentType, PixmapContentType, ColorContentType};
> +
> +    GraphicsLayerQtPrivate(GraphicsLayerQt* newLayer, GraphicsLayerClient* newClient);
> +    virtual ~GraphicsLayerQtPrivate();
> +
> +    // reimps from QGraphicsItem
> +    virtual QPainterPath opaqueArea() const;

I believe that QPainterPath can be quite expensive

> +    virtual QRectF boundingRect() const;
> +    virtual void paint(QPainter* painter, const QStyleOptionGraphicsItem* option, QWidget* widget);

In header files we don't add redundant arguments... like QPainter* painter, but just QPainter*. When it is
not redundant (like QRect viewArea) it is added!

Remove painter, option and widget here

> +
> +    // we manage transforms ourselves because transform-origin acts differently in webkit and in Qt
> +    void setBaseTransform(const QTransform& t);    

remove t here

> +    void drawContents(QPainter* painter, const QRectF& r, bool mask = false);

you know the drill now :-)

> +
> +    // let the compositor-API tell us which properties were changed
> +    void noteChange(PossibleChange pc);

remove pc

> +
> +    // called when the compositor is ready for us to show the changes on screen
> +    // this is called indirectly from ChromeClientQt::setNeedsOneShotDrawingSynchronization
> +    // (meaning the sync would happen together with the next draw)
> +    // or ChromeClientQt::scheduleCompositingLayerSync (meaning the sync will happen ASAP)
> +    void flushChanges(bool recursive = true);
> +
> +public slots:
> +    // we need to notify the client (aka the layer compositor) when the animation actually starts
> +    void notifyAnimationStarted();
> +
> +public:
> +    GraphicsLayerQt* layer;
> +    GraphicsLayerClient* client;
> +
> +    QTransform baseTransform;
> +    bool transformAnimationRunning;
> +    bool opacityAnimationRunning;

If these are used from the class itself you should make methods for accessing them instead. They will be inline anyway.

> +
> +    struct ContentData {
> +        QPixmap pixmap;
> +        QRegion regionToUpdate;
> +        bool updateAll;
> +        bool backgroundColorValid;
> +        QColor contentsBackgroundColor;
> +        QColor backgroundColor;
> +        StaticContentType contentType;
> +        float opacity;
> +        ContentData()
> +                : updateAll(false)
> +                , backgroundColorValid(false)
> +                , contentType(HTMLContentType)
> +                , opacity(1)

indentation seems wrong

> +        {
> +        }
> +
> +    };
> +
> +    ContentData pendingContent;
> +    ContentData currentContent;
> +
> +    PossibleChange possibleChanges;
> +
> +    QSizeF size;
> +    QList<QWeakPointer<QAbstractAnimation> > animations;
> +    QTimer suspendTimer;
> +
> +    struct State {
> +        GraphicsLayer* maskLayer;
> +        FloatPoint pos;
> +        FloatPoint3D anchorPoint;
> +        FloatSize size;
> +        TransformationMatrix transform;
> +        TransformationMatrix childrenTransform;
> +        Color backgroundColor;
> +        Color currentColor;
> +        GraphicsLayer::CompositingCoordinatesOrientation geoOrientation;

geo? geometric?

> +        GraphicsLayer::CompositingCoordinatesOrientation contentsOrientation;
> +        float opacity;
> +        QRect contentsRect;
> +
> +        bool preserves3D: 1;
> +        bool masksToBounds: 1;
> +        bool drawsContent: 1;
> +        bool contentsOpaque: 1;
> +        bool backfaceVisibility: 1;
> +        bool distributeOpacity: 1;
> +        bool align: 2;

align what?

> +        State() : maskLayer(0), opacity(1), preserves3D(false), masksToBounds(false),

opacity == 1.0 ?

> +                  drawsContent(false), contentsOpaque(false), backfaceVisibility(false),
> +                  distributeOpacity(false)
> +        {
> +        }
> +    } state;
> +};
> +
> +GraphicsLayerQtPrivate::GraphicsLayerQtPrivate(GraphicsLayerQt* newLayer, GraphicsLayerClient* newClient)
> +        : QGraphicsObject(0)
> +        , layer(newLayer)
> +        , transformAnimationRunning(false)
> +        , possibleChanges(NoChanges)
> +
> +{
> +    this->client = newClient;

how is the client and layer new when this is a constructor?

> +
> +    // better to calculate the exposed rect in QGraphicsView than over-render in WebCore
> +    // though this might be tweakable
> +    setFlag(QGraphicsItem::ItemUsesExtendedStyleOption, true);
> +
> +    // we use graphics-view for compositing, not for interactivity
> +    setAcceptedMouseButtons(Qt::NoButton);

What about keyevents?

> +    setEnabled(false);
> +
> +    // we'll set the cache when we know what's going on
> +    setCacheMode(NoCache);
> +}
> +
> +GraphicsLayerQtPrivate::~GraphicsLayerQtPrivate()
> +{
> +    // the compositor manages item lifecycle - we don't want the graphics-view
> +    // system to automatically delete our items
> +    foreach (QGraphicsItem* i, childItems()) {
> +        if (scene())
> +            scene()->removeItem(i);
> +        i->setParentItem(0);
> +    }

foreach on non-consts is quite slow, consider using stl iterators

> +    
> +    // we do, however, own the animations...
> +    foreach (QWeakPointer<QAbstractAnimation> anim, animations) {
> +        if (anim)
> +            delete anim.data();
> +    }
> +}

here as well

> +
> +void GraphicsLayerQtPrivate::setBaseTransform(const QTransform& t)
> +{
> +    if (layer) {
> +        // webkit has relative-to-size originPoint, graphics-view has a pixel originPoint
> +        // here we convert
> +        QPointF originTranslate(
> +                layer->anchorPoint().x()*layer->size().width(), layer->anchorPoint().y()*layer->size().height());
> +
> +        resetTransform();
> +
> +        // we have to manage this ourselves because QGraphicsView's transformOrigin is incomplete
> +        translate(originTranslate.x(), originTranslate.y());
> +        setTransform(t, true);
> +        translate(-originTranslate.x(), -originTranslate.y());
> +        baseTransform = t;
> +    }
> +}

[Kenneth Rohde Christiansen]

Comment on attachment 46808 [details]
accelerated compositing in Qt: with massive style improvements and in-code comments

Second part!

> +QPainterPath GraphicsLayerQtPrivate::opaqueArea() const
> +{
> +    QPainterPath pp;
> +    // we try out best to return the opaque area, maybe it will help graphics-view render less items
> +    if (currentContent.backgroundColorValid && currentContent.backgroundColor.alpha() == 0xff)
> +        pp.addRect(boundingRect());
> +    else {
> +        if (state.contentsOpaque
> +            || (currentContent.contentType == ColorContentType && currentContent.contentsBackgroundColor.alpha() == 0xff)
> +            || (currentContent.contentType == PixmapContentType && !currentContent.pixmap.hasAlpha())) {
> +
> +            pp.addRect(state.contentsRect);
> +        }
> +    }
> +    return pp;
> +}
> +
> +QRectF GraphicsLayerQtPrivate::boundingRect() const
> +{
> +    return QRectF(QPointF(0, 0), QSizeF(size));
> +}
> +
> +void GraphicsLayerQtPrivate::paint(QPainter* painter, const QStyleOptionGraphicsItem* option, QWidget* widget)
> +{
> +    if (state.maskLayer && state.maskLayer->platformLayer()) {
> +        // maybe this shouldn't be in paint (rendering the mask-layer)
> +        // though it does work for now

Why comment on two lines, when it on one line is about the size of the line below? 

> +        GraphicsLayerQtPrivate* otherMask = (GraphicsLayerQtPrivate*)state.maskLayer->platformLayer();
> +        otherMask->flushChanges(true);
> +        
> +        // CSS3 mask and QGraphicsOpacityEffect are the same thing! we just need to convert...
> +        if (!graphicsEffect()) {
> +            QPixmap mask(QSize(state.maskLayer->size().width(), state.maskLayer->size().height()));
> +            mask.fill(Qt::transparent);
> +            {
> +                QPainter p(&mask);
> +                p.setRenderHints(painter->renderHints(), true);
> +                p.setCompositionMode(QPainter::CompositionMode_Source);
> +                static_cast<GraphicsLayerQtPrivate*>(state.maskLayer->platformLayer())->drawContents(&p, option->exposedRect, true);
> +            }
> +            QGraphicsOpacityEffect* eff = new QGraphicsOpacityEffect(this);

eff :-) I like fx more :-) hehe.

Is this conversion fast?

> +            eff->setOpacity(1);
> +            eff->setOpacityMask(QBrush(mask));
> +            setGraphicsEffect(eff);
> +        }
> +    }
> +    drawContents(painter, option->exposedRect);
> +}
> +
> +void GraphicsLayerQtPrivate::drawContents(QPainter* painter, const QRectF& r, bool mask)
> +{
> +    // webkit uses IntRect, we use QRectF. we don't want missing pixels    
> +    QRect rect = r.toRect().adjusted(-1, -1, 1, 1);
> +    
> +    if (currentContent.contentType != HTMLContentType && !state.contentsRect.isEmpty())
> +        rect = rect.intersected(state.contentsRect);
> +
> +    if (currentContent.backgroundColorValid)
> +        painter->fillRect(r, QColor(currentContent.backgroundColor));
> +
> +    if (!rect.isEmpty()) {
> +        switch (currentContent.contentType) {
> +        case PixmapContentType:
> +            // we have to scale the image to the contentsRect
> +            // maybe there's a better way to this, but this works for now
> +            painter->drawPixmap(rect.topLeft(),
> +                                currentContent.pixmap.scaled(state.contentsRect.size()), r);
> +            break;
> +        case ColorContentType:
> +            painter->fillRect(rect, currentContent.contentsBackgroundColor);
> +            break;
> +        default:
> +            if (state.drawsContent) {
> +                // this is the "expensive" bit. we try to minimize calls to this
> +                // neck of the woods by proper caching
> +                GraphicsContext gc(painter);
> +                layer->paintGraphicsLayerContents(gc, rect);
> +            }
> +            break;
> +        }
> +    }
> +}
> +
> +void GraphicsLayerQtPrivate::noteChange(PossibleChange pc)
> +{
> +    if (!this)
> +        return;
> +
> +    possibleChanges = static_cast<PossibleChange>(static_cast<int>(possibleChanges) | static_cast<int>(pc));
> +
> +    if (layer->client())
> +        layer->client()->notifySyncRequired(layer);
> +}
> +
> +void GraphicsLayerQtPrivate::flushChanges(bool recursive)
> +{
> +    // this is the bulk of the work. understanding what the compositor is trying to achieve,
> +    // what graphics-view can do, and trying to find a sane common-grounds
> +    if (layer) {
> +        if (possibleChanges != NoChanges) {
> +
> +            if (currentContent.contentType == HTMLContentType && (possibleChanges & ParentChange)) {
> +                // the WebCore compositor manages item ownership. We have to make sure
> +                // graphics-view doesn't try to snatch that ownership...
> +                if (!layer->parent() && !parentItem())
> +                    setParentItem(0);
> +                else if (layer && layer->parent() && layer->parent()->nativeLayer() != parentItem())
> +                    setParentItem(layer->parent()->nativeLayer());
> +            }
> +
> +            if (possibleChanges & ChildrenChange) {
> +                // we basically do an XOR operation on the list of current children
> +                // and the list of wanted children, and remove/add
> +                QSet<QGraphicsItem*> newChildren;
> +                const Vector<GraphicsLayer*> newChildrenVector = (layer->children());
> +                newChildren.reserve(newChildrenVector.size());
> +
> +                for (size_t i = 0; i < newChildrenVector.size(); ++i)
> +                    newChildren.insert(newChildrenVector[i]->platformLayer());
> +
> +                const QSet<QGraphicsItem*> curChildren = childItems().toSet();

currentChildren, or maybe just children

> +                const QSet<QGraphicsItem*> childrenToAdd = newChildren - curChildren;
> +                const QSet<QGraphicsItem*> childrenToRemove = curChildren - newChildren;
> +                for (QSet<QGraphicsItem*>::const_iterator it = childrenToAdd.begin(); it != childrenToAdd.end(); ++it) {
> +                     QGraphicsItem* w = *it;
> +                     if (w)
> +                        w->setParentItem(this);

Here it would probably be OK to do if (QGraphicsItem* w = *it)

> +                }
> +                for (QSet<QGraphicsItem*>::const_iterator it = childrenToRemove.begin(); it != childrenToRemove.end(); ++it) {
> +                    QGraphicsItem* w = *it;
> +                    if (w)
> +                        w->setParentItem(0);
> +                }
> +
> +                // children are ordered by z-value, let graphics-view know.
> +                for (size_t i = 0; i < newChildrenVector.size(); ++i)
> +                    if (newChildrenVector[i]->platformLayer())
> +                        newChildrenVector[i]->platformLayer()->setZValue(i);
> +            }
> +
> +            if (possibleChanges & MaskLayerChange) {
> +                // we can't paint here, because we don't know if the mask layer
> +                // itself is ready... we'll have to wait till this layer tries to paint
> +                setGraphicsEffect(0);
> +                if (layer->maskLayer())
> +                    setFlag(ItemClipsChildrenToShape, true);
> +                else
> +                    setFlag(ItemClipsChildrenToShape, layer->masksToBounds());
> +                update();
> +            }
> +
> +            if (possibleChanges & PositionChange) {   

Maybe instead of possibleChanges use ChangeMask or so? to make it obvious that it is a mask??

             
> +                if (layer->position() != state.pos) {
> +                    setPos(layer->position().x(),
> +                           layer->position().y());

Why wrap here?

> +                }
> +            }
> +
> +            if (possibleChanges & SizeChange) {
> +                if (layer->size() != state.size) {
> +                    prepareGeometryChange();
> +                    size = QSizeF(layer->size().width(), layer->size().height());
> +                }
> +            }
> +
> +            if (possibleChanges & (TransformChange | AnchorPointChange | SizeChange)) {
> +                // since we convert a percentage-based origin-point to a pixel-based one,
> +                // the anchor-point, transform and size from WebCore all affect the one
> +                // that we give Qt
> +                if (state.transform != layer->transform() || state.anchorPoint != layer->anchorPoint() || state.size != layer->size())
> +                    setBaseTransform(QTransform(layer->transform()));
> +            }
> +
> +            // not sure how this is used; no LayoutTest actually utilized it
> +            if (possibleChanges & ChildrenTransformChange) {
> +                if (state.childrenTransform != layer->childrenTransform()) {
> +                    const QTransform tr(layer->childrenTransform());
> +                    QList<QGraphicsItem*> ch = childItems();
> +                    for (QList<QGraphicsItem*>::iterator it = ch.begin(); it != ch.end(); ++it)
> +                        (*it)->setTransform(tr);
> +                }
> +            }
> +
> +            if (possibleChanges & (ContentChange | DrawsContentChange)) {
> +

not needed new line
> +                switch (pendingContent.contentType) {
> +                case PixmapContentType:
> +                    // we need cache even for images, because they need to be resized
> +                    // to the contents rect. maybe this can be optimized though
> +                    setCacheMode(transformAnimationRunning ? ItemCoordinateCache : DeviceCoordinateCache);
> +                    update();
> +                    setFlag(ItemHasNoContents, false);
> +                    break;
> +
> +                case ColorContentType:
> +                    // no point in caching a solid-color rectangle
> +                    setCacheMode(QGraphicsItem::NoCache);
> +                    if (pendingContent.contentType != currentContent.contentType
> +                        || pendingContent.contentsBackgroundColor != currentContent.contentsBackgroundColor) {
> +                            update();
> +                    }
> +                    state.drawsContent = false;
> +                    setFlag(ItemHasNoContents, false);
> +                    break;
> +
> +                case HTMLContentType:
> +                    if (pendingContent.contentType != currentContent.contentType)
> +                        update();
> +                    if (state.drawsContent != layer->drawsContent()) {
> +                        if (!state.drawsContent && layer->drawsContent())
> +                            update();
> +                    }
> +                    if (layer->drawsContent())
> +                        setCacheMode(transformAnimationRunning ? ItemCoordinateCache : DeviceCoordinateCache);
> +                    else
> +                        setCacheMode(NoCache);
> +
> +                    setFlag(ItemHasNoContents, !layer->drawsContent());
> +                    break;
> +                }
> +            }
> +
> +            if ((possibleChanges & OpacityChange) && state.opacity != layer->opacity())
> +                    setOpacity(layer->opacity());
> +
> +            if (possibleChanges & ContentsRectChange) {
> +                const QRect r(layer->contentsRect());
> +                if (state.contentsRect != r) {
> +                    state.contentsRect = r;
> +                    update();
> +                }
> +            }
> +
> +            if ((possibleChanges & MasksToBoundsChange)
> +                && state.masksToBounds != layer->masksToBounds()) {
> +
> +                setFlag(QGraphicsItem::ItemClipsToShape, layer->masksToBounds());
> +                setFlag(QGraphicsItem::ItemClipsChildrenToShape, layer->masksToBounds());
> +            }
> +
> +            if ((possibleChanges & ContentsOpaqueChange)
> +                && state.contentsOpaque != layer->contentsOpaque())
> +                    prepareGeometryChange();
> +
> +            if (possibleChanges & DisplayChange) {
> +                if (pendingContent.updateAll)
> +                    update();
> +                else
> +                    update(pendingContent.regionToUpdate.boundingRect());
> +            }
> +
> +            if (possibleChanges & BackgroundColorChange) {
> +                if ((pendingContent.backgroundColorValid != currentContent.backgroundColorValid)
> +                    || (pendingContent.backgroundColorValid && (pendingContent.backgroundColor != currentContent.backgroundColor))) {
> +                    update();
> +
> +                }
> +            }
> +
> +            /*
> +              TODO:
> +              GeometryOrientationChange
> +              ContentsOrientationChange
> +              BackfaceVisibilityChange
> +            */
> +
> +            state.maskLayer = layer->maskLayer();
> +            state.pos = layer->position();
> +            state.anchorPoint = layer->anchorPoint();
> +            state.size = layer->size();
> +            state.transform = layer->transform();
> +            state.geoOrientation = layer->geometryOrientation();
> +            state.contentsOrientation = layer->contentsOrientation();
> +            state.opacity = layer->opacity();
> +            state.contentsRect = layer->contentsRect();
> +            state.preserves3D = layer->preserves3D();
> +            state.masksToBounds = layer->masksToBounds();
> +            state.drawsContent = layer->drawsContent();
> +            state.contentsOpaque = layer->contentsOpaque();
> +            state.backfaceVisibility = layer->backfaceVisibility();
> +            currentContent.pixmap = pendingContent.pixmap;
> +            currentContent.contentType = pendingContent.contentType;
> +            currentContent.backgroundColor = pendingContent.backgroundColor;
> +            currentContent.backgroundColorValid = pendingContent.backgroundColorValid;
> +            currentContent.regionToUpdate |= pendingContent.regionToUpdate;
> +            currentContent.updateAll |= pendingContent.updateAll;
> +            currentContent.contentsBackgroundColor = pendingContent.contentsBackgroundColor;
> +            pendingContent.regionToUpdate = QRegion();
> +            pendingContent.updateAll = false;
> +            possibleChanges = NoChanges;
> +        }
> +    }
> +
> +    if (recursive) {
> +        QList<QGraphicsItem*> ch = childItems();
> +        foreach (QGraphicsItem* item, ch) {
> +            GraphicsLayerQtPrivate* wdg = qobject_cast<GraphicsLayerQtPrivate*>(item->toGraphicsObject());
> +            if (wdg)
> +                wdg->flushChanges(true);
> +        }
> +    }
> +}
> +
> +void GraphicsLayerQtPrivate::notifyAnimationStarted()
> +{
> +    // WebCore notifies javascript when the animation starts
> +    // here we're letting it know
> +    client->notifyAnimationStarted(layer, WTF::currentTime());
> +}
> +
> +
> +GraphicsLayerQt::GraphicsLayerQt(GraphicsLayerClient* client)
> +        :GraphicsLayer(client)

space missing after :

> +        , qtLayer(new GraphicsLayerQtPrivate(this, client))

What is a qtLayer? find a better name

> +{
> +}
> +
> +GraphicsLayerQt::~GraphicsLayerQt()
> +{
> +    delete qtLayer;
> +}
> +
> +
> +PassOwnPtr<GraphicsLayer> GraphicsLayer::create(GraphicsLayerClient* client)
> +{
> +    return new GraphicsLayerQt(client);
> +}
> +
> +GraphicsLayer::CompositingCoordinatesOrientation GraphicsLayer::compositingCoordinatesOrientation()
> +{
> +    return CompositingCoordinatesTopDown;
> +}
> +
> +void GraphicsLayerQt::setNeedsDisplay()

setNeedsToBeDisplayed? or does it really need a display? Maybe the naming can be improved

> +{
> +    qtLayer->pendingContent.updateAll = true;
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::DisplayChange);
> +}
> +void GraphicsLayerQt::setNeedsDisplayInRect(const FloatRect& r)
> +{
> +    qtLayer->pendingContent.regionToUpdate|= QRectF(r).toRect().adjusted(-1, -1, 1, 1);
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::DisplayChange);
> +}
> +
> +void GraphicsLayerQt::setName(const String& name)
> +{
> +    qtLayer->setObjectName(QString(name));
> +    GraphicsLayer::setName(name);
> +}
> +bool GraphicsLayerQt::setChildren(const Vector<GraphicsLayer*>& children)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::ChildrenChange);
> +    return GraphicsLayer::setChildren(children);
> +}
> +
> +void GraphicsLayerQt::addChild(GraphicsLayer* layer)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::ChildrenChange);
> +    GraphicsLayer::addChild(layer);
> +}
> +void GraphicsLayerQt::addChildAtIndex(GraphicsLayer* layer, int index)
> +{
> +    GraphicsLayer::addChildAtIndex(layer, index);
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::ChildrenChange);
> +}
> +void GraphicsLayerQt::addChildAbove(GraphicsLayer* layer, GraphicsLayer* sibling)
> +{
> +     GraphicsLayer::addChildAbove(layer, sibling);
> +     qtLayer->noteChange(GraphicsLayerQtPrivate::ChildrenChange);
> +}
> +void GraphicsLayerQt::addChildBelow(GraphicsLayer* layer, GraphicsLayer* sibling)
> +{
> +
> +    GraphicsLayer::addChildBelow(layer, sibling);
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::ChildrenChange);
> +}
> +bool GraphicsLayerQt::replaceChild(GraphicsLayer* oldChild, GraphicsLayer* newChild)
> +{
> +    if (GraphicsLayer::replaceChild(oldChild, newChild)) {
> +       qtLayer->noteChange(GraphicsLayerQtPrivate::ChildrenChange);
> +       return true;
> +    } else
> +        return false;

indentation problems here

> +}
> +void GraphicsLayerQt::removeFromParent()
> +{
> +    if (parent())
> +        qtLayer->noteChange(GraphicsLayerQtPrivate::ParentChange);
> +    GraphicsLayer::removeFromParent();
> +}
> +void GraphicsLayerQt::setMaskLayer(GraphicsLayer* layer)
> +{
> +    GraphicsLayer::setMaskLayer(layer);
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::MaskLayerChange);
> +}
> +void GraphicsLayerQt::setPosition(const FloatPoint& p)
> +{
> +    if (position() != p)
> +       qtLayer->noteChange(GraphicsLayerQtPrivate::PositionChange);
> +    GraphicsLayer::setPosition(p);
> +}
> +void GraphicsLayerQt::setAnchorPoint(const FloatPoint3D& p)
> +{
> +    if (anchorPoint() != p)
> +        qtLayer->noteChange(GraphicsLayerQtPrivate::AnchorPointChange);
> +    GraphicsLayer::setAnchorPoint(p);
> +}
> +void GraphicsLayerQt::setSize(const FloatSize& size)
> +{
> +    if (this->size() != size)
> +        qtLayer->noteChange(GraphicsLayerQtPrivate::SizeChange);
> +    GraphicsLayer::setSize(size);
> +}
> +void GraphicsLayerQt::setTransform(const TransformationMatrix& t)
> +{
> +    if (!qtLayer->transformAnimationRunning && transform() != t)
> +       qtLayer->noteChange(GraphicsLayerQtPrivate::TransformChange);
> +    GraphicsLayer::setTransform(t);
> +}
> +void GraphicsLayerQt::setChildrenTransform(const TransformationMatrix& t)
> +{
> +    GraphicsLayer::setChildrenTransform(t);
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::ChildrenTransformChange);
> +}
> +void GraphicsLayerQt::setPreserves3D(bool b)
> +{
> +    if (b != preserves3D());
> +       qtLayer->noteChange(GraphicsLayerQtPrivate::Preserves3DChange);
> +    GraphicsLayer::setPreserves3D(b);
> +}
> +void GraphicsLayerQt::setMasksToBounds(bool b)
> +{
> +    GraphicsLayer::setMasksToBounds(b);
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::MasksToBoundsChange);
> +}
> +void GraphicsLayerQt::setDrawsContent(bool b)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::DrawsContentChange);
> +    GraphicsLayer::setDrawsContent(b);
> +}
> +void GraphicsLayerQt::setBackgroundColor(const Color& c)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::BackgroundColorChange);
> +    qtLayer->pendingContent.backgroundColor = c;
> +    qtLayer->pendingContent.backgroundColorValid = true;
> +    GraphicsLayer::setBackgroundColor(c);
> +}
> +void GraphicsLayerQt::clearBackgroundColor()
> +{
> +    qtLayer->pendingContent.backgroundColorValid = false;
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::BackgroundColorChange);
> +    GraphicsLayer::clearBackgroundColor();
> +}
> +void GraphicsLayerQt::setContentsOpaque(bool b)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::ContentsOpaqueChange);
> +    GraphicsLayer::setContentsOpaque(b);
> +}
> +void GraphicsLayerQt::setBackfaceVisibility(bool b)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::BackfaceVisibilityChange);
> +    GraphicsLayer::setBackfaceVisibility(b);
> +}
> +

I wonder if adding documentation to some of these functions would be a help

> +void GraphicsLayerQt::setOpacity(float o)
> +{
> +    if (!qtLayer->opacityAnimationRunning && opacity() != o)
> +       qtLayer->noteChange(GraphicsLayerQtPrivate::OpacityChange);
> +    GraphicsLayer::setOpacity(o);
> +}
> +void GraphicsLayerQt::setContentsRect(const IntRect& r)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::ContentsRectChange);
> +    GraphicsLayer::setContentsRect(r);
> +}
> +
> +void GraphicsLayerQt::setContentsToImage(Image* image)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::ContentChange);

informChange ???? or notifyChange

> +    qtLayer->pendingContent.contentType = GraphicsLayerQtPrivate::HTMLContentType;
> +    GraphicsLayer::setContentsToImage(image);
> +    if (image) {
> +        QPixmap* pxm = image->nativeImageForCurrentFrame();
> +        if (pxm) {
> +            qtLayer->pendingContent.pixmap = *pxm;
> +            qtLayer->pendingContent.contentType = GraphicsLayerQtPrivate::PixmapContentType;
> +            return;
> +        }        
> +    }
> +    qtLayer->pendingContent.pixmap = QPixmap();
> +}
> +
> +void GraphicsLayerQt::setContentsBackgroundColor(const Color& color)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::ContentChange);
> +    qtLayer->pendingContent.contentType = GraphicsLayerQtPrivate::ColorContentType;
> +    qtLayer->pendingContent.contentsBackgroundColor = QColor(color);
> +    GraphicsLayer::setContentsBackgroundColor(color);
> +}
> +
> +void GraphicsLayerQt::setGeometryOrientation(CompositingCoordinatesOrientation orientation)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::GeometryOrientationChange);
> +    GraphicsLayer::setGeometryOrientation(orientation);
> +}
> +
> +void GraphicsLayerQt::setContentsOrientation(CompositingCoordinatesOrientation orientation)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::ContentsOrientationChange);
> +    GraphicsLayer::setContentsOrientation(orientation);
> +}
> +
> +void GraphicsLayerQt::distributeOpacity(float o)
> +{
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::OpacityChange);
> +    qtLayer->state.distributeOpacity = true;
> +}
> +
> +float GraphicsLayerQt::accumulatedOpacity() const
> +{
> +    return qtLayer->effectiveOpacity();
> +}
> +
> +void GraphicsLayerQt::syncCompositingState()
> +{
> +    qtLayer->flushChanges();
> +    GraphicsLayer::syncCompositingState();
> +}
> +
> +NativeLayer GraphicsLayerQt::nativeLayer() const
> +{
> +    return qtLayer;
> +}
> +
> +PlatformLayer* GraphicsLayerQt::platformLayer() const
> +{
> +    return qtLayer;
> +}
> +
> +// now we start dealing with WebCore animations translated to Qt animations
> +
> +template <typename T>
> +struct KeyframeValueQt {
> +    TimingFunction timingFunction;
> +    T value;
> +};
> +
> +// we copy this from the WebCore animation code

from which file... would be nice to add in the comment

> +static inline double solveEpsilon(double duration)
> +{
> +    return 1.0 / (200.0 * duration);
> +}
> +
> +static inline double solveCubicBezierFunction(qreal p1x, qreal p1y, qreal p2x, qreal p2y, double t, double duration)
> +{
> +    UnitBezier bezier(p1x, p1y, p2x, p2y);
> +    return bezier.solve(t, solveEpsilon(duration));
> +}
> +
> +// we want the timing function to be as close as possible to what the web-developer
> +// intended, so we're using the same function used by WebCore when compositing is disabled
> +static inline qreal applyTimingFunction(const TimingFunction& timingFunction, qreal progress, int duration)
> +{
> +        if (timingFunction.type() == LinearTimingFunction)
> +            return progress;
> +        if (timingFunction.type() == CubicBezierTimingFunction) {
> +            return solveCubicBezierFunction(timingFunction.x1(),
> +                                            timingFunction.y1(),
> +                                            timingFunction.x2(),
> +                                            timingFunction.y2(),
> +                                            double(progress), double(duration) / 1000);
> +        }
> +        return progress;
> +}

So with compositing you never get the same timing function as you use the Qt animation framework??? What if I use keyframes?

> +
> +// helper functions to safely get a value out of WebCore's AnimationValue*
> +static void webkitAnimationToQtAnimationValue(const AnimationValue* animationValue, TransformOperations& transformOperations)
> +{
> +        if (animationValue) {
> +            const TransformOperations* ops = static_cast<const TransformAnimationValue*>(animationValue)->value();
> +            transformOperations = ops?*ops:TransformOperations();

spacing missing

> +        } else
> +            transformOperations = TransformOperations();
> +
> +}
> +
> +static void webkitAnimationToQtAnimationValue(const AnimationValue* animationValue, qreal& realValue)
> +{
> +        if (animationValue)
> +            realValue = static_cast<const FloatAnimationValue*>(animationValue)->value();
> +        else
> +            realValue = 0;
> +}
> +
> +// we put a bit of the functionality in a base class to allow casting and to save some code size
> +class AnimationQtBase : public QAbstractAnimation {
> +public:
> +    AnimationQtBase(GraphicsLayerQtPrivate* aLayer
> +                    , const KeyframeValueList& values
> +                    , const IntSize& aBoxSize
> +                    , const Animation* anim
> +                    , const QString & kfName
> +                    )

We don't wrap function arguments

> +            : QAbstractAnimation(0)
> +            , target(aLayer)
> +            , boxSize(aBoxSize)
> +            , mDuration(anim->duration() * 1000)
> +            , isAlternate(anim->direction() == Animation::AnimationDirectionAlternate)
> +            , webkitPropertyID(values.property())
> +            , keyframesName(kfName)
> +    {
> +    }
> +
> +    virtual void updateState(QAbstractAnimation::State newState, QAbstractAnimation::State oldState)
> +    {
> +        QAbstractAnimation::updateState(newState, oldState);
> +
> +        if (newState == Running && oldState == Stopped)
> +        // for some reason I have do this asynchronously - or the animation won't work
> +            QTimer::singleShot(0, target.data(), SLOT(notifyAnimationStarted()));

When adding comments inside an if, you need to add {} according to the style guide

> +    }
> +
> +    virtual int duration() const { return mDuration; }

m_duration!

> +
> +    QWeakPointer<GraphicsLayerQtPrivate> target;
> +    IntSize boxSize;
> +    int mDuration;

again

> +    bool isAlternate;
> +    AnimatedPropertyID webkitPropertyID;

webCorePropertyID ?

> +    QString keyframesName;
> +};
> +
> +// we'd rather have a templatized QAbstractAnimation than QPropertyAnimation / QVariantAnimation;
> +// Since we know the types that we're dealing with, the QObject/QProperty/QVariant abstraction
> +// buys us very little in this case, for too much overhead
> +template <typename T>
> +class AnimationQt : public AnimationQtBase {
> +
> +public:
> +    AnimationQt(GraphicsLayerQtPrivate* aLayer
> +                , const KeyframeValueList& values
> +                , const IntSize& aBoxSize
> +                , const Animation* anim
> +                , const QString & name)

again!

> +            :AnimationQtBase(aLayer, values, aBoxSize, anim, name)

Space missing before :

> +    {
> +        // copying those WebCore structures is not trivial, we have to do it like this
> +        for (size_t i = 0; i < values.size(); ++i) {
> +            const AnimationValue* animVal = values.at(i);
> +            KeyframeValueQt<T> kfv;
> +            if (animVal->timingFunction())
> +                kfv.timingFunction = *animVal->timingFunction();

kfv? bad name

> +            webkitAnimationToQtAnimationValue(animVal, kfv.value);
> +            keyframeValues[animVal->keyTime()] = kfv;
> +        }
> +    }
> +
> +protected:
> +
> +    // this is the part that differs between animated properties
> +    virtual void applyFrame(const T& fromValue, const T& toValue, qreal progress) = 0;
> +
> +    virtual void updateCurrentTime(int currentTime)
> +    {
> +        if (!target)
> +            return;
> +
> +        qreal progress = qreal(currentLoopTime()) / duration();
> +
> +        if (isAlternate && currentLoop()%2)
> +            progress = 1-progress;
> +
> +        if (keyframeValues.isEmpty())
> +            return;
> +
> +        // we find the current from-to keyframes in our little map
> +        typename QMap<qreal, KeyframeValueQt<T> >::iterator it = keyframeValues.find(progress);
> +        if (it == keyframeValues.end())
> +            it = keyframeValues.lowerBound(progress)-1;
> +        if (it == keyframeValues.end())
> +            it = keyframeValues.begin();
> +
> +        typename QMap<qreal, KeyframeValueQt<T> >::iterator it2 = it+1;
> +        if (it2 == keyframeValues.end())
> +            it2 = keyframeValues.begin();

if end, begin? maybe add a comment

> +        const KeyframeValueQt<T>& fromKeyframe = it.value();
> +        const KeyframeValueQt<T>& toKeyframe = it2.value();
> +
> +        const TimingFunction& timingFunc = fromKeyframe.timingFunction;
> +        const T& fromValue = fromKeyframe.value;
> +        const T& toValue = toKeyframe.value;
> +
> +        // now we have a source keyframe, origin keyframe and a timing function
> +        // we can now process the progress and apply the frame
> +        qreal normalizedProgress = (it.key() == it2.key()) ? 0 : (progress - it.key()) / (it2.key() - it.key());
> +        normalizedProgress = applyTimingFunction(timingFunc, normalizedProgress, duration() / 1000);
> +        applyFrame(fromValue, toValue, normalizedProgress);
> +    }
> +
> +    QMap<qreal, KeyframeValueQt<T> > keyframeValues;
> +};
> +
> +class TransformAnimationQt : public AnimationQt<TransformOperations> {
> +public:
> +    TransformAnimationQt(GraphicsLayerQtPrivate* aLayer,
> +                                     const KeyframeValueList& values, const IntSize& boxSize, const Animation* anim, const QString & name)

put on one line.

> +                :AnimationQt<TransformOperations>(aLayer, values, boxSize, anim, name)

Space missing

> +    {
> +    }
> +
> +    ~TransformAnimationQt()
> +    {
> +        // this came up during the compositing/animation LayoutTests
> +        // when the animation dies, the transform has to go back to default
> +        target.data()->setBaseTransform(QTransform(target.data()->layer->transform()));
> +    }
> +
> +    // the idea is that we let WebCore manage the transform-operations
> +    // and Qt just manages the animation heartbeat and the bottom-line QTransform
> +    // we get the performance not by using QTransform instead of TransformationMatrix, but by proper caching of
> +    // items that are expensive for WebCore to render. We want the rest to be as close to WebCore's idea as possible.
> +    virtual void applyFrame(const TransformOperations& fromOps, const TransformOperations& toOps, qreal progress)

maybe source, target instead of fromOps, toOps

> +    {
> +        TransformationMatrix tf;
> +
> +        // this looks simple but is really tricky to get right. Use caution.
> +        for (size_t i = 0; i < toOps.size(); ++i)
> +            toOps.operations()[i]->blend(fromOps.at(i), progress)->apply(tf, boxSize);
> +
> +        target.data()->setBaseTransform(QTransform(tf));

tf? bad name

> +    }
> +
> +    virtual void updateState(QAbstractAnimation::State newState, QAbstractAnimation::State oldState)
> +    {
> +        AnimationQtBase::updateState(newState, oldState);
> +        if (!target)
> +            return;
> +        target.data()->flushChanges(true);
> +
> +        // to increase FPS, we use a less accurate caching mechanism while animation is going on
> +        // this is a UX choice that should probably be customizable
> +        if (newState == QAbstractAnimation::Running) {
> +            target.data()->transformAnimationRunning = true;
> +            if (target.data()->cacheMode() == QGraphicsItem::DeviceCoordinateCache)
> +                target.data()->setCacheMode(QGraphicsItem::ItemCoordinateCache);
> +        } else {
> +            target.data()->transformAnimationRunning = false;
> +            if (target.data()->cacheMode() == QGraphicsItem::ItemCoordinateCache)
> +                target.data()->setCacheMode(QGraphicsItem::DeviceCoordinateCache);
> +        }
> +    }
> +};
> +
> +class OpacityAnimationQt : public AnimationQt<qreal> {
> +public:
> +    OpacityAnimationQt(GraphicsLayerQtPrivate* aLayer,
> +                                     const KeyframeValueList& values, const IntSize& boxSize, const Animation* anim, const QString & name)

One line

> +                :AnimationQt<qreal>(aLayer, values, boxSize, anim, name)
> +    {
> +    }
> +
> +    virtual void applyFrame(const qreal& fromValue, const qreal& toValue, qreal progress)
> +    {
> +        target.data()->setOpacity(qMin<qreal>(qMax<qreal>(fromValue + (toValue-fromValue)*progress, 0), 1));
> +    }
> +
> +    virtual void updateState(QAbstractAnimation::State newState, QAbstractAnimation::State oldState)
> +    {
> +        QAbstractAnimation::updateState(newState, oldState);
> +
> +        if (!target)
> +            return;
> +
> +        if (newState == QAbstractAnimation::Running)
> +            target.data()->opacityAnimationRunning = true;
> +        else
> +            target.data()->opacityAnimationRunning = false;
> +    }
> +};
> +
> +bool GraphicsLayerQt::addAnimation(const KeyframeValueList& values, const IntSize& boxSize, const Animation* anim, const String& keyframesName, double timeOffset)
> +{
> +    if (!anim->duration() || !anim->iterationCount())
> +        return false;
> +
> +    QAbstractAnimation* newAnim;
> +
> +    switch (values.property()) {
> +    case AnimatedPropertyOpacity:
> +        newAnim = new OpacityAnimationQt(qtLayer, values, boxSize, anim, keyframesName);
> +        break;
> +    case AnimatedPropertyWebkitTransform:
> +        newAnim = new TransformAnimationQt(qtLayer, values, boxSize, anim, keyframesName);
> +        break;
> +    default:
> +        return false;
> +    }
> +
> +    // we make sure WebCore::Animation and QAnimation are on the same terms
> +    newAnim->setLoopCount(anim->iterationCount());
> +    qtLayer->animations.append(QWeakPointer<QAbstractAnimation>(newAnim));
> +    QObject::connect(&qtLayer->suspendTimer, SIGNAL(timeout()), newAnim, SLOT(resume()));
> +    timeOffset += anim->delay();
> +
> +    // flush now or flicker...
> +    qtLayer->flushChanges(false);
> +
> +    if (timeOffset)
> +        QTimer::singleShot(timeOffset*1000, newAnim, SLOT(start()));

space *

> +    else
> +        newAnim->start();
> +
> +    QObject::connect(newAnim, SIGNAL(finished()), newAnim, SLOT(deleteLater()));
> +
> +    return true;
> +}
> +
> +void GraphicsLayerQt::removeAnimationsForProperty(AnimatedPropertyID id)
> +{
> +    for (QList<QWeakPointer<QAbstractAnimation> >::iterator it = qtLayer->animations.begin();
> +            it != qtLayer->animations.end(); ++it) {
> +
> +        if (*it) {
> +            AnimationQtBase* anim = static_cast<AnimationQtBase*>(it->data());
> +            if (anim && anim->webkitPropertyID == id) {
> +                delete anim;
> +                it = qtLayer->animations.erase(it);
> +                --it;
> +            }
> +        }
> +    }
> +}
> +
> +void GraphicsLayerQt::removeAnimationsForKeyframes(const String& kfName)
> +{
> +    for (QList<QWeakPointer<QAbstractAnimation> >::iterator it = qtLayer->animations.begin(); it != qtLayer->animations.end(); ++it) {
> +        if (*it) {
> +            AnimationQtBase* anim = static_cast<AnimationQtBase*>((*it).data());
> +            if (anim && anim->keyframesName == QString(kfName)) {
> +                (*it).data()->deleteLater();
> +                it = qtLayer->animations.erase(it);
> +                --it;
> +            }
> +        }
> +    }
> +}
> +
> +void GraphicsLayerQt::pauseAnimation(const String& kfName, double timeOfs)
> +{
> +    for (QList<QWeakPointer<QAbstractAnimation> >::iterator it = qtLayer->animations.begin(); it != qtLayer->animations.end(); ++it) {
> +        if (*it) {
> +            AnimationQtBase* anim = static_cast<AnimationQtBase*>((*it).data());
> +            if (anim && anim->keyframesName == QString(kfName))

kfName ..

> +                QTimer::singleShot(timeOfs*1000, anim, SLOT(pause()));
> +        }
> +    }
> +}
> +
> +void GraphicsLayerQt::suspendAnimations(double time)
> +{
> +    if (qtLayer->suspendTimer.isActive()) {
> +        qtLayer->suspendTimer.stop();
> +        qtLayer->suspendTimer.start(time*1000);

space *

> +    } else {
> +        for (QList<QWeakPointer<QAbstractAnimation> >::iterator it = qtLayer->animations.begin(); it != qtLayer->animations.end(); ++it) {
> +            QAbstractAnimation* anim = it->data();
> +            if (anim)
> +                anim->pause();
> +        }
> +    }
> +}
> +
> +void GraphicsLayerQt::resumeAnimations()
> +{
> +    if (qtLayer->suspendTimer.isActive()) {
> +        qtLayer->suspendTimer.stop();
> +        for (QList<QWeakPointer<QAbstractAnimation> >::iterator it = qtLayer->animations.begin(); it != qtLayer->animations.end(); ++it) {
> +            QAbstractAnimation* anim = (*it).data();
> +            if (anim)
> +                anim->resume();
> +        }
> +    }
> +}
> +
> +
> +}
> +
> +#include <GraphicsLayerQt.moc>
> Index: WebCore/platform/graphics/qt/GraphicsLayerQt.h
> ===================================================================
> --- WebCore/platform/graphics/qt/GraphicsLayerQt.h	(revision 0)
> +++ WebCore/platform/graphics/qt/GraphicsLayerQt.h	(revision 0)
> @@ -0,0 +1,83 @@
> +/*
> +    Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies)
> +
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public
> +    License as published by the Free Software Foundation; either
> +    version 2 of the License, or (at your option) any later version.
> +
> +    This library is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +    Library General Public License for more details.
> +
> +    You should have received a copy of the GNU Library General Public License
> +    along with this library; see the file COPYING.LIB.  If not, write to
> +    the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> +    Boston, MA 02110-1301, USA.
> +*/
> +
> +#ifndef GraphicsLayerQt_h
> +#define GraphicsLayerQt_h
> +
> +#include "GraphicsLayer.h"
> +#include "GraphicsLayerClient.h"
> +
> +namespace WebCore {
> +class GraphicsLayerQtPrivate;
> +class GraphicsLayerQt : public GraphicsLayer {
> +    friend class GraphicsLayerQtPrivate;
> +public:
> +    GraphicsLayerQt(GraphicsLayerClient*);
> +    virtual void setNeedsDisplay();
> +    // mark the given rect (in layer coords) as needing dispay. Never goes deep.
> +    virtual void setNeedsDisplayInRect(const FloatRect&);
> +    virtual NativeLayer nativeLayer() const;
> +    virtual PlatformLayer* platformLayer() const;
> +
> +    virtual ~GraphicsLayerQt();
> +    void setParent(GraphicsLayer* layer);
> +    virtual void setName(const String& name);
> +    virtual bool setChildren(const Vector<GraphicsLayer*>&);
> +    virtual void addChild(GraphicsLayer*);
> +    virtual void addChildAtIndex(GraphicsLayer*, int index);
> +    virtual void addChildAbove(GraphicsLayer* layer, GraphicsLayer* sibling);
> +    virtual void addChildBelow(GraphicsLayer* layer, GraphicsLayer* sibling);
> +    virtual bool replaceChild(GraphicsLayer* oldChild, GraphicsLayer* newChild);
> +    virtual void removeFromParent();
> +    virtual void setMaskLayer(GraphicsLayer* layer);
> +    virtual void setPosition(const FloatPoint& p);
> +    virtual void setAnchorPoint(const FloatPoint3D& p);
> +    virtual void setSize(const FloatSize& size);
> +    virtual void setTransform(const TransformationMatrix& t);
> +    virtual void setChildrenTransform(const TransformationMatrix& t);
> +    virtual void setPreserves3D(bool b);
> +    virtual void setMasksToBounds(bool b);
> +    virtual void setDrawsContent(bool b);
> +    virtual void setBackgroundColor(const Color&);
> +    virtual void clearBackgroundColor();
> +    virtual void setContentsOpaque(bool b);
> +    virtual void setBackfaceVisibility(bool b);
> +    virtual void setOpacity(float opacity);
> +    virtual void setContentsRect(const IntRect& r);
> +    virtual bool addAnimation(const KeyframeValueList&, const IntSize& /*boxSize*/, const Animation*, const String& /*keyframesName*/, double /*timeOffset*/);

why the /* */ ???? just write it out

> +    virtual void removeAnimationsForProperty(AnimatedPropertyID);
> +    virtual void removeAnimationsForKeyframes(const String& /* keyframesName */);
> +    virtual void pauseAnimation(const String& /* keyframesName */, double /*timeOffset*/);
> +    virtual void suspendAnimations(double time);
> +    virtual void resumeAnimations();
> +    virtual void setContentsToImage(Image*);
> +    virtual void setContentsBackgroundColor(const Color&);
> +    virtual void setGeometryOrientation(CompositingCoordinatesOrientation orientation);
> +    virtual void setContentsOrientation(CompositingCoordinatesOrientation orientation);
> +    virtual void distributeOpacity(float);
> +    virtual float accumulatedOpacity() const;
> +    virtual void syncCompositingState();
> +private:
> +    GraphicsLayerQtPrivate* qtLayer;
> +
> +};
> +
> +
> +}
> +#endif // GraphicsLayerQt_h
> Index: WebCore/platform/graphics/GraphicsLayer.h
> ===================================================================
> --- WebCore/platform/graphics/GraphicsLayer.h	(revision 53364)
> +++ WebCore/platform/graphics/GraphicsLayer.h	(working copy)
> @@ -59,6 +59,10 @@
>  typedef WKCACFLayer PlatformLayer;
>  typedef void* NativeLayer;
>  }
> +#elif PLATFORM(QT)
> +class QGraphicsItem;
> +typedef QGraphicsItem PlatformLayer;
> +typedef QGraphicsItem* NativeLayer;
>  #else
>  typedef void* PlatformLayer;
>  typedef void* NativeLayer;
> 
> Index: WebKit.pri
> ===================================================================
> --- WebKit.pri	(revision 53364)
> +++ WebKit.pri	(working copy)
> @@ -45,6 +45,7 @@
>      }
>      DEPENDPATH += $$PWD/WebKit/qt/Api
>  }
> +DEFINES += WTF_USE_ACCELERATED_COMPOSITING
>  
>  !mac:!unix|symbian {
>      DEFINES += USE_SYSTEM_MALLOC

[Antti Koivisto]

Comment on attachment 46808 [details]
accelerated compositing in Qt: with massive style improvements and in-code comments

Looks great! Some style issues and a few questions inline.


> Index: WebKit/qt/QGVLauncher/main.cpp
> +#if USE(ACCELERATED_COMPOSITING)
> +        rootGraphicsLayer(0),
> +        overlay(new QGraphicsWebViewOverlay(parent)),
> +        shouldSync(true)
> +#endif

Should the overlay be created only if the composition is enabled in settings? Or perhaps it doesn't hurt even if it is not strictly needed?

>      // If the page had no title, still make sure it gets the signal
> @@ -83,11 +206,18 @@
>  void QGraphicsWebViewPrivate::scroll(int dx, int dy, const QRect& rectToScroll)
>  {
>      q->scroll(qreal(dx), qreal(dy), QRectF(rectToScroll));
> +#if USE(ACCELERATED_COMPOSITING)
> +    updateCompositingScrollPosition();
> +//    QApplication::instance()->sendPostedEvents();
> +#endif
>  }

Some commented out code here.

>  }
>  
> +

don't add the empty line


> Index: WebKit/qt/Api/qgraphicswebview.h
> ===================================================================
> --- WebKit/qt/Api/qgraphicswebview.h	(revision 53364)
> +++ WebKit/qt/Api/qgraphicswebview.h	(working copy)
> @@ -105,7 +105,6 @@
>      void iconChanged();
>      void statusBarMessage(const QString& message);
>      void linkClicked(const QUrl&);
> -
>  protected:

don't remove the empty line

>      virtual void mousePressEvent(QGraphicsSceneMouseEvent*);
>      virtual void mouseDoubleClickEvent(QGraphicsSceneMouseEvent*);
> @@ -134,7 +133,9 @@
>  
>  private:
>      Q_PRIVATE_SLOT(d, void _q_doLoadFinished(bool success))
> -
> +    // we don't want to change the moc based on USE() macro, so this function is here
> +    // but will be empty if ACCLERATED_COMPOSITING is disabled
> +    Q_PRIVATE_SLOT(d, void syncLayers())
>      QGraphicsWebViewPrivate* const d;

keep the empty line after private slots


>  #include <QRect>
> -
> +class QGraphicsItem;
>  class QWebPageClient {

don't remove the empty line

> +
> +namespace WebCore {
> +
> +
> +class GraphicsLayerQtPrivate : public QGraphicsObject {

one empty line only after namespace

> +    Q_OBJECT
> +
> +public:
> +    // this set of flags help us defer which properties of the layer have been
> +    // modified by the compositor, so we can know what to look for in the next flush
> +    enum PossibleChange {
> +        NoChanges       =           0,
> +        ChildrenChange  =           (1L << 1),
> +        MaskLayerChange =           (1L << 2),
> +        PositionChange  =           (1L << 3),
> +        AnchorPointChange =         (1L << 4),
> +        SizeChange      =           (1L << 5),
> +        TransformChange =           (1L << 6),
> +        ContentChange   =           (1L << 7),
> +        GeometryOrientationChange = (1L << 8),
> +        ContentsOrientationChange = (1L << 9),
> +        OpacityChange   =           (1L << 10),
> +        ContentsRectChange =        (1L << 11),
> +        Preserves3DChange =         (1L << 12),
> +        MasksToBoundsChange =       (1L << 13),
> +        DrawsContentChange =        (1L << 14),
> +        ContentsOpaqueChange =      (1L << 15),
> +        BackfaceVisibilityChange =  (1L << 16),
> +        ChildrenTransformChange =   (1L << 17),
> +        DisplayChange =             (1L << 18),
> +        BackgroundColorChange =     (1L << 19),
> +        ParentChange =              (1L << 20),
> +        DistributesOpacityChange =  (1L << 21)
> +    };

What does word "possible" communicate here? Could it be removed?
Maybe call these *ChangeMask? 
Placement of = is inconsistent. 
No need for 1L << I think, 1 << should do.

> +
> +    // let the compositor-API tell us which properties were changed
> +    void noteChange(PossibleChange pc);

notifyChanged()? "note" sounds bit strange.

> +
> +    ContentData pendingContent;
> +    ContentData currentContent;
> +
> +    PossibleChange possibleChanges;
> +
> +    QSizeF size;
> +    QList<QWeakPointer<QAbstractAnimation> > animations;
> +    QTimer suspendTimer;

WebKit style is to use m_ prefix for all member variables (pure structs may be an exception).

> +
> +GraphicsLayerQtPrivate::~GraphicsLayerQtPrivate()
> +{
> +    // the compositor manages item lifecycle - we don't want the graphics-view
> +    // system to automatically delete our items
> +    foreach (QGraphicsItem* i, childItems()) {

I would prefer not to have foreach and similar Qt macros in WebKit code. It may unnecessarily confuse non-Qt developers.

> +
> +void GraphicsLayerQtPrivate::setBaseTransform(const QTransform& t)
> +{
> +    if (layer) {
> +        // webkit has relative-to-size originPoint, graphics-view has a pixel originPoint
> +        // here we convert

Please use early return style:

if (!layer)
   return;
...

> +        QPointF originTranslate(
> +                layer->anchorPoint().x()*layer->size().width(), layer->anchorPoint().y()*layer->size().height());
> +
> +        resetTransform();
> +
> +        // we have to manage this ourselves because QGraphicsView's transformOrigin is incomplete
> +        translate(originTranslate.x(), originTranslate.y());
> +        setTransform(t, true);
> +        translate(-originTranslate.x(), -originTranslate.y());
> +        baseTransform = t;
> +    }
> +}
> +
> +QPainterPath GraphicsLayerQtPrivate::opaqueArea() const
> +{
> +    QPainterPath pp;

Please use full words for variable names.

> +
> +void GraphicsLayerQtPrivate::paint(QPainter* painter, const QStyleOptionGraphicsItem* option, QWidget* widget)
> +{
> +    if (state.maskLayer && state.maskLayer->platformLayer()) {
> +        // maybe this shouldn't be in paint (rendering the mask-layer)
> +        // though it does work for now

You could use FIXME for comments on things that may need to be looked again later.

> +void GraphicsLayerQtPrivate::drawContents(QPainter* painter, const QRectF& r, bool mask)
> +{
> +    // webkit uses IntRect, we use QRectF. we don't want missing pixels    
> +    QRect rect = r.toRect().adjusted(-1, -1, 1, 1);

This looks bit hacky, perhaps you should add a FIXME?

> +    if (currentContent.contentType != HTMLContentType && !state.contentsRect.isEmpty())
> +        rect = rect.intersected(state.contentsRect);
> +
> +    if (currentContent.backgroundColorValid)
> +        painter->fillRect(r, QColor(currentContent.backgroundColor));


Why is backgroundColorValid bit needed? Why wouldn't currentContent.backgroundColor.isValid() work?


> +void GraphicsLayerQtPrivate::noteChange(PossibleChange pc)
> +{
> +    if (!this)
> +        return;
> +
> +    possibleChanges = static_cast<PossibleChange>(static_cast<int>(possibleChanges) | static_cast<int>(pc));

This kind if stuff becomes more understandable when you use m_ prefixes for member variables (and full words for local variable names).

> +
> +void GraphicsLayerQtPrivate::flushChanges(bool recursive)
> +{
> +    // this is the bulk of the work. understanding what the compositor is trying to achieve,
> +    // what graphics-view can do, and trying to find a sane common-grounds
> +    if (layer) {
> +        if (possibleChanges != NoChanges) {
> +

Please use early return to avoid indenting the whole function (or possibly goto to end actions).

> +
> +    if (recursive) {
> +        QList<QGraphicsItem*> ch = childItems();
> +        foreach (QGraphicsItem* item, ch) {
> +            GraphicsLayerQtPrivate* wdg = qobject_cast<GraphicsLayerQtPrivate*>(item->toGraphicsObject());
> +            if (wdg)
> +                wdg->flushChanges(true);
> +        }
> +    }

Would it be helpful to have a PossibleChange bit that tells if any child layer needs flushing? That way you could avoid recursing to children unnecessarily.


> +
> +GraphicsLayerQt::GraphicsLayerQt(GraphicsLayerClient* client)
> +        :GraphicsLayer(client)
> +        , qtLayer(new GraphicsLayerQtPrivate(this, client))
> +{
> +}

Double indentation.

> +
> +GraphicsLayerQt::~GraphicsLayerQt()
> +{
> +    delete qtLayer;
> +}

Perhaps you could use OwnPtrs?

> +void GraphicsLayerQt::setNeedsDisplay()
> +{
> +    qtLayer->pendingContent.updateAll = true;
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::DisplayChange);
> +}
> +void GraphicsLayerQt::setNeedsDisplayInRect(const FloatRect& r)
> +{
> +    qtLayer->pendingContent.regionToUpdate|= QRectF(r).toRect().adjusted(-1, -1, 1, 1);
> +    qtLayer->noteChange(GraphicsLayerQtPrivate::DisplayChange);
> +}

Could the separate updateAll flag be removed by simply invalidating full frame sized region instead?

> +void GraphicsLayerQt::setName(const String& name)
> +{
> +    qtLayer->setObjectName(QString(name));
> +    GraphicsLayer::setName(name);
> +}

I think QString() is unnecessary, the conversion operator should get called automatically.

> +bool GraphicsLayerQt::replaceChild(GraphicsLayer* oldChild, GraphicsLayer* newChild)
> +{
> +    if (GraphicsLayer::replaceChild(oldChild, newChild)) {
> +       qtLayer->noteChange(GraphicsLayerQtPrivate::ChildrenChange);
> +       return true;
> +    } else
> +        return false;
> +}

No need for else after return.

> +// helper functions to safely get a value out of WebCore's AnimationValue*
> +static void webkitAnimationToQtAnimationValue(const AnimationValue* animationValue, TransformOperations& transformOperations)
> +{
> +        if (animationValue) {
> +            const TransformOperations* ops = static_cast<const TransformAnimationValue*>(animationValue)->value();
> +            transformOperations = ops?*ops:TransformOperations();

please add spaces around ? and :

> +        } else
> +            transformOperations = TransformOperations();
> +
> +}
> +
> +static void webkitAnimationToQtAnimationValue(const AnimationValue* animationValue, qreal& realValue)
> +{
> +        if (animationValue)
> +            realValue = static_cast<const FloatAnimationValue*>(animationValue)->value();
> +        else
> +            realValue = 0;
> +}

ternary operator would look slightly nicer here

> +
> +// we put a bit of the functionality in a base class to allow casting and to save some code size
> +class AnimationQtBase : public QAbstractAnimation {
> +public:
> +    AnimationQtBase(GraphicsLayerQtPrivate* aLayer
> +                    , const KeyframeValueList& values
> +                    , const IntSize& aBoxSize
> +                    , const Animation* anim
> +                    , const QString & kfName
> +                    )
> +            : QAbstractAnimation(0)
> +            , target(aLayer)
> +            , boxSize(aBoxSize)
> +            , mDuration(anim->duration() * 1000)
> +            , isAlternate(anim->direction() == Animation::AnimationDirectionAlternate)
> +            , webkitPropertyID(values.property())
> +            , keyframesName(kfName)
> +    {
> +    }

please use m_ prefix for members so you can use cleaner variable names (aLayer -> layer etc).


> +
> +    QMap<qreal, KeyframeValueQt<T> > keyframeValues;

m_ prefix.


> +
> +        // to increase FPS, we use a less accurate caching mechanism while animation is going on
> +        // this is a UX choice that should probably be customizable
> +        if (newState == QAbstractAnimation::Running) {
> +            target.data()->transformAnimationRunning = true;
> +            if (target.data()->cacheMode() == QGraphicsItem::DeviceCoordinateCache)
> +                target.data()->setCacheMode(QGraphicsItem::ItemCoordinateCache);
> +        } else {
> +            target.data()->transformAnimationRunning = false;
> +            if (target.data()->cacheMode() == QGraphicsItem::ItemCoordinateCache)
> +                target.data()->setCacheMode(QGraphicsItem::DeviceCoordinateCache);
> +        }


Is this correct? According to the Qt documentation DeviceCoordinateCache can only be used for translated item but not for rotated, sheared or scaled items. Will this work in these cases too?

> +
> +void GraphicsLayerQt::removeAnimationsForProperty(AnimatedPropertyID id)
> +{
> +    for (QList<QWeakPointer<QAbstractAnimation> >::iterator it = qtLayer->animations.begin();
> +            it != qtLayer->animations.end(); ++it) {

Please keep these on the same line.

> +
> +#include "GraphicsLayer.h"
> +#include "GraphicsLayerClient.h"
> +
> +namespace WebCore {
> +class GraphicsLayerQtPrivate;
> +class GraphicsLayerQt : public GraphicsLayer {
> +    friend class GraphicsLayerQtPrivate;
> +public:
> +    GraphicsLayerQt(GraphicsLayerClient*

Please add line breaks after namespace, before class name and before public:

> +    virtual void syncCompositingState();
> +private:
> +    GraphicsLayerQtPrivate* qtLayer;
> +
>

m_qtLayer

Line break before private:

[Benjamin Poulain]

Could you please run the benchmarks with and without the patch to see if there is not any regression for the cases without any transformation?

The memory profile would be interesting for the cycler as well. In that case we expect regressions in exchange of speed, but it would be good to have numbers of how much the memory usage increase.

[Antti Koivisto]

(In reply to comment #10)
> Could you please run the benchmarks with and without the patch to see if there
> is not any regression for the cases without any transformation?

I suppose only way this could have any effect in non-transformed case is if the additional scrollbar layer has some impact.

[Noam Rosenthal]

> setNeedsToBeDisplayed? or does it really need a display? Maybe the naming can
> be improved

Maybe, but this is not my call... it's a reimp from GraphicsLayer.h
Maybe I should add /*! reimp */ everywhere?

Getting on to fixing the other style issues...

[Kenneth Rohde Christiansen]

(In reply to comment #12)
> > setNeedsToBeDisplayed? or does it really need a display? Maybe the naming can
> > be improved
> 
> Maybe, but this is not my call... it's a reimp from GraphicsLayer.h
> Maybe I should add /*! reimp */ everywhere?
> 
> Getting on to fixing the other style issues...

Personally, I would find that useful.

[Noam Rosenthal]

(In reply to comment #9)
> (From update of attachment 46808 [details])
> Should the overlay be created only if the composition is enabled in settings?
> Or perhaps it doesn't hurt even if it is not strictly needed?
My guess is that having the overhead of creating/destroying that layer based on settings changes would make things more difficult than always having two items. Also having that extra layer would allow hybrid apps in the future to add graphics-items that go on top of the content layer but under the scrollbar overlay. I believe the current route is good, if the performance overhead is not too bad (which I'm going to test).

> Is this correct? According to the Qt documentation DeviceCoordinateCache can
> only be used for translated item but not for rotated, sheared or scaled items.
This should be configurable (I'll put that in FIXME). The current implementation makes a choice of slightly degrading the animated object while a scale/rotate animation is going on, otherwise it'd be too slow (it would have to render the HTML for every frame in a scale/rotate animation, which would make compositing completely irrelevant for anything but translate/opacity). 

The other style issues I'll just fix :)
N

[Noam Rosenthal]

Created attachment 46859 [details]
compositing layers for Qt: Take 3

The fixes are:
- many style fixes based on the previous two reviews, GraphicsLayerQt.cpp is hopefully much more readable now
- modified QGVLauncher to enable a more parameterized approach (caching? viewport udpate mode?). This would allow to easily test how compositing reacts to different QGV modes
- The scrollbar overlay item in QGraphicsWebView now only instantiates when compositing is needed - and gets deleted when compositing is cleared. That means that the overhead for non-composited web pages is close to none (a couple of boolean checks)

[WebKit Review Bot]

Attachment 46859 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring WebKit/qt/Api/qwebsettings.cpp; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebview.cpp; This file is exempt from the style guide.
WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1125:  Alphabetical sorting problem.  [build/include_order] [4]
Ignoring WebKit/qt/Api/qgraphicswebview.cpp; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebsettings.h; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qgraphicswebview.h; This file is exempt from the style guide.
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 46859 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/200126

[Noam Rosenthal]

Created attachment 46885 [details]
compositing layers

Apparently I needed to add a check for Qt version.
I'm using too many Qt 4.6 features so I added a test for Qt version that disables accelerated compositing if Qt version is before 4.6

[WebKit Review Bot]

Attachment 46885 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring WebKit/qt/Api/qwebsettings.cpp; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebview.cpp; This file is exempt from the style guide.
WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1125:  Alphabetical sorting problem.  [build/include_order] [4]
Ignoring WebKit/qt/Api/qgraphicswebview.cpp; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebsettings.h; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qgraphicswebview.h; This file is exempt from the style guide.
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 46885 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/198505

[Antti Koivisto]

Comment on attachment 46885 [details]
compositing layers

Just some minor stylistic issues left so r=me. But please consider fixing some of the thing below before landing.

> Index: WebCore/platform/qt/QWebPageClient.h
> ===================================================================
> --- WebCore/platform/qt/QWebPageClient.h	(revision 53427)
> +++ WebCore/platform/qt/QWebPageClient.h	(working copy)
> @@ -29,8 +29,9 @@
>  #ifndef QT_NO_CURSOR
>  #include <QCursor>
>  #endif
> +
>  #include <QRect>
> -
> +class QGraphicsItem;

Don't remove the empty line here

> +class GraphicsLayerQtImpl : public QGraphicsObject {
> +    Q_OBJECT
> +
> +public:
> +    // this set of flags help us defer which properties of the layer have been
> +    // modified by the compositor, so we can know what to look for in the next flush
> +    enum ChangeFlag {

I would prefer "ChangeFlags" or "ChangeMask" to indicate that this is a bit field, not a single value.

> +    virtual void paint(QPainter* painter, const QStyleOptionGraphicsItem* option, QWidget* widget);

Named arguments should not be used in headers unless they add some information (QPainter* painter and QWidget* widget are redundant)) .

> +    // we manage transforms ourselves because transform-origin acts differently in webkit and in Qt
> +    void setBaseTransform(const QTransform& t);

as above

> +    void drawContents(QPainter* painter, const QRectF& r, bool mask = false);

as above (painter and r should be removed)

> +
> +    // let the compositor-API tell us which properties were changed
> +    void notifyChange(ChangeFlag changeFlag);

as above

> +
> +    QTransform m_baseTransfom;
> +    bool m_transformAnimationRunning;
> +    bool opacityAnimationRunning;

m_opacityAnimationRunning

> +
> +    struct ContentData {
> +        QPixmap pixmap;
> +        QRegion regionToUpdate;
> +        bool updateAll;
> +        QColor contentsBackgroundColor;
> +        QColor backgroundColor;
> +        StaticContentType contentType;
> +        float opacity;
> +        ContentData()
> +                : updateAll(false)
> +                , backgroundColor(QColor())

No need for initializer for color

> +                , contentType(HTMLContentType)
> +                , opacity(1)

1.f

> +    struct State {
> +        GraphicsLayer* maskLayer;
> +        FloatPoint pos;
> +        FloatPoint3D anchorPoint;
> +        FloatSize size;
> +        TransformationMatrix transform;
> +        TransformationMatrix childrenTransform;
> +        Color backgroundColor;
> +        Color currentColor;
> +        GraphicsLayer::CompositingCoordinatesOrientation geoOrientation;
> +        GraphicsLayer::CompositingCoordinatesOrientation contentsOrientation;
> +        float opacity;
> +        QRect contentsRect;
> +
> +        bool preserves3D: 1;
> +        bool masksToBounds: 1;
> +        bool drawsContent: 1;
> +        bool contentsOpaque: 1;
> +        bool backfaceVisibility: 1;
> +        bool distributeOpacity: 1;
> +        bool align: 2;

We use space before :

> +GraphicsLayerQtImpl::GraphicsLayerQtImpl(GraphicsLayerQt* newLayer)
> +        : QGraphicsObject(0)
> +        , m_layer(newLayer)
> +        , m_transformAnimationRunning(false)
> +        , m_changeMask(NoChanges)
> +
> +{

We use 4 space indentation for initializer lists.
Extra empty line.

> +
> +GraphicsLayerQt::GraphicsLayerQt(GraphicsLayerClient* client)
> +        : GraphicsLayer(client)
> +        , m_impl(PassOwnPtr<GraphicsLayerQtImpl>(new GraphicsLayerQtImpl(this)))
> +{
> +}

We use 4 space indentation for initializer lists.

> +// we want the timing function to be as close as possible to what the web-developer intended, so we're using the same function used by WebCore when compositing is disabled
> +// Using easing-curves would probably work for some of the cases, but wouldn't really buy us anything as we'd have to convert the bezier function back to an easing curve
> +static inline qreal applyTimingFunction(const TimingFunction& timingFunction, qreal progress, int duration)
> +{
> +        if (timingFunction.type() == LinearTimingFunction)
> +            return progress;
> +        if (timingFunction.type() == CubicBezierTimingFunction) {
> +            return solveCubicBezierFunction(timingFunction.x1(),
> +                                            timingFunction.y1(),
> +                                            timingFunction.x2(),
> +                                            timingFunction.y2(),
> +                                            double(progress), double(duration) / 1000);
> +        }
> +        return progress;
> +}

We use 4 space indentation for code.

+static void webkitAnimationToQtAnimationValue(const AnimationValue* animationValue, TransformOperations& transformOperations)
+{
+    transformOperations = TransformOperations();
+    if (!animationValue)
+        return;
+
+    const TransformOperations* ops = static_cast<const TransformAnimationValue*>(animationValue)->value();
+
+    if (ops)
+        transformOperations = *ops;
+}

Could be written as

    if (const TransformOperations* ops = static_cast<const TransformAnimationValue*>(animationValue)->value())
        transformOperations = *ops;

> +// we put a bit of the functionality in a base class to allow casting and to save some code size
> +class AnimationQtBase : public QAbstractAnimation {
> +public:
> +    AnimationQtBase(GraphicsLayerQtImpl* layer, const KeyframeValueList& values, const IntSize& boxSize, const Animation* anim, const QString & name)
> +            : QAbstractAnimation(0)
> +            , m_layer(layer)
> +            , m_boxSize(boxSize)
> +            , m_duration(anim->duration() * 1000)
> +            , m_isAlternate(anim->direction() == Animation::AnimationDirectionAlternate)
> +            , m_webkitPropertyID(values.property())
> +            , m_keyframesName(name)
> +    {
> +    }

We use 4 space indentation for initializer lists.

> +// buys us very little in this case, for too much overhead
> +template <typename T>
> +class AnimationQt : public AnimationQtBase {
> +
...
> +
> +    QMap<qreal, KeyframeValueQt<T> > keyframeValues;

m_keyframeValues.

> +class OpacityAnimationQt : public AnimationQt<qreal> {
> +public:
> +    OpacityAnimationQt(GraphicsLayerQtImpl* layer, const KeyframeValueList& values, const IntSize& boxSize, const Animation* anim, const QString & name)
> +                : AnimationQt<qreal>(layer, values, boxSize, anim, name)
> +    {
> +    }

We use 4 space indentation for initializer lists.

> +
> +    virtual void applyFrame(const qreal& fromValue, const qreal& toValue, qreal progress)
> +    {
> +        m_layer.data()->setOpacity(qMin<qreal>(qMax<qreal>(fromValue + (toValue-fromValue)*progress, 0), 1));
> +    }
> +
> +    virtual void updateState(QAbstractAnimation::State newState, QAbstractAnimation::State oldState)
> +    {
> +        QAbstractAnimation::updateState(newState, oldState);
> +
> +        if (!m_layer)
> +            return;
> +
> +        if (newState == QAbstractAnimation::Running)
> +            m_layer.data()->opacityAnimationRunning = true;
> +        else
> +            m_layer.data()->opacityAnimationRunning = false;
> +    }

Could be written as
m_layer.data()->opacityAnimationRunning = (newState == QAbstractAnimation::Running);

> +private:
> +    OwnPtr<GraphicsLayerQtImpl> m_impl;
> +
> +};

unneeded empty line.

> +
> +
> +}

unneeded empty line.

[Antti Koivisto]

Also it would be good if it would build in the bot :)

[Kenneth Rohde Christiansen]

What about renaming markSync to setNeedsSync ?

[Antti Koivisto]

Comment on attachment 46885 [details]
compositing layers

Changing to r- for not building on bot.

[Noam Rosenthal]

hmmm... any idea why the bot doesn't manage to build it? It seems like it's using an older Qt version or something? What would be a way to reproduce what the bot tries to do?

[Kenneth Rohde Christiansen]

The bugzilla Qt build bot is running 4.5 it appears, so you need some ifdef guards.

[Noam Rosenthal]

Created attachment 46978 [details]
compositing layers: +Compile fixes for Qt 4.5, ChangeLog

- Tested that it compiles with Qt 4.5, 
- applied recent style comments
- ChangeLog

[WebKit Review Bot]

Attachment 46978 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring WebKit/qt/Api/qwebsettings.cpp; This file is exempt from the style guide.
ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Ignoring WebKit/qt/Api/qwebview.cpp; This file is exempt from the style guide.
WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1117:  Alphabetical sorting problem.  [build/include_order] [4]
Ignoring WebKit/qt/Api/qgraphicswebview.cpp; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebsettings.h; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qgraphicswebview.h; This file is exempt from the style guide.
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Noam Rosenthal]

Created attachment 46986 [details]
GraphicsLayers in Qt: +4.5 build +changelog +last minute crash fix

Last minute fix

[WebKit Review Bot]

Attachment 46986 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring WebKit/qt/Api/qwebsettings.cpp; This file is exempt from the style guide.
ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Ignoring WebKit/qt/Api/qwebview.cpp; This file is exempt from the style guide.
WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1118:  Alphabetical sorting problem.  [build/include_order] [4]
Ignoring WebKit/qt/Api/qgraphicswebview.cpp; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebsettings.h; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qgraphicswebview.h; This file is exempt from the style guide.
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

> Why are these exempted from the style guide? We don't use Qt style in them, but
> WebKit style

There's a bunch of code in there that triggers false positives because that code uses a bunch of Qt specific stuff that doesn't match WebKit style.  In the long term, we should teach the style checker about these details, but I'm on an anti-false positive rampage at the moment.

[Kenneth Rohde Christiansen]

(In reply to comment #30)
> Attachment 46986 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> Ignoring WebKit/qt/Api/qwebsettings.cpp; This file is exempt from the style
> guide.
> ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]

This seems like a real issue though!

> Ignoring WebKit/qt/Api/qwebview.cpp; This file is exempt from the style guide.
> WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1118:  Alphabetical sorting
> problem.  [build/include_order] [4]

This one probably is as well.

If you fix that, r=me

[Antti Koivisto]

Comment on attachment 46986 [details]
GraphicsLayers in Qt: +4.5 build +changelog +last minute crash fix

Yeah, looks ready to me as well.

> +void GraphicsLayerQtImpl::notifyChange(ChangeMask changeMask)
> +{
> +    if (!this)
> +        return;
> +
> +    m_changeMask = static_cast<ChangeMask>(static_cast<int>(m_changeMask) | static_cast<int>(changeMask));
> 

Since m_changeMask is an int, most of those static_casts are unnecessary.

[Kenneth Rohde Christiansen]

(In reply to comment #33)
> (From update of attachment 46986 [details])
> Yeah, looks ready to me as well.
> 
> > +void GraphicsLayerQtImpl::notifyChange(ChangeMask changeMask)
> > +{
> > +    if (!this)
> > +        return;
> > +
> > +    m_changeMask = static_cast<ChangeMask>(static_cast<int>(m_changeMask) | static_cast<int>(changeMask));
> > 
> 
> Since m_changeMask is an int, most of those static_casts are unnecessary.

Also in C++ a cast to int is normally done as int(boolValue)

[Noam Rosenthal]

> > WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1118:  Alphabetical sorting
> > problem.  [build/include_order] [4]
> 
> This one probably is as well.

This is the moc at the end of the file, can't work around it...

[Noam Rosenthal]

Created attachment 47034 [details]
accelerated composition layers in Qt: +Still had a stray tab +removed unnecessary static_cast

- as I said, the other style-checker issues is a false positive (we just can't include the .moc file at the top)
but the other stuff is fixed - hopefully this is enough for our efficient army of bots :)

[Antti Koivisto]

Comment on attachment 47034 [details]
accelerated composition layers in Qt: +Still had a stray tab +removed unnecessary static_cast

r=me

[Adam Barth]

> This is the moc at the end of the file, can't work around it...

I thought we already fixed that bug.  Will investigate.

[Adam Barth]

https://bugs.webkit.org/show_bug.cgi?id=33934

[Noam Rosenthal]

Before I request for this to be added to the commit queue, do I need to fix something in the ChangeLog? bug 32461 failed on some obscure ChangeLog-based problem I don't understand, and I don't want it to repeat for this one.

[Kenneth Rohde Christiansen]

(In reply to comment #40)
> Before I request for this to be added to the commit queue, do I need to fix
> something in the ChangeLog? bug 32461 failed on some obscure ChangeLog-based
> problem I don't understand, and I don't want it to repeat for this one.

Just make sure you have no tabs in it, and then please create the patch using webkit-patch or git format-patch

[Simon Hausmann]

Committed r53618: <http://trac.webkit.org/changeset/53618>

[Antti Koivisto]

Woo!

[Manish Gurnaney]

Hi, 
   I have gone through the Accel code and also its working fine. but i have a lil problem . when i try to compile code in debug mode it building fine. but when stated debugging and put one breakpoint, it doesn't shows the source code for the same. 
So can you suggest what settings i need to make to debug WebCore.
I am using the steps mentioned in WebKit org.

Command :- ./build_webkit --qt --debug

[Noam Rosenthal]

(In reply to comment #44)
> Hi, 
>    I have gone through the Accel code and also its working fine. but i have a lil problem . when i try to compile code in debug mode it building fine. but when stated debugging and put one breakpoint, it doesn't shows the source code for the same. 
> So can you suggest what settings i need to make to debug WebCore.
> I am using the steps mentioned in WebKit org.
> 
> Command :- ./build_webkit --qt --debug
this looks fine; Looks like a normal debugger configuration problem and not necessarily related to the accel code.

[Antonio Gomes]

(In reply to comment #44)
> Hi, 
>    I have gone through the Accel code and also its working fine. but i have a lil problem . when i try to compile code in debug mode it building fine. but when stated debugging and put one breakpoint, it doesn't shows the source code for the same. 
> So can you suggest what settings i need to make to debug WebCore.
> I am using the steps mentioned in WebKit org.
> 
> Command :- ./build_webkit --qt --debug

maybe related to this http://lists.macosforge.org/pipermail/webkit-qt/2010-April/000375.html ? symbols show up fine for me on trunk now.

[Manish Gurnaney]

I performed the steps mentioned in the http://lists.macosforge.org/pipermail/webkit-qt/2010-April/000375.html 

can you tell me the source from which to take the code. 
Currently i have downloaded the source from git clone git://gitorious.org/webkit/webkit.git

[Noam Rosenthal]

That should be fine. Please use the mailing list or IRC for this, not bugzilla.