[webkit-reviews] review granted: [Bug 33514] [Qt] Implement GraphicsLayer for accelerated layer compositing : [Attachment 46885] compositing layers

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted 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 46885: compositing layers
https://bugs.webkit.org/attachment.cgi?id=46885&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
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.