IDL: Support optional argument default value syntax

IDL: Support optional argument default value syntax

Adds support for parsing default values of different types, but
only handles null default values when generating code.

Replaces existing

  [Default=Null] optional SomeInterface arg
  [Default=NullString] optional DOMString arg

with the now equivalent

  optional SomeInterface arg = null
  optional DOMString arg = null

in IDL files, and drops support for those [Default] attributes.

No changes to generated code.

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

[Jens Widell, 2014-06-03 14:56:19]

Here's my default value patch (which I've previously talked about but not shown
anyone,) slightly polished up and improved.

I don't mean to duplicate too much effort here (some duplication had already
happened of course) so if this is not adding much value to our effort then don't
spend too much time looking at it. :-)

[jsbell, 2014-06-03 18:33:15]

Just some drive-by notes for places where this could be used to let us use real
default values from specs and exercise the change.

(Just specs that I know well, not comprehensive)

https://codereview.chromium.org/312683005/diff/1/Source/modules/encoding/Text...
File Source/modules/encoding/TextDecoder.idl (right):

https://codereview.chromium.org/312683005/diff/1/Source/modules/encoding/Text...
Source/modules/encoding/TextDecoder.idl:34: Constructor(optional DOMString label
= null, optional Dictionary options),
Per spec, default is "utf-8" - does everything pass if that is used?

https://codereview.chromium.org/312683005/diff/1/Source/modules/encoding/Text...
File Source/modules/encoding/TextEncoder.idl (right):

https://codereview.chromium.org/312683005/diff/1/Source/modules/encoding/Text...
Source/modules/encoding/TextEncoder.idl:34: Constructor(optional DOMString
utfLabel = null),
Per spec, default is "utf-8" - does everything pass if that is used?

https://codereview.chromium.org/312683005/diff/1/Source/modules/encoding/Text...
Source/modules/encoding/TextEncoder.idl:40: [MeasureAs=TextEncoderEncode]
Uint8Array encode(optional DOMString input = null, optional Dictionary options);
Per spec, default is "" - does everything pass if that is used?

https://codereview.chromium.org/312683005/diff/1/Source/modules/indexeddb/IDB...
File Source/modules/indexeddb/IDBDatabase.idl (right):

https://codereview.chromium.org/312683005/diff/1/Source/modules/indexeddb/IDB...
Source/modules/indexeddb/IDBDatabase.idl:39: [CallWith=ExecutionContext,
RaisesException] IDBTransaction transaction(DOMString storeName, optional
DOMString mode = null);
Per spec, the default is "readonly" - do all tests pass if that is used?

https://codereview.chromium.org/312683005/diff/1/Source/modules/indexeddb/IDB...
File Source/modules/indexeddb/IDBIndex.idl (right):

https://codereview.chromium.org/312683005/diff/1/Source/modules/indexeddb/IDB...
Source/modules/indexeddb/IDBIndex.idl:35: [CallWith=ScriptState,
RaisesException] IDBRequest openCursor([Default=Undefined] optional any range,
optional DOMString direction = null);
Per spec, default here (and other direction params) is "next" - does everything
pass if you use that here?

https://codereview.chromium.org/312683005/diff/1/Source/modules/indexeddb/IDB...
File Source/modules/indexeddb/IDBObjectStore.idl (right):

https://codereview.chromium.org/312683005/diff/1/Source/modules/indexeddb/IDB...
Source/modules/indexeddb/IDBObjectStore.idl:42: [CallWith=ScriptState,
RaisesException] IDBRequest openCursor([Default=Undefined] optional any range,
optional DOMString direction = null);
ditto

[Nils Barth (inactive), 2014-06-04 04:08:06]

On 2014/06/03 18:33:15, jsbell wrote:
> Just some drive-by notes for places where this could be used to let us use
real
> default values from specs and exercise the change.
> 
> (Just specs that I know well, not comprehensive)

Thanks for notes Joshua!

I'd prefer to make any behavior changes in a separate CL,
(i.e., not change any default values)
but putting FIXMEs on these sites in this CL is appropriate!

[Nils Barth (inactive), 2014-06-04 05:22:25]

Thanks Jens!
This is quite clean, makes the IDLs much cleaner, and will allow us to implement
proper default values!

Substantive comment is:
could you not change generated code in this CL, instead doing that in a
followup?
(I.e., just have this do parser changes.)

Structural comments are primarily that an IdlLiteral type
(and then a monkey-patched cpp_value property) makes this cleaner.

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_definitions.py:509: class
IdlDefaultValue(TypedObject):
You don't need TypedObject here I don't think:
TypedObject is only for resolving typedefs,
and typedefs aren't applicable to *literals*.

E.g., even if you had
foo(TypeDef x = "bar")
here you're only using "bar" (and the TYPE property in the AST),
not the type of the argument.

Also this can more generically be called IdlLiteral, so I'd write this:
class IdlLiteral(object):
    ...
(In principle this could be used for constants as well,
but those are in practice very simple, so no need.)

Also could you update the class diagram in the module docstring at top?

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_definitions.py:511: self.value_type =
node.GetProperty('TYPE')
Naming: idl_type, rather than value_type.
(might need a bit of converting)

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_definitions.py:512: if self.value_type ==
'DOMString':
Could you factor out this evaluation into a function?

if idl_type == 'DOMString':
    return node.GetProperty('NAME')
if ...:
    ...
raise ...
...has simpler flow

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_definitions.py:515: self.value =
eval(node.GetProperty('NAME'))
!!!
Could you please use constructors int() and float()?
...as eval() causes conniptions and hives.

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_definitions.py:517: self.value =
eval(node.GetProperty('VALUE'))
Could you add a FIXME for the irregular upstream treatment of NAME/VALUE?

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_definitions.py:520: elif self.value_type == 'NULL':
Handling null and undefined will be a bit ugly.
We'd handle this by having properties is_null and is_undefined
(like IdlType.is_nullable).

Cleanest would be to have IdlLiteralNull and IdlLiteralUndefined
which inherit from IdlLiteral
and just override the value for is_null and is_undefined,
respectively.

In that case you'd also need to use a factory function:
default_node_to_idl_literal
...instead of calling the IdlLiteral constructor directly.

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:171: 'default_value':
default_value_to_cpp_value(argument),
Using a property this is more cleanly:
argument.default_value.cpp_value if argument.default_value else None

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:293: def
default_value_to_cpp_value(argument):
Could you make this a property?
(For slickness and consistency.)
...via:
IdlLiteral.cpp_value = property(idl_literal_to_cpp_value)

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:294: default_value =
argument.default_value
Might be clearer to also have:
value_type = default_value.value_type
value = default_value.value
(after the None check)
...since these are used throughout.

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:295: if default_value is None:
if not default_value:
...per:
Use the "implicit" false if at all possible.
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=True/F...

