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

[bugzilla-daemon at webkit.org]

Dean Jackson <dino 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 203623: Patch
https://bugs.webkit.org/attachment.cgi?id=203623&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
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.