478251 - Implement the Null and Undefined annotations from WebIDL

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Make quickstubs do the right thing

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip, with a fix for the recent document.write regression too

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Jason Orendorff [:jorendorff]]

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

Comment 5

[Jeff Walden [:Waldo]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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

[Benjamin Smedberg]

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

Comment 8

[Jason Orendorff [:jorendorff]]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to all comments

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.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 401163 [details] [diff] [review]
Updated to all comments

Looks good!

Comment 12

[Benjamin Smedberg]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Filed bug 518561.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/39b6673348d2