Support WebIDL optional default value syntax

Support WebIDL optional default value syntax

Currently default values are (in some cases) supported by the
non-standard [Default] extended attribute, namely as
[Default=Undefined] and [Default=NullString].
This is first patch in the effort to replace it with WebIDL
standard syntax.

BUG=258153
R=nbarth@chromium.org, haraken@chromium.org

[pavane, 2014-05-15 14:33:06]

Please have a look at this patch.

There are still two problems which need to be handled.

1. From my observation, it looks like keywords like true and false are
separately handled. 
For eg. it is able to resolve statements like below
Node cloneNode(optional bool deep = "true");
Node cloneNode(optional bool deep = 1);
whereas it is not able to resolve the below statement,
Node cloneNode(optional bool deep = true);

We may have to hack the script to handle these booleans. One way to handle this
is like below:
diff --git a/Source/bindings/scripts/idl_definitions.py
b/Source/bindings/scripts/idl_definitions.py
index 2ad1509..f49dc03 100644
--- a/Source/bindings/scripts/idl_definitions.py
+++ b/Source/bindings/scripts/idl_definitions.py
@@ -490,7 +490,12 @@ class IdlArgument(TypedObject):
                     raise ValueError('Unrecognized Argument node; expected
"...", got "%s"' % child_name)
                 self.is_variadic = child.GetProperty('ELLIPSIS') or False
             elif child_class == 'Default':
-                self.default_value = child.GetName()
+                if child.GetProperty('VALUE') == True:
+                    self.default_value = 'true'
+                elif child.GetProperty('VALUE') == False:
+                    self.default_value = 'false'
+                else:
+                    self.default_value = child.GetName()
             else:
                 raise ValueError('Unrecognized node class: %s' % child_class)

Need your opinion on this.

2. Other case is PassRefPtr. We cannot pass a default value as null to a
PassRefPtr.
For eg. below statement throws compilation errors 
NodeIterator createNodeIterator(Node root, optional unsigned long whatToShow =
0xFFFFFFFF, optional NodeFilter filter = null);
where as this one works fine
boolean isSameNode(optional Node other = null);

I observed that there are two classes which are specially handled in the current
script 'NodeFilter' and 'XPathNSResolver'. I want to know whether similar can be
done in this case. Please suggest if you have better ideas to handle this case.

Thanks in advance.

[Jens Widell, 2014-05-15 15:52:36]

On 2014/05/15 14:33:06, pavane wrote:
> Please have a look at this patch.

Context: I had started on an implementation of this too, so I'll be talking
about my alternative implementation (which is far from finished.)

It would be helpful if you added tests to
Source/bindings/tests/idls/TestObject.idl so that we can see your code in action
directly in the CL. I had added this block of methods on my branch:

  // Optional arguments with defaults
  void voidMethodDefaultStringArg(optional DOMString defaultStringArg = "foo");
  void voidMethodDefaultLongArg(optional long defaultLongArg = 10);
  void voidMethodDefaultDoubleArg(optional double defaultDoubleArg = 0.5);
  void voidMethodDefaultTrueBooleanArg(optional boolean defaultBooleanArg =
true);
  void voidMethodDefaultFalseBooleanArg(optional boolean defaultBooleanArg =
false);
  void voidMethodDefaultNullableStringArg(optional DOMString? defaultStringArg =
null);
  void voidMethodDefaultNullableTestInterfaceArg(optional TestInterface?
defaultTestInterfaceArg = null);

Note: I'm not sure all of those make sense to test, but at least some of them
ought to.

> There are still two problems which need to be handled.
> 
> 1. From my observation, it looks like keywords like true and false are
> separately handled. 
> For eg. it is able to resolve statements like below
> Node cloneNode(optional bool deep = "true");
> Node cloneNode(optional bool deep = 1);
> whereas it is not able to resolve the below statement,
> Node cloneNode(optional bool deep = true);
> 
> We may have to hack the script to handle these booleans. One way to handle
this
> is like below:
> diff --git a/Source/bindings/scripts/idl_definitions.py
> b/Source/bindings/scripts/idl_definitions.py
> index 2ad1509..f49dc03 100644
> --- a/Source/bindings/scripts/idl_definitions.py
> +++ b/Source/bindings/scripts/idl_definitions.py
> @@ -490,7 +490,12 @@ class IdlArgument(TypedObject):
>                      raise ValueError('Unrecognized Argument node; expected
> "...", got "%s"' % child_name)
>                  self.is_variadic = child.GetProperty('ELLIPSIS') or False
>              elif child_class == 'Default':
> -                self.default_value = child.GetName()
> +                if child.GetProperty('VALUE') == True:
> +                    self.default_value = 'true'
> +                elif child.GetProperty('VALUE') == False:
> +                    self.default_value = 'false'
> +                else:
> +                    self.default_value = child.GetName()
>              else:
>                  raise ValueError('Unrecognized node class: %s' % child_class)
> 
> Need your opinion on this.

It's probably best that the experts weigh in on this. My next step, had I
continued with my implementation, would have been to check if the base IDL
parser in {chromium}/tools/idl_parser/ could be made a bit more uniform in how
it represents literal values, so that the code here could be (ideally IMO) just

  self.default_value = child.GetProperty('VALUE')

Using the 'NAME' property as the value for a literal is a bit awkward. You also
need to look at the 'TYPE' property, I think.

And why does "0.5" come out as a node with 'VALUE' == "0.5" and 'TYPE' ==
"float", but "10" come out as a node with 'NAME' == "10" and 'TYPE' ==
"integer". Crazy. :-)

> 2. Other case is PassRefPtr. We cannot pass a default value as null to a
> PassRefPtr.
> For eg. below statement throws compilation errors 
> NodeIterator createNodeIterator(Node root, optional unsigned long whatToShow =
> 0xFFFFFFFF, optional NodeFilter filter = null);
> where as this one works fine
> boolean isSameNode(optional Node other = null);
> 
> I observed that there are two classes which are specially handled in the
current
> script 'NodeFilter' and 'XPathNSResolver'. I want to know whether similar can
be
> done in this case. Please suggest if you have better ideas to handle this
case.

I based my experience on the unfinished CL
https://codereview.chromium.org/265293004/, where local variables for all
arguments are declared first, before type checking and conversion is done for
the arguments. Based on that, I used the declared default arguments to
initialize the locals instead of passing them as arguments directly to the C++
implementation. IOW, the generated code for "void method(double A, optional
double B = 0.5)" would look something like this:

  double requiredArg;
  double optionalArg = 0.5;
  do {
    requiredArg = ...;
    if (info.Length() < 2)
      break;
    optionalArg = ...;
  } while (false);
  impl->method(requiredArg, optionalArg);

