IDL: Support argument default values

IDL: Support argument default values

This also changes the code generated for existing null default values,
by dropping use of argumentOrNull() (which then becomes unused and is
removed) and setting the argument's local variable explicitly by
assigning "nullptr" to it.

The use of argumentOrNull() in V8HistoryCustom.cpp was redundant;
reading info[N] directly yields an undefined value, which is converted
to a null string by V8StringResource<WithUndefinedOrNullCheck>.

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

[Jens Widell, 2014-06-16 13:22:29]

Next step, now supporting more useful syntax, and with some CG changes.

There's of course also the small matter of actually defining real default values
in various IDL files. And first of all replacing all the more or less bogus
default-null strings.

How would you prefer we handle that? Some or all such changes in this one CL, or
as one or many follow-ups?

https://codereview.chromium.org/335113002/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/V8TestInterfaceNamedConstructor.cpp (left):

https://codereview.chromium.org/335113002/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/V8TestInterfaceNamedConstructor.cpp:113:
TOSTRING_VOID_INTERNAL(defaultNullStringOptionalstringArg, argumentOrNull(info,
4));
The generated code changes like this everywhere "= null" is currently used on an
optional DOMString argument (all modified by
https://codereview.chromium.org/312683005/).

That's the only code generation change right now.

[haraken, 2014-06-16 15:09:16]

What's your plan overall about how to treat "missing" arguments? The change
looks OK but I'd like to make sure that we're on the same page.

> How would you prefer we handle that? Some or all such changes in this one CL,
or as one or many follow-ups?

Incremental changes are welcomed :)

(BTW, I do appreciate you're working on this project. How to treat missing
arguments/null/undefined and various type issues has been a huge headache for
Blink developers. This project is just fixing "edge cases" of the web but is
very important to make the IDL compiler saner and thus improve our development
speed.)

[Nils Barth (inactive), 2014-06-17 03:40:32]

LGTM; a question or two.

https://codereview.chromium.org/335113002/diff/1/Source/bindings/tests/idls/T...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/335113002/diff/1/Source/bindings/tests/idls/T...
Source/bindings/tests/idls/TestObject.idl:389: void
voidMethodDefaultNullableByteStringArg(optional ByteString? defaultStringArg =
null);
default*Byte*StringArg

https://codereview.chromium.org/335113002/diff/1/Source/bindings/v8/V8StringR...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/335113002/diff/1/Source/bindings/v8/V8StringR...
Source/bindings/v8/V8StringResource.h:193: void operator=(std::nullptr_t)
BTW, is this going away, as |null| is invalid for DOMString?

[Nils Barth (inactive), 2014-06-17 03:41:39]

On 2014/06/16 13:22:29, Jens Lindström wrote:
> There's of course also the small matter of actually defining real default
values
> in various IDL files. And first of all replacing all the more or less bogus
> default-null strings.
> 
> How would you prefer we handle that? Some or all such changes in this one CL,
or
> as one or many follow-ups?

Followup, as noted by haraken.

[Nils Barth (inactive), 2014-06-17 03:53:47]

On 2014/06/16 15:09:16, haraken wrote:
> What's your plan overall about how to treat "missing" arguments? The change
> looks OK but I'd like to make sure that we're on the same page.

"missing" is a tricky question, and is separate.
(We don't need to figure this out now.)

Currently shorter calls in JS, without default values,
generate shorter calls in C++, which are handled by C++ overloads.
IDL: f(optional long x);
(JS => C++)
f() => f()
f(1) => f(1)

Shorter calls in JS, with default values,
generate identical length calls in C++, so no overloading is necessary.
f(optional long x = 1);
f() => f(1)
f(1) => f(1)

This handles all sane cases.


"missing" shows up in two cases:
1. explicit |undefined|
f(optional long x);
f(undefined) => f(missing)
http://crbug.com/335871

2. optional-with-default after optional-without-default:
f(optional long x, optional long y = 1);
f() => f(missing, 1)

These are both v. special edge cases;
they need handling eventually,
but are not necessary for cleaning up our *current* handling.

[Jens Widell, 2014-06-24 11:04:17]

On 2014/06/16 15:09:16, haraken wrote:
> What's your plan overall about how to treat "missing" arguments? The change
> looks OK but I'd like to make sure that we're on the same page.

I've been reviewing specs and Blink IDL files to figure out what we might want
to do here, but it's made complicated by the fact that Blink's current IDL files
differ a lot from specs in argument optionality, and of course use of default
arguments.

Since IMO the handling of trailing optional arguments with defaults added here
(always call C++ with the argument, either as supplied or the default) is what
we want, regardless of other cases, and this will let us go through IDL files
and clean up all optional arguments without default or with Default=Undefined
that should really just have a proper default value, I think we land this, then
update IDL files (and implementations) to use defaults where appropriate, and
then revisit remaining optionality issues after that.

WDYT?

[haraken, 2014-06-24 15:59:45]

On 2014/06/24 11:04:17, Jens Lindström wrote:
> On 2014/06/16 15:09:16, haraken wrote:
> > What's your plan overall about how to treat "missing" arguments? The change
> > looks OK but I'd like to make sure that we're on the same page.
> 
> I've been reviewing specs and Blink IDL files to figure out what we might want
> to do here, but it's made complicated by the fact that Blink's current IDL
files
> differ a lot from specs in argument optionality, and of course use of default
> arguments.
> 
> Since IMO the handling of trailing optional arguments with defaults added here
> (always call C++ with the argument, either as supplied or the default) is what
> we want, regardless of other cases, and this will let us go through IDL files
> and clean up all optional arguments without default or with Default=Undefined
> that should really just have a proper default value, I think we land this,
then
> update IDL files (and implementations) to use defaults where appropriate, and
> then revisit remaining optionality issues after that.
> 
> WDYT?

Thanks for the investigation! Your plan sounds reasonable to me.

Using the default values is less intrusive than introducing Optional & Nullable,
so I think it's a good idea to start with the default values and see how it
goes. If we can cover 98% of real IDL files with the default values, I think
that's a good balancing point between the complexity of the binding layer and
the conformance with the spec.

LGTM.

[Jens Widell, 2014-06-24 16:11:28]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-06-24 16:11:56]

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

[commit-bot: I haz the power, 2014-06-24 17:33:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[commit-bot: I haz the power, 2014-06-24 18:04:10]

Change committed as 176861