[webkit-reviews] review denied: [Bug 65708] Logic to compute visible display rect in GraphicsLayerCA::syncCompositingState : [Attachment 102952] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Aug 4 12:49:41 PDT 2011


Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 65708: Logic to compute visible display rect in
GraphicsLayerCA::syncCompositingState
https://bugs.webkit.org/show_bug.cgi?id=65708

Attachment 102952: Patch
https://bugs.webkit.org/attachment.cgi?id=102952&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=102952&action=review


I wish we could test this; I see a number of issues (not taking perspective
into account, possible scrolling issues).

> Source/WebCore/platform/graphics/GraphicsLayer.h:368
> +    virtual void syncCompositingState(const FloatRect&) { }

I think it's worth naming the parameter in a C-style comment.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:836
> +	   state.translate(-anchorPoint().x(), -anchorPoint().y(),
-anchorPoint().z(), accumulation);
> +	   state.applyTransform(transform(), accumulation);
> +	   state.translate(anchorPoint().x(), anchorPoint().y(),
anchorPoint().z(), accumulation);

Rather than having to add  TransformState::translate() for this, can't you just
compute the transform taking the anchor point into account, then apply it?

Also, you're ignoring childTransform here.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:842
> +    if (masksToBounds()) {
> +	   // Replace the quad in the TransformState with one that is clipped
to this layer's bounds

I think you might want to assert here that accumulation == FlattenTransform,
otherwise state.lastPlanarQuad might be in some ancestor's coordinate system.

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:96
> + at interface CATiledLayer(GraphicsLayerCAPrivate)
> +- (void)displayInRect:(CGRect)r levelOfDetail:(int)lod options:(NSDictionary
*)dict;
> +- (BOOL)canDrawConcurrently;
> +- (void)setCanDrawConcurrently:(BOOL)flag;
> + at end

You should #ifdef this for not Leopard and not SnowLeopard.

> Source/WebCore/platform/graphics/transforms/TransformState.cpp:62
> +void TransformState::translate(float x, float y, float z,
TransformAccumulation accumulate)

I don't think this is necessary.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:210
> +    if (GraphicsLayer* rootLayer = rootGraphicsLayer()) {
> +	   FrameView* frameView = m_renderView ? m_renderView->frameView() : 0;

> +	   if (frameView)
> +	       rootLayer->syncCompositingState(frameView->frameRect());
> +    }

Is frameRect() the right thing? How is scrolling taken into account?


More information about the webkit-reviews mailing list