742206 - Decide how we want to represent IDL Date on the C++ side

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I think we do JSObject* for now, which is completely sucky.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Introduce a simple mozilla::dom::Date for use in WebIDL bindings.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Implement WebIDL parser support for Date.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Implement type conversions for Date in WebIDL.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Start using the new Date stuff for HTMLInputElement.valueAsDate.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 updated to not assume that our JS apis are sane

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 updated to the changes in part 1

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 744197 [details] [diff] [review]
Part 1 updated to not assume that our JS apis are sane

I'm not sure about the coding style.
{ in class and method declaration should go to its own line and 
params should be in form aFoo, but bindings code use some odd style.
 

>+Date::SetTimeStamp(JSContext* cx, JSObject* objArg)
>+{
Couldn't you take JS::Handle<JSObject*> as param.

>+Date::ToDateObject(JSContext* cx, JS::Value* vp) const
And here maybe JS::MutableHandle<Value> vp


r- until rooting fixed or explained why we want more old style API.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 743801 [details] [diff] [review]
part 2.  Implement WebIDL parser support for Date.

># HG changeset patch
># User Boris Zbarsky <[e-mail]>
># Date 1367350309 14400
># Node ID dacadac04a77b018611b8c3e2349f2e2a3863294
># Parent  0e8a07f35791f91b38157836ad3e9aaed8be3f2e
>Bug 742206 part 2.  Implement WebIDL parser support for Date.  r=smaug
>
>diff --git a/dom/bindings/parser/WebIDL.py b/dom/bindings/parser/WebIDL.py
>--- a/dom/bindings/parser/WebIDL.py
>+++ b/dom/bindings/parser/WebIDL.py
>@@ -1268,23 +1268,23 @@ class IDLType(IDLObject):
> 
>     def isDictionary(self):
>         return False
> 
>     def isInterface(self):
>         return False
> 
>     def isAny(self):
>-        return self.tag() == IDLType.Tags.any and not self.isSequence()
>+        return self.tag() == IDLType.Tags.any and not self.isSequence() and not self.isArray()
> 
>     def isDate(self):
>-        return self.tag() == IDLType.Tags.date
>+        return self.tag() == IDLType.Tags.date and not self.isSequence() and not self.isArray()
> 
>     def isObject(self):
>-        return self.tag() == IDLType.Tags.object
>+        return self.tag() == IDLType.Tags.object and not self.isSequence() and not self.isArray()

This could use some comments. Why "not self.isSequence() and not self.isArray()"
Based on https://bugzilla.mozilla.org/show_bug.cgi?id=768684#c1 it hasn't been too clear to you either ;)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 744197 [details] [diff] [review]
Part 1 updated to not assume that our JS apis are sane

Ok, I guess this makes sense after all in the correct codegen.
At least in the case I was looking at we have value rooted and then
pass it as toObject() to SetTimeStamp.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 744198 [details] [diff] [review]
Part 3 updated to the changes in part 1

It is a bit ugly that we need to do something like
+        if type.isDate():
+            result = CGGeneric("Date")
+            if type.nullable():
+                result = CGTemplatedType("Nullable", result)
in many places. But don't have better suggestions.


I hope I didn't miss anything. Still slow at reviewing codegen stuff.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I'm not sure about the coding style.

I'll fix it up.

> Couldn't you take JS::Handle<JSObject*> as param.

Not easily, as you noted.

> And here maybe JS::MutableHandle<Value> vp

Also not easily, since the codegen in fact has a Value* it needs to write into.  At some point we can fix that, but we'll need to do it for all conversion functions at once...

> This could use some comments.

Will add some.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I'll fix it up.

So I put braces on own line, but left cx/obj/vp since those are more jsapi-like bits and should look like JS code, imo.  Odd style, as you said, but somewhat internally consistent...

Comment 14

[Boris Zbarsky [:bzbarsky]]

> This could use some comments.

Actually, I just fixed it to not suck and had khuey review that change.

Comment 15

[Mounir Lamouri (:mounir)]

Comment on attachment 743807 [details] [diff] [review]
part 4.  Start using the new Date stuff for HTMLInputElement.valueAsDate.

Review of attachment 743807 [details] [diff] [review]:
-----------------------------------------------------------------

r/sr=me. So good to see that leaving :)

::: dom/interfaces/html/nsIDOMHTMLInputElement.idl
@@ -68,5 @@
>             attribute DOMString             type;
>             attribute DOMString             defaultValue;
>             attribute DOMString             value;
>             attribute double                valueAsNumber;
> -  [implicit_jscontext] attribute jsval     valueAsDate;

Could you add a comment saying that valueAsDate isn't present on purpose and why? It will prevent ppl believing that it is not supported by Gecko.

Comment 16

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/b89e5dc046a6
   https://hg.mozilla.org/integration/mozilla-inbound/rev/c4013f62540e
   https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8c503744ea
   https://hg.mozilla.org/integration/mozilla-inbound/rev/2f0d5fc274ed

Comment 17

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/dffb1419bb19 to fix the unexpected-pass that I thought I'd tested for and clearly failed to...

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b89e5dc046a6
https://hg.mozilla.org/mozilla-central/rev/c4013f62540e
https://hg.mozilla.org/mozilla-central/rev/3a8c503744ea
https://hg.mozilla.org/mozilla-central/rev/2f0d5fc274ed
https://hg.mozilla.org/mozilla-central/rev/dffb1419bb19

Comment 19

[Boris Zbarsky [:bzbarsky]]

Documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Date and https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Date-struct

Comment 20

[Reuben Morais [:reuben]]

Attachment: Learn how to wrap Dates in constructors

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750860 [details] [diff] [review]
Learn how to wrap Dates in constructors

r=me, but this should go in a separate bug.