Bug 35389 – [Qt] GraphicsLayer: support reflections

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Noam Rosenthal]

LayoutTests/reflections should work in AC mode

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit

    http://trac.webkit.org/wiki/QtWebKitBugs#Component

[Noam Rosenthal]

*** Bug 34762 has been marked as a duplicate of this bug. ***

[Noam Rosenthal]

Created attachment 54200 [details]
initial reflection support in GraphicsLayerQt

[Simon Hausmann]

Comment on attachment 54200 [details]
initial reflection support in GraphicsLayerQt


WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:562
 +              replica->m_replicaEffect.data()->setOpacity(m_layer->opacity());
Out of curiousity: Isn't m_replicaEffect->setOpacity much easier to read than m_replicaEffect.data()->setOpacity? :)

[Noam Rosenthal]

hmmm... yes, for some reason I thought it was not supported, probably a brain lapse. Should I resubmit?

[Noam Rosenthal]

Created attachment 54866 [details]
Revised documentation - all tests work with 4.7, though not all of them work with 4.6.

btw I can't use m_replicaEffect-> instead of .data(). It just doesn't compile :)

[Noam Rosenthal]

Created attachment 54868 [details]
Make sure the scene updates

The previous patch only worked on FullViewportUpdateMode. Now fixed.

[Noam Rosenthal]

Created attachment 54869 [details]
correct upload...

[Kenneth Rohde Christiansen]

Comment on attachment 54869 [details]
correct upload...

Big patch, first comments.

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:56
 +  class ReplicaEffectQt : public QGraphicsEffect {
Why that name? Why not ReflectionEffectQt or so?

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:61
 +      void renderMask(GraphicsLayer* layer, QPixmap& pixmap, const QRectF& boundingRect);
you can leave out layer and pixmap, that is redundant, according to the style guide

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:62
 +      void draw(QPainter* painter);
same here

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:66
 +      QRectF boundingRectFor(const QRectF& sourceRect) const;
bounding rect for? 

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:116
 +      bool isAncestorReplicated() const;
hasReplicatedAncestor() ?

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:384
 +          if (scene())
This code here needs braces and its contents spans more than one non-logical line

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:404
 +          return qobject_cast<GraphicsLayerQtImpl*>(m_layer->parent()->platformLayer()->toGraphicsObject())->isAncestorReplicated();
maybe we need some accessors for this? 

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1528
 +      // FIXME: use a smaller pixmap.
Maybe explain why

[Noam Rosenthal]

>  +  class ReplicaEffectQt : public QGraphicsEffect {
> Why that name? Why not ReflectionEffectQt or so?

because in GraphicsLayer.h it's called setReplicaLayer()... I don't mind changing it to reflection but I feel it doesn't really matter

[Noam Rosenthal]

Comment on attachment 54869 [details]
correct upload...

The current approach would be too slow on OpenGL due to the intermediate pixmaps. Researching other avenues.

[Noam Rosenthal]

Created attachment 56266 [details]
Addressed Kenneth's comments and added some ChangeLog comments about OpenGL

[Noam Rosenthal]

Comment on attachment 56266 [details]
Addressed Kenneth's comments and added some ChangeLog comments about OpenGL 

Need to revise the patch, because it conflicts with other already-accepted patches

[Noam Rosenthal]

Created attachment 57584 [details]
Re-merged patch

I tested the new patch with all the compositing layout tests, it doesn't break anything and makes reflections work.

[Noam Rosenthal]

Created attachment 57585 [details]
Had an extra line-break

[Kenneth Rohde Christiansen]

Comment on attachment 57585 [details]
Had an extra line-break

r+ with the below changes.

 604             if (m_layer->replicaLayer())
 605                 if (GraphicsLayerQtImpl* replica = toGraphicsLayerQtImpl(m_layer->replicaLayer()->platformLayer()))
 606                     replica->m_reflectionEffect.data()->updateGeometry();

This would need braces.WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1130
 +  
unnecessary newline

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1209
 +          : QGraphicsEffect(parent)
too much indentation.

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1212
 +          , m_opacity(1)
Shouldnt this be 1.0?

1224     if (GraphicsLayer* maskLayer = layer->maskLayer())
 1225         if (QGraphicsItem* maskLayerImpl = maskLayer->nativeLayer()) {
 1226             QPainter maskPainter(&pixmap);
 1227             maskPainter.setCompositionMode(QPainter::CompositionMode_DestinationIn);
 1228             QStyleOptionGraphicsItem option;
 1229             option.exposedRect = option.rect = QRect(0, 0, boundingRect.width(), boundingRect.height());
 1230             maskLayerImpl->paint(&maskPainter, &option);
 1231         }

The above is missing braces as well. Maybe invert the logic and add an early return.


WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1258
 +      QPixmap intermediatePixmap(painter->device()->width(), painter->device()->height());
Isn't it better to get a handle of the device first, or is it implicitly shared?

[Simon Hausmann]

Assigning to Noam so he can land the patch with Kenneth's proposed changes.

[Noam Rosenthal]

Not adding new features to GraphicsLayerQt. TextureMapper would be a replacement.