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

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #203625|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #35 from Darin Adler <darin at apple.com>  2013-06-03 15:21:09 PST ---
(From update of attachment 203625)
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.

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