Closed Bug 816187 Opened 8 years ago Closed 8 years ago
.html Web GL 1 .0 .1 conformance test
The spec around getContextAttributes is still in flux, has changed recently to specify that it should return null when the context is lost, so we are no longer conformant; and fixing that is blocked on the issue that the current spec says that its return value is a nullable WebGLContextAttributes dictionary but the WebIDL spec says dictionaries can't be nullable and accordingly our WebIDL parser rejects that. Here's a patch that we could take in a parallel universe where nullable dictionaries would be allowed as return values. If anyone has some antimatter to spare, we might be able to make this parallel universe meet ours. See public_webgl discussion: https://www.khronos.org/webgl/public-mailing-list/archives/1211/msg00201.html
This builds.... and even runs... and even passes the conformance test! But it's just pure luck. Is there a path from here to a landable patch?
Comment on attachment 686388 [details] [diff] [review] let getContextAttributes's retval be nullable; make codegen accept nullable dictionary retvals This is generally the right approach. A few issues: 1) The check for isPrimitive() that came before the nested getWrapTemplateForType call in the nullable case should stay there and just check for isPrimitive() or isDictionary(). 2) We have to keep doing the Initializer thing. That's what makes sure the dictionary is initialized properly so that callees only have to set the non-default values in it. It doesn't make me happy, because Nullable<WebGLContextAttributesInitializer> is annoying... the other option is to rejigger things in codegen so that we can actually initialize the return value in some way before calling the method, not just declare it. Maybe that's the way to go; let me know if you want me to give that a stab. Note that we could live with the ugly for now and do the "initialize the return value" as a followup. 3) The test for nullable unions containing a dictionary went away. Also, and IDLArgument is never an IDLNullableType (though argument.type could be). 4) The right place to do the "nullable dictionary arg" check is probably in IDLMethod.validate, where we can just walk through all the argument types and check them, and not have to worry about them not being fully resolved or whatever. 5) We probably want to add some parser unit tests for this (both for allowing the nullable dictionary return value, and for still disallowing the arguments, if we don't have tests for that part already). You can run the unit tests with "make -C $objdir/dom/bindings/test check-interactive".
Attachment #686388 - Flags: feedback?(bzbarsky) → feedback+
Thanks! Applied your comments but: - have no time (due to b2g) to do the nice things to remove Initializer - wrote a test for what should succeed, but don't know how to test something that should fail - I also get a test failure: $ make -C obj-firefox-debug/dom/bindings/test check-interactive make: Entering directory `/hack/mozilla-central/obj-firefox-debug/dom/bindings/test' PYTHONDONTWRITEBYTECODE=1 /hack/mozilla-central/obj-firefox-debug/_virtualenv/bin/python /hack/mozilla-central/config/pythonpath.py \ -I/hack/mozilla-central/other-licenses/ply /hack/mozilla-central/dom/bindings/test/../parser/runtests.py -q Starting test /hack/mozilla-central/dom/bindings/test/../parser/tests/test_dictionary.py TEST-UNEXPECTED-FAIL | Dictionary arg must not be nullable Finished test /hack/mozilla-central/dom/bindings/test/../parser/tests/test_dictionary.py make: Leaving directory `/hack/mozilla-central/obj-firefox-debug/dom/bindings/test'
Comment on attachment 686849 [details] [diff] [review] let getContextAttributes's retval be nullable; make codegen accept nullable dictionary retvals >+ if nullable: >+ returnType = returnType.inner I don't think you need this part, now that I think about it. The makeDictionaryName call is already doing returnType.unroll(), which will strip off IDLNullableType. The checks for dictionaries that you left in IDLNullableType doesn't look right to me. It should only happen for arguments. You shouldn't really need TestNullDictionaryInterface. Just throw the new getter on TestInterface and TestExampleInterface? If the latter makes "make -C $objdir/dom/bindings/test" not work, comment it out for now and file a followup to make it work on me. But also, what I was looking for were additional unit tests (see dom/bindings/parser/tests/test_dictionary.py where you can just add some more tests; it has examples of how to test that something fails, so you may not need to add any more tests for stuff that should fail, I think). Those are what you're apparently failing. That's happening beacause you added your code at a point that's only reached for overloaded methods; see the len(possibleOverloads) == 1 test earlier in that function. Now that I look at this code more closely, the best place to do it is probably either earlier in validate() or actually in finish() where there's an existing argument.type.isDictionary() test. You could just test for argument.type.nullable() there, and move the union check there too.
Attachment #686849 - Flags: review?(bzbarsky) → review-
Did I get this right? Also, this python code is full of # Foo may not be a Bar if (Foo is a Bar) error("Foo may not be a Bar") Maybe comments are not useful when they say the same thing as the string below?
Attachment #686849 - Attachment is obsolete: true
Attachment #687197 - Flags: review?(bzbarsky)
Comment on attachment 687197 [details] [diff] [review] let getContextAttributes's retval be nullable; make codegen accept nullable dictionary retvals >+ if argument.type.isUnion(): >+ if argument.type.nullable(): Use a single if with "or"? >+ raise WebIDLError("An argument cannot be a nullable union " >+ "containing a dictionary", >+ [self.location, memberType.location]) Probably better to use [argument.location, memberType.location] as the second arg here. With that, looks great. r=me Please do file a followup on me to stop having to expose this "Initializer" thing to consumers, ok?
Attachment #687197 - Flags: review?(bzbarsky) → review+
Assignee: nobody → bjacob
Target Milestone: --- → mozilla20
(In reply to Boris Zbarsky (:bz) from comment #6) > Please do file a followup on me to stop having to expose this "Initializer" > thing to consumers, ok? Filed bug 817194.
Comment on attachment 687197 [details] [diff] [review] let getContextAttributes's retval be nullable; make codegen accept nullable dictionary retvals [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 798187 / Firefox 19 User impact if declined: WebGL 1.0.1 conformance failure. That is most unfortunate as we were finally about to officialize WebGL 1.0.1 conformance. Testing completed (on m-c, etc.): just landed on m-i Risk to taking this patch (and alternatives if risky): This is a small patch to our WebIDL parser and codegen, and most of it is validation so a mistake would give either a build error or a failure in the unit test covering this; the rest of the patch, on the WebGL impl, is just boilerplate, so again a mistake there should give a compilation error. Also, we are in early aurora stages. String or UUID changes made by this patch: none
Also, the good news is that the regression had not propagated to beta yet!
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #687197 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Ryan, I can't tell you enough how much I appreciate these branch landings.
You need to log in before you can comment on or make changes to this bug.