...but actually can be eliminated by moving the check for default value to the
call site.

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:302: return '%d' % default_value.value
Could you put all this "string representation" logic in
IdlLiteral.__str__ ?

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:311: 
...do you mean to implicitly return None here?
(...or raise an exception?)

https://codereview.chromium.org/312683005/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/312683005/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:87: {% if argument.default_value %}
This is a behavior change, right?

Could you put this in a followup, having this CL just change the syntax and
parsing?

https://codereview.chromium.org/312683005/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:122: not (argument.has_default or
argument.default_value) and
Can you avoid this by changing the definition in v8_methods:

from:
'has_default': 'Default' in extended_attributes,

to:
# FIXME: remove once [Default] removed and just use argument.default_value
'has_default': 'Default' in extended_attributes or argument.default_value,

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

https://codereview.chromium.org/312683005/diff/1/Source/bindings/v8/V8StringR...
Source/bindings/v8/V8StringResource.h:188: void operator=(String string)
...and this also in followup?

[Jens Widell, 2014-06-04 06:12:19]

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_definitions.py:515: self.value =
eval(node.GetProperty('NAME'))
On 2014/06/04 05:22:24, Nils Barth wrote:
> !!!
> Could you please use constructors int() and float()?
> ...as eval() causes conniptions and hives.

Using eval() here is clearly ridiculous. Sorry about that.

I think I meant to handle integer and float using the same expression  in which
case eval() at least added something (but then realized one used NAME and the
other VALUE, and didn't.)

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:171: 'default_value':
default_value_to_cpp_value(argument),
On 2014/06/04 05:22:24, Nils Barth wrote:
> Using a property this is more cleanly:
> argument.default_value.cpp_value if argument.default_value else None

I'm using the argument's type to generate different code for null values, which
can't be done the exact same way if this is changed to read a property off of
the default value object.

Do you have a preferred approach to get around that?

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:311: 
On 2014/06/04 05:22:24, Nils Barth wrote:
> ...do you mean to implicitly return None here?
> (...or raise an exception?)

Not really, no. I am handling all value types that the parser accepts, so it
currently won't happen, but to be clear, ought to raise an exception here as
well.

https://codereview.chromium.org/312683005/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/312683005/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:87: {% if argument.default_value %}
On 2014/06/04 05:22:24, Nils Barth wrote:
> This is a behavior change, right?

Along with the IDL file changes, yes. Although I believe I changed the IDL files
such that the end result is the same.
 
> Could you put this in a followup, having this CL just change the syntax and
> parsing?

That would leave the idl_definitions.cpp change, and possibly some of the
changes in v8_methods.py and some of the additions in TestObject.idl, yes?

https://codereview.chromium.org/312683005/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:122: not (argument.has_default or
argument.default_value) and
On 2014/06/04 05:22:24, Nils Barth wrote:
> Can you avoid this by changing the definition in v8_methods:
> 
> from:
> 'has_default': 'Default' in extended_attributes,
> 
> to:
> # FIXME: remove once [Default] removed and just use argument.default_value
> 'has_default': 'Default' in extended_attributes or argument.default_value,

Will do. In general, 'has_default' and its meaning is a bit strange after this
patch; follow-ups will clean it up further, I'd expect.

[Nils Barth (inactive), 2014-06-04 06:33:36]

Couple followups: key point is that there is a (minor) change in generated
source code, which I'd prefer to have in a followup.

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_definitions.py:515: self.value =
eval(node.GetProperty('NAME'))
On 2014/06/04 06:12:18, Jens Lindström wrote:
> Using eval() here is clearly ridiculous. Sorry about that.
> 
> I think I meant to handle integer and float using the same expression  in
which
> case eval() at least added something (but then realized one used NAME and the
> other VALUE, and didn't.)

n/p, that makes sense -- I was wondering what exactly you were on,
and this explains it!

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (left):

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:291: v8_value = 'argumentOrNull(info, %s)'
% index
You can get rid of argumentOrNull from V8Binding.h now, right?

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:171: 'default_value':
default_value_to_cpp_value(argument),
On 2014/06/04 06:12:18, Jens Lindström wrote:
> I'm using the argument's type to generate different code for null values,
which
> can't be done the exact same way if this is changed to read a property off of
> the default value object.
> 
> Do you have a preferred approach to get around that?

Ow, good point. (>.<)

2 possible approaches:
1. Make it a property on the argument:
argument.default_cpp_value
This is easier, and perhaps more correct.

2. Put the IDL type (of the argument) in the IdlLiteral
...so it's available to default_value.
This is a bit weird at the level of the parser (violates tree structure of AST
in object construction), but seems cleaner from the CG.

1 (property on the argument) seems better (simpler, more correct), though I
can't tell without actually implementing it.

WDYT?

https://codereview.chromium.org/312683005/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/312683005/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:87: {% if argument.default_value %}
On 2014/06/04 06:12:18, Jens Lindström wrote:
> On 2014/06/04 05:22:24, Nils Barth wrote:
> > This is a behavior change, right?
> 
> Along with the IDL file changes, yes. Although I believe I changed the IDL
files
> such that the end result is the same.

The generated code is different (the object code may be the same);
compare old:
voidMethodDefaultNullStringStringArg
with new:
voidMethodDefaultNullStringArg
in:
https://codereview.chromium.org/312683005/diff/1/Source/bindings/tests/result...

Notably you replaced argumentOrNull with this if clause.

So I'd prefer if in the generated code weren't different in this CL:
you could put argumentOrNull in the template here, and a FIXME
to replace it with an if clause.

> > Could you put this in a followup, having this CL just change the syntax and
> > parsing?
> 
> That would leave the idl_definitions.cpp change, and possibly some of the
> changes in v8_methods.py and some of the additions in TestObject.idl, yes?

...and also ok to change "[Default=NullString] ..." to "... = null" in the real
IDL files, if there is no change in output.

https://codereview.chromium.org/312683005/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:122: not (argument.has_default or
argument.default_value) and
On 2014/06/04 06:12:18, Jens Lindström wrote:
> Will do. In general, 'has_default' and its meaning is a bit strange after this
> patch; follow-ups will clean it up further, I'd expect.

Yeah, at this point it becomes vestigial, and "going away soon".

[Nils Barth (inactive), 2014-06-04 07:33:27]

BTW, see earlier:
Support WebIDL optional default value syntax
https://codereview.chromium.org/272213005/

[Jens Widell, 2014-06-04 09:49:16]

On 2014/06/04 06:33:36, Nils Barth wrote:
> Couple followups: key point is that there is a (minor) change in generated
> source code, which I'd prefer to have in a followup.

Valid point. I've updated the patch now to be most of the changes, all of the
IDL file changes, but in such a way that code generation isn't changed. This
means it parses "all" default value variants but only handles "= null" on
strings.

