[webkit-reviews] review denied: [Bug 33514] [Qt] Implement GraphicsLayer for accelerated layer compositing : [Attachment 46808] accelerated compositing in Qt: with massive style improvements and in-code comments
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 18 04:49:38 PST 2010
Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Noam Rosenthal
<noam.rosenthal at nokia.com>'s request for review:
Bug 33514: [Qt] Implement GraphicsLayer for accelerated layer compositing
https://bugs.webkit.org/show_bug.cgi?id=33514
Attachment 46808: accelerated compositing in Qt: with massive style
improvements and in-code comments
https://bugs.webkit.org/attachment.cgi?id=46808&action=review
------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
> 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::AcceleratedCompositi
ngEnabled, 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::ContentsL
ayer)), 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()->syncCompositingStateRec
ursive();
> + }
> +
> + 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()->syncCompositingStateRec
ursive();
> + 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;
> + }
> +}
More information about the webkit-reviews
mailing list