bindings: Use Visitor to collect union types

bindings: Use Visitor to collect union types

Before this CL, we doesn't collect union types which are used in sequence,
array or nullable. It causes build errors.

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

[bashi, 2015-04-08 08:36:24]

Patchset #1 (id:1) has been deleted

[bashi, 2015-04-08 09:23:44]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, jl@opera.com

[bashi, 2015-04-08 09:23:45]

PTAL?

[Jens Widell, 2015-04-08 09:35:20]

https://codereview.chromium.org/1063253005/diff/20001/Source/bindings/scripts...
File Source/bindings/scripts/compute_interfaces_info_individual.py (right):

https://codereview.chromium.org/1063253005/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:133: class
UnionTypeCollector(Visitor):
I guess we could have kept a collect_union_types_from_defitions() function, and
just declared the visitor sub-class inside it. Would make the call-site a little
more natural; it now declares an essentially stateless temporary object just to
call a method on it.

https://codereview.chromium.org/1063253005/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:143: idl_type =
getattr(typed_object, 'idl_type')
It would be better to use typed_object.idl_type_attributes, which is a list of
attributes whose values are types (and thus need to be considered in a context
such as this.)

This matters for IdlIterable, IdlMaplike and IdlSetlike, where the attributes
are named value_type and/or key_type rather than idl_type.

Also, with

  idl_type = getattr(typed_object, name, None)

you could skip the try/except and just check idl_type (as you do already.)

https://codereview.chromium.org/1063253005/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:150: if
idl_type.element_type and idl_type.element_type.is_union_type:
I think it would be better to have a process_type() function that handles union
types, and recurses for nested types like sequences and nullable.

This implementation mistreats sequence<union_type?> and sequence<union_type>?, I
think?

[bashi, 2015-04-10 00:05:14]

Thank you for review!

https://codereview.chromium.org/1063253005/diff/20001/Source/bindings/scripts...
File Source/bindings/scripts/compute_interfaces_info_individual.py (right):

https://codereview.chromium.org/1063253005/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:133: class
UnionTypeCollector(Visitor):
On 2015/04/08 09:35:19, Jens Widell wrote:
> I guess we could have kept a collect_union_types_from_defitions() function,
and
> just declared the visitor sub-class inside it. Would make the call-site a
little
> more natural; it now declares an essentially stateless temporary object just
to
> call a method on it.

Done.

https://codereview.chromium.org/1063253005/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:143: idl_type =
getattr(typed_object, 'idl_type')
On 2015/04/08 09:35:19, Jens Widell wrote:
> It would be better to use typed_object.idl_type_attributes, which is a list of
> attributes whose values are types (and thus need to be considered in a context
> such as this.)
> 
> This matters for IdlIterable, IdlMaplike and IdlSetlike, where the attributes
> are named value_type and/or key_type rather than idl_type.
> 
> Also, with
> 
>   idl_type = getattr(typed_object, name, None)
> 
> you could skip the try/except and just check idl_type (as you do already.)

Done.

https://codereview.chromium.org/1063253005/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:150: if
idl_type.element_type and idl_type.element_type.is_union_type:
On 2015/04/08 09:35:19, Jens Widell wrote:
> I think it would be better to have a process_type() function that handles
union
> types, and recurses for nested types like sequences and nullable.
> 
> This implementation mistreats sequence<union_type?> and sequence<union_type>?,
I
> think?

Good suggestion. I added idl_types() to IdlTypeBase and its sub classes, which
returns a list of IdlType to be processed.

[haraken, 2015-04-10 00:16:28]

LGTM, but let's wait for Jens' approval.

[Jens Widell, 2015-04-10 05:09:39]

LGTM, with one minor fix.

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

https://codereview.chromium.org/1063253005/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:145: return
return => continue

[bashi, 2015-04-10 05:14:10]

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

https://codereview.chromium.org/1063253005/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:145: return
On 2015/04/10 05:09:39, Jens Widell wrote:
> return => continue

Done. Thanks for pointing this out!

[bashi, 2015-04-10 05:14:19]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-04-10 05:14:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jl@opera.com
Link to the patchset: https://codereview.chromium.org/1063253005/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-04-10 05:14:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1063253005/60001

[commit-bot: I haz the power, 2015-04-10 08:07:39]

Committed patchset #3 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193524