[webkit-reviews] review granted: [Bug 70117] Expose HTMLCanvasElement supportsContext : [Attachment 203954] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 203954: Patch
https://bugs.webkit.org/attachment.cgi?id=203954&action=review

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