[webkit-reviews] review denied: [Bug 70117] Expose HTMLCanvasElement supportsContext : [Attachment 203727] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Ruth Fong <ruthiecftg at gmail.com>'s
request for review:
Bug 70117: Expose HTMLCanvasElement supportsContext
https://bugs.webkit.org/show_bug.cgi?id=70117

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=203727&action=review


> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:47
> +static PassRefPtr<CanvasContextAttributes> get3DContextAttributes(ExecState*
exec)
> +{

This should return a PassRefPtr<WebGLContextAttributes>, since that is what it
always returns.

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:71
> +    RefPtr<CanvasContextAttributes> attrs =
WebGLContextAttributes::create();
> +    WebGLContextAttributes* webGLAttrs =
static_cast<WebGLContextAttributes*>(attrs.get());
> +    if (exec->argumentCount() > 1 && exec->argument(1).isObject()) {
> +	   JSObject* jsAttrs = exec->argument(1).getObject();
> +	   Identifier alpha(exec, "alpha");
> +	   if (jsAttrs->hasProperty(exec, alpha))
> +	       webGLAttrs->setAlpha(jsAttrs->get(exec, alpha).toBoolean(exec));

> +	   Identifier depth(exec, "depth");
> +	   if (jsAttrs->hasProperty(exec, depth))
> +	       webGLAttrs->setDepth(jsAttrs->get(exec, depth).toBoolean(exec));

> +	   Identifier stencil(exec, "stencil");
> +	   if (jsAttrs->hasProperty(exec, stencil))
> +	       webGLAttrs->setStencil(jsAttrs->get(exec,
stencil).toBoolean(exec));
> +	   Identifier antialias(exec, "antialias");
> +	   if (jsAttrs->hasProperty(exec, antialias))
> +	       webGLAttrs->setAntialias(jsAttrs->get(exec,
antialias).toBoolean(exec));
> +	   Identifier premultipliedAlpha(exec, "premultipliedAlpha");
> +	   if (jsAttrs->hasProperty(exec, premultipliedAlpha))
> +	       webGLAttrs->setPremultipliedAlpha(jsAttrs->get(exec,
premultipliedAlpha).toBoolean(exec));
> +	   Identifier preserveDrawingBuffer(exec, "preserveDrawingBuffer");
> +	   if (jsAttrs->hasProperty(exec, preserveDrawingBuffer))
> +	       webGLAttrs->setPreserveDrawingBuffer(jsAttrs->get(exec,
preserveDrawingBuffer).toBoolean(exec));
> +    }
> +    return attrs.release();

I still think the JSDictionary version was better.  This is not checking for
exceptions (the JSDictionary version was).

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:112
> +    if (exec->hadException())
> +	   return jsBoolean(false);

It is idiomatic to return jsUndefined() in the case of an exception.