Closed Bug 767933 Opened 12 years ago Closed 12 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: peterv, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

15.93 KB, patch
khuey
: review+
Details | Diff | Splinter Review
7.57 KB, patch
khuey
: review+
Details | Diff | Splinter Review
9.16 KB, patch
khuey
: review+
Details | Diff | Splinter Review
13.25 KB, patch
khuey
: review+
Details | Diff | Splinter Review
17.28 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
2.48 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
87.21 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
6.16 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
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...
Attachment #682716 - Flags: review?(bas)
Depends on: 812742
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.
Attachment #682695 - Flags: review?(khuey) → review+
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 ...
Attachment #682697 - Flags: review?(khuey) → review+
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.
Attachment #682715 - Flags: review?(ehsan) → review+
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>?
Attachment #683123 - Flags: review?(bjacob) → review+
> 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.
Assignee: nobody → bzbarsky
Whiteboard: [needs review]
Whiteboard: [needs review] → [need review]
Attachment #682716 - Flags: review?(bas) → review?(ncameron)
Attachment #682701 - Flags: review?(bas) → review?(ncameron)
Attachment #682716 - Flags: review?(ncameron) → review?(bas)
Attachment #682701 - Flags: review?(ncameron) → review?(bas)
Attachment #682716 - Flags: review?(bas) → review+
Attachment #682701 - Flags: review?(bas) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.