This approach is not necessarily ideal, either, but does reduce amount of
(source and binary) code in some cases, and would work for RefPtr/PassRefPtr, I
believe. The RefPtr local would just be left "uninitialized", i.e. null, but
"optionalArg.release()" would still work fine as the value for a PassRefPtr
argument (I assume.)

[Nils Barth (inactive), 2014-05-16 12:13:00]

Hi Pavane,
Thanks for doing this!
not LGTM currently, but with revisions (addressing Jens's concerns) would be
very welcome!

Issues to address:
1. Test cases.
Please add test cases only for examples that are actually used in real IDL
files.

2. Update IDL files.
As a rule we make changes to IDL files *in sync* with changes to the CG, so it's
clear from the revision history how changes are related.
Thus could you please change *all* cases of [Default=...] that are handled by
this CL?

Here's a quick bash file I use called |grep-idl|; you can call it with
grep-idl Default
...to find all uses of the [Default] extended attribute in the Blink codebase:
########################################################
#!/bin/bash
# Find IDL files with a particular extended attribute,
# optionally with some following restriction (e.g., attribute, const, interface)
cd "$BLINK_DIR"

EXT_ATTR="$1"
EXT_ATTR_RE="\[[^\]]*$EXT_ATTR[^\]]*\]"

if [ $# -eq 2 ]
then
  REST="$2"
else
  REST=''
fi
REST_RE="[^{;]*$REST[^{;]*({|;)"
RE="(?s)$EXT_ATTR_RE$REST_RE"

# FIXME: add a filename list mode
grep \
  --color=auto \
  -Pzo "$RE" \
  $(git ls-files '*.idl')
########################################################

3. Once you've handled all cases, please remove the existing [Default] extended
attribute from the CG, IDLExtendedAttributes.txt, and the IDL extended
attributes documentation at:
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes


On 2014/05/15 14:33:06, pavane wrote:
> Please have a look at this patch.
> 
> There are still two problems which need to be handled.
> 
> 1. From my observation, it looks like keywords like true and false are
> separately handled.

It's fine to not handle these for now:
we work very incrementally in bindings, so you can just handle the easy cases
first,
and followup with handling these other cases.

This also makes reviewing easier.

> We may have to hack the script to handle these booleans. One way to handle
this
> is like below:
> diff --git a/Source/bindings/scripts/idl_definitions.py
> b/Source/bindings/scripts/idl_definitions.py
> index 2ad1509..f49dc03 100644
> --- a/Source/bindings/scripts/idl_definitions.py
> +++ b/Source/bindings/scripts/idl_definitions.py
> @@ -490,7 +490,12 @@ class IdlArgument(TypedObject):
>                      raise ValueError('Unrecognized Argument node; expected
> "...", got "%s"' % child_name)
>                  self.is_variadic = child.GetProperty('ELLIPSIS') or False
>              elif child_class == 'Default':
> -                self.default_value = child.GetName()
> +                if child.GetProperty('VALUE') == True:
> +                    self.default_value = 'true'
> +                elif child.GetProperty('VALUE') == False:
> +                    self.default_value = 'false'
> +                else:
> +                    self.default_value = child.GetName()
>              else:
>                  raise ValueError('Unrecognized node class: %s' % child_class)

Have you considered using a function? ;)
More seriously, parsing is very naturally recursive and factored as "one
function per node type".
Please follow the existing code, as per:
self.default_value = default_node_to_default_value(child)

This also means that the function can use |return| instead of elif and
assignment, which is clearer.

> 2. Other case is PassRefPtr. We cannot pass a default value as null to a
> PassRefPtr.
> For eg. below statement throws compilation errors 
> NodeIterator createNodeIterator(Node root, optional unsigned long whatToShow =
> 0xFFFFFFFF, optional NodeFilter filter = null);
> where as this one works fine
> boolean isSameNode(optional Node other = null);
> 
> I observed that there are two classes which are specially handled in the
current
> script 'NodeFilter' and 'XPathNSResolver'. I want to know whether similar can
be
> done in this case. Please suggest if you have better ideas to handle this
case.

Handle these in a followup.

For PassRefPtr handling (I think this is for callback functions, right?), please
ask haraken@ for advice.

[Nils Barth (inactive), 2014-05-16 12:22:42]

Thanks for review Jens!
As you note, covering all cases is a bit intricate.

On 2014/05/15 15:52:36, Jens Lindström wrote:
> Context: I had started on an implementation of this too, so I'll be talking
> about my alternative implementation (which is far from finished.)

Jens, it's fine to upload v. small and simple CLs
(like this one, but with tests and actual use);
no need to have it completely perfect for all cases before posting!

(As a rule in bindings we're v. agile, and we're fine with refactoring and
restructuring as we go: the churn is part of the review process around here ;)

> It would be helpful if you added tests to
> Source/bindings/tests/idls/TestObject.idl so that we can see your code in
action
> directly in the CL. I had added this block of methods on my branch:
> 
>   // Optional arguments with defaults
<snip>
> 
> Note: I'm not sure all of those make sense to test, but at least some of them
> ought to.

Thanks for the great examples!
As noted, we add test cases based on actual code;
I grep through .idl files or generated code to see what is actually used.


> > There are still two problems which need to be handled.
> > 
> > 1. From my observation, it looks like keywords like true and false are
> > separately handled. 
<snip>
> It's probably best that the experts weigh in on this. My next step, had I
> continued with my implementation, would have been to check if the base IDL
> parser in {chromium}/tools/idl_parser/ could be made a bit more uniform in how
> it represents literal values, so that the code here could be (ideally IMO)
just
> 
>   self.default_value = child.GetProperty('VALUE')

Cleaning up the base parser will be much easier once Chromium and Blink repos
are merged, as otherwise we have lots of 3-sided patches to handle transition.
In this case that's not necessary since we're not yet using this.

In general we're fine with adding compatibility layer and a FIXME, particularly
for this case.
(Parser is pretty stable and not where the action is, so technical debt in the
idl_definitions compatibility layer is acceptable.)

> Using the 'NAME' property as the value for a literal is a bit awkward. You
also
> need to look at the 'TYPE' property, I think.
> 
> And why does "0.5" come out as a node with 'VALUE' == "0.5" and 'TYPE' ==
> "float", but "10" come out as a node with 'NAME' == "10" and 'TYPE' ==
> "integer". Crazy. :-)

Could you file a bug for this?
This level of inconsistency should be fixed.
The contact for the base IDL parser is Noel Allen (noelallen@), and he's very
helpful and open to fixes.

> > 2. Other case is PassRefPtr.
<snip: v. helpful response>

haraken: is Jens's approach for RefPtr/PassRefPtr handling correct?
See earlier:
https://codereview.chromium.org/272213005/#msg2

[Nils Barth (inactive), 2014-05-16 12:22:54]

Inline style points.

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

https://codereview.chromium.org/272213005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:171: 'default_value':
argument.default_value,
Alphabetical order please.

https://codereview.chromium.org/272213005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:198: # Truncate omitted optional arguments
Could you move these down to the cpp_arguments.extend() lines, and omit the
auxiliary variables?
(We tend to avoid these if possible.)

https://codereview.chromium.org/272213005/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:208:
cpp_arguments.extend(cpp_argument(argument) for argument in required_arguments)
Line breaks please, here and next line:
loop and condition both on separate lines.

[pavane, 2014-05-16 13:11:30]

Thanks Nils and Jens for your reviews.

Nils, I thought of handling this issue in bits and pieces because of complexity
involved but what you said makes sense. So, i will first solve the simpler cases
and later go for complex cases one by one and in the end i will remove the
syntax handling code for complete set of [Default] extended attributes.

Regarding the below comment:
> And why does "0.5" come out as a node with 'VALUE' == "0.5" and 'TYPE' ==
> "float", but "10" come out as a node with 'NAME' == "10" and 'TYPE' ==
> "integer". Crazy. :-)