Also attempted to address your comments. I'm sure you'll have more for me. :-)

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/312683005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:171: 'default_value':
default_value_to_cpp_value(argument),
On 2014/06/04 06:33:35, Nils Barth wrote:
> On 2014/06/04 06:12:18, Jens Lindström wrote:
> > I'm using the argument's type to generate different code for null values,
> which
> > can't be done the exact same way if this is changed to read a property off
of
> > the default value object.
> > 
> > Do you have a preferred approach to get around that?
> 
> Ow, good point. (>.<)
> 
> 2 possible approaches:
> 1. Make it a property on the argument:
> argument.default_cpp_value
> This is easier, and perhaps more correct.
> 
> 2. Put the IDL type (of the argument) in the IdlLiteral
> ...so it's available to default_value.
> This is a bit weird at the level of the parser (violates tree structure of AST
> in object construction), but seems cleaner from the CG.
> 
> 1 (property on the argument) seems better (simpler, more correct), though I
> can't tell without actually implementing it.
> 
> WDYT?

I ended up tentatively going for a third option:

3) Add operator=(nullptr_t) to V8StringResource and always emit "nullptr" for
null values.

But I also reverted all the changes to V8StringResource.h in this CL, so the
only part of it that is visible is the "always emit "nullptr"" part.

If we don't want that assignment operator, I can always implement your option 1
in the follow-up in such a way that it just checks for the "null string" case,
emits "String()" then, and otherwise uses "str(default_value)".

[Jens Widell, 2014-06-04 10:14:21]

On 2014/06/03 18:33:15, jsbell wrote:
> Just some drive-by notes for places where this could be used to let us use
real
> default values from specs and exercise the change.

Thanks for the pointers! Setting all the suggested real default values and
dropping the corresponding manual if-isNull()-then-this code in the method
implementations passed all the tests. I will follow up with such changes once
this has landed.

[Jens Widell, 2014-06-13 10:42:48]

Patch rebased, with some minor conflicts (e.g. with jsbell's ByteString patch).

Any chance this could receive another review? :-)

[haraken, 2014-06-13 13:46:53]

This is a great improvement!

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_definitions.py:434: value = node.GetProperty('NAME')

Shouldn't this be node.GetProperty('VALUE') ?

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_definitions.py:439: return IdlLiteral(idl_type,
int(node.GetProperty('NAME')))

Shouldn't this be int(node.GetProperty('VALUE') ?

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:306: argument.default_value and
argument.default_value.is_null):

Just to confirm: If you write:

  void foo(optional DOMString value = null);

and call:

  foo();

then a null String (instead of a String "null") is expected to be passed to
Blink, right?

I'm asking this because if you write:

  void foo(DOMString);

and call:

  foo(null);

then a String "null" is passed to Blink.

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.idl (right):

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.idl:41: [RaisesException,
TypeChecking=Interface] void add(HTMLElement element, optional HTMLElement?
before = null);

Help me understand: How is this different from:

  [RaisesException, TypeChecking=Interface] void add(HTMLElement element,
optional HTMLElement? before);

?

[Jens Widell, 2014-06-13 14:14:52]

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_definitions.py:434: value = node.GetProperty('NAME')
On 2014/06/13 13:46:52, haraken wrote:
> 
> Shouldn't this be node.GetProperty('VALUE') ?

The use of NAME/VALUE here is entirely dictated by what the IDL parser actually
outputs. See http://crbug.com/374178.

I intend to look into fixing that bug, but wasn't planning to do it before
landing this patch.

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:306: argument.default_value and
argument.default_value.is_null):
On 2014/06/13 13:46:52, haraken wrote:
> 
> Just to confirm: If you write:
> 
>   void foo(optional DOMString value = null);
> 
> and call:
> 
>   foo();
> 
> then a null String (instead of a String "null") is expected to be passed to
> Blink, right?

Yes. The argumentOrNull() function returns an empty v8::Handle if the index is
out of range (IOW, not a Javascript null value as one might think given the
function name) and that gets converted into a null string
(WTF::String::isNull()==true) by V8StringResource<>.

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.idl (right):

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.idl:41: [RaisesException,
TypeChecking=Interface] void add(HTMLElement element, optional HTMLElement?
before = null);
On 2014/06/13 13:46:53, haraken wrote:
> 
> Help me understand: How is this different from:
> 
>   [RaisesException, TypeChecking=Interface] void add(HTMLElement element,
> optional HTMLElement? before);
> 
> ?

An optional argument without a default value causes the C++ implementation to be
called with fewer parameters, thus depending on overloads or default values on
the C++ side.

When there is a default value, that short-cutting is disabled, and we go ahead
with the value conversion, which I think mostly leads to coercing the undefined
into the argument's type.

[haraken, 2014-06-13 14:32:54]

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:306: argument.default_value and
argument.default_value.is_null):
On 2014/06/13 14:14:52, Jens Lindström wrote:
> On 2014/06/13 13:46:52, haraken wrote:
> > 
> > Just to confirm: If you write:
> > 
> >   void foo(optional DOMString value = null);
> > 
> > and call:
> > 
> >   foo();
> > 
> > then a null String (instead of a String "null") is expected to be passed to
> > Blink, right?
> 
> Yes. The argumentOrNull() function returns an empty v8::Handle if the index is
> out of range (IOW, not a Javascript null value as one might think given the
> function name) and that gets converted into a null string
> (WTF::String::isNull()==true) by V8StringResource<>.

Sorry for not being clear. I understand that your CL does the conversion. My
question is whether the behavior aligns with the IDL spec or not.

  [a] foo(DOMString str) & foo(null) => a String "null"
  [b] foo(optional DOMString str = null) & foo() => a null String
  [c] foo(optional DOMString str = null) & foo(null) => a String "null"

I'm confident about [a] and [c], but not confident about [b]. (Sorry I'm asking
this question without reading the spec seriously...)

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.idl (right):

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.idl:41: [RaisesException,
TypeChecking=Interface] void add(HTMLElement element, optional HTMLElement?
before = null);
On 2014/06/13 14:14:52, Jens Lindström wrote:
> On 2014/06/13 13:46:53, haraken wrote:
> > 
> > Help me understand: How is this different from:
> > 
> >   [RaisesException, TypeChecking=Interface] void add(HTMLElement element,
> > optional HTMLElement? before);
> > 
> > ?
> 
> An optional argument without a default value causes the C++ implementation to
be
> called with fewer parameters, thus depending on overloads or default values on
> the C++ side.
> 
> When there is a default value, that short-cutting is disabled, and we go ahead
> with the value conversion, which I think mostly leads to coercing the
undefined
> into the argument's type.

Ah, got it, thanks! Is there any reason we support overloading in both C++ side
and binding side? It seems confusing that we have two ways of resolving method
overloading.

[Jens Widell, 2014-06-13 14:49:46]

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:306: argument.default_value and
argument.default_value.is_null):
On 2014/06/13 14:32:53, haraken wrote:
> My question is whether the behavior aligns with the IDL spec or not.
> 
>   [a] foo(DOMString str) & foo(null) => a String "null"
>   [b] foo(optional DOMString str = null) & foo() => a null String
>   [c] foo(optional DOMString str = null) & foo(null) => a String "null"
> 
> I'm confident about [a] and [c], but not confident about [b]. (Sorry I'm
asking
> this question without reading the spec seriously...)

