[Webkit-unassigned] [Bug 70117] Expose HTMLCanvasElement supportsContext

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=70117





--- Comment #37 from Sam Weinig <sam at webkit.org>  2013-06-03 17:29:56 PST ---
(From update of attachment 203625)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.