[webkit-reviews] review granted: [Bug 28018] Need to implement Canvas3d/WebGL for 3D rendering : [Attachment 38483] Patch to add Canvas3D files to build

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has granted 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 38483: Patch to add Canvas3D files to build
https://bugs.webkit.org/attachment.cgi?id=38483&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>

> +JSValue JSDocument::getCSSCanvasContext(JSC::ExecState* exec, JSC::ArgList
const& args)
> +{
> +    const UString& contextId = args.at(0).toString(exec);
> +    const UString& name = args.at(1).toString(exec);
> +    int width = args.at(2).toInt32(exec);
> +    int height = args.at(3).toInt32(exec);
> +
> +    JSValue result = jsUndefined();
> +    
> +    if (contextId == "2d")
> +	   result = toJS(exec,
WTF::getPtr(reinterpret_cast<CanvasRenderingContext2D*>(impl()->getCSSCanvasCon
text(contextId, name, width, height))));
> +#if ENABLE(3D_CANVAS)
> +    else if (contextId == "webkit-3d")
> +	   result = toJS(exec,
WTF::getPtr(reinterpret_cast<CanvasRenderingContext3D*>(impl()->getCSSCanvasCon
text(contextId, name, width, height))));
> +#endif
> +    
> +    return result;
> +}
rather than result =...

i'd prefer return toJS ... return toJS ... return jsUndefined();

>  CanvasRenderingContext* HTMLCanvasElement::getContext(const String& type)
>  {
>      if (type == "2d") {
> -	   if (!m_2DContext)
> -	       m_2DContext.set(new CanvasRenderingContext2D(this));
> -	   return m_2DContext.get();
> +	   if (m_context && !m_context->is2d())
> +	       m_context.clear();
> +	       if (!m_context)
> +		   m_context.set(new CanvasRenderingContext2D(this));
>      }
> -    return 0;
> +#if ENABLE(3D_CANVAS)    
> +    else if (type == "webkit-3d") {
> +	   if (m_context && !m_context->is3d())
> +	       m_context.clear();
> +	   if (!m_context) {
> +	       m_context.set(new CanvasRenderingContext3D(this));
> +	       setNeedsStyleRecalc();
> +	   }
> +    }
> +#endif
> +    else if (m_context.get())
> +	   m_context.clear();
> +    
> +    return m_context.get();

What happens when you ask for an invalid context type (before this patch) is
you get undefined back, but the context remains live.  After this patch the
context is destroyed.  My reading of the spec says that this is incorrect.  A
minor fix is to change 
> +    else if (m_context.get())
> +	   m_context.clear();
to
> +    else
> +	   return 0;

I feel better way would be to make the if (2d) and if (3d) paths return
directly, then you can just drop all the else blocks, and return 0 at the end

> +PlatformGraphicsContext3D HTMLCanvasElement::context3D() const
> +{
> +    if (m_context && m_context->is3d())
> +	   return
static_cast<CanvasRenderingContext3D*>(m_context.get())->context3D();
> +    else
> +	   return 0;
drop the else and return 0 directly.

> +}
> +
> +Platform3DObject HTMLCanvasElement::texture3D() const
> +{
> +    if (m_context && m_context->is3d())
> +	   return
static_cast<CanvasRenderingContext3D*>(m_context.get())->texture3D();
> +    else
> +	   return 0;
> +}
ditto

I think simon had a few comments so if you address those as well, r=me