Since r277543, which introduced canvas settings handling, imported/w3c/web-platform-tests/html/canvas/offscreen/the-offscreen-canvas/2d.getcontext.extraargs.html and the worker equivalent have been failing. This is due to a TypeError when trying to convert the non-object arguments into a dictionary. Reading the spec, both the section on getContext[1] and the section on context creation[2], it seems that arguments are expected to be converted to either an object or null before being converted to a settings dictionary. As we lack this step, there are more situations where we throw an exception when a compliant browser wouldn't. [1] https://html.spec.whatwg.org/multipage/canvas.html#dom-offscreencanvas-getcontext [2] https://html.spec.whatwg.org/multipage/canvas.html#offscreen-2d-context-creation-algorithm Ms2ger is working on more tests for this in WPT, we should fix this bug and sync those tests here once they exist.
Created attachment 433322 [details] Patch
Created attachment 433327 [details] Patch
Created attachment 433328 [details] Patch
<rdar://problem/80625487>
Comment on attachment 433328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433328&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:288 > + auto settings = convert<IDLDictionary<CanvasRenderingContext2DSettings>>(state, (!arguments.isEmpty() && arguments[0].isObject()) ? arguments[0].get() : JSC::jsUndefined()); I don't think we need the isEmpty() check if we are checking isObject(). That said, I would perfer we make this a bit closer o the spec language (e.g. https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext) and use jsNull() if not an object).
(In reply to Sam Weinig from comment #5) > Comment on attachment 433328 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433328&action=review > > > Source/WebCore/html/HTMLCanvasElement.cpp:288 > > + auto settings = convert<IDLDictionary<CanvasRenderingContext2DSettings>>(state, (!arguments.isEmpty() && arguments[0].isObject()) ? arguments[0].get() : JSC::jsUndefined()); > > I don't think we need the isEmpty() check if we are checking isObject(). > That said, I would perfer we make this a bit closer o the spec language > (e.g. > https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext) > and use jsNull() if not an object). We still need the check as arguments can be empty, but from this I'm guessing what you're saying is it should be undefined (no arguments), null (argument that isn't an object) or dictionary (at least one object argument specified that converts without an exception). I don't know that there's any observable side-effect from what you're suggesting, but I'll amend the patch.
Created attachment 433862 [details] Patch
(In reply to Chris Lord from comment #6) > (In reply to Sam Weinig from comment #5) > > Comment on attachment 433328 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=433328&action=review > > > > > Source/WebCore/html/HTMLCanvasElement.cpp:288 > > > + auto settings = convert<IDLDictionary<CanvasRenderingContext2DSettings>>(state, (!arguments.isEmpty() && arguments[0].isObject()) ? arguments[0].get() : JSC::jsUndefined()); > > > > I don't think we need the isEmpty() check if we are checking isObject(). > > That said, I would perfer we make this a bit closer o the spec language > > (e.g. > > https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext) > > and use jsNull() if not an object). > > We still need the check as arguments can be empty, but from this I'm > guessing what you're saying is it should be undefined (no arguments), null > (argument that isn't an object) or dictionary (at least one object argument > specified that converts without an exception). > > I don't know that there's any observable side-effect from what you're > suggesting, but I'll amend the patch. Apologies, I totally read that wrong. I read it as arguments[0].isEmpty() when that's not what it is at all :(. I think you can keep that part the same, and just replace the jsUndefined() with jsNull(), but either will result in the same thing so r=me.
Comment on attachment 433862 [details] Patch I guess this doesn't hurt and it does read closer to the spec, even if it doesn't make a difference, so let's go for it :)
Committed r280084 (239809@main): <https://commits.webkit.org/239809@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433862 [details].