Closed
Bug 742197
Opened 13 years ago
Closed 13 years ago
Verify that unannotated string conversions are correct in Paris bindings
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
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...
Attachment #613353 -
Flags: review?(peterv)
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Comment 5•13 years ago
|
||
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?
Assignee | ||
Comment 6•13 years ago
|
||
> 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•13 years ago
|
||
(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
Assignee | ||
Comment 8•13 years ago
|
||
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....
Assignee | ||
Comment 9•13 years ago
|
||
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... ;)
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.
Assignee | ||
Comment 11•13 years ago
|
||
> 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.
Updated•13 years ago
|
Attachment #613353 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 12•13 years ago
|
||
Flags: in-testsuite?
Whiteboard: [need review] → [need birch merge]
Target Milestone: --- → mozilla15
Comment 13•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [need birch merge]
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
•