[webkit-reviews] review denied: [Bug 33514] [Qt] Implement GraphicsLayer for accelerated layer compositing : [Attachment 46752] compositing layers for Qt
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jan 17 03:18:13 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 46752: compositing layers for Qt
https://bugs.webkit.org/attachment.cgi?id=46752&action=review
------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
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::DeveloperExtrasEnabl
ed, true);
> +
QWebSettings::globalSettings()->setAttribute(QWebSettings::LocalContentCanAcces
sRemoteUrls, 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::AcceleratedCompositi
ngEnabled, 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->nativeL
ayer():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()->syncCompositingStateRec
ursive();
> + }
> + 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()->syncCompositingStateRecurs
ive();
> + 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
More information about the webkit-reviews
mailing list