I see. :-) Well, if I read the spec correctly, your [b] is correct because
default values bypass value conversion, which is where "", "null" and
"undefined" would appear.

See http://heycam.github.io/webidl/#dfn-overload-resolution-algorithm, 9.4.1 vs
9.5 and 12.4.1 vs 12.5.

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.idl (right):

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.idl:41: [RaisesException,
TypeChecking=Interface] void add(HTMLElement element, optional HTMLElement?
before = null);
On 2014/06/13 14:32:53, haraken wrote:
> Is there any reason we support overloading in both C++ side
> and binding side? It seems confusing that we have two ways of resolving method
> overloading.

I don't think there's a strong reason, and I think it would be nice if we could
drop the C++ side overloading (and possibility that C++ argument defaults affect
behavior) and instead let the IDL define everything.

The problem is, I guess, cases where we want to support "missing" as a separate
value for arguments (and null pointer, null string, et c. doesn't suffice.) And
Web IDL also notes that a trailing optional-without-default argument is
equivalent to having two overloads, one with and one without the argument, which
I guess matches how we currently call a different C++ "overload".

See also discussion in http://crbug.com/335871.

[haraken, 2014-06-13 14:59:04]

LGTM.

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:306: argument.default_value and
argument.default_value.is_null):
On 2014/06/13 14:49:46, Jens Lindström wrote:
> On 2014/06/13 14:32:53, haraken wrote:
> > My question is whether the behavior aligns with the IDL spec or not.
> > 
> >   [a] foo(DOMString str) & foo(null) => a String "null"
> >   [b] foo(optional DOMString str = null) & foo() => a null String
> >   [c] foo(optional DOMString str = null) & foo(null) => a String "null"
> > 
> > I'm confident about [a] and [c], but not confident about [b]. (Sorry I'm
> asking
> > this question without reading the spec seriously...)
> 
> I see. :-) Well, if I read the spec correctly, your [b] is correct because
> default values bypass value conversion, which is where "", "null" and
> "undefined" would appear.
> 
> See http://heycam.github.io/webidl/#dfn-overload-resolution-algorithm, 9.4.1
vs
> 9.5 and 12.4.1 vs 12.5.

Makes sense, thanks!

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.idl (right):

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.idl:41: [RaisesException,
TypeChecking=Interface] void add(HTMLElement element, optional HTMLElement?
before = null);
On 2014/06/13 14:49:46, Jens Lindström wrote:
> On 2014/06/13 14:32:53, haraken wrote:
> > Is there any reason we support overloading in both C++ side
> > and binding side? It seems confusing that we have two ways of resolving
method
> > overloading.
> 
> I don't think there's a strong reason, and I think it would be nice if we
could
> drop the C++ side overloading (and possibility that C++ argument defaults
affect
> behavior) and instead let the IDL define everything.
> 
> The problem is, I guess, cases where we want to support "missing" as a
separate
> value for arguments (and null pointer, null string, et c. doesn't suffice.)
And
> Web IDL also notes that a trailing optional-without-default argument is
> equivalent to having two overloads, one with and one without the argument,
which
> I guess matches how we currently call a different C++ "overload".
> 
> See also discussion in http://crbug.com/335871.

Understood.

I think we need a better rule for this. It's confusing to have multiple ways to
do one thing. For example: "we use IDL's overloading as much as possible except
for cases where we can't".

[jsbell, 2014-06-13 16:43:45]

On 2014/06/13 14:59:04, haraken wrote:
> > > 
> > >   [b] foo(optional DOMString str = null) & foo() => a null String

As I understand things, what Blink calls a null string (string.isNull()) doesn't
exist in Web IDL. So I don't believe this syntax is valid Web IDL:

foo(optional DOMString str = null)

... since 'null' is not in the set of DOMString values. The closest thing would
be:

foo(optional DOMString? str = null)

(i.e. making it a nullable type)

I suspect that anywhere we use [Default=NullString] we're approximating the
desired behavior that was not expressible before we had explicit nullable and
true default value support.

(It also wouldn't surprise me if specs and web-reality disagreed in some of
these cases as well.)

[jsbell, 2014-06-13 16:53:37]

... but I don't think that should stop us from going forward with this change.
As planned, we should follow up by replacing all of the 'optional DOMString xxx
= null' instances in Blink IDL files with the correct default, and then we can
enforce the restriction at code-gen time.

[Nils Barth (inactive), 2014-06-16 09:18:17]

LGTM with a nit or two; apologies for delayed review.

Thanks for grit in revising and refactoring this!
Looks good, and gets us much closer to cleaner, simpler IDL and bindings!

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_definitions.py:440: elif idl_type == 'float':
No else after return: these can all be |if|

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:184: 'default_value':
str(argument.default_value) if argument.default_value else None,
Could you add a FIXME here to check that the type of the default value is
compatible with the type of the argument?
(Currently fails due to |null| in DOMString)

(This is a bit too complicated to do in the parser,
but it's useful validation.)

Concretely, it might be good to add a property |v8_default_value|
which returns str(argument.default_value) or None,
or raises if invalid, which will keep this code simple.

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:306: argument.default_value and
argument.default_value.is_null):
On 2014/06/13 14:49:46, Jens Lindström wrote:
> I see. :-) Well, if I read the spec correctly, your [b] is correct because
> default values bypass value conversion, which is where "", "null" and
> "undefined" would appear.
> 
> See http://heycam.github.io/webidl/#dfn-overload-resolution-algorithm, 9.4.1
vs
> 9.5 and 12.4.1 vs 12.5.

Same conclusion as Joshua
(i.e., go ahead with this change, fix illegal |null| defaults in followup);
just adding some details.

Properly,
optional DOMString s = null
is *illegal*, as |null| is not a valid value for a DOMString
(as Joshua notes); it's like writing
optional DOMString s = 1

Formally the correct thing would be something like:
  optional DOMString? s = null
...or:
  [TreatNullAs=NullString] optional DOMString s = null
...per:
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-TreatN...
...our take on the standard [TreatNullAs]
http://heycam.github.io/webidl/#TreatNullAs
...but that would change the behavior of *explicitly* passing |null|.

Also, regarding argument conversion, the correct section is 13.1:
13. While i is less than the number of arguments callable is declared to take:
1. If callable’s argument at index i is declared with a default value, then
append to values that default value.

That's the relevant section when trailing optional arguments are *omitted*;
the sections you cited are when |undefined| is passed as an optional argument.
(>.<)

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.idl (right):

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.idl:41: [RaisesException,
TypeChecking=Interface] void add(HTMLElement element, optional HTMLElement?
before = null);
On 2014/06/13 14:59:03, haraken wrote:
> I think we need a better rule for this. It's confusing to have multiple ways
to
> do one thing. For example: "we use IDL's overloading as much as possible
except
> for cases where we can't".