Yes, i too noted that this behavior is awkward. My personal feeling is that we
should have everything coming into 'VALUE'. Then below statement will suffice
for all cases

self.default_value = child.GetProperty('VALUE')

But problem is Types like String, Integer etc are coming into 'NAME' and Types
like Boolean, Double, Float etc are coming into 'VALUE'.

Yes, it is better to raise a bug for it. For now, i will put a hack for solving
these cases till the time this issue is solved.

[Nils Barth (inactive), 2014-05-16 14:37:33]

On 2014/05/16 13:11:30, pavane wrote:
> Nils, I thought of handling this issue in bits and pieces because of
complexity
> involved but what you said makes sense. So, i will first solve the simpler
cases
> and later go for complex cases one by one and in the end i will remove the
> syntax handling code for complete set of [Default] extended attributes.

Sounds good!
BTW, I'm OoO next week, so please direct reviews to haraken and Jens, but feel
free to CC me; I'll be back the following week (May 26th) and can review again
then.

[Nils Barth (inactive), 2014-06-02 02:48:33]

Hi Pavane,
Thanks for fixes!
(I know you haven't sent it for another review, but added a few notes.)
Assuming this works, please:
* Add some test cases.
* Fix current uses (that this covers).
...and I'll be happy to ok it!

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

https://codereview.chromium.org/272213005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_definitions.py:514: # FIXME: IDL parser currently
outputs literal values inconsistently(crbug.com/374178).
Please use complete URLs (so syntax matching detects them).
In fact, once it's consistent, you can remove this function, since it's just a
single expression!

So the comment should be something like:
    # FIXME: Should be: return node.GetProperty('VALUE') (inlined)
    # but IDL parser currently outputs literal values inconsistently
    # http://crbug.com/374178

https://codereview.chromium.org/272213005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_definitions.py:516: if (node.GetProperty('TYPE') ==
"boolean" or
Single quotes.

https://codereview.chromium.org/272213005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_definitions.py:517: node.GetProperty('TYPE') ==
"float"):
You can use |in| for checking against a list:
node.GetProperty('TYPE') in ('boolean', 'float')
Slick, eh?

[Nils Barth (inactive), 2014-06-02 02:54:14]

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

https://codereview.chromium.org/272213005/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_definitions.py:517: node.GetProperty('TYPE') ==
"float"):
On 2014/06/02 02:48:34, Nils Barth wrote:
> You can use |in| for checking against a list:
> node.GetProperty('TYPE') in ('boolean', 'float')
> Slick, eh?

BTW, since it's probably unfamiliar:
You can also use |in| for substring testing!
https://docs.python.org/2/library/stdtypes.html#index-22

[Nils Barth (inactive), 2014-06-04 07:32:05]

Closing, superseded by:
https://codereview.chromium.org/312683005/