767933 - Implement |unrestricted double/float| in the WebIDL parser and restrict other double and float values

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Implement parser support for 'unrestricted float' and 'unrestricted double', and for the [LenientFloat] extended attribute.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Implement codegen for 'unrestricted float' and 'unrestricted double', as aliases for float and double for now.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Update our IDL as needed to use unrestricted float/double or [LenientFloat].

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Make 'float' and 'double' types throw on non-finite values.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove no longer needed FloatValidate calls from canvas code.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3 addendum. Fix the Web Audio tests to test for the right exception when passing NaN.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4 addendum. Fix the Canvas tests to test for the right exception in various cases.

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 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3 addendum. Fix the WebGL IDL as well.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 11

[(no longer active)]

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 12

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

   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

Comment 18

[Ed Morley [:emorley]]

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