JavaScript developers wanting to detect WebGL might think to write a check like so: if (window.WebGLRenderingContext) { ... } but window.WebGLRenderingContext is defined even when WebGL is disabled.
<rdar://problem/10287834>
Note that in various ports we've fluctuated on this decision. Currently, in Chromium, we provide the WebGLRenderingContext property on the window even if WebGL is disabled on the command line. On the other hand, at least until WebGL is enabled by default in Safari, it may make sense to only expose this property when it's enabled. The rationale is that the error message applications would likely print if window.WebGLRenderingContext is undefined is "Your browser doesn't support WebGL", not "Either your browser doesn't support WebGL, or it was disabled via a preference setting". http://get.webgl.org/ is the canonical diagnostic of WebGL support. We can update the error messages there to better match browsers' behavior.
The reason we want this is that on Apple systems tools like Modernizr test for WebGL by creating a context, which causes us to switch to the discrete GPU. This is particularly annoying on pages that have zero intention of using WebGL, but include the stock Modernizr script.
The code on get.webgl.org does this: if (gl == null) { try { gl = canvas.getContext("experimental-webgl"); experimental = true; } catch (x) { gl = null; } } Creating a context just to test if WebGL is enabled is exactly what we want to avoid. It's OK for get.webgl.org because that has to be a conclusive result, but for scripts like modernizr, doing this on EVERY load of a page, even those that will never use WebGL, is a huge waste (and may trigger side effects like on OS X where we jump to a discrete GPU). I think we need to come up with another method. I'm not sure about window.WebGLRenderingContext being hidden when WebGL is not enabled.
The reason I don't like making that particular attribute undefined, is that we'd probably have to do it for all the WebGL attributes on window: attribute [Conditional=WEBGL] WebGLActiveInfoConstructor WebGLActiveInfo; attribute [Conditional=WEBGL] WebGLBufferConstructor WebGLBuffer; attribute [Conditional=WEBGL] WebGLFramebufferConstructor WebGLFramebuffer; attribute [Conditional=WEBGL] WebGLProgramConstructor WebGLProgram; attribute [Conditional=WEBGL] WebGLRenderbufferConstructor WebGLRenderbuffer; attribute [Conditional=WEBGL] WebGLRenderingContextConstructor WebGLRenderingContext; attribute [Conditional=WEBGL] WebGLShaderConstructor WebGLShader; attribute [Conditional=WEBGL] WebGLShaderPrecisionFormatConstructor WebGLShaderPrecisionFormat; attribute [Conditional=WEBGL] WebGLTextureConstructor WebGLTexture; attribute [Conditional=WEBGL] WebGLUniformLocationConstructor WebGLUniformLocation; I think a better idea is if a canvas element could return a list of context names it supports. e.g. canvas.getSupportedContexts() -> ["2d", "webkit-3d", "experimental-webgl"]; (probably prefixed with webkit for now)
Actually, canvas.supportsContext("experimental-webgl", options) -> bool might be better, as long as testing the option doesn't require us to create a context.
I sent a proposal to whatwg and html-wg http://www.w3.org/mid/B2FFF68C-CD91-4273-8087-EC3058D24322@apple.com Renaming this bug.
As jamesr pointed out on IRC, there may be instances where supportsContext returns true but getContext fails. I think that's ok. The suggestion is that supportsContext is the implementation saying if it does support a context in theory. A return value of false is a clear indication that getContext will fail. This would allow an implementation that supports WebGL but not on particular hardware to return false.
We've seen this on economist.com (using modernizr) in the past, although I believe it doesn't do that any more. I think a soft-promise type API could work better - supportsContext("...") -> false means we definitely won't return non-null for getContext("..."), but we're still allowed to return null for getContext("...") even after saying true to supportsContext("...")
Modernizr no longer creates a webgl context due to the reasons you all have specified it does a soft detect, window.WebGLRenderingContext, that is very prone to a false positive (which is mentioned). So this would be very welcome.
http://html5.org/tools/web-apps-tracker?from=7481&to=7482
(In reply to comment #11) > http://html5.org/tools/web-apps-tracker?from=7481&to=7482 Excellent. If anyone watching wants to take this, feel free.
Created attachment 203462 [details] Patch
Attachment 203462 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/webgl/canvas-supports-context-expected.txt', u'LayoutTests/fast/canvas/webgl/canvas-supports-context.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp', u'Source/WebCore/html/HTMLCanvasElement.cpp', u'Source/WebCore/html/HTMLCanvasElement.h', u'Source/WebCore/html/HTMLCanvasElement.idl']" exit_code: 1 Source/WebCore/html/HTMLCanvasElement.h:95: The parameter name "attributes" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 203462 [details] Patch 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.
Comment on attachment 203462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:220 >> + } > > 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. Tim just convinced me I'm wrong (he always does). Please ignore. >> Source/WebCore/html/HTMLCanvasElement.cpp:234 >> + return false; > > Same here maybe? And this.
Comment on attachment 203462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review > Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:123 > + 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)) These toBooleans() can all throw exceptions, so we should probably be checking for exceptions. We should also make sure we are testing all those cases as well. Can this use a JSDictionary to simplify things? >> 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. You should make it match the spec and have a "any... arguments" second parameter.
Comment on attachment 203462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:212 >> + if (attrs) { } > > Please file a bug and include the link. Don't include the empty statements. I'm not sure if this is the proper way to handle the fact that I'm not using the attributes but that the W3 definition of supportsContext supports additional arguments. I got this error when submitting the patch: Source/WebCore/html/HTMLCanvasElement.h:95: The parameter name "attributes" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebCore/html/HTMLCanvasElement.cpp:216 > + if (type == "2d") { This method is modeled after how HTMLCanvasElement::getContext was written, but I'm not sure that's necessary. >> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:7 >> +<script src="resources/webgl-test.js"></script> > > Nor this. Do I need this for finishTest() (which calls notifyDone)?
Comment on attachment 203462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review >> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:123 >> + if (jsAttrs->hasProperty(exec, preserveDrawingBuffer)) > > These toBooleans() can all throw exceptions, so we should probably be checking for exceptions. We should also make sure we are testing all those cases as well. Can this use a JSDictionary to simplify things? Right now JSHTMLCanvasElement::getContext is implemented like this (without exception handling) - should I still add exception handling?
Comment on attachment 203462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review >>> 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. > > You should make it match the spec and have a "any... arguments" second parameter. getContext should also have "any... arguments" second parameter; should that be added as well?
(In reply to comment #19) > (From update of attachment 203462 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review > > >> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:123 > >> + if (jsAttrs->hasProperty(exec, preserveDrawingBuffer)) > > > > These toBooleans() can all throw exceptions, so we should probably be checking for exceptions. We should also make sure we are testing all those cases as well. Can this use a JSDictionary to simplify things? > > Right now JSHTMLCanvasElement::getContext is implemented like this (without exception handling) - should I still add exception handling? You should add the checking, yes.
(In reply to comment #20) > (From update of attachment 203462 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review > > >>> 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. > > > > You should make it match the spec and have a "any... arguments" second parameter. > > getContext should also have "any... arguments" second parameter; should that be added as well? You can do that in another bug.
Comment on attachment 203462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review >>>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:123 >>>> + if (jsAttrs->hasProperty(exec, preserveDrawingBuffer)) >>> >>> These toBooleans() can all throw exceptions, so we should probably be checking for exceptions. We should also make sure we are testing all those cases as well. Can this use a JSDictionary to simplify things? >> >> Right now JSHTMLCanvasElement::getContext is implemented like this (without exception handling) - should I still add exception handling? > > You should add the checking, yes. I tried wrapping one of the toBoolean statements in a try-catch and got an error in Xcode saying that exceptions are disabled; not sure where this is set/what's it's scope.
Created attachment 203472 [details] Patch
(In reply to comment #23) > I tried wrapping one of the toBoolean statements in a try-catch and got an error in Xcode saying that exceptions are disabled; not sure where this is set/what's it's scope. I should have given you some sample code, so, here you go: +JSValue JSHTMLCanvasElement::supportsContext(ExecState* exec) +{ + HTMLCanvasElement* canvas = static_cast<HTMLCanvasElement*>(impl()); + if (!exec->argumentCount()) + return jsBoolean(false); + + const String& contextId = exec->argument(0).toString(exec)->value(exec); + if (exec->hadException()) + return jsUndefined(); + + RefPtr<CanvasContextAttributes> attrs; +#if ENABLE(WEBGL) + if (contextId == "experimental-webgl" || contextId == "webkit-3d") { + JSValue initializerValue = exec->argument(1); + if (!initializerValue.isUndefinedOrNull()) { + // Given the above test, this will always yield an object. + JSObject* initializerObject = initializerValue.toObject(exec); + + // Create the dictionary wrapper from the initializer object. + JSDictionary dictionary(exec, initializerObject); + + GraphicsContext3D::Attributes attrs; + if (!dictionary.tryGetProperty("alpha", attrs.alpha)) + return jsUndefined(); + if (!dictionary.tryGetProperty("depth", attrs.depth)) + return jsUndefined(); + if (!dictionary.tryGetProperty("stencil", attrs.stencil)) + return jsUndefined(); + if (!dictionary.tryGetProperty("antialias", attrs.antialias)) + return jsUndefined(); + if (!dictionary.tryGetProperty("premultipliedAlpha", attrs.premultipliedAlpha)) + return jsUndefined(); + if (!dictionary.tryGetProperty("preserveDrawingBuffer", attrs.preserveDrawingBuffer)) + return jsUndefined(); + + attrs = WebGLContextAttributes::create(attrs); + } + } +#endif + + return jsBoolean(canvas->supportsContext(contextId, attrs.get())); +} Not sure if this compiles.
Created attachment 203623 [details] Patch
Comment on attachment 203623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203623&action=review Just a few minor questions. > Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:122 > + if (!dictionary.tryGetProperty("antialias", graphicattrs.premultipliedAlpha)) > + return jsUndefined(); Is this correct? antialias corresponds to premultiplied? I don’t think that sounds right. > Source/WebCore/html/HTMLCanvasElement.cpp:219 > + if (m_context && !m_context->is2d()) > + return false; > + return true; Might as well make this return (!m_context || m_context->is2d()) > Source/WebCore/html/HTMLCanvasElement.cpp:235 > + if (m_context && !m_context->is3d()) > + return false; > + return true; Same here. > LayoutTests/fast/canvas/webgl/canvas-supports-context.html:33 > +function check_context(type_of_context) { Please put this at the top of the <script> > LayoutTests/fast/canvas/webgl/canvas-supports-context.html:59 > + testPassed("supportsContext('" + type_of_context + "') is consistent with getContext('" + type_of_context + "')"); > + > +if (window.testRunner) > + testRunner.notifyDone(); > +} I’m a bit confused here. It looks like the check_context function includes this notifyDone(). Is that intentional? The indentation implies not.
Comment on attachment 203623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203623&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:226 > + if (settings && settings->webGLEnabled() > +#if !PLATFORM(GTK) && !PLATFORM(EFL) && !PLATFORM(QT) > + && settings->acceleratedCompositingEnabled() > +#endif I'm a bit confused about this.
Created attachment 203625 [details] Patch
Comment on attachment 203625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review I’m ok with this, although I’d like to see some tests for the attributes. I’ll leave it open for Sam to review the custom binding part. > Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:126 > + GraphicsContext3D::Attributes graphicattrs; > + if (!dictionary.tryGetProperty("alpha", graphicattrs.alpha)) > + return jsUndefined(); > + if (!dictionary.tryGetProperty("depth", graphicattrs.depth)) > + return jsUndefined(); > + if (!dictionary.tryGetProperty("stencil", graphicattrs.stencil)) > + return jsUndefined(); > + if (!dictionary.tryGetProperty("antialias", graphicattrs.antialias)) > + return jsUndefined(); > + if (!dictionary.tryGetProperty("premultipliedAlpha", graphicattrs.premultipliedAlpha)) > + return jsUndefined(); > + if (!dictionary.tryGetProperty("preserveDrawingBuffer", graphicattrs.preserveDrawingBuffer)) > + return jsUndefined(); Is there any way we can test this code? > Source/WebCore/html/HTMLCanvasElement.cpp:228 > + // Accept the legacy "webkit-3d" name as well as the provisional "experimental-webgl" name. > + bool is3dContext = (type == "webkit-3d") || (type == "experimental-webgl"); > + if (is3dContext) Sorry, now that I see this.. it might as well be just an if statement - no need for local variable.
Created a new bug that calls for creation of a helper method to check that context attributes are well-formed. (https://bugs.webkit.org/show_bug.cgi?id=117169)
Comment on attachment 203625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review >> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:126 >> + return jsUndefined(); > > Is there any way we can test this code? Yes; I think modifying LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias for supportsContext. Ideally, this chunk of code should probably be refactored, as it's called in both getContext and supportsContext
Comment on attachment 203625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review >>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:126 >>> + return jsUndefined(); >> >> Is there any way we can test this code? > > Yes; I think modifying LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias for supportsContext. Ideally, this chunk of code should probably be refactored, as it's called in both getContext and supportsContext Actually, I think the best way to test this chunk of code is to use it in getContext and then run (and modifying as needed) LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html. I don't think testing it falls under the domain of supportsContext, since supportsContext doesn't actually create a context. I think we should siphon this exception handling as a separate bug, testing/handling it for both getContext and supportsContext, and simply using the non-exception handling version of the code (that's currently used in getContext) for this patch. Thoughts?
Comment on attachment 203625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review Need a lot more test coverage. > Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:112 > + JSDictionary dictionary(exec, initializerObject); Why are we using JSDictionary here instead of using JSObject functions for getting values directly? Even if we do need a helper it should just be a get function helper, not a while class. I don’t understand why we need a class to get properties out of a JavaScript object. I know there is existing code doing this, but that existing code is not good. >>>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:126 >>>> + return jsUndefined(); >>> >>> Is there any way we can test this code? >> >> Yes; I think modifying LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias for supportsContext. Ideally, this chunk of code should probably be refactored, as it's called in both getContext and supportsContext > > Actually, I think the best way to test this chunk of code is to use it in getContext and then run (and modifying as needed) LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html. I don't think testing it falls under the domain of supportsContext, since supportsContext doesn't actually create a context. I think we should siphon this exception handling as a separate bug, testing/handling it for both getContext and supportsContext, and simply using the non-exception handling version of the code (that's currently used in getContext) for this patch. Thoughts? It seems strange to just silently return undefined if one of these properties is missing. I could imagine instead raising an exception when this happens, or defining a default value for use when the property is not present. We need test cases covering that behavior. > Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:133 > + bool supportsContext = canvas->supportsContext(contextId, attrs.get()); > + return jsBoolean(supportsContext); I don’t think the local variable is helpful here. I suggest combining these two lines into one. > Source/WebCore/html/HTMLCanvasElement.cpp:212 > + // FIXME: Provide implementation that accounts for attributes. Bugzilla bug 117093 > + // https://bugs.webkit.org/show_bug.cgi?id=117093 It doesn’t seem right to me that we should land this patch with untestable code that parses the attributes. We should hold off until we have both halves instead of landing untestable code. > Source/WebCore/html/HTMLCanvasElement.cpp:217 > + return (!m_context || m_context->is2d()); In WebKit coding style we normally don’t use the extra parentheses here. What is the !m_context case about? I’d like to see test cases covering this. > Source/WebCore/html/HTMLCanvasElement.cpp:224 > +#if !PLATFORM(GTK) && !PLATFORM(EFL) && !PLATFORM(QT) > + && settings->acceleratedCompositingEnabled() > +#endif This list of platforms is ugly in an #if here. And it’s hard to understand. What makes these platforms special? Instead we should encapsulate this rule in some clearer way. Maybe a flag WEBGL_REQUIRES_ACCELERATED_COMPOSITING or something like that. If this exists somewhere else in the file we should refactor so the list only appears once.
Comment on attachment 203625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review >>>>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:126 >>>>> + return jsUndefined(); >>>> >>>> Is there any way we can test this code? >>> >>> Yes; I think modifying LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias for supportsContext. Ideally, this chunk of code should probably be refactored, as it's called in both getContext and supportsContext >> >> Actually, I think the best way to test this chunk of code is to use it in getContext and then run (and modifying as needed) LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html. I don't think testing it falls under the domain of supportsContext, since supportsContext doesn't actually create a context. I think we should siphon this exception handling as a separate bug, testing/handling it for both getContext and supportsContext, and simply using the non-exception handling version of the code (that's currently used in getContext) for this patch. Thoughts? > > It seems strange to just silently return undefined if one of these properties is missing. I could imagine instead raising an exception when this happens, or defining a default value for use when the property is not present. We need test cases covering that behavior. Regarding undefined return, I agree. For getContext, the default values are set at WebGLRendering::create, line 423. Refactoring getContext's method of setting context attributes (starting at line 52) so both getContext and supportsContext can use it. Will add test cases to check that defaults are set properly in LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html.
Comment on attachment 203625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review >> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:112 >> + JSDictionary dictionary(exec, initializerObject); > > Why are we using JSDictionary here instead of using JSObject functions for getting values directly? Even if we do need a helper it should just be a get function helper, not a while class. I don’t understand why we need a class to get properties out of a JavaScript object. I know there is existing code doing this, but that existing code is not good. I don't agree. JSDictionary is just a helper class that encapsulates all this object access including exception checking. I agree that a class is probably not necessary, and we could probably do this all with just functions that take an Exec and a JSObject. >>>>>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:126 >>>>>> + return jsUndefined(); >>>>> >>>>> Is there any way we can test this code? >>>> >>>> Yes; I think modifying LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias for supportsContext. Ideally, this chunk of code should probably be refactored, as it's called in both getContext and supportsContext >>> >>> Actually, I think the best way to test this chunk of code is to use it in getContext and then run (and modifying as needed) LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html. I don't think testing it falls under the domain of supportsContext, since supportsContext doesn't actually create a context. I think we should siphon this exception handling as a separate bug, testing/handling it for both getContext and supportsContext, and simply using the non-exception handling version of the code (that's currently used in getContext) for this patch. Thoughts? >> >> It seems strange to just silently return undefined if one of these properties is missing. I could imagine instead raising an exception when this happens, or defining a default value for use when the property is not present. We need test cases covering that behavior. > > Regarding undefined return, I agree. For getContext, the default values are set at WebGLRendering::create, line 423. Refactoring getContext's method of setting context attributes (starting at line 52) so both getContext and supportsContext can use it. Will add test cases to check that defaults are set properly in LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html. Darin is not correct here (and it pains me to have to write that). tryGetProperty returns false in the case that an exception is throw. When an exception is thrown, it is idiomatic to return jsUndefined(). This totally can be tested by the way. One aspect that you can test is that if the property getter throws, we correctly propagate the exception. Something like: shouldThrow("canvas.supportsContext('webkit-3d"', { get alpha() { throw 'Custom Error'; } } )"); >> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:133 >> + return jsBoolean(supportsContext); > > I don’t think the local variable is helpful here. I suggest combining these two lines into one. Agreed.
Created attachment 203639 [details] Patch
(In reply to comment #37) I just saw Sam's comments (after uploading new patch). On further inspection, I don't think toBoolean will throw an exception in the current implementation because 1) we're first checking that the property exists 2) toBoolean just returns false in the case that the object it's acting on is malformed or null. Let me know your thoughts on the new patch. > (From update of attachment 203625 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review > > >> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:112 > >> + JSDictionary dictionary(exec, initializerObject); > > > > Why are we using JSDictionary here instead of using JSObject functions for getting values directly? Even if we do need a helper it should just be a get function helper, not a while class. I don’t understand why we need a class to get properties out of a JavaScript object. I know there is existing code doing this, but that existing code is not good. > > I don't agree. JSDictionary is just a helper class that encapsulates all this object access including exception checking. I agree that a class is probably not necessary, and we could probably do this all with just functions that take an Exec and a JSObject. > > >>>>>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:126 > >>>>>> + return jsUndefined(); > >>>>> > >>>>> Is there any way we can test this code? > >>>> > >>>> Yes; I think modifying LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias for supportsContext. Ideally, this chunk of code should probably be refactored, as it's called in both getContext and supportsContext > >>> > >>> Actually, I think the best way to test this chunk of code is to use it in getContext and then run (and modifying as needed) LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html. I don't think testing it falls under the domain of supportsContext, since supportsContext doesn't actually create a context. I think we should siphon this exception handling as a separate bug, testing/handling it for both getContext and supportsContext, and simply using the non-exception handling version of the code (that's currently used in getContext) for this patch. Thoughts? > >> > >> It seems strange to just silently return undefined if one of these properties is missing. I could imagine instead raising an exception when this happens, or defining a default value for use when the property is not present. We need test cases covering that behavior. > > > > Regarding undefined return, I agree. For getContext, the default values are set at WebGLRendering::create, line 423. Refactoring getContext's method of setting context attributes (starting at line 52) so both getContext and supportsContext can use it. Will add test cases to check that defaults are set properly in LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html. > > Darin is not correct here (and it pains me to have to write that). tryGetProperty returns false in the case that an exception is throw. When an exception is thrown, it is idiomatic to return jsUndefined(). > > This totally can be tested by the way. One aspect that you can test is that if the property getter throws, we correctly propagate the exception. Something like: > > shouldThrow("canvas.supportsContext('webkit-3d"', { get alpha() { throw 'Custom Error'; } } )"); > > >> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:133 > >> + return jsBoolean(supportsContext); > > > > I don’t think the local variable is helpful here. I suggest combining these two lines into one. > > Agreed.
Comment on attachment 203639 [details] Patch Attachment 203639 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/672931
Comment on attachment 203639 [details] Patch Attachment 203639 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/770088
Comment on attachment 203639 [details] Patch Attachment 203639 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/771023
Comment on attachment 203639 [details] Patch Attachment 203639 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/741486
Comment on attachment 203639 [details] Patch Attachment 203639 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/700601
Comment on attachment 203639 [details] Patch Attachment 203639 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/749547
Comment on attachment 203639 [details] Patch Attachment 203639 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/665957
Comment on attachment 203625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:224 >> +#endif > > This list of platforms is ugly in an #if here. And it’s hard to understand. What makes these platforms special? Instead we should encapsulate this rule in some clearer way. Maybe a flag WEBGL_REQUIRES_ACCELERATED_COMPOSITING or something like that. If this exists somewhere else in the file we should refactor so the list only appears once. Noam Rosenthal finished a small patch to refactor the #if handling in bug 117181 that just landed.
Comment on attachment 203625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review I will be submitting a WIP patch soon that 1) refactors the parsing attributes code used in getContext and supportsContext (I'm reverting back to the original way the parsing was implemented in getContext for now but will talk to Sam in person to better understand his suggestions) 2) includes tests to a) test default values for attributes and b) test the m_context case 3) uses Noam's bug 117181 patch that refactors checking if webgl is properly set >> Source/WebCore/html/HTMLCanvasElement.cpp:212 >> + // https://bugs.webkit.org/show_bug.cgi?id=117093 > > It doesn’t seem right to me that we should land this patch with untestable code that parses the attributes. We should hold off until we have both halves instead of landing untestable code. The code that is parsing the attributes is already used in JSHTMLCanvasElement::getContext. In my next patch, I'm refactoring it out to a static helper method so both getContext and supportsContext parse attributes the same way. Alternatively, since supportsContext does not yet account for context attributes, in JSHTMLCanvasElement::supportsContext, I can simply instantiate a WebGLContextAttributes object and pass that in (without parsing attributes). >> Source/WebCore/html/HTMLCanvasElement.cpp:217 >> + return (!m_context || m_context->is2d()); > > In WebKit coding style we normally don’t use the extra parentheses here. > > What is the !m_context case about? I’d like to see test cases covering this. m_context is the instance of the context object; instantiated in HTMLCanvasElement::getContext. Currently, HTMLCanvasElement::getContext is implemented such that once a context is created, it can't be changed (bug 117095). Thus, if I've already created a 3d context and call supportsContext('2d'), it should return false. Looking through the canvas layout tests, I don't think any are testing this; I'll add another test in my next patch.
Created attachment 203725 [details] Patch
Comment on attachment 203725 [details] Patch Attachment 203725 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/778068
Comment on attachment 203725 [details] Patch Attachment 203725 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/700821
Comment on attachment 203725 [details] Patch Attachment 203725 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/778067
Comment on attachment 203725 [details] Patch Attachment 203725 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/778066
Comment on attachment 203725 [details] Patch Attachment 203725 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/673562
Created attachment 203727 [details] Patch
Comment on attachment 203727 [details] Patch 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.
Created attachment 203864 [details] Patch
Comment on attachment 203864 [details] Patch Attachment 203864 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/743479 New failing tests: editing/style/5065910.html
Created attachment 203866 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Comment on attachment 203864 [details] Patch Working on adding tests to check for malformed attributes inputs.
Created attachment 203895 [details] Patch
Comment on attachment 203895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203895&action=review This is WIP patch (I need to add and polish the tests for testing well-formed and malformed attributes). > LayoutTests/fast/canvas/webgl/canvas-supports-context.html:90 > + } catch (e) { This part needs to be revised and more test cases added. Sam, is there a better way to do this? I tried using shouldThrow and shouldBeUndefined but was getting a ReferenceError saying it couldn't reference/find my variables (i.e. canvas, etc.).
(In reply to comment #62) > (From update of attachment 203895 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203895&action=review > > This is WIP patch (I need to add and polish the tests for testing well-formed and malformed attributes). The right thing to do in that case is not to set either r? or r-. r- is used by a reviewer to indicate the patch has been rejected.
(In reply to comment #63) > (In reply to comment #62) > > (From update of attachment 203895 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=203895&action=review > > > > This is WIP patch (I need to add and polish the tests for testing well-formed and malformed attributes). > > The right thing to do in that case is not to set either r? or r-. r- is used by a reviewer to indicate the patch has been rejected. Is there a way to upload a patch without setting the r field? Also, how should supportsContext('webkit-3d', {alpha: 'bob'}) be handled? Should supportsContext be checking that the values of the dictionary are really booleans (before JS conversion)?
Created attachment 203954 [details] Patch
(In reply to comment #64) > (In reply to comment #63) > > (In reply to comment #62) > > > (From update of attachment 203895 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=203895&action=review > > > > > > This is WIP patch (I need to add and polish the tests for testing well-formed and malformed attributes). > > > > The right thing to do in that case is not to set either r? or r-. r- is used by a reviewer to indicate the patch has been rejected. > > Is there a way to upload a patch without setting the r field? > > Also, how should supportsContext('webkit-3d', {alpha: 'bob'}) be handled? Should supportsContext be checking that the values of the dictionary are really booleans (before JS conversion)? This behavior is defined by the Web IDL and ECMAScript specs. See in particular http://dev.w3.org/2006/webapi/WebIDL/#es-boolean . I don't think any exception should be thrown in this case; the String should be coerced into a boolean using ECMAScript's ToBoolean abstract operation.
Comment on attachment 203954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203954&action=review Much better. I didn’t carefully review the entire test, but it looks like it has pretty good coverage now. Still a few small things we could improve. > Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:84 > + if (!get3DContextAttributes(exec, attrs)) > + return jsUndefined(); The get3DContextAttributes function doesn’t really need a return value. Instead it can be: get3DContextAttributes(exec, attrs); if (exec->hadException()) return jsUndefined(); In fact, I prefer this idiom because it makes it more clear why we are returning undefined and matches what we do in other cases. > Source/WebCore/html/HTMLCanvasElement.h:97 > + static bool is2dType(const String& type) { return type == "2d"; } > + static bool is3dType(const String& type) Not sure these should really be inline functions. It clutters the header and I’m not sure there’s a significant performance boost. > Source/WebCore/html/HTMLCanvasElement.h:100 > + return (type == "webkit-3d") || (type == "experimental-webgl"); No need for the parentheses in this expression.
(In reply to comment #64) > (In reply to comment #63) > > (In reply to comment #62) > > > (From update of attachment 203895 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=203895&action=review > > > > > > This is WIP patch (I need to add and polish the tests for testing well-formed and malformed attributes). > > > > The right thing to do in that case is not to set either r? or r-. r- is used by a reviewer to indicate the patch has been rejected. > > Is there a way to upload a patch without setting the r field? webkit-patch upload —no-review.
Comment on attachment 203954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203954&action=review >> Source/WebCore/html/HTMLCanvasElement.h:97 >> + static bool is3dType(const String& type) > > Not sure these should really be inline functions. It clutters the header and I’m not sure there’s a significant performance boost. This function should be wrapped inside ENABLE(WEBGL).
Created attachment 203974 [details] Patch
Comment on attachment 203974 [details] Patch Clearing flags on attachment: 203974 Committed r151298: <http://trac.webkit.org/changeset/151298>
All reviewed patches have been landed. Closing bug.
Yay Ruth!! Thanks for getting this through.