[webkit-reviews] review denied: [Bug 35389] [Qt] GraphicsLayer: support reflections : [Attachment 54869] correct upload...

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Noam Rosenthal
<noam.rosenthal at nokia.com>'s request for review:
Bug 35389: [Qt] GraphicsLayer: support reflections
https://bugs.webkit.org/show_bug.cgi?id=35389

Attachment 54869: correct upload...
https://bugs.webkit.org/attachment.cgi?id=54869&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
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()->toGraphi
csObject())->isAncestorReplicated();
maybe we need some accessors for this? 

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