IDL: Treat undefined as missing for optional arguments with defaults

IDL: Treat undefined as missing for optional arguments with defaults

For optional arguments with defaults, treat undefined as missing simply
by checking 'info[N].IsUndefined()' instead of 'N <= info.Length()'.

This works since V8's indexing operator returns an undefined value for
all out-of-range indexes.

Changes in behavior that had (or now has) testing coverade matches
Firefox (Nightly), except in the case of WebKitCSSMatrix's constructor,
which Firefox naturally doesn't have.

BUG=335871
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177920

[Jens Widell, 2014-07-08 12:10:21]

PTAL

This is an incomplete fix: it addresses the issue for one class of optional
arguments but not for all. We may actually want to wait with this until we're
closer to a conclusion on how to handle missing for optional arguments without
default values, to avoid introducing inconsistent behavior.

Note that not all changes in behavior had testing coverage. Our use of IDL
default values is still quite limited though, so there aren't that many cases in
practice:

- boolean x = false: no change since undefined coerces to false.

- DOMString x = null,
- DOMString x = "": typically "undefined" wasn't meaningful, so unlikely that
anyone relied on it IMO.

- DOMString x = "enumerator": "undefined" wasn't a valid value, so would have
raised an exception before.

- SomeInterface x = null: undefined would have coerced to null or raised an
exception, depending on nullability and [TypeChecking=Interface].

[haraken, 2014-07-08 13:10:48]

LGTM.

Just to confirm: The new behavior aligns with both the spec and Firefox (except
for WebKitCSSMatrix which Firefox doesn't implement), right?

[Jens Widell, 2014-07-08 13:25:25]

On 2014/07/08 13:10:48, haraken wrote:
> LGTM.
> 
> Just to confirm: The new behavior aligns with both the spec and Firefox
(except
> for WebKitCSSMatrix which Firefox doesn't implement), right?

All changed behavior for which there was tests, yes. And surely we have testing
coverage for everything that matters. Right..? :-)

I believe there are quite a few cases where we have "optional DOMString x =
null" currently (and previously [Default=NullString]) where the specification is
just "DOMString x". In those cases we behaved incorrectly already when the
argument was missing. Now we will also behave incorrectly when the argument is
undefined. So more incorrectness. But OTOH, since undefined is supposed to mean
missing, you could argue that we're just becoming more consistent: if we decide
to make the argument optional against the specification, then undefined should
also behave this way.

I can investigate this more before committing.

[Jens Widell, 2014-07-10 11:08:26]

On 2014/07/08 13:25:25, Jens Lindström wrote:
> I can investigate this more before committing.

I've now looked closer at every argument with a default, and added some more
tests to cover additional cases. Firefox passes the added tests in all cases
where it supports the tested API at all.

In many cases, undefined would previously have caused an exception, and now does
something meaningful. Doesn't seem likely that this breaks anything. It also
doesn't seem likely that someone passes undefined to a string argument and
expects it to be "undefined" (especially since neither specs nor other modern
implementations typically work like that.)

So all in all, I'm confident that this change is safe.

[haraken, 2014-07-10 11:23:27]

On 2014/07/10 11:08:26, Jens Lindström wrote:
> On 2014/07/08 13:25:25, Jens Lindström wrote:
> > I can investigate this more before committing.
> 
> I've now looked closer at every argument with a default, and added some more
> tests to cover additional cases. Firefox passes the added tests in all cases
> where it supports the tested API at all.
> 
> In many cases, undefined would previously have caused an exception, and now
does
> something meaningful. Doesn't seem likely that this breaks anything. It also
> doesn't seem likely that someone passes undefined to a string argument and
> expects it to be "undefined" (especially since neither specs nor other modern
> implementations typically work like that.)
> 
> So all in all, I'm confident that this change is safe.

Thanks for the detailed investigation, LGTM!

[Jens Widell, 2014-07-10 11:50:22]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-07-10 11:50:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/373043004/80001

[commit-bot: I haz the power, 2014-07-10 12:55:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16226)

[commit-bot: I haz the power, 2014-07-10 13:36:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-10 13:36:07]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16237)

[Jens Widell, 2014-07-10 17:27:49]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-07-10 17:28:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/373043004/80001

[commit-bot: I haz the power, 2014-07-10 17:30:08]

Change committed as 177857

[Justin Novosad, 2014-07-10 18:23:13]

A revert of this CL has been created in
https://codereview.chromium.org/382833003/ by junov@chromium.org.

The reason for reverting is: The following layout tests fail on Mac:
fast/css/shorthand-setProperty-important.html
fast/dom/Comment/comment-constructor.html
fast/dom/DOMImplementation/createHTMLDocument-optional-title.html
fast/dom/HTMLDialogElement/dialog-return-value.html
fast/dom/NodeIterator/NodeIterator-basic.html
fast/dom/Text/text-constructor.html
fast/dom/TreeWalker/TreeWalker-basic.html
fast/dom/Window/custom-constructors.html
fast/dom/Window/alert-undefined.html
fast/encoding/api/basics.html
fast/js/custom-constructors.html
fast/workers/shared-worker-name.html
media/encrypted-media/encrypted-media-istypesupported.html.

[Jens Widell, 2014-07-11 05:54:55]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-07-11 05:55:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/373043004/100001

[Jens Widell, 2014-07-11 05:55:56]

On 2014/07/11 05:54:55, Jens Lindström wrote:
> The CQ bit was checked by mailto:jl@opera.com

Resubmitting. The failure that caused this to be reverted was most likely a bot
flake; the updated tests were tested against a build without the fix.

[commit-bot: I haz the power, 2014-07-11 07:05:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16386)

[commit-bot: I haz the power, 2014-07-11 08:09:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-11 08:09:42]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[Jens Widell, 2014-07-11 08:17:30]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-07-11 08:17:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/373043004/100001

[commit-bot: I haz the power, 2014-07-11 09:30:41]

Change committed as 177920