[Webkit-unassigned] [Bug 70117] Expose HTMLCanvasElement supportsContext
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 31 12:32:39 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=70117
Dean Jackson <dino at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #203462|review? |review-
Flag| |
--- Comment #15 from Dean Jackson <dino at apple.com> 2013-05-31 12:31:10 PST ---
(From update of attachment 203462)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list