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

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 203625: Patch
https://bugs.webkit.org/attachment.cgi?id=203625&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.