Last Comment Bug 742197 - Verify that unannotated string conversions are correct in Paris bindings
: Verify that unannotated string conversions are correct in Paris bindings
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: ParisBindings 740069
  Show dependency treegraph
 
Reported: 2012-04-03 22:01 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2012-04-24 17:59 PDT (History)
8 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (801 bytes, patch)
2012-04-09 11:55 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
peterv: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-03 22:01:57 PDT
Need to check that we're doing what WebIDL actually calls for; I think right now we do what quickstubs do, which may not be the same thing.

We should think about whether we want to do this before initial ship...
Comment 1 :Ms2ger 2012-04-04 02:34:34 PDT
I believe new bindings match quickstubs, which match old XPConnect, which converts null to a void nsAString. WebIDL wants "null", and I think we should move towards that.

Maybe it would be best to annotate all the Paris bindings we add with the old behaviour for now, though; we're already changing a lot of (I assume) web-compat sensitive stuff.
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-04 06:43:39 PDT
WebIDL wants "null" for non-nullable strings.  For nullable strings it wants the null passed through (in our case void nsAString).

The cases which were really web compat sensitive in the Paris bindings are all "DOMString?" in the IDL, so they're going to Just Work.

So yes, I think we should move towards what webidl wants, but I don't think we should mass-annotate with the old behavior.  The only question is whether we want to wait until we branch for 14 to flip XHR to the webidl behavior, to mitigate our initial-landing risk a bit.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-09 09:47:55 PDT
So to be precise, the per-spec WebIDL behavior mapped onto our code would be:

1)  If the string is nullable, then null is converted to a void string.
2)  If the string is nullable and [TreatUndefinedAs] is "EmptyString" then undefined is
    converted to an empty string.
3)  If the string is nullable and [TreatUndefinedAs] is "Null" then undefined is 
    converted to a void string.
4)  If the string is nullable and [TreatUndefinedAs] is "Missing" then the spec does not
    define behavior, if I read it right.  Perhaps this code cannot be entered for
    Missing?
5)  If the string is not nullable, null is converted to either "" or "null" depending on
    whether [TreatNullAs=EmptyString] is used.
6)  If the string is not nullable, undefined is converted to either "" or "undefined"
    depending on whether [TreatUndefinedAs=EmptyString] is used.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-09 11:55:20 PDT
Created attachment 613353 [details] [diff] [review]
Fix

We talked this over in the meeting today and figure we'll land it after the next aurora branchpoint.  But just to start the ball rolling...
Comment 5 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2012-04-09 19:51:17 PDT
The spec does call into the "convert an ECMAScript value to an IDL value of type DOMString" algorithm for an argument that is [TreatUndefinedAs=Missing], in cases such as:

  void f(long x,
         [TreatUndefinedAs=Missing] optional DOMString y,
         [TreatUndefinedAs=Missing] optional DOMString z);

and you call it with:

  f(1, undefined, "a");

The behaviour is defined, since when the undefined is converted to a DOMString it falls through to step 3 of #es-DOMString.  The spec doesn't actually allow for example [TreatUndefinedAs=Missing,EmptyString] to cause the above call to be treated as f(1, "", "a").  If we need this, I'll need to change the spec.  I haven't tested: does XHR need an undefined passed as the username be treated as "" if a non-undefined value is passed as the password?
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-09 19:59:14 PDT
> The behaviour is defined, since when the undefined is converted to a DOMString it falls
> through to step 3 of #es-DOMString.

Te behavior I was complaining about being undefined involves nullable strings annotated with [TreatUndefinedAs=Missing].  In that sitation, nothing even says we end up in #es-DOMString that I can tell.

> The spec doesn't actually allow for example [TreatUndefinedAs=Missing,EmptyString] to
> cause the above call to be treated as f(1, "", "a")

Hrm.  That seems ... broken to me.  I didn't realize that you could specify both Missing and EmptyString together, for that matter!

> does XHR need an undefined passed as the username be treated as "" if a non-undefined
> value is passed as the password?

No idea, but XHR uses nullable strings, and doesn't use Missing, so for its purposes what Missing does is completely irrelevant.
Comment 7 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2012-04-09 20:04:41 PDT
(In reply to Boris Zbarsky (:bz) from comment #6)
> Te behavior I was complaining about being undefined involves nullable
> strings annotated with [TreatUndefinedAs=Missing].  In that sitation,
> nothing even says we end up in #es-DOMString that I can tell.

Oh right, in that case yes the #es-nullable-type wants to say that null gets returned.  The algorithm there does need tweaking to make sure it continues on with step 2.

> Hrm.  That seems ... broken to me.  I didn't realize that you could specify
> both Missing and EmptyString together, for that matter!

Oh you can't.  But if we need it to we can allow it.

> > does XHR need an undefined passed as the username be treated as "" if a non-undefined
> > value is passed as the password?
> 
> No idea, but XHR uses nullable strings, and doesn't use Missing, so for its
> purposes what Missing does is completely irrelevant.

Huh.  I did introduce Missing for XHR:

  http://lists.w3.org/Archives/Public/public-script-coord/2012JanMar/0011.html
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-09 20:05:23 PDT
That said, the fact that XHR is not using TreatUndefinedAs=Missing may well be a spec bug in XHR, since it _does_ want that behavior, iirc....
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-09 20:07:46 PDT
I wonder whether we should file a separate bug on carefully reviewing the XHR IDL here.  It looks like what's in the spec isn't quite right.  :(

> The algorithm there does need tweaking to make sure it continues on with step 2.

Yes, that would solve the problem.

> Oh you can't.  But if we need it to we can allow it.

Let's not add that complexity until someone asks for it... ;)
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-10 11:30:09 PDT
For XHR we absolutely need all of the following to be equivalent:

xhr.send();
xhr.send(null);
xhr.send("");

I'm not sure, but I suspect that the following also should be treated the same way

xhr.send(undefined);


Note that in general I prefer 'undefined' to always be treated the same as if the argument wasn't passed at all for optional arguments. This isn't what WebIDL currently says though, but it's an open issue at TC39.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-10 11:49:20 PDT
> For XHR we absolutely need all of the following to be equivalent:

Yes, we have that behavior.  Sort of.  Because the string overload for send() is nullable.

But note that per spec as currently written those first two you list are not equivalent to the third one (though I'm not sure we do that right now).  The case that take undefined _is_ equivalent to the one that takes null per spec and in our current impl.

If you actually want all of those to be equivalent, then the IDL should be:

  void send([TreatNullAs=EmptyString,TreatUndefinedAs=EmptyString]
            optional DOMString data = "");

or so...  But again, that gives a different behavior from what the spec has right now.  Whether the spec is correct is a separate matter.
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-18 12:31:27 PDT
https://hg.mozilla.org/projects/birch/rev/28d4b29abc21
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-24 17:59:53 PDT
https://hg.mozilla.org/mozilla-central/rev/28d4b29abc21

Note You need to log in before you can comment on or make changes to this bug.