Bug 70117 – Expose HTMLCanvasElement supportsContext

• Component: DOM
• Status: RESOLVED FIXED

[Theresa O'Connor]

JavaScript developers wanting to detect WebGL might think to write a check like so:

if (window.WebGLRenderingContext) {
 ...
}

but window.WebGLRenderingContext is defined even when WebGL is disabled.

[Radar WebKit Bug Importer]

<rdar://problem/10287834>

[Kenneth Russell]

Note that in various ports we've fluctuated on this decision. Currently, in Chromium, we provide the WebGLRenderingContext property on the window even if WebGL is disabled on the command line. On the other hand, at least until WebGL is enabled by default in Safari, it may make sense to only expose this property when it's enabled.

The rationale is that the error message applications would likely print if window.WebGLRenderingContext is undefined is "Your browser doesn't support WebGL", not "Either your browser doesn't support WebGL, or it was disabled via a preference setting".

http://get.webgl.org/ is the canonical diagnostic of WebGL support. We can update the error messages there to better match browsers' behavior.

[Dean Jackson]

The reason we want this is that on Apple systems tools like Modernizr test for WebGL by creating a context, which causes us to switch to the discrete GPU. This is particularly annoying on pages that have zero intention of using WebGL, but include the stock Modernizr script.

[Dean Jackson]

The code on get.webgl.org does this:


    if (gl == null) {
        try { gl = canvas.getContext("experimental-webgl"); experimental = true; }
        catch (x) { gl = null; }
    }

Creating a context just to test if WebGL is enabled is exactly what we want to avoid. It's OK for get.webgl.org because that has to be a conclusive result, but for scripts like modernizr, doing this on EVERY load of a page, even those that will never use WebGL, is a huge waste (and may trigger side effects like on OS X where we jump to a discrete GPU).

I think we need to come up with another method. I'm not sure about window.WebGLRenderingContext being hidden when WebGL is not enabled.

[Dean Jackson]

The reason I don't like making that particular attribute undefined, is that we'd probably have to do it for all the WebGL attributes on window:


        attribute [Conditional=WEBGL] WebGLActiveInfoConstructor WebGLActiveInfo;
        attribute [Conditional=WEBGL] WebGLBufferConstructor WebGLBuffer;
        attribute [Conditional=WEBGL] WebGLFramebufferConstructor WebGLFramebuffer;
        attribute [Conditional=WEBGL] WebGLProgramConstructor WebGLProgram;
        attribute [Conditional=WEBGL] WebGLRenderbufferConstructor WebGLRenderbuffer;
        attribute [Conditional=WEBGL] WebGLRenderingContextConstructor WebGLRenderingContext;
        attribute [Conditional=WEBGL] WebGLShaderConstructor WebGLShader;
        attribute [Conditional=WEBGL] WebGLShaderPrecisionFormatConstructor WebGLShaderPrecisionFormat;
        attribute [Conditional=WEBGL] WebGLTextureConstructor WebGLTexture;
        attribute [Conditional=WEBGL] WebGLUniformLocationConstructor WebGLUniformLocation;

I think a better idea is if a canvas element could return a list of context names it supports. e.g.

canvas.getSupportedContexts() -> ["2d", "webkit-3d", "experimental-webgl"];

(probably prefixed with webkit for now)

[Dean Jackson]

Actually, canvas.supportsContext("experimental-webgl", options) -> bool might be better, as long as testing the option doesn't require us to create a context.

[Dean Jackson]

I sent a proposal to whatwg and html-wg

http://www.w3.org/mid/[e-mail]

Renaming this bug.

[Dean Jackson]

As jamesr pointed out on IRC, there may be instances where supportsContext returns true but getContext fails. I think that's ok.

The suggestion is that supportsContext is the implementation saying if it does support a context in theory. A return value of false is a clear indication that getContext will fail.

This would allow an implementation that supports WebGL but not on particular hardware to return false.

[James Robinson]

We've seen this on economist.com (using modernizr) in the past, although I believe it doesn't do that any more.  I think a soft-promise type API could work better - supportsContext("...") -> false means we definitely won't return non-null for getContext("..."), but we're still allowed to return null for getContext("...") even after saying true to supportsContext("...")

[Ryan]

Modernizr no longer creates a webgl context due to the reasons you all have specified it does a soft detect, window.WebGLRenderingContext, that is very prone to a false positive (which is mentioned). So this would be very welcome.

[Ian 'Hixie' Hickson]

http://html5.org/tools/web-apps-tracker?from=7481&to=7482

[Dean Jackson]

(In reply to comment #11)
> http://html5.org/tools/web-apps-tracker?from=7481&to=7482

Excellent. If anyone watching wants to take this, feel free.

[Ruth Fong]

Created attachment 203462 [details]
Patch

[WebKit Commit Bot]

Attachment 203462 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/webgl/canvas-supports-context-expected.txt', u'LayoutTests/fast/canvas/webgl/canvas-supports-context.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp', u'Source/WebCore/html/HTMLCanvasElement.cpp', u'Source/WebCore/html/HTMLCanvasElement.h', u'Source/WebCore/html/HTMLCanvasElement.idl']" exit_code: 1
Source/WebCore/html/HTMLCanvasElement.h:95:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 203462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review

Very close!

> Source/WebCore/ChangeLog:15
> +        * bindings/js/JSHTMLCanvasElementCustom.cpp:
> +        (WebCore::JSHTMLCanvasElement::supportsContext):
> +        * html/HTMLCanvasElement.cpp:
> +        (WebCore::HTMLCanvasElement::supportsContext):

You should provide per-function comments here.

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:99
> +    HTMLCanvasElement* canvas = static_cast<HTMLCanvasElement*>(impl());
> +    const String& contextId = exec->argument(0).toString(exec)->value(exec);

You should check argumentCount to make sure there is an argument. If not, return... jsNull or jsBoolean(false)???

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:125
> +            JSObject* jsAttrs = exec->argument(1).getObject();
> +            Identifier alpha(exec, "alpha");
> +            if (jsAttrs->hasProperty(exec, alpha))
> +                webGLAttrs->setAlpha(jsAttrs->get(exec, alpha).toBoolean(exec));
> +            Identifier depth(exec, "depth");
> +            if (jsAttrs->hasProperty(exec, depth))
> +                webGLAttrs->setDepth(jsAttrs->get(exec, depth).toBoolean(exec));
> +            Identifier stencil(exec, "stencil");
> +            if (jsAttrs->hasProperty(exec, stencil))
> +                webGLAttrs->setStencil(jsAttrs->get(exec, stencil).toBoolean(exec));
> +            Identifier antialias(exec, "antialias");
> +            if (jsAttrs->hasProperty(exec, antialias))
> +                webGLAttrs->setAntialias(jsAttrs->get(exec, antialias).toBoolean(exec));
> +            Identifier premultipliedAlpha(exec, "premultipliedAlpha");
> +            if (jsAttrs->hasProperty(exec, premultipliedAlpha))
> +                webGLAttrs->setPremultipliedAlpha(jsAttrs->get(exec, premultipliedAlpha).toBoolean(exec));
> +            Identifier preserveDrawingBuffer(exec, "preserveDrawingBuffer");
> +            if (jsAttrs->hasProperty(exec, preserveDrawingBuffer))
> +                webGLAttrs->setPreserveDrawingBuffer(jsAttrs->get(exec, preserveDrawingBuffer).toBoolean(exec));
> +        }

I wonder if we should eventually collect this into a helper function we can share with WebGL - don't change this now though.

> Source/WebCore/html/HTMLCanvasElement.cpp:61
> -
> +    

Nit: added space.

> Source/WebCore/html/HTMLCanvasElement.cpp:212
> +    // FIXME - Provide implementation that accounts for attributes
> +    if (attrs) { }

Please file a bug and include the link. Don't include the empty statements.

> Source/WebCore/html/HTMLCanvasElement.cpp:215
> +    // FIXME - The code depends on the context not going away once created (as getContext
> +    // is implemented under this assumption)

Again, file a bug and include the link.

> Source/WebCore/html/HTMLCanvasElement.cpp:220
> +    if (type == "2d") {
> +        if (m_context && !m_context->is2d())
> +            return false;
> +        return true;
> +    }

My concern about this is that a script will take an existing canvas and ask supportsContext. If that canvas already has a webgl context, it will tell the user that 2d isn't supported. I think we should just always return true for 2d.

> Source/WebCore/html/HTMLCanvasElement.cpp:234
> +            if (m_context && !m_context->is3d())
> +                return false;

Same here maybe?

>> Source/WebCore/html/HTMLCanvasElement.h:95
>> +    bool supportsContext(const String&, CanvasContextAttributes* attributes = 0);
> 
> The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]

