[webkit-reviews] review denied: [Bug 28018] Need to implement Canvas3d/WebGL for 3D rendering : [Attachment 38667] Final patch for Canvas3D work

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 28018: Need to implement Canvas3d/WebGL for 3D rendering
https://bugs.webkit.org/show_bug.cgi?id=28018

Attachment 38667: Final patch for Canvas3D work
https://bugs.webkit.org/attachment.cgi?id=38667&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================

> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Final patch for Canvas 3D support
> +	   https://bugs.webkit.org/show_bug.cgi?id=28018
> +
> +	   This hooks everything up and provides a working implementation of
> +	   Canvas 3D.

You need to explain a few things here, like the style recalc hack, the fact
that a
3d canvas needs a RenderLayer and a compositing layer, and why.

The whole point of changelogs (and the commit message) is to explain why the
changes were made so that someone can come back later and figure out why you
made the changes you did. Often that person is the committer!

> Index: WebCore/bindings/js/JSCanvasRenderingContext3DCustom.cpp
> ===================================================================

> -	   count = args.at(2).toInt32(exec);
> +	   unsigned int count = args.at(2).toInt32(exec);

You are shadowing a variable at function scope here. A different name is
probably better.

> Index: WebCore/html/HTMLCanvasElement.cpp
> ===================================================================

