We should also then remove checks for valid double or float from the implementations (for example the FloatValidate calls in the canvas 2D rendering context).

One more note: Part 5 does change the behavior of fillText/strokeText to silently do nothing instead of throwing on non-finites, which is what the spec says right now...

Comment on attachment 682695 [details] [diff] [review] part 1. Implement parser support for 'unrestricted float' and 'unrestricted double', and for the [LenientFloat] extended attribute. Review of attachment 682695 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/parser/WebIDL.py @@ +1029,5 @@ > > def isComplete(self): > return True > > + def includesFloat(self): I think you should name this something that makes it clear that this deals with the restricted/unrestricted divide. Perhaps includesRestrictedFloat? @@ +1031,5 @@ > return True > > + def includesFloat(self): > + return False > + Why isn't isUnrestricted here? If it's because you can only call isUnrestricted on things that are isFloat, I think you should have a version that asserts false on the base class so that the failure mode is more clear. @@ +2645,5 @@ > + raise WebIDLError("[LenientFloat] used on a non-void method", > + [attr.location, self.location]) > + if not any(arg.type.includesFloat() for arg in sig[1]): > + raise WebIDLError("[LenientFloat] used on an operation with no " > + "float type arguments", no restricted float type arguments. ::: dom/bindings/parser/tests/test_float_types.py @@ +1,3 @@ > +import WebIDL > + > +def WebIDLTest(parser, harness): It's kind of nitpicky, but could you test typedefs and unions here too? @@ +71,5 @@ > + """) > + except Exception, x: > + threw = True > + harness.ok(threw, "[LenientFloat] only allowed on methods with float args") > + Extra whitespace. @@ +84,5 @@ > + """) > + except Exception, x: > + threw = True > + harness.ok(threw, "[LenientFloat] only allowed on writable attributes") > + Here too.

Comment on attachment 682697 [details] [diff] [review] part 3. Update our IDL as needed to use unrestricted float/double or [LenientFloat]. Review of attachment 682697 [details] [diff] [review]: ----------------------------------------------------------------- I'm just going to assume you did this correctly ...

Comment on attachment 682715 [details] [diff] [review] part 3 addendum. Fix the Web Audio tests to test for the right exception when passing NaN. Sorry for the delay.

Comment on attachment 683123 [details] [diff] [review] part 3 addendum. Fix the WebGL IDL as well. Review of attachment 683123 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/WebGLRenderingContext.webidl @@ +704,5 @@ > GLenum format, GLenum type, HTMLVideoElement video); // May throw DOMException > > void uniform1f(WebGLUniformLocation? location, GLfloat x); > void uniform1fv(WebGLUniformLocation? location, Float32Array v); > + void uniform1fv(WebGLUniformLocation? location, sequence<unrestricted float> v); Do you have any idea why this was sequence<float> and not sequence<GLfloat>? Can't WebIDL have sequence<typedef>?

> Do you have any idea why this was sequence<float> and not sequence<GLfloat>? Nope. That's just what the spec says to do. I can easily make it sequence<GLfloat> and raise a spec issue if you'd prefer. > Can't WebIDL have sequence<typedef>? It can.

(In reply to Boris Zbarsky (:bz) from comment #13) > > Do you have any idea why this was sequence<float> and not sequence<GLfloat>? > > Nope. That's just what the spec says to do. I can easily make it > sequence<GLfloat> and raise a spec issue if you'd prefer. That would seem cleaner to me, but not necessarily worth your time. You decide.

Done. Search-and-replace is fast. ;)

> I think you should name this something that makes it clear that this deals with the > restricted/unrestricted divide. Perhaps includesRestrictedFloat? Done. > If it's because you can only call isUnrestricted on things that are isFloat, I think > you should have a version that asserts false on the base class That was the reason, and done. Applied the other comments too.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8375549bb321 https://hg.mozilla.org/integration/mozilla-inbound/rev/c426c6ea5ff7 https://hg.mozilla.org/integration/mozilla-inbound/rev/935af8796e8f https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce2a3b3b08e https://hg.mozilla.org/integration/mozilla-inbound/rev/d0a5bc47e590

https://hg.mozilla.org/mozilla-central/rev/8375549bb321 https://hg.mozilla.org/mozilla-central/rev/c426c6ea5ff7 https://hg.mozilla.org/mozilla-central/rev/935af8796e8f https://hg.mozilla.org/mozilla-central/rev/4ce2a3b3b08e https://hg.mozilla.org/mozilla-central/rev/d0a5bc47e590