Can you have a default value for a parameter without a name? If not, ignore this warning.

> Source/WebCore/html/HTMLCanvasElement.idl:41
> +    [Custom] any supportsContext([Default=Undefined] optional DOMString contextId);

Weird that the IDL doesn't mention the attributes parameter. Not your problem.

> LayoutTests/fast/canvas/webgl/canvas-supports-context-expected.txt:7
> +PASS 2dcontext exists

Add space after context name.

> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:5
> +<script src="resources/desktop-gl-constants.js" type="text/javascript"></script>

Don't need this.

> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:7
> +<script src="resources/webgl-test.js"></script>

Nor this.

> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:16
> +if (window.initNonKhronosFramework) {
> +    window.initNonKhronosFramework(true);
> +}
> +

Nor this.

> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:52
> +	if (context) {
> +		testPassed(type_of_context + "context exists");
> +	}

WebKit style has no braces for single line blocks. There are a few below here too.

Also, this looks like an 8 space indent. Should be 4.

> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:66
> +		testFailed(type_of_context + "context exists yet canvas.supportsContext('" 
> +			+ type_of_context + "') returns false");

Put this on one line so that it becomes a single line block.

[Dean Jackson]

Comment on attachment 203462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review

>> Source/WebCore/html/HTMLCanvasElement.cpp:220
>> +    }
> 
> My concern about this is that a script will take an existing canvas and ask supportsContext. If that canvas already has a webgl context, it will tell the user that 2d isn't supported. I think we should just always return true for 2d.

