Have the equivalent of [TreatUndefinedAs=Missing] be the default behavior for WebIDL optional arguments

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla28
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

12.19 KB, patch
Details | Diff | Splinter Review
12.86 KB, patch
khuey
: review+
Details | Diff | Splinter Review
11.85 KB, patch
khuey
: review+
Details | Diff | Splinter Review
13.52 KB, patch
khuey
: review+
Details | Diff | Splinter Review
24.90 KB, patch
khuey
: review+
Ms2ger
: review+
Details | Diff | Splinter Review
3.43 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → bzbarsky
There's an open question about how overloads should work...
(I still don't get how foo(undefined) has its first argument missing, but I don't care enough to fight this.)
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.
Isn't it a spec issue?
Sure; there has been plenty of discussion about the spec end on public-script-coord.
The spec changes have been made now.  [TreatUndefinedAs=Missing] has also been removed.
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.
Actually overloads and optional arguments had a difference even before the change. See <https://www.w3.org/Bugs/Public/show_bug.cgi?id=19646>.
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.
Attachment #813212 - Flags: review?(khuey)
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?
Attachment #813421 - Flags: review?(khuey)
Attachment #813421 - Flags: review?(Ms2ger)
Attachment #813213 - Attachment is obsolete: true
Attachment #813213 - Flags: review?(khuey)
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.
Attachment #813421 - Flags: review?(Ms2ger) → review+
Upstreamed the test changes (and a bit more) at https://github.com/w3c/web-platform-tests/pull/361
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.
Attachment #813212 - Flags: review?(khuey) → review+
Depends on: 926305
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.
Note to self: need to fix importNode too.
Depends on: 933964
Depends on: 999315
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.