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

RESOLVED FIXED in mozilla23

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

Trunk
mozilla23
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

I think we do JSObject* for now, which is completely sucky.
OS: Mac OS X → All
Hardware: x86 → All
Duplicate of this bug: 847416
Blocks: 850430
Blocks: 847376
Created attachment 743800 [details] [diff] [review]
part 1.  Introduce a simple mozilla::dom::Date for use in WebIDL bindings.
Attachment #743800 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Created attachment 743801 [details] [diff] [review]
part 2.  Implement WebIDL parser support for Date.
Attachment #743801 - Flags: review?(bugs)
Created attachment 743804 [details] [diff] [review]
part 3.  Implement type conversions for Date in WebIDL.
Attachment #743804 - Flags: review?(bugs)
Created attachment 743807 [details] [diff] [review]
part 4.  Start using the new Date stuff for HTMLInputElement.valueAsDate.
Attachment #743807 - Flags: review?(bugs)
No longer depends on: 857138
Created attachment 744197 [details] [diff] [review]
Part 1 updated to not assume that our JS apis are sane
Attachment #744197 - Flags: review?(bugs)
Attachment #743800 - Attachment is obsolete: true
Attachment #743800 - Flags: review?(bugs)
Created attachment 744198 [details] [diff] [review]
Part 3 updated to the changes in part 1
Attachment #744198 - Flags: review?(bugs)
Attachment #743804 - Attachment is obsolete: true
Attachment #743804 - Flags: review?(bugs)

Updated

5 years ago
Attachment #743807 - Flags: review?(mounir)

Comment 8

5 years ago
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.
Attachment #744197 - Flags: review?(bugs) → review-

Comment 9

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

># HG changeset patch
># User Boris Zbarsky <bzbarsky@mit.edu>
># 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 ;)
Attachment #743801 - Flags: review?(bugs) → review+
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.
Attachment #744197 - Flags: review- → review+
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.
Attachment #744198 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #743807 - Flags: review?(bugs) → review+
> 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.
> 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...
> This could use some comments.

Actually, I just fixed it to not suck and had khuey review that change.
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.
Attachment #743807 - Flags: review?(mounir) → review+
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...
Created attachment 750860 [details] [diff] [review]
Learn how to wrap Dates in constructors
Attachment #750860 - Flags: review?(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.
Attachment #750860 - Flags: review?(bzbarsky) → review+
Blocks: 873647
You need to log in before you can comment on or make changes to this bug.