Closed
Bug 478251
Opened 16 years ago
Closed 15 years ago
Implement the Null and Undefined annotations from WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file, 2 obsolete files)
24.02 KB,
patch
|
benjamin
:
review+
jorendorff
:
review+
|
Details | Diff | Splinter Review |
WebIDL defines some IDL annotations on DOMString to indicate how |null| and |undefined| should be converted to strings. I think we should implement these.
The attached patch does so for quickstubbed methods and properties (and adds quickstubs for querySelector/querySelectorAll, so that we no completely pass jresig's tests for those).
Open issues:
1) Our default behavior is different from the WebIDL default behavior. We
should keep this for now until we annotate enough stuff.
2) Making this work for xpidl/xpconnect would take more work.
3) The syntax I'm using is Null(value) and Undefined(value) instead of the
Null=value and Undefined=value found in WebIDL, since the former is what
our idl-parsing stuff already knows about. This means we can't just drop
in those IDL files for now,
4) Should we expose eStringify to the IDL annotations so that you can do
Null(Stringify) for now until we flip the default?
Thoughts? Do we want to take the quickstub part even before we get the xpidl part done, and just restrict use of these annotations to quickstubbed members?
Assignee | ||
Comment 1•15 years ago
|
||
I think we should take this, modulo the issues I list in comment 0. If we need this for some method that's not quickstubbed, we can just quickstub it or something....
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #397529 -
Flags: superreview?(jst)
Attachment #397529 -
Flags: review?(jst)
Assignee | ||
Comment 2•15 years ago
|
||
Oh, and I suppose I should add tests for the querySelector thing and the document.write thing. If the rest of the patch is ok, I'll do that.
Assignee | ||
Comment 3•15 years ago
|
||
Comment on attachment 397529 [details] [diff] [review]
Updated to tip, with a fix for the recent document.write regression too
Jason should probably review the qsgen changes; I'd like Benjamin to look over the general idea as far as xpidl goes.
Attachment #397529 -
Flags: superreview?(jst)
Attachment #397529 -
Flags: superreview?(benjamin)
Attachment #397529 -
Flags: review?(jorendorff)
Comment 4•15 years ago
|
||
Comment on attachment 397529 [details] [diff] [review]
Updated to tip, with a fix for the recent document.write regression too
> '[domstring]':
>- " xpc_qsDOMString ${name}(cx, ${argVal}, ${argPtr});\n"
>+ " xpc_qsDOMString ${name}(cx, ${argVal}, ${argPtr},\n"
>+ " xpc_qsDOMString::e${nullBehavior},\n"
>+ " xpc_qsDOMString::e${undefinedBehavior});\n"
> " if (!${name}.IsValid())\n"
> " return JS_FALSE;\n",
When I wrote this, I duplicated code to avoid having these two run-time
parameters. Probably a mistake; it only saves a couple stack writes per
call (in the common case where the argument is a string anyway) and a
few bytes per stub of code size.
>- 'argPtr': argPtr
>+ 'argPtr': argPtr,
>+ 'nullBehavior': 'DefaultNullBehavior',
>+ 'undefinedBehavior': 'DefaultUndefinedBehavior'
> }
>
>+ if (nullBehavior is not None):
>+ params["nullBehavior"] = nullBehavior;
>+ if (undefinedBehavior is not None):
>+ params["undefinedBehavior"] = undefinedBehavior;
Don't parenthesize the expression after `if` or `while` in Python, if it
fits on one line.
Here the if statements aren't necessary, though:
+ 'nullBehavior': nullBehavior or 'DefaultNullBehavior',
+ 'undefinedBehavior': undefinedBehavior or 'DefaultUndefinedBehavior'
}
In checkStringification:
>+ if (param is not None):
>+ memberOrParam = param
>+ else:
>+ memberOrParam = member
memberOrParam = param or member
>+ if (memberOrParam.null is not None and
>+ memberOrParam.null not in ('Empty', 'Null')):
if memberOrParam.null not in (None, 'Empty', 'Null'):
But to me this seems redundant with the checks xpidl.py does. If this is
just for extra robustness against possibly someday allowing even more
values for Null and Undefined, it would be ok to make them asserts. I
would just delete them.
>+ * the WebIDL specification. eStringify means convert to the string "null"
>+ * or "undefined" respectively, via the standard JS toString() operation;
>+ * eEmpty means convert to the string ""; eNull means convert to an empty
>+ * string with the void bit set.
>+ *
>+ * Per webidl the default behavior of an unannotated interface is
The stars are not aligned!
"via the standard JS toString() operation" seems superfluous here, and I
would probably use periods instead of semicolons. (But that's just advice--do what you think works.)
r=me with those comments addressed
Attachment #397529 -
Flags: review?(jorendorff) → review+
Comment 5•15 years ago
|
||
(In reply to comment #4)
> "via the standard JS toString() operation" seems superfluous here, and I
> would probably use periods instead of semicolons. (But that's just advice--do
> what you think works.)
If you keep it, capitalize as ToString as that's the precise name of the spec-internal helper method to which you're referring.
I approve of the correct use of semicolons, by the way. :-)
Assignee | ||
Comment 6•15 years ago
|
||
> But to me this seems redundant with the checks xpidl.py does.
xpidl.py checks that there is an argument if the attribute is present, but not the possible values of that argument; it just passes it on. So this check isn't redundant.
> If this is just for extra robustness against possibly someday allowing even
> more values for Null and Undefined
It's for failing fast if someone sticks in a value we don't recognize for some reason (whether because they typoed or because they're adding a new one and forgot to update this code).
I can remove the checks if you prefer, of course. Just let me know.
I left the semicolons and "via the standard .." but addressed the other issues.
Comment 7•15 years ago
|
||
Comment on attachment 397529 [details] [diff] [review]
Updated to tip, with a fix for the recent document.write regression too
This is fine as code, but I really want tests somehow...
Attachment #397529 -
Flags: superreview?(benjamin) → superreview+
Comment 8•15 years ago
|
||
(In reply to comment #6)
> > But to me this seems redundant with the checks xpidl.py does.
>
> xpidl.py checks that there is an argument if the attribute is present, but not
> the possible values of that argument; it just passes it on. So this check
> isn't redundant.
Sorry, I misread it. The check should definitely appear in xpidl.py. Other code using xpidl.py to parse IDL would want it to check. As far as I know, it's intended to check everything it can.
Assignee | ||
Comment 9•15 years ago
|
||
> This is fine as code, but I really want tests somehow...
See comment 2. I'll add some; do you want to review them?
> The check should definitely appear in xpidl.py.
OK. I can do that.
Updated•15 years ago
|
Attachment #397529 -
Flags: review?(jst) → review+
Assignee | ||
Comment 10•15 years ago
|
||
Moved the error handling into xpidl.py; not sure how best to test this (or whether it's even possible). I did test it by hand a bit.
Added a test for write/writeln and fixed the querySelector test to pass (by removing the todos in it). In the process of adding write/writeln tests discovered that my fix didn't work for the very first argument, and hence that we do need a Stringify value for null (not undefined, where it's the default). So added that.
This should be ready to go if it looks ok.
Attachment #362027 -
Attachment is obsolete: true
Attachment #397529 -
Attachment is obsolete: true
Attachment #401163 -
Flags: review?(jorendorff)
Attachment #401163 -
Flags: review?(benjamin)
Comment 11•15 years ago
|
||
Comment on attachment 401163 [details] [diff] [review]
Updated to all comments
Looks good!
Attachment #401163 -
Flags: review?(jorendorff) → review+
Comment 12•15 years ago
|
||
Comment on attachment 401163 [details] [diff] [review]
Updated to all comments
Please file a followup about xpidl tests: they aren't hard, I meant to write some when I originally wrote xpidl.py but it kinda snuck into production without them.
Attachment #401163 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 13•15 years ago
|
||
Filed bug 518561.
Assignee | ||
Comment 14•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•