[webkit-reviews] review granted: [Bug 40643] Move ownership of Canvas3DLayer to GraphicsContext3D : [Attachment 59413] Final patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 40643: Move ownership of Canvas3DLayer to GraphicsContext3D
https://bugs.webkit.org/show_bug.cgi?id=40643

Attachment 59413: Final patch
https://bugs.webkit.org/attachment.cgi?id=59413&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.mm
> ===================================================================
> +#ifndef NDEBUG
> +	   [m_webGLLayer.get() setName:@"3D Layer"];
> +#endif    

I think "WebGL Layer" would be a better name.

> Index: WebCore/platform/graphics/mac/GraphicsLayerCA.h
> ===================================================================

>  #if ENABLE(3D_CANVAS)
> -    virtual void setGraphicsContext3DNeedsDisplay();
> +    virtual void setWebGLNeedsDisplay();
>  #endif

Do you really need the setWebGLNeedsDisplay() method? The GraphicsContext3D now
holds onto the layer, so maybe it should do the -setNeedsDisplay itself?

However, this will bypass the syncing logic, so may cause flashes with some
content.

> Index: WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> ===================================================================

> -void GraphicsLayerCA::setGraphicsContext3DNeedsDisplay()
> +void GraphicsLayerCA::setWebGLNeedsDisplay()
>  {
> -    if (m_contentsLayerPurpose == ContentsLayerForGraphicsLayer3D)
> +    if (m_contentsLayerPurpose == ContentsLayerForWebGL)
>	   [m_contentsLayer.get() setNeedsDisplay];
>  }

This may cause an early CA commit, which can result in flashes in some content.
You should really use a dirty bit for this.

r=me but please reconsider setWebGLNeedsDisplay().