Tim just convinced me I'm wrong (he always does). Please ignore.

>> Source/WebCore/html/HTMLCanvasElement.cpp:234
>> +                return false;
> 
> Same here maybe?

And this.

[Sam Weinig]

Comment on attachment 203462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:123
> +        WebGLContextAttributes* webGLAttrs = static_cast<WebGLContextAttributes*>(attrs.get());
> +        if (exec->argumentCount() > 1 && exec->argument(1).isObject()) {
> +            JSObject* jsAttrs = exec->argument(1).getObject();
> +            Identifier alpha(exec, "alpha");
> +            if (jsAttrs->hasProperty(exec, alpha))
> +                webGLAttrs->setAlpha(jsAttrs->get(exec, alpha).toBoolean(exec));
> +            Identifier depth(exec, "depth");
> +            if (jsAttrs->hasProperty(exec, depth))
> +                webGLAttrs->setDepth(jsAttrs->get(exec, depth).toBoolean(exec));
> +            Identifier stencil(exec, "stencil");
> +            if (jsAttrs->hasProperty(exec, stencil))
> +                webGLAttrs->setStencil(jsAttrs->get(exec, stencil).toBoolean(exec));
> +            Identifier antialias(exec, "antialias");
> +            if (jsAttrs->hasProperty(exec, antialias))
> +                webGLAttrs->setAntialias(jsAttrs->get(exec, antialias).toBoolean(exec));
> +            Identifier premultipliedAlpha(exec, "premultipliedAlpha");
> +            if (jsAttrs->hasProperty(exec, premultipliedAlpha))
> +                webGLAttrs->setPremultipliedAlpha(jsAttrs->get(exec, premultipliedAlpha).toBoolean(exec));
> +            Identifier preserveDrawingBuffer(exec, "preserveDrawingBuffer");
> +            if (jsAttrs->hasProperty(exec, preserveDrawingBuffer))

These toBooleans() can all throw exceptions, so we should probably be checking for exceptions.  We should also make sure we are testing all those cases as well.  Can this use a JSDictionary to simplify things?

>> Source/WebCore/html/HTMLCanvasElement.idl:41
>> +    [Custom] any supportsContext([Default=Undefined] optional DOMString contextId);
> 
> Weird that the IDL doesn't mention the attributes parameter. Not your problem.

You should make it match the spec and have a "any... arguments" second parameter.

[Ruth Fong]

Comment on attachment 203462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review

>> Source/WebCore/html/HTMLCanvasElement.cpp:212
>> +    if (attrs) { }
> 
> Please file a bug and include the link. Don't include the empty statements.

I'm not sure if this is the proper way to handle the fact that I'm not using the attributes but that the W3 definition of supportsContext supports additional arguments. I got this error when submitting the patch: 
Source/WebCore/html/HTMLCanvasElement.h:95:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]