Agreed; here're my thoughts:
* We should match IDL, which means using default values in IDL if present
(this means not needing to have them in C++, just making these required
arguments).
* C++ overloading is fine if there's no default value specified, and needed
anyway for overloads.

This gives a simple rule; I've updated the docs:
http://www.chromium.org/developers/web-idl-interfaces#TOC-Names

* Web IDL overloading and IDL optional arguments without default values map
directly to C++ overloading.
** Optional arguments without default values correspond to an overload either
including or excluding the argument.
* IDL optional arguments with default values map to C++ calls with these values
filled in, and thus do not require C++ overloading.
C++ default values SHOULD NOT be used.

[haraken, 2014-06-16 09:35:40]

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.idl (right):

https://codereview.chromium.org/312683005/diff/40001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.idl:41: [RaisesException,
TypeChecking=Interface] void add(HTMLElement element, optional HTMLElement?
before = null);
On 2014/06/16 09:18:17, Nils Barth wrote:
> On 2014/06/13 14:59:03, haraken wrote:
> > I think we need a better rule for this. It's confusing to have multiple ways
> to
> > do one thing. For example: "we use IDL's overloading as much as possible
> except
> > for cases where we can't".
> 
> Agreed; here're my thoughts:
> * We should match IDL, which means using default values in IDL if present
> (this means not needing to have them in C++, just making these required
> arguments).
> * C++ overloading is fine if there's no default value specified, and needed
> anyway for overloads.
> 
> This gives a simple rule; I've updated the docs:
> http://www.chromium.org/developers/web-idl-interfaces#TOC-Names
> 
> * Web IDL overloading and IDL optional arguments without default values map
> directly to C++ overloading.
> ** Optional arguments without default values correspond to an overload either
> including or excluding the argument.
> * IDL optional arguments with default values map to C++ calls with these
values
> filled in, and thus do not require C++ overloading.
> C++ default values SHOULD NOT be used.


Sounds reasonable to me.

[Jens Widell, 2014-06-16 09:37:14]

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/312683005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:306: argument.default_value and
argument.default_value.is_null):
On 2014/06/16 09:18:17, Nils Barth wrote:
> Same conclusion as Joshua
> (i.e., go ahead with this change, fix illegal |null| defaults in followup);
> just adding some details.
>
> Properly,
> optional DOMString s = null
> is *illegal*, as |null| is not a valid value for a DOMString
> (as Joshua notes); it's like writing
> optional DOMString s = 1

You are (both) right, naturally. And it was of course always the intent to have
the per-spec default values in the IDL, and they will not be null since that's
not allowed. Our null strings (whether as [Default=Null] or "= null") was
always, and still is with this patch, a way to work around lacking support for
default values in the IDL. Soon, though... :-)

[Jens Widell, 2014-06-16 09:47:59]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-06-16 09:48:47]

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

[commit-bot: I haz the power, 2014-06-16 11:47:52]

Change committed as 176200

[Jens Widell, 2014-06-16 13:47:49]

On 2014/06/16 09:18:17, Nils Barth wrote:
> Agreed; here're my thoughts:
> * We should match IDL, which means using default values in IDL if present
> (this means not needing to have them in C++, just making these required
> arguments).
> * C++ overloading is fine if there's no default value specified, and needed
> anyway for overloads.
> 
> This gives a simple rule; I've updated the docs:
> http://www.chromium.org/developers/web-idl-interfaces#TOC-Names
> 
> * Web IDL overloading and IDL optional arguments without default values map
> directly to C++ overloading.
> ** Optional arguments without default values correspond to an overload either
> including or excluding the argument.
> * IDL optional arguments with default values map to C++ calls with these
values
> filled in, and thus do not require C++ overloading.
> C++ default values SHOULD NOT be used.

In the context of this, HTMLOptionElement's named Option constructor is an
interesting case:

  Option(optional DOMString data = "",
         optional DOMString value,
         optional boolean defaultSelected = false,
         optional boolean selected = false)

IOW, the first, third and fourth arguments have defaults, but the second
doesn't. How to map this to C++?

