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)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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...
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.
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.
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.
Attached patch Fix (deleted) — Splinter Review
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: nobody → bzbarsky
Whiteboard: [need review]
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?
> 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.
(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
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....
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.
> 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.
Attachment #613353 - Flags: review?(peterv) → review+
Flags: in-testsuite?
Whiteboard: [need review] → [need birch merge]
Target Milestone: --- → mozilla15
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [need birch merge]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: