Closed Bug 478251 Opened 11 years ago Closed 11 years ago

Implement the Null and Undefined annotations from WebIDL

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 2 obsolete files)

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?
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)
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 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 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+
(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.  :-)
> 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 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+
(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.
> 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.
Attachment #397529 - Flags: review?(jst) → review+
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 on attachment 401163 [details] [diff] [review]
Updated to all comments

Looks good!
Attachment #401163 - Flags: review?(jorendorff) → review+
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+
Pushed http://hg.mozilla.org/mozilla-central/rev/39b6673348d2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 521115
Blocks: 667856
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.