Bug 35389 - [Qt] GraphicsLayer: support reflections
Summary: [Qt] GraphicsLayer: support reflections
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Enhancement
Assignee: Noam Rosenthal
URL:
Keywords: Qt
: 34762 (view as bug list)
Depends on: 35382
Blocks: 38744
  Show dependency treegraph
 
Reported: 2010-02-25 09:08 PST by Noam Rosenthal
Modified: 2010-08-13 10:10 PDT (History)
3 users (show)

See Also:


Attachments
initial reflection support in GraphicsLayerQt (18.68 KB, patch)
2010-04-23 16:25 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Revised documentation - all tests work with 4.7, though not all of them work with 4.6. (17.46 KB, patch)
2010-05-02 03:22 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Make sure the scene updates (19.16 KB, patch)
2010-05-02 04:08 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
correct upload... (19.20 KB, patch)
2010-05-02 04:31 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Addressed Kenneth's comments and added some ChangeLog comments about OpenGL (21.31 KB, patch)
2010-05-17 13:30 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Re-merged patch (19.58 KB, patch)
2010-06-01 12:54 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Had an extra line-break (19.57 KB, patch)
2010-06-01 12:56 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2010-02-25 09:08:31 PST
LayoutTests/reflections should work in AC mode
Comment 1 Tor Arne Vestbø 2010-03-05 09:39:48 PST
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
Comment 2 Noam Rosenthal 2010-03-16 13:51:24 PDT
*** Bug 34762 has been marked as a duplicate of this bug. ***
Comment 3 Noam Rosenthal 2010-04-23 16:25:48 PDT
Created attachment 54200 [details]
initial reflection support in GraphicsLayerQt
Comment 4 Simon Hausmann 2010-04-28 13:40:01 PDT
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? :)
Comment 5 Noam Rosenthal 2010-04-28 13:51:28 PDT
hmmm... yes, for some reason I thought it was not supported, probably a brain lapse. Should I resubmit?
Comment 6 Noam Rosenthal 2010-05-02 03:22:43 PDT
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 :)
Comment 7 Noam Rosenthal 2010-05-02 04:08:23 PDT
Created attachment 54868 [details]
Make sure the scene updates

The previous patch only worked on FullViewportUpdateMode. Now fixed.
Comment 8 Noam Rosenthal 2010-05-02 04:31:00 PDT
Created attachment 54869 [details]
correct upload...
Comment 9 Kenneth Rohde Christiansen 2010-05-12 05:53:02 PDT
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
Comment 10 Noam Rosenthal 2010-05-12 06:04:07 PDT
>  +  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
Comment 11 Noam Rosenthal 2010-05-16 02:29:11 PDT
Comment on attachment 54869 [details]
correct upload...

The current approach would be too slow on OpenGL due to the intermediate pixmaps. Researching other avenues.
Comment 12 Noam Rosenthal 2010-05-17 13:30:30 PDT
Created attachment 56266 [details]
Addressed Kenneth's comments and added some ChangeLog comments about OpenGL
Comment 13 Noam Rosenthal 2010-06-01 11:28:39 PDT
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
Comment 14 Noam Rosenthal 2010-06-01 12:54:31 PDT
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.
Comment 15 Noam Rosenthal 2010-06-01 12:56:33 PDT
Created attachment 57585 [details]
Had an extra line-break
Comment 16 Kenneth Rohde Christiansen 2010-06-13 07:50:13 PDT
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?
Comment 17 Simon Hausmann 2010-06-20 04:21:54 PDT
Assigning to Noam so he can land the patch with Kenneth's proposed changes.
Comment 18 Noam Rosenthal 2010-08-13 10:10:28 PDT
Not adding new features to GraphicsLayerQt. TextureMapper would be a replacement.