>	   if (!m_context) {
>	       m_context.set(new CanvasRenderingContext3D(this));
> -	       setNeedsStyleRecalc();
> +	       setNeedsStyleRecalc(SyntheticStyleChange);

You need a comment here explaining why you call setNeedsStyleRecalc().

> Index: WebCore/platform/graphics/GraphicsContext3D.h
> ===================================================================
>  
>  typedef void* PlatformGraphicsContext3D;
> +#define NO_PLATFORM_GRAPHICS_CONTEXT_3D 0
>  typedef GLuint Platform3DObject;
> +#define NO_PLATFORM_3D_OBJECT 0
>  #else
>  typedef void* PlatformGraphicsContext3D;
> +#define NO_PLATFORM_GRAPHICS_CONTEXT_3D 0
>  typedef int Platform3DObject;
> +#define NO_PLATFORM_3D_OBJECT 0
>  #endif

It seems that NO_PLATFORM_GRAPHICS_CONTEXT_3D and NO_PLATFORM_3D_OBJECT should
be constants, rather than #defines, like:

typedef void* PlatformGraphicsContext3D;
const PlatformGraphicsContext3D NullPlatformGraphicsContext3D = 0; 

typedef GLuint Platform3DObject;
const Platform3DObject NullPlatform3DObject = 0; 

> Index: WebCore/platform/graphics/GraphicsLayer.h
> ===================================================================
>  typedef void* PlatformLayer;
> -typedef void* NativeLayer;
> +typedef int NativeLayer;

Why this change? It's not 64-bit friendly.

> +#if ENABLE(3D_CANVAS)
> +    virtual void setContentsTo3DCanvas(PlatformGraphicsContext3D,
Platform3DObject) { }

For setContentsTo3DCanvas(), the method names needs to match the arguments.
You're not passing
in a canvas 3d, so perhaps it should be setContentsToTexture() or something.
Can the argument
just be a GraphicsContext3D?

> +    virtual void redraw3DCanvas() { }

I don't like "redraw". What does it really mean? "recomposite"? Can
setNeedsDisplay() do
the same job?

> +#if ENABLE(3D_CANVAS)
> +    void setHas3DLayer(bool has3DLayer, PlatformGraphicsContext3D context =
NO_PLATFORM_GRAPHICS_CONTEXT_3D, Platform3DObject texture =
NO_PLATFORM_3D_OBJECT);
> +#endif

Why do you need both this and the above method?

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

> +#if ENABLE(3D_CANVAS)
> +    void updateContents3DCanvas();
> +#endif

Maybe updateContentsTexture()?

> +#if ENABLE(3D_CANVAS)
> +    PlatformGraphicsContext3D m_3DContext;
> +    Platform3DObject m_3DTexture;
> +#endif

What's the ownership model for these members? Who is responsible for freeing
the
associated resources?

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

> +#if ENABLE(3D_CANVAS)
> +void GraphicsLayerCA::setContentsTo3DCanvas(PlatformGraphicsContext3D
context, Platform3DObject texture)
> +{
> +    if (context == m_3DContext && texture == m_3DTexture)
> +	   return;
> +	   
> +    m_3DContext = context;
> +    m_3DTexture = texture;
> +    
> +    noteLayerPropertyChanged(ChildrenChanged);
> +    
> +    BEGIN_BLOCK_OBJC_EXCEPTIONS
> +
> +    if (context != NO_PLATFORM_GRAPHICS_CONTEXT_3D && texture !=
NO_PLATFORM_3D_OBJECT) {
> +	   // create the inner 3d layer
> +	   m_contentsLayer = [[Canvas3DLayer alloc]
initWithContext:(CGLContextObj) context texture: (GLuint) texture];

This is leaking the Canvas3DLayer. you need to use AdoptNS.

No spaces after the, and use static_cast<>.

> +#if ENABLE(3D_CANVAS)
> +void GraphicsLayerCA::redraw3DCanvas()
> +{
> +    if (m_contentsLayerPurpose == ContentsLayerFor3DCanvas)
> +	   [m_contentsLayer.get() setNeedsDisplay];
> +}
> +#endif

Aha, it is just setNeedsDisplay!

> Index: WebCore/rendering/RenderHTMLCanvas.cpp
> ===================================================================

> +bool RenderHTMLCanvas::requiresLayer() const
> +{
> +    if (RenderReplaced::requiresLayer())
> +	   return true;
> +    
> +#if ENABLE(3D_CANVAS)
> +    HTMLCanvasElement* canvas =
reinterpret_cast<HTMLCanvasElement*>(node());

I think a static_cast<> should work there

> Index: WebCore/rendering/RenderLayerBacking.cpp
> ===================================================================

> +#if ENABLE(3D_CANVAS)    
> +	   else if (renderer()->isCanvas()) {
> +	       HTMLCanvasElement* canvas =
reinterpret_cast<HTMLCanvasElement*>(renderer()->node());

static_cast<>

> +		   if (canvas->context3D()) {
> +		      
m_graphicsLayer->setContentsTo3DCanvas(canvas->context3D(),
canvas->texture3D());
> +		       m_graphicsLayer->setDrawsContent(true);

Why setDrawsContent(true)? That means we'll create backing store for the
primary layer,
which you don't want.

Have you tested borders, outlines etc. on a 3d canvas? You should include some
layout tests
with pixel results that verify the rendering of these.

> +#if ENABLE(3D_CANVAS)    
> +    bool is3DCanvas = false;
> +    if (renderer()->isCanvas()) {
> +	   HTMLCanvasElement* canvas =
reinterpret_cast<HTMLCanvasElement*>(renderer()->node());
> +	   is3DCanvas = canvas->is3D();
> +    }
> +    if (is3DCanvas)
> +	   return true;

Seems like you could eliminate is3DCanvas and just do an earlier return here.
In fact,
if you know it's a canvas, you can always return.

>  void RenderLayerBacking::rendererContentChanged()
>  {
> -    if (canUseDirectCompositing() && renderer()->isImage())
> -	   updateImageContents();
> +    if (canUseDirectCompositing()) {
> +	   if (renderer()->isImage())
> +	       updateImageContents();
> +	   else {
> +#if ENABLE(3D_CANVAS)    
> +	       if (renderer()->isCanvas()) {
> +		   HTMLCanvasElement* canvas =
reinterpret_cast<HTMLCanvasElement*>(renderer()->node());

static_cast<>

> Index: WebCore/rendering/RenderLayerCompositor.cpp
> ===================================================================

>  bool RenderLayerCompositor::requiresCompositingLayer(const RenderLayer*
layer) const
>  {
> +#if ENABLE(3D_CANVAS)    
> +    bool is3DCanvas = false;
> +    if (layer->renderer()->isCanvas()) {
> +	   HTMLCanvasElement* canvas =
reinterpret_cast<HTMLCanvasElement*>(layer->renderer()->node());
> +	   is3DCanvas = canvas->is3D();
> +    }
> +#endif

I'd prefer this be wrapped up in its own method, requiresCompositingForCanvs().


> Index: LayoutTests/fast/dom/Window/window-properties.html
> ===================================================================
> --- LayoutTests/fast/dom/Window/window-properties.html	(revision
47753)
> +++ LayoutTests/fast/dom/Window/window-properties.html	(working copy)
> @@ -60,7 +60,9 @@ var __skip__ = {
>      "window.objCPlugin" : 1,
>      "window.objCPluginFunction" : 1,
>      "window.plainText" : 1,
> -    "window.textInputController" : 1
> +    "window.textInputController" : 1,
> +    
> +    "window.CanvasRenderingContext3D" : 1 // We ignore
CanvasRenderingContext3D and test it elsewhere, since it is not in all builds

Any reason for the blank line?

r- for lack of some basic tests and the few quibbles noted above. It's mostly
good though.