Currently (with https://codereview.chromium.org/335113002/ applied) what we do
for such an operation is:

* Called with zero or one argument: call C++ function with only the first
argument, possibly set to the empty string by default. (Doesn't compile since
there is no such overload, but let's ignore that for now.)
* Called with two or more argument: call C++ function with all four arguments,
third and fourth possibly set to false by default.

This doesn't really meet the goal of having all default values in the IDL, since
the third and fourth argument would need default values in C++ in the first
case.

I would think we need to support conveying "missing" somehow to the C++
implementation. For strings, a null string would do, since, as has been pointed
out, null is not a valid value for strings. (Except nullable strings, of
course.) Alternatively, we could call the C++ implementation with an extra "bool
missing" argument for every optional argument (without a default.) Or we could
introduce some wrapper type Optional<ArgType> that packs the actual value
together with that bool.

WDYT?

[Stephen Chennney, 2014-06-16 16:25:19]

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

The reason for reverting is: This patch is the most likely candidate for browser
test failures:
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests%20%28dbg%2...

BrowserTestBase signal handler received SIGTERM. Backtrace:
#0 0x7f2c2a201a1d base::debug::StackTrace::StackTrace()
#1 0x00000460caf2 content::(anonymous namespace)::DumpStackTraceSignalHandler()
#2 0x7f2c232cd4a0 \u003Cunknown>
#3 0x7f2c2337f313 __poll
#4 0x7f2c240b6036 \u003Cunknown>
#5 0x7f2c240b6164 g_main_context_iteration
#6 0x7f2c2a1c9b45 base::MessagePumpGlib::Run()
#7 0x7f2c2a29eed7 base::MessageLoop::RunHandler()
#8 0x7f2c2a2f69d8 base::RunLoop::Run()
#9 0x00000466bd59 content::RunThisRunLoop()
#10 0x00000466c1b8 content::MessageLoopRunner::Run()
#11 0x000004610946 content::TitleWatcher::WaitAndGetTitle()
#12 0x0000011fdf00 MediaBrowserTest::RunTest()
#13 0x0000011fdc00 MediaBrowserTest::RunMediaTestPage()
#14 0x0000011ec98b EncryptedMediaTestBase::RunEncryptedMediaTestPage()
#15 0x0000011ee5fa EncryptedMediaTest::TestConfigChange()
#16 0x0000011eb73a
EncryptedMediaTest_ConfigChangeVideo_Test::RunTestOnMainThread()
...

Please look into it. An alterate possible candidate is 
https://codereview.chromium.org/327553002 but I think that's less likely..

[Nils Barth (inactive), 2014-06-17 01:12:01]

haraken, thoughts on how to pass possibly missing optional arguments,
and also union types?


On 2014/06/16 13:47:49, Jens Lindström wrote:
> In the context of this, HTMLOptionElement's named Option constructor is an
> interesting case:
> 
>   Option(optional DOMString data = "",
>          optional DOMString value,
>          optional boolean defaultSelected = false,
>          optional boolean selected = false)
> 
> IOW, the first, third and fourth arguments have defaults, but the second
> doesn't. How to map this to C++?

Ow, ow, good point; non-trailing optional arguments without defaults make this
complicated (>.<).

> Currently (with https://codereview.chromium.org/335113002/ applied) what we do
> for such an operation is:
> 
> * Called with zero or one argument: call C++ function with only the first
> argument, possibly set to the empty string by default. (Doesn't compile since
> there is no such overload, but let's ignore that for now.)
> * Called with two or more argument: call C++ function with all four arguments,
> third and fourth possibly set to false by default.
> 
> This doesn't really meet the goal of having all default values in the IDL,
since
> the third and fourth argument would need default values in C++ in the first
> case.

Hmm...
* We can list all the default values in the IDL (yay!),
  but you're right, there does need to be some way of representing the trailing
default values;
  this can be hard-coded into the (zero or one arg) overloaded function, I
believe.

> I would think we need to support conveying "missing" somehow to the C++
> implementation. For strings, a null string would do, since, as has been
pointed
> out, null is not a valid value for strings. (Except nullable strings, of
> course.) Alternatively, we could call the C++ implementation with an extra
"bool
> missing" argument for every optional argument (without a default.) Or we could
> introduce some wrapper type Optional<ArgType> that packs the actual value
> together with that bool.
> 
> WDYT?

I think you're right that "missing" needs to be conveyed in some cases;
per spec:

13. While i is less than the number of arguments callable is declared to take:
 1. If callable’s argument at index i is declared with a default value, then
append to values that default value.
 2. Otherwise, if callable’s argument at index i is not variadic, then append to
values the special value “missing”.

...which reflects that these can be mixed.

It might be easier to *not* map optional args to C++ overloads, and instead
always
pass "missing"; that makes the C++ closer to the Web IDL spec.
OTOH, using C++ overloading may make the Blink-side C++ code simpler (and
faster),
so we might want to only pass "missing" if opt-w/o-default args are mixed in
(follow) opt-w/-default args.

Regarding the "extra bool missing" parameter or "wrapper type",
whatever is done should be consistent with other argument passing,
notably union types (which are only partially implemented currently).

Currently union type args are passed as one arg for each possible type,
plus a bool arg for each type, with only the actually passed one being true.
This yields *huge* signatures and makes it a bit of a pain to update functions
if the union type is changed. I suspect it also adds to register pressure.

I think a wrapper type for each union type would make life much cleaner:
a tagged union is purpose-made for this, and it seems the natural answer.

haraken?

[Nils Barth (inactive), 2014-06-17 01:15:46]

On 2014/06/16 16:25:19, Stephen Chenney wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/339683002/

For the record, this was then un-reverted (revert was reverted),
so this CL *did* land successfully:
https://codereview.chromium.org/339723002/

[haraken, 2014-06-17 01:55:13]

On 2014/06/17 01:12:01, Nils Barth wrote:
> haraken, thoughts on how to pass possibly missing optional arguments,
> and also union types?
> 
> 
> On 2014/06/16 13:47:49, Jens Lindström wrote:
> > In the context of this, HTMLOptionElement's named Option constructor is an
> > interesting case:
> > 
> >   Option(optional DOMString data = "",
> >          optional DOMString value,
> >          optional boolean defaultSelected = false,
> >          optional boolean selected = false)
> > 
> > IOW, the first, third and fourth arguments have defaults, but the second
> > doesn't. How to map this to C++?
> 
> Ow, ow, good point; non-trailing optional arguments without defaults make this
> complicated (>.<).
> 
> > Currently (with https://codereview.chromium.org/335113002/ applied) what we
do
> > for such an operation is:
> > 
> > * Called with zero or one argument: call C++ function with only the first
> > argument, possibly set to the empty string by default. (Doesn't compile
since
> > there is no such overload, but let's ignore that for now.)
> > * Called with two or more argument: call C++ function with all four
arguments,
> > third and fourth possibly set to false by default.
> > 
> > This doesn't really meet the goal of having all default values in the IDL,
> since
> > the third and fourth argument would need default values in C++ in the first
> > case.
> 
> Hmm...
> * We can list all the default values in the IDL (yay!),
>   but you're right, there does need to be some way of representing the
trailing
> default values;
>   this can be hard-coded into the (zero or one arg) overloaded function, I
> believe.
> 
> > I would think we need to support conveying "missing" somehow to the C++
> > implementation. For strings, a null string would do, since, as has been
> pointed
> > out, null is not a valid value for strings. (Except nullable strings, of
> > course.) Alternatively, we could call the C++ implementation with an extra
> "bool
> > missing" argument for every optional argument (without a default.) Or we
could
> > introduce some wrapper type Optional<ArgType> that packs the actual value
> > together with that bool.
> > 
> > WDYT?
> 
> I think you're right that "missing" needs to be conveyed in some cases;
> per spec:
> 
> 13. While i is less than the number of arguments callable is declared to take:
>  1. If callable’s argument at index i is declared with a default value, then
> append to values that default value.
>  2. Otherwise, if callable’s argument at index i is not variadic, then append
to
> values the special value “missing”.
> 
> ...which reflects that these can be mixed.
> 
> It might be easier to *not* map optional args to C++ overloads, and instead
> always
> pass "missing"; that makes the C++ closer to the Web IDL spec.
> OTOH, using C++ overloading may make the Blink-side C++ code simpler (and
> faster),
> so we might want to only pass "missing" if opt-w/o-default args are mixed in
> (follow) opt-w/-default args.
> 
> Regarding the "extra bool missing" parameter or "wrapper type",
> whatever is done should be consistent with other argument passing,
> notably union types (which are only partially implemented currently).
> 
> Currently union type args are passed as one arg for each possible type,
> plus a bool arg for each type, with only the actually passed one being true.
> This yields *huge* signatures and makes it a bit of a pain to update functions
> if the union type is changed. I suspect it also adds to register pressure.
> 
> I think a wrapper type for each union type would make life much cleaner:
> a tagged union is purpose-made for this, and it seems the natural answer.
> 
> haraken?

jl@ and you are more familiar with the situation :)

We'll also need to consider how to use Nullable.h.

I'm fine with any rule as long as the rule is consistent and minimizes # of the
programming rules.

[Nils Barth (inactive), 2014-06-17 02:20:49]

On 2014/06/17 01:55:13, haraken wrote:
> jl@ and you are more familiar with the situation :)
> 
> We'll also need to consider how to use Nullable.h.
> 
> I'm fine with any rule as long as the rule is consistent and minimizes # of
the
> programming rules.

Good point re: Nullable.
I think easiest is to use consistent internal representations through the spec,
the compiler, and the Blink code.

E.g., Web IDL has a notional of a nullable type (with an inner type),
so easiest is if the IDL parser produces an IdlNullableType object,
and then the C++ has a Nullable<T> object.
(etc.)

There are cases where we'll want to do something different,
primarily for performance (e.g., Nullable interface types can just be a pointer,
since nullptr (C++) can represent null (JS/IDL)),
but trying to keep it consistent seems best.

[Jens Widell, 2014-06-17 09:57:10]

On 2014/06/17 01:55:13, haraken wrote:
> On 2014/06/17 01:12:01, Nils Barth wrote:
> > haraken, thoughts on how to pass possibly missing optional arguments,
> > and also union types?
> > 
> > 
> > On 2014/06/16 13:47:49, Jens Lindström wrote:
> > > In the context of this, HTMLOptionElement's named Option constructor is an
> > > interesting case:
> > > 
> > >   Option(optional DOMString data = "",
> > >          optional DOMString value,
> > >          optional boolean defaultSelected = false,
> > >          optional boolean selected = false)
> > > 
> > > IOW, the first, third and fourth arguments have defaults, but the second
> > > doesn't. How to map this to C++?
> > 
> > Ow, ow, good point; non-trailing optional arguments without defaults make
this
> > complicated (>.<).
> > 
> > > Currently (with https://codereview.chromium.org/335113002/ applied) what
we
> do
> > > for such an operation is:
> > > 
> > > * Called with zero or one argument: call C++ function with only the first
> > > argument, possibly set to the empty string by default. (Doesn't compile
> since
> > > there is no such overload, but let's ignore that for now.)
> > > * Called with two or more argument: call C++ function with all four
> arguments,
> > > third and fourth possibly set to false by default.
> > > 
> > > This doesn't really meet the goal of having all default values in the IDL,
> > since
> > > the third and fourth argument would need default values in C++ in the
first
> > > case.
> > 
> > Hmm...
> > * We can list all the default values in the IDL (yay!),
> >   but you're right, there does need to be some way of representing the
> trailing
> > default values;
> >   this can be hard-coded into the (zero or one arg) overloaded function, I
> > believe.
> > 
> > > I would think we need to support conveying "missing" somehow to the C++
> > > implementation. For strings, a null string would do, since, as has been
> > pointed
> > > out, null is not a valid value for strings. (Except nullable strings, of
> > > course.) Alternatively, we could call the C++ implementation with an extra
> > "bool
> > > missing" argument for every optional argument (without a default.) Or we
> could
> > > introduce some wrapper type Optional<ArgType> that packs the actual value
> > > together with that bool.
> > > 
> > > WDYT?
> > 
> > I think you're right that "missing" needs to be conveyed in some cases;
> > per spec:
> > 
> > 13. While i is less than the number of arguments callable is declared to
take:
> >  1. If callable’s argument at index i is declared with a default value, then
> > append to values that default value.
> >  2. Otherwise, if callable’s argument at index i is not variadic, then
append
> to
> > values the special value “missing”.
> > 
> > ...which reflects that these can be mixed.
> > 
> > It might be easier to *not* map optional args to C++ overloads, and instead
> > always
> > pass "missing"; that makes the C++ closer to the Web IDL spec.
> > OTOH, using C++ overloading may make the Blink-side C++ code simpler (and
> > faster),
> > so we might want to only pass "missing" if opt-w/o-default args are mixed in
> > (follow) opt-w/-default args.
> > 
> > Regarding the "extra bool missing" parameter or "wrapper type",
> > whatever is done should be consistent with other argument passing,
> > notably union types (which are only partially implemented currently).
> > 
> > Currently union type args are passed as one arg for each possible type,
> > plus a bool arg for each type, with only the actually passed one being true.
> > This yields *huge* signatures and makes it a bit of a pain to update
functions
> > if the union type is changed. I suspect it also adds to register pressure.
> > 
> > I think a wrapper type for each union type would make life much cleaner:
> > a tagged union is purpose-made for this, and it seems the natural answer.
> > 
> > haraken?
> 
> jl@ and you are more familiar with the situation :)
> 
> We'll also need to consider how to use Nullable.h.
> 
> I'm fine with any rule as long as the rule is consistent and minimizes # of
the
> programming rules.

I've uploaded https://codereview.chromium.org/337343002/, which is one
alternative approach we could take. Please note that it's quite untested.

Also, while working on it, I ended up rewriting constructor (and named
constructor) handling quite a bit to make them share more code with regular
methods, since I meant to use the Option() constructor as a test case, and found
that my modifications to method and argument handling mostly didn't apply,
because other (simpler) code was triggered for constructors. Will upload a
separate patch with those changes.

[haraken, 2014-06-17 11:05:37]

> > We'll also need to consider how to use Nullable.h.
> > 
> > I'm fine with any rule as long as the rule is consistent and minimizes # of
> the
> > programming rules.
> 
> I've uploaded https://codereview.chromium.org/337343002/, which is one
> alternative approach we could take. Please note that it's quite untested.
> 
> Also, while working on it, I ended up rewriting constructor (and named
> constructor) handling quite a bit to make them share more code with regular
> methods, since I meant to use the Option() constructor as a test case, and
found
> that my modifications to method and argument handling mostly didn't apply,
> because other (simpler) code was triggered for constructors. Will upload a
> separate patch with those changes.

Thanks for looking into this issue.

My first impression is that introducing "Optional" is a bit too overkilling. If,
for example, we introduce an "Argument" which can cover all argument status
(is_null, is_optional, default_value etc), it will solve all problems in
general. However, we don't want to do that because it will regress binding
performance and it will add more complexity to the Blink side.

Probably a good balancing point would be:

(a) Resolve overloading in the IDL side as much as possible. If we can't,
resolve it in the C++ side. If we can't either, we need custom bindings.

or

(b) Resolve overloading in the C++ side as much as possible. If we can't,
resolve it in the IDL side. If we can't either, we need custom bindings.

Which do you think is better, (a) or (b) ?

[Jens Widell, 2014-06-17 11:25:47]

On 2014/06/17 11:05:37, haraken wrote:
> My first impression is that introducing "Optional" is a bit too overkilling.

Yeah, it might be. It would only be used in rare cases, but I guess that's just
an argument for not introducing a lot of extra machinery to handle it.

> Probably a good balancing point would be:
> 
> (a) Resolve overloading in the IDL side as much as possible. If we can't,
> resolve it in the C++ side. If we can't either, we need custom bindings.
> 
> or
> 
> (b) Resolve overloading in the C++ side as much as possible. If we can't,
> resolve it in the IDL side. If we can't either, we need custom bindings.
> 
> Which do you think is better, (a) or (b) ?

Given those choices, (a) is the preferable one, I think.

I would prefer that you had to implement as few C++ methods with as simple
signatures as possible (e.g. no defaults), and that no information should be
encoded in the C++ code that's also in the IDL file.

It would be sad if we had to resort to custom bindings for this. (It's always
sad when we need to do that, I guess.)

We should also consider http://crbug.com/335871 here, I think. Given

  void f(optional long a, optional long b)

I could have the call

  f(undefined, 10)

where we also ought to convey somehow to the C++ side that the first argument
was missing. This is similar to

  void f(optional long a, optional long b = 10)

and

  f()

except it invalidates the assumption that we can identify a few optional
arguments that need to support missing. We could support that too with my
Optional<>, but only if we used it at least for all non-last optional arguments
without defaults. And that might impact performance, and would lead to lots of
changes to the C++ code to implement.

And alternative that would make it more light-weight would be if we used
nullptr/null-string to represent missing for those arguments that support it,
and Optional<> for types that has no such "null" value (bool and numeric types.)

At this point, I'm really failing to see a simple light-weight solution that
solves all the problems. :-/

[haraken, 2014-06-17 11:54:45]

> And alternative that would make it more light-weight would be if we used
> nullptr/null-string to represent missing for those arguments that support it,
> and Optional<> for types that has no such "null" value (bool and numeric
types.)

Probably I like this approach. We can use nullptr for pointers, a null string
for Strings, and Optional<> for other primitive types.

Help me understand: Do we need both Nullable<T> and Optional<T>? What's the
difference? I'd like to understand how much we can simplify things.

[Jens Widell, 2014-06-17 12:07:33]

On 2014/06/17 11:54:45, haraken wrote:
> Help me understand: Do we need both Nullable<T> and Optional<T>? What's the
> difference? I'd like to understand how much we can simplify things.

Short answer is: I don't know.

My guess is that we could implement "missing or null" semantics (a merged
Nullable<> and Optional<>) rather than both missing and null as separate states
(for an optional nullable argument.) IOW, handle optional, nullable and
optional-nullable using the same mechanism, with the same representation of both
missing and null arguments.

This really depends on whether there's a spec that defines different behavior
for missing and null for an optional-nullable(-and-not-default-null) argument.
One could at least hope there's no such spec.

Also, maybe *that* would truly be a good candidate for custom bindings, should
we encounter it?

[Nils Barth (inactive), 2014-06-17 12:38:48]

On 2014/06/17 12:07:33, Jens Lindström wrote:
> On 2014/06/17 11:54:45, haraken wrote:
> > Help me understand: Do we need both Nullable<T> and Optional<T>? What's the
> > difference? I'd like to understand how much we can simplify things.

Nutshell:
Support whatever Blink actually uses, as simply as possible;
no need to implement unused corners of the spec.

I'd avoid adding complex code to support |missing| semantics unless there's a
use case;
code that simplifies existing uses and doesn't break anything is fine even if
it's not
compatible with hypothetical hard cases.

If some hard cases come up which *are* used, or that we want to use, we can try
implementing, or provide pushback to standards if the result is really
complicated.
See for example the "commas in extended attribute value lists" that makes
parsing very difficult:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24959

Very common and easy:
* Nullable arguments/attributes/return types
* Optional arguments

My sense is that most nullable types are interfaces, maybe DOMString, both of
which we can support easily;
other cases (nullable primitive types, like long? ??) would be harder, but I
don't know how common they are.

Rare AFAICT and hard:
* Optional nullable arguments (w/ or w/o default) -- potentially requires
separate handling of nullable and optional
* Optional w/o default followed by optional w/ default -- needs |missing|


We definitely need nullable, but Nullable<T> may not be needed much if at all.

IDL level:
Formally, |null| is a possible value, and nullable is a property of a *type*,
while |optional| means an argument can be omitted: they're thus completed
unrelated *formally*.

In practice, they fulfill much the same purpose when used for *arguments*:
preferred practice is to use an *optional* argument, while *nullable* is mostly
for legacy
interfaces.
*However*, only nullable can be used for *attributes* and for *return values*
(these cannot meaningfully be optional, as |null| really is a valid value).
Thus we definitely need nullable somewhere, though I suspect it's almost always
used for
interfaces, in which case we can simplify to just nullptr.

What's more complicated is null vs. undefined (passing both explicitly):
|null| is an actual value passed onto C++, while |undefined| is *not* really a
value,
and has the special "missing" semantics which cause all our problems.

Concretely, consider:
f(optional DOMString x);
g(optional DOMString? y);

Then:
f() => f()
f(undefined) => f(missing)
f(null) => TypeError (null is not valid DOMString)

g() => g()
g(undefined) => g(missing)
g(null) => g(null) (null is a valid DOMString?)

[haraken, 2014-06-17 16:02:34]

Thanks for the great replay, nbarth@!

> Nutshell:
> Support whatever Blink actually uses, as simply as possible;
> no need to implement unused corners of the spec.
> 
> I'd avoid adding complex code to support |missing| semantics unless there's a
> use case;
> code that simplifies existing uses and doesn't break anything is fine even if
> it's not
> compatible with hypothetical hard cases.

This is a very good point; what matters is what happens in practice.

jl@: Keeping Nils' input in mind, would you take a look at what real IDL files
are using and make a proposal about how to treat missing/nullable/optional
things?

> Very common and easy:
> * Nullable arguments/attributes/return types
> * Optional arguments
> 
> My sense is that most nullable types are interfaces, maybe DOMString, both of
> which we can support easily;
> other cases (nullable primitive types, like long? ??) would be harder, but I
> don't know how common they are.
> 
> Rare AFAICT and hard:
> * Optional nullable arguments (w/ or w/o default) -- potentially requires
> separate handling of nullable and optional
> * Optional w/o default followed by optional w/ default -- needs |missing|
> 
> We definitely need nullable, but Nullable<T> may not be needed much if at all.

Off the topic: Not related to missing/nullable/optional, we might want to use
Nullable<T> for handling values of union types. It's not nice to use
resultXEnabled to judge if the result has a valid value or not.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Jens Widell, 2014-06-17 16:12:21]

On 2014/06/17 16:02:34, haraken wrote:

> jl@: Keeping Nils' input in mind, would you take a look at what real IDL files
> are using and make a proposal about how to treat missing/nullable/optional
> things?

Yes, I'll have a more thorough look at IDL files (and the underlying C++ code)
and see if I can come up with something reasonable.

[Nils Barth (inactive), 2014-06-18 02:35:34]

On 2014/06/17 16:02:34, haraken wrote:
> Off the topic: Not related to missing/nullable/optional, we might want to use
> Nullable<T> for handling values of union types. It's not nice to use
> resultXEnabled to judge if the result has a valid value or not.

Agreed; having auxiliary boolean parameters is really ugly.
A tagged union seems like a much nicer approach, though I don't know the
details.

May just be a question of an enum for possible types, and a tagged union
structure
(more likely using pointers for interface types), like:

enum FooOrBarTag {
  FOO,
  BAR,
};

struct FooOrBar {
  FooOrBarTag tag;
  union {
    Foo* foo;
    Bar* bar;
  }
};