> Source/WebCore/html/HTMLCanvasElement.cpp:216
> +    if (type == "2d") {

This method is modeled after how HTMLCanvasElement::getContext was written, but I'm not sure that's necessary.

>> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:7
>> +<script src="resources/webgl-test.js"></script>
> 
> Nor this.

Do I need this for finishTest() (which calls notifyDone)?

[Ruth Fong]

Comment on attachment 203462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review

>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:123
>> +            if (jsAttrs->hasProperty(exec, preserveDrawingBuffer))
> 
> These toBooleans() can all throw exceptions, so we should probably be checking for exceptions.  We should also make sure we are testing all those cases as well.  Can this use a JSDictionary to simplify things?

Right now JSHTMLCanvasElement::getContext is implemented like this (without exception handling) - should I still add exception handling?

[Ruth Fong]

Comment on attachment 203462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review

>>> Source/WebCore/html/HTMLCanvasElement.idl:41
>>> +    [Custom] any supportsContext([Default=Undefined] optional DOMString contextId);
>> 
>> Weird that the IDL doesn't mention the attributes parameter. Not your problem.
> 
> You should make it match the spec and have a "any... arguments" second parameter.

getContext should also have "any... arguments" second parameter; should that be added as well?

[Sam Weinig]

(In reply to comment #19)
> (From update of attachment 203462 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review
> 
> >> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:123
> >> +            if (jsAttrs->hasProperty(exec, preserveDrawingBuffer))
> > 
> > These toBooleans() can all throw exceptions, so we should probably be checking for exceptions.  We should also make sure we are testing all those cases as well.  Can this use a JSDictionary to simplify things?
> 
> Right now JSHTMLCanvasElement::getContext is implemented like this (without exception handling) - should I still add exception handling?

You should add the checking, yes.

[Sam Weinig]

(In reply to comment #20)
> (From update of attachment 203462 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review
> 
> >>> Source/WebCore/html/HTMLCanvasElement.idl:41
> >>> +    [Custom] any supportsContext([Default=Undefined] optional DOMString contextId);
> >> 
> >> Weird that the IDL doesn't mention the attributes parameter. Not your problem.
> > 
> > You should make it match the spec and have a "any... arguments" second parameter.
> 
> getContext should also have "any... arguments" second parameter; should that be added as well?

You can do that in another bug.

[Ruth Fong]

Comment on attachment 203462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203462&action=review

>>>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:123
>>>> +            if (jsAttrs->hasProperty(exec, preserveDrawingBuffer))
>>> 
>>> These toBooleans() can all throw exceptions, so we should probably be checking for exceptions.  We should also make sure we are testing all those cases as well.  Can this use a JSDictionary to simplify things?
>> 
>> Right now JSHTMLCanvasElement::getContext is implemented like this (without exception handling) - should I still add exception handling?
> 
> You should add the checking, yes.

I tried wrapping one of the toBoolean statements in a try-catch and got an error in Xcode saying that exceptions are disabled; not sure where this is set/what's it's scope.

[Ruth Fong]

Created attachment 203472 [details]
Patch

[Sam Weinig]

(In reply to comment #23)
> I tried wrapping one of the toBoolean statements in a try-catch and got an error in Xcode saying that exceptions are disabled; not sure where this is set/what's it's scope.

I should have given you some sample code, so, here you go:

+JSValue JSHTMLCanvasElement::supportsContext(ExecState* exec)
+{
+    HTMLCanvasElement* canvas = static_cast<HTMLCanvasElement*>(impl());
+    if (!exec->argumentCount())
+        return jsBoolean(false);
+
+    const String& contextId = exec->argument(0).toString(exec)->value(exec);
+    if (exec->hadException())
+        return jsUndefined();
+
+    RefPtr<CanvasContextAttributes> attrs;
+#if ENABLE(WEBGL)
+    if (contextId == "experimental-webgl" || contextId == "webkit-3d") {
+        JSValue initializerValue = exec->argument(1);
+        if (!initializerValue.isUndefinedOrNull()) {
+            // Given the above test, this will always yield an object.
+            JSObject* initializerObject = initializerValue.toObject(exec);
+
+            // Create the dictionary wrapper from the initializer object.
+            JSDictionary dictionary(exec, initializerObject);
+
+            GraphicsContext3D::Attributes attrs;
+            if (!dictionary.tryGetProperty("alpha", attrs.alpha))
+                return jsUndefined();
+            if (!dictionary.tryGetProperty("depth", attrs.depth))
+                return jsUndefined();
+            if (!dictionary.tryGetProperty("stencil", attrs.stencil))
+                return jsUndefined();
+            if (!dictionary.tryGetProperty("antialias", attrs.antialias))
+                return jsUndefined();
+            if (!dictionary.tryGetProperty("premultipliedAlpha", attrs.premultipliedAlpha))
+                return jsUndefined();
+            if (!dictionary.tryGetProperty("preserveDrawingBuffer", attrs.preserveDrawingBuffer))
+                return jsUndefined();
+
+            attrs = WebGLContextAttributes::create(attrs);
+        }
+    }
+#endif
+
+    return jsBoolean(canvas->supportsContext(contextId, attrs.get()));
+}

Not sure if this compiles.

[Ruth Fong]

Created attachment 203623 [details]
Patch

[Dean Jackson]

Comment on attachment 203623 [details]
Patch

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.

[Tim Horton]

Comment on attachment 203623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203623&action=review

> Source/WebCore/html/HTMLCanvasElement.cpp:226
> +    if (settings && settings->webGLEnabled()
> +#if !PLATFORM(GTK) && !PLATFORM(EFL) && !PLATFORM(QT)
> +        && settings->acceleratedCompositingEnabled()
> +#endif

I'm a bit confused about this.

[Tim Horton]

Comment on attachment 203623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203623&action=review

> Source/WebCore/html/HTMLCanvasElement.cpp:226
> +    if (settings && settings->webGLEnabled()
> +#if !PLATFORM(GTK) && !PLATFORM(EFL) && !PLATFORM(QT)
> +        && settings->acceleratedCompositingEnabled()
> +#endif

I'm a bit confused about this.

[Ruth Fong]

Created attachment 203625 [details]
Patch

[Dean Jackson]

Comment on attachment 203625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review

I’m ok with this, although I’d like to see some tests for the attributes. I’ll leave it open for Sam to review the custom binding part.

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:126
> +            GraphicsContext3D::Attributes graphicattrs;
> +            if (!dictionary.tryGetProperty("alpha", graphicattrs.alpha))
> +                return jsUndefined();
> +            if (!dictionary.tryGetProperty("depth", graphicattrs.depth))
> +                return jsUndefined();
> +            if (!dictionary.tryGetProperty("stencil", graphicattrs.stencil))
> +                return jsUndefined();
> +            if (!dictionary.tryGetProperty("antialias", graphicattrs.antialias))
> +                return jsUndefined();
> +            if (!dictionary.tryGetProperty("premultipliedAlpha", graphicattrs.premultipliedAlpha))
> +                return jsUndefined();
> +            if (!dictionary.tryGetProperty("preserveDrawingBuffer", graphicattrs.preserveDrawingBuffer))
> +                return jsUndefined();

Is there any way we can test this code?

> Source/WebCore/html/HTMLCanvasElement.cpp:228
> +        // Accept the legacy "webkit-3d" name as well as the provisional "experimental-webgl" name.
> +        bool is3dContext = (type == "webkit-3d") || (type == "experimental-webgl");
> +        if (is3dContext)

Sorry, now that I see this.. it might as well be just an if statement - no need for local variable.

[Ruth Fong]

Created a new bug that calls for creation of a helper method to check that context attributes are well-formed. (https://bugs.webkit.org/show_bug.cgi?id=117169)

[Ruth Fong]

Comment on attachment 203625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review

>> 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

[Ruth Fong]

Comment on attachment 203625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review

>>> 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?

[Darin Adler]

Comment on attachment 203625 [details]
Patch

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.

[Ruth Fong]

Comment on attachment 203625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review

>>>>> 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.

[Sam Weinig]

Comment on attachment 203625 [details]
Patch

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.

[Ruth Fong]

Created attachment 203639 [details]
Patch

[Ruth Fong]

(In reply to comment #37)

I just saw Sam's comments (after uploading new patch). On further inspection, I don't think toBoolean will throw an exception in the current implementation because 1) we're first checking that the property exists 2) toBoolean just returns false in the case that the object it's acting on is malformed or null. 

Let me know your thoughts on the new patch.

> (From update of attachment 203625 [details])
> 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.

[Early Warning System Bot]

Comment on attachment 203639 [details]
Patch

Attachment 203639 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/672931

[Early Warning System Bot]

Comment on attachment 203639 [details]
Patch

Attachment 203639 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/770088

[EFL EWS Bot]

Comment on attachment 203639 [details]
Patch

Attachment 203639 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/771023

[EFL EWS Bot]

Comment on attachment 203639 [details]
Patch

Attachment 203639 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/741486

[Build Bot]

Comment on attachment 203639 [details]
Patch

Attachment 203639 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/700601

[kov's GTK+ EWS bot]

Comment on attachment 203639 [details]
Patch

Attachment 203639 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/749547

[Build Bot]

Comment on attachment 203639 [details]
Patch

Attachment 203639 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/665957

[Ruth Fong]

Comment on attachment 203625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review

>> Source/WebCore/html/HTMLCanvasElement.cpp:224
>> +#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.

Noam Rosenthal finished a small patch to refactor the #if handling in bug 117181 that just landed.

[Ruth Fong]

Comment on attachment 203625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203625&action=review

I will be submitting a WIP patch soon that 
1) refactors the parsing attributes code used in getContext and supportsContext (I'm reverting back to the original way the parsing was implemented in getContext for now but will talk to Sam in person to better understand his suggestions)
2) includes tests to a) test default values for attributes and b) test the m_context case
3) uses Noam's bug 117181 patch that refactors checking if webgl is properly set

>> Source/WebCore/html/HTMLCanvasElement.cpp:212
>> +    // 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.

The code that is parsing the attributes is already used in JSHTMLCanvasElement::getContext. 

In my next patch, I'm refactoring it out to a static helper method so both getContext and supportsContext parse attributes the same way. Alternatively, since supportsContext does not yet account for context attributes, in JSHTMLCanvasElement::supportsContext, I can simply instantiate a WebGLContextAttributes object and pass that in (without parsing attributes).

>> 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.

m_context is the instance of the context object; instantiated in HTMLCanvasElement::getContext. Currently, HTMLCanvasElement::getContext is implemented such that once a context is created, it can't be changed (bug 117095). Thus, if I've already created a 3d context and call supportsContext('2d'), it should return false. 

Looking through the canvas layout tests, I don't think any are testing this; I'll add another test in my next patch.

[Ruth Fong]

Created attachment 203725 [details]
Patch

[EFL EWS Bot]

Comment on attachment 203725 [details]
Patch

Attachment 203725 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/778068

[EFL EWS Bot]

Comment on attachment 203725 [details]
Patch

Attachment 203725 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/700821

[Early Warning System Bot]

Comment on attachment 203725 [details]
Patch

Attachment 203725 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/778067

[Early Warning System Bot]

Comment on attachment 203725 [details]
Patch

Attachment 203725 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/778066

[Build Bot]

Comment on attachment 203725 [details]
Patch

Attachment 203725 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/673562

[Ruth Fong]

Created attachment 203727 [details]
Patch

[Sam Weinig]

Comment on attachment 203727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203727&action=review

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:47
> +static PassRefPtr<CanvasContextAttributes> get3DContextAttributes(ExecState* exec)
> +{

This should return a PassRefPtr<WebGLContextAttributes>, since that is what it always returns.

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:71
> +    RefPtr<CanvasContextAttributes> attrs = WebGLContextAttributes::create();
> +    WebGLContextAttributes* webGLAttrs = static_cast<WebGLContextAttributes*>(attrs.get());
> +    if (exec->argumentCount() > 1 && exec->argument(1).isObject()) {
> +        JSObject* jsAttrs = exec->argument(1).getObject();
> +        Identifier alpha(exec, "alpha");
> +        if (jsAttrs->hasProperty(exec, alpha))
> +            webGLAttrs->setAlpha(jsAttrs->get(exec, alpha).toBoolean(exec));
> +        Identifier depth(exec, "depth");
> +        if (jsAttrs->hasProperty(exec, depth))
> +            webGLAttrs->setDepth(jsAttrs->get(exec, depth).toBoolean(exec));
> +        Identifier stencil(exec, "stencil");
> +        if (jsAttrs->hasProperty(exec, stencil))
> +            webGLAttrs->setStencil(jsAttrs->get(exec, stencil).toBoolean(exec));
> +        Identifier antialias(exec, "antialias");
> +        if (jsAttrs->hasProperty(exec, antialias))
> +            webGLAttrs->setAntialias(jsAttrs->get(exec, antialias).toBoolean(exec));
> +        Identifier premultipliedAlpha(exec, "premultipliedAlpha");
> +        if (jsAttrs->hasProperty(exec, premultipliedAlpha))
> +            webGLAttrs->setPremultipliedAlpha(jsAttrs->get(exec, premultipliedAlpha).toBoolean(exec));
> +        Identifier preserveDrawingBuffer(exec, "preserveDrawingBuffer");
> +        if (jsAttrs->hasProperty(exec, preserveDrawingBuffer))
> +            webGLAttrs->setPreserveDrawingBuffer(jsAttrs->get(exec, preserveDrawingBuffer).toBoolean(exec));
> +    }
> +    return attrs.release();

I still think the JSDictionary version was better.  This is not checking for exceptions (the JSDictionary version was).

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:112
> +    if (exec->hadException())
> +        return jsBoolean(false);

It is idiomatic to return jsUndefined() in the case of an exception.

[Ruth Fong]

Created attachment 203864 [details]
Patch

[Build Bot]

Comment on attachment 203864 [details]
Patch

Attachment 203864 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/743479

New failing tests:
editing/style/5065910.html

[Build Bot]

Created attachment 203866 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Ruth Fong]

Comment on attachment 203864 [details]
Patch

Working on adding tests to check for malformed attributes inputs.

[Ruth Fong]

Created attachment 203895 [details]
Patch

[Ruth Fong]

Comment on attachment 203895 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203895&action=review

This is WIP patch (I need to add and polish the tests for testing well-formed and malformed attributes).

> LayoutTests/fast/canvas/webgl/canvas-supports-context.html:90
> +    } catch (e) {

This part needs to be revised and more test cases added. 

Sam, is there a better way to do this? I tried using shouldThrow and shouldBeUndefined but was getting a ReferenceError saying it couldn't reference/find my variables (i.e. canvas, etc.).

[Ryosuke Niwa]

(In reply to comment #62)
> (From update of attachment 203895 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203895&action=review
> 
> This is WIP patch (I need to add and polish the tests for testing well-formed and malformed attributes).

The right thing to do in that case is not to set either r? or r-. r- is used by a reviewer to indicate the patch has been rejected.

[Ruth Fong]

(In reply to comment #63)
> (In reply to comment #62)
> > (From update of attachment 203895 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=203895&action=review
> > 
> > This is WIP patch (I need to add and polish the tests for testing well-formed and malformed attributes).
> 
> The right thing to do in that case is not to set either r? or r-. r- is used by a reviewer to indicate the patch has been rejected.

Is there a way to upload a patch without setting the r field?

Also, how should supportsContext('webkit-3d', {alpha: 'bob'}) be handled? Should supportsContext be checking that the values of the dictionary are really booleans (before JS conversion)?

[Ruth Fong]

Created attachment 203954 [details]
Patch

[Kenneth Russell]

(In reply to comment #64)
> (In reply to comment #63)
> > (In reply to comment #62)
> > > (From update of attachment 203895 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=203895&action=review
> > > 
> > > This is WIP patch (I need to add and polish the tests for testing well-formed and malformed attributes).
> > 
> > The right thing to do in that case is not to set either r? or r-. r- is used by a reviewer to indicate the patch has been rejected.
> 
> Is there a way to upload a patch without setting the r field?
> 
> Also, how should supportsContext('webkit-3d', {alpha: 'bob'}) be handled? Should supportsContext be checking that the values of the dictionary are really booleans (before JS conversion)?

This behavior is defined by the Web IDL and ECMAScript specs. See in particular http://dev.w3.org/2006/webapi/WebIDL/#es-boolean . I don't think any exception should be thrown in this case; the String should be coerced into a boolean using ECMAScript's ToBoolean abstract operation.

[Darin Adler]

Comment on attachment 203954 [details]
Patch

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.

[Ryosuke Niwa]

(In reply to comment #64)
> (In reply to comment #63)
> > (In reply to comment #62)
> > > (From update of attachment 203895 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=203895&action=review
> > > 
> > > This is WIP patch (I need to add and polish the tests for testing well-formed and malformed attributes).
> > 
> > The right thing to do in that case is not to set either r? or r-. r- is used by a reviewer to indicate the patch has been rejected.
> 
> Is there a way to upload a patch without setting the r field?

webkit-patch upload —no-review.

[Ryosuke Niwa]

Comment on attachment 203954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203954&action=review

>> Source/WebCore/html/HTMLCanvasElement.h:97
>> +    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.

This function should be wrapped inside ENABLE(WEBGL).

[Ruth Fong]

Created attachment 203974 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 203974 [details]
Patch

Clearing flags on attachment: 203974

Committed r151298: <http://trac.webkit.org/changeset/151298>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

Yay Ruth!! Thanks for getting this through.