Boris Zbarsky [:bzbarsky] Assignee
	Description • 11 years ago

Please cc anyone who might care whom I missed.

I don't know whether we need a [TreatUndefinedAs=Coerce] or something to produce our current behavior, but I'm going to assume for now that we do not.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 11 years ago

There's an open question about how overloads should work...

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 3 • 11 years ago

(I still don't get how foo(undefined) has its first argument missing, but I don't care enough to fight this.)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 11 years ago

The issue is that if you have script like this:

  function g(a) {
    foo(a);
  }

and then someone calls g like so:

  g();

then you get foo(undefined) in a natural way.  And having to write g like this:

  function g(a) {
    foo.apply(null, arguments);
  }

or worse yet (for cases when you don't pass through your arguments wholesale):

  function g(a) {
    if (arguments.length == 0) {
      foo();
    } else {
      foo(a);
    }
  }

is pretty uncalled for.

	Masatoshi Kimura [:emk]
	Comment 5 • 11 years ago

Isn't it a spec issue?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 11 years ago

Sure; there has been plenty of discussion about the spec end on public-script-coord.

	Cameron McCormack (:heycam)
	Comment 7 • 11 years ago

The spec changes have been made now.  [TreatUndefinedAs=Missing] has also been removed.

	Cameron McCormack (:heycam)
	Comment 8 • 11 years ago

The way I've handled overloads is to not consider them as being equivalent to optional arguments.  That now means that:

  void f();
  void f(long x);

is different from:

  void f(optional long x);

which is an equivalence I wanted to keep if possible, but I think we need to break it now.

When calling f(undefined), the former will select the (long) overload and convert the undefined to 0, while the latter will select the (optional long) overload and treat it as missing.

	Masatoshi Kimura [:emk]
	Comment 9 • 11 years ago

Actually overloads and optional arguments had a difference even before the change. See <https://www.w3.org/Bugs/Public/show_bug.cgi?id=19646>.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 11 years ago

1) The new algorithm no longer adjusts "argcount" based on the
   interaction of trailing undefined and [TreatUndefinedAs=Missing]
   arguments.  We never implemented this; just asserted that we didn't
   have to deal with this situation.

2) If the distinguishing argument is undefined and there is an
   overload that has an optional argument at the distinguishing
   argument index, that overload is selected.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 11 years ago

See http://lists.w3.org/Archives/Public/public-script-coord/2013OctDec/0041.html for why we need the XHR webidl change.  Ms2ger, can you review the imptests changes?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 15 • 11 years ago

Comment on attachment 813421 [details] [diff] [review]
Part 4 with test fixes and such

Review of attachment 813421 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the dom/imptests bits.

::: content/base/test/test_createHTMLDocument.html
@@ -22,1 @@
>    // Opera doesn't have a doctype: DSK-311092

We can probably remove this file.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 16 • 11 years ago

Upstreamed the test changes (and a bit more) at https://github.com/w3c/web-platform-tests/pull/361

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 11 years ago

Try run: https://tbpl.mozilla.org/?tree=Try&rev=ce24ed23155a

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 18 • 11 years ago

Comment on attachment 813212 [details] [diff] [review]
part 3.  Rework the overload resolution algorithm to WebIDL spec changes in handling of optional arguments.   major changes are as follows:

Review of attachment 813212 [details] [diff] [review]:
-----------------------------------------------------------------

I managed to convince myself this is correct.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 19 • 11 years ago

   https://hg.mozilla.org/integration/mozilla-inbound/rev/877a227c502f
   https://hg.mozilla.org/integration/mozilla-inbound/rev/d8636e485e85
   https://hg.mozilla.org/integration/mozilla-inbound/rev/61092280cb2a
   https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf11ae08ba2

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/877a227c502f
https://hg.mozilla.org/mozilla-central/rev/d8636e485e85
https://hg.mozilla.org/mozilla-central/rev/61092280cb2a
https://hg.mozilla.org/mozilla-central/rev/ccf11ae08ba2

	Ed Morley [:emorley]
	Comment 21 • 11 years ago

Backed out for bug 926305:
remote:   https://hg.mozilla.org/mozilla-central/rev/f313d33bdbc4
remote:   https://hg.mozilla.org/mozilla-central/rev/8a2bb2622978
remote:   https://hg.mozilla.org/mozilla-central/rev/b5dc6d9578e7
remote:   https://hg.mozilla.org/mozilla-central/rev/fcddc11fb2ef

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 22 • 11 years ago

See bug 926305 comment 4.  We need to fix Node.cloneNode just like we fixed XMLHttpRequest.open... and add a test.

I grepped our IDL; the only other API that has this optional-boolean-defaulting-true pattern is Element.scrollIntoView.  I'm going to fix that one as well.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 11 years ago

Note to self: need to fix importNode too.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 25 • 11 years ago

   https://hg.mozilla.org/integration/mozilla-inbound/rev/dfaf4e97dc29
   https://hg.mozilla.org/integration/mozilla-inbound/rev/e052d7967418
   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd1cfd84378d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/250ae9645c79

	Ed Morley [:emorley]
	Comment 26 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/dfaf4e97dc29
https://hg.mozilla.org/mozilla-central/rev/e052d7967418
https://hg.mozilla.org/mozilla-central/rev/cd1cfd84378d
https://hg.mozilla.org/mozilla-central/rev/250ae9645c79