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

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> 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 203462: Patch
https://bugs.webkit.org/attachment.cgi?id=203462&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review


Very close!

> Source/WebCore/ChangeLog:15
> +	   * bindings/js/JSHTMLCanvasElementCustom.cpp:
> +	   (WebCore::JSHTMLCanvasElement::supportsContext):
> +	   * html/HTMLCanvasElement.cpp:
> +	   (WebCore::HTMLCanvasElement::supportsContext):

You should provide per-function comments here.

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:99
> +    HTMLCanvasElement* canvas = static_cast<HTMLCanvasElement*>(impl());
> +    const String& contextId = exec->argument(0).toString(exec)->value(exec);


You should check argumentCount to make sure there is an argument. If not,
return... jsNull or jsBoolean(false)???

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:125
> +	       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));
> +	   }

I wonder if we should eventually collect this into a helper function we can
share with WebGL - don't change this now though.

> Source/WebCore/html/HTMLCanvasElement.cpp:61
> -
> +    

Nit: added space.

> Source/WebCore/html/HTMLCanvasElement.cpp:212
> +    // FIXME - Provide implementation that accounts for attributes
> +    if (attrs) { }

Please file a bug and include the link. Don't include the empty statements.

> Source/WebCore/html/HTMLCanvasElement.cpp:215
> +    // FIXME - The code depends on the context not going away once created
(as getContext
> +    // is implemented under this assumption)

Again, file a bug and include the link.

> Source/WebCore/html/HTMLCanvasElement.cpp:220
> +    if (type == "2d") {
> +	   if (m_context && !m_context->is2d())
> +	       return false;
> +	   return true;
> +    }

My concern about this is that a script will take an existing canvas and ask
supportsContext. If that canvas already has a webgl context, it will tell the
user that 2d isn't supported. I think we should just always return true for 2d.


> Source/WebCore/html/HTMLCanvasElement.cpp:234
> +	       if (m_context && !m_context->is3d())
> +		   return false;

Same here maybe?

>> Source/WebCore/html/HTMLCanvasElement.h:95
>> +	bool supportsContext(const String&, CanvasContextAttributes* attributes
= 0);
> 
> The parameter name "attributes" adds no information, so it should be removed.
 [readability/parameter_name] [5]

Can you have a default value for a parameter without a name? If not, ignore
this warning.

> Source/WebCore/html/HTMLCanvasElement.idl:41
> +    [Custom] any supportsContext([Default=Undefined] optional DOMString
contextId);

Weird that the IDL doesn't mention the attributes parameter. Not your problem.

> LayoutTests/fast/canvas/webgl/canvas-supports-context-expected.txt:7
> +PASS 2dcontext exists

Add space after context name.

> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:5
> +<script src="resources/desktop-gl-constants.js"
type="text/javascript"></script>

Don't need this.

> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:7
> +<script src="resources/webgl-test.js"></script>

Nor this.

> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:16
> +if (window.initNonKhronosFramework) {
> +    window.initNonKhronosFramework(true);
> +}
> +

Nor this.

> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:52
> +	if (context) {
> +		testPassed(type_of_context + "context exists");
> +	}

WebKit style has no braces for single line blocks. There are a few below here
too.

Also, this looks like an 8 space indent. Should be 4.

> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:66
> +		testFailed(type_of_context + "context exists yet
canvas.supportsContext('" 
> +			+ type_of_context + "') returns false");

Put this on one line so that it becomes a single line block.