IDL: Use IdlArrayOrSequenceType for array/sequence IDL types

IDL: Use IdlArrayOrSequenceType for array/sequence IDL types

Representing array and sequence types this way, rather than via auxiliary
flags on the member type, means exotic types like array-of-arrays,
array-of-sequences, arrays-of-nullables and similar can be supported.

More importantly, this also means that an array or sequence type can't be
mistaken for its member type if you forget to check the "is array" and
"is sequence" flags, and thus lets us remove a bunch of "is this an array
or sequence type" checks from code that has nothing to do with supporting
arrays or sequences.

This patch doesn't change code generation.

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

[Jens Widell, 2014-08-14 13:56:51]

PTAL

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:499: return
attribute.idl_type.name.endswith('Constructor')
Changed since base_type is sometimes None (more often so now.) This would fail
if the attribute was nullable, but that doesn't seem likely. Besides, there's
already a FIXME about improving it.

[Nils Barth (inactive), 2014-08-14 14:24:45]

LGTM with nits; thanks for this cleanup!!

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:179: return self.inner_name + "OrNull"
Nit: single quotes

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:314: def resolve_typedefs(self, typedefs):
Could you add a comment here that this does *not* always mutate self?
This is confusing because in IdlDefinitions it does just mutate in place; sorry
about the poor naming.

We could use a different name (like typedefs_resolved;
I'm thinking list.sort() vs. sorted()) to be clearer,
but that might be verbose or confusing itself -- WDYT?

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:357: # IdlArrayOrSequenceType
It would be a bit cleaner to have separate
IdlArrayType and IdlSequenceType
wouldn't it?
e.g., replaces the rather redundant:
return IdlArrayOrSequenceType(base_type, is_array=True,
is_nullable=array_is_nullable)
...with:
return IdlArrayType(base_type, is_nullable=array_is_nullable)

Also makes the methods one-liners.

I.e., I'm suggesting having a shared IdlArrayOrSequenceType
to simplify the monkey patching and some inits:
class IdlArrayOrSequenceType(IdlTypeBase):
    def __init__(self, element_type, is_nullable=False):
        super(IdlArrayOrSequenceType, self).__init__(is_nullable)
        self.element_type = element_type

    def resolve_typedefs(self, typedefs): ...

...but then implement inner_string, inner_name
(and the flags, if necessary (?))
on derived classes.

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:143: 'v8_type':
v8_types.v8_type(base_idl_type) if idl_type.is_interface_type else None,
Is this worth taking onto IdlType(Base)?
IdlType(Base).v8_type = property(lambda self:
    v8_type(self.base_idl_type) if self.is_interface_type else None)
...or the like?

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:499: return
attribute.idl_type.name.endswith('Constructor')
On 2014/08/14 13:56:50, Jens Widell wrote:
> Changed since base_type is sometimes None (more often so now.) This would fail
> if the attribute was nullable, but that doesn't seem likely. Besides, there's
> already a FIXME about improving it.

Acknowledged; [ConstructorAttribute] is another long-overdue cleanup.

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_dictionary.py:86: 'v8_type':
(v8_types.v8_type(idl_type.base_type)
Now, if this were a property on IdlType(Base) ... ;)

[haraken, 2014-08-14 14:48:59]

LGTM

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:362: assert is_array != is_sequence

Also shall we assert is_array or is_sequence?

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_dictionary.py:87: if idl_type.is_interface_type else
None),

Just help me understand: Why do we want to set None for types where
idl_type.is_interface_type is false?

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (left):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:351: native_array_element_type =
idl_type.native_array_element_type

Why can we remove this code?

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:477: # Array and sequence types

Nit: and => or (You use "or" in other places.)

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:743: IdlArrayOrSequenceType.release = False

False => None ?

[Nils Barth (inactive), 2014-08-14 14:54:56]

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:362: assert is_array != is_sequence
On 2014/08/14 14:48:58, haraken wrote:
> Also shall we assert is_array or is_sequence?

That does follow logically...
(X != Y => ((X && !Y) || (!X && Y)) => (X || Y))
...and this can be removed anyway if we use derived classes for Array and
Sequence.

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:743: IdlArrayOrSequenceType.release = False
On 2014/08/14 14:48:58, haraken wrote:
> False => None ?

release is a bool (see above: is_interface_type), hence True/False appropriate.
Maybe this should be renamed is_release?

[Jens Widell, 2014-08-14 15:02:55]

Thanks for quick review!

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:179: return self.inner_name + "OrNull"
On 2014/08/14 14:24:44, Nils Barth (inactive) wrote:
> Nit: single quotes

Sometimes I forget...

Double quotes is what typically I use in "my own" code, as you might have
guessed.

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:314: def resolve_typedefs(self, typedefs):
On 2014/08/14 14:24:44, Nils Barth (inactive) wrote:
> Could you add a comment here that this does *not* always mutate self?
> This is confusing because in IdlDefinitions it does just mutate in place;
sorry
> about the poor naming.

Comment added.

> We could use a different name (like typedefs_resolved;
> I'm thinking list.sort() vs. sorted()) to be clearer,
> but that might be verbose or confusing itself -- WDYT?

As long as this either mutates or returns a new type, neither name will be
perfect. The current name suggests it always mutates; typedefs_resolved would
suggest it always returns a new type.

If we want to fix, we should probably make it always return a new object if the
type is different (always mutating |self| isn't possible,) at which point we
could rename it typedefs_resolved(). I can't think of a reason not to return a
new object.

I'd be happy to address this is a follow-up CL.

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:357: # IdlArrayOrSequenceType
On 2014/08/14 14:24:44, Nils Barth (inactive) wrote:
> It would be a bit cleaner to have separate
> IdlArrayType and IdlSequenceType
> wouldn't it?
> e.g., replaces the rather redundant:
> return IdlArrayOrSequenceType(base_type, is_array=True,
> is_nullable=array_is_nullable)
> ...with:
> return IdlArrayType(base_type, is_nullable=array_is_nullable)
> 
> Also makes the methods one-liners.
> 
> I.e., I'm suggesting having a shared IdlArrayOrSequenceType
> to simplify the monkey patching and some inits:
> class IdlArrayOrSequenceType(IdlTypeBase):
>     def __init__(self, element_type, is_nullable=False):
>         super(IdlArrayOrSequenceType, self).__init__(is_nullable)
>         self.element_type = element_type
> 
>     def resolve_typedefs(self, typedefs): ...
> 
> ...but then implement inner_string, inner_name
> (and the flags, if necessary (?))
> on derived classes.

Agree, that's a better approach. Done.

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:362: assert is_array != is_sequence
On 2014/08/14 14:54:55, Nils Barth (inactive) wrote:
> On 2014/08/14 14:48:58, haraken wrote:
> > Also shall we assert is_array or is_sequence?
> 
> That does follow logically...
> (X != Y => ((X && !Y) || (!X && Y)) => (X || Y))
> ...and this can be removed anyway if we use derived classes for Array and
> Sequence.

Yes, all gone now. It existing in the first place was a good indication that the
design wasn't quite spot on.

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:143: 'v8_type':
v8_types.v8_type(base_idl_type) if idl_type.is_interface_type else None,
On 2014/08/14 14:24:44, Nils Barth (inactive) wrote:
> Is this worth taking onto IdlType(Base)?
> IdlType(Base).v8_type = property(lambda self:
>     v8_type(self.base_idl_type) if self.is_interface_type else None)
> ...or the like?

Can't think of a reason not to. :-)

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_dictionary.py:87: if idl_type.is_interface_type else
None),
On 2014/08/14 14:48:58, haraken wrote:
> 
> Just help me understand: Why do we want to set None for types where
> idl_type.is_interface_type is false?

The very useful v8_types.v8_type() function takes an "interface name" and
prepends 'V8'. So it seemed appropriate to assume it was only relevant for
interface types.

Maybe it's poorly named?

[Jens Widell, 2014-08-14 15:22:03]

bashi@: Can you comment on the currently unused 'v8_type' item set by
member_context()? Will it be used in the future?

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_dictionary.py:87: if idl_type.is_interface_type else
None),
On 2014/08/14 15:02:55, Jens Widell wrote:
> On 2014/08/14 14:48:58, haraken wrote:
> > 
> > Just help me understand: Why do we want to set None for types where
> > idl_type.is_interface_type is false?
> 
> The very useful v8_types.v8_type() function takes an "interface name" and
> prepends 'V8'. So it seemed appropriate to assume it was only relevant for
> interface types.
> 
> Maybe it's poorly named?

Actually, this 'v8_type' doesn't appear to be used at all, at least not
currently.

The 'v8_type' set by attribute_context() is used in one place in the
attributes.cpp template, and that use could be replaced with
"V8{{attribute.idl_type}}" if I'm not mistaken.

That leaves the 'v8_type' set by the code handling [SetWrapperReferenceTo], and
it uses IdlType.name, not IdlType.base_type. And all uses in .idl files refer to
interface types.

[Jens Widell, 2014-08-14 15:40:58]

On 2014/08/14 15:22:03, Jens Widell wrote:
> bashi@: Can you comment on the currently unused 'v8_type' item set by
> member_context()? Will it be used in the future?

I've updated the patch to drop the 'v8_type' items from attribute_context() and
member_context(), and removed the IdlType.v8_type property that I added.

haraken@/nbarth@: PTAL?

[haraken, 2014-08-14 15:51:35]

LGTM

https://codereview.chromium.org/470063003/diff/120001/Source/bindings/scripts...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/470063003/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/idl_types.py:391: self.is_array = True

This should be:

  self.is_sequence = True

[Jens Widell, 2014-08-14 16:03:18]

https://codereview.chromium.org/470063003/diff/120001/Source/bindings/scripts...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/470063003/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/idl_types.py:391: self.is_array = True
On 2014/08/14 15:51:34, haraken wrote:
> 
> This should be:
> 
>   self.is_sequence = True

Oups! Fixed.

That had no effect on code generation, evidently. Good to know. :-)

[Nils Barth (inactive), 2014-08-14 16:18:56]

* Could you try removing is_array and is_sequence?
* Fine to fix resolve_typedefs() => typedefs_resolved() in followup.
...and a few nits.

Thanks, this code is getting *much* cleaner!

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:314: def resolve_typedefs(self, typedefs):
On 2014/08/14 15:02:54, Jens Widell wrote:
> As long as this either mutates or returns a new type, neither name will be
> perfect. The current name suggests it always mutates; typedefs_resolved would
> suggest it always returns a new type.
> 
> If we want to fix, we should probably make it always return a new object if
the
> type is different (always mutating |self| isn't possible,) at which point we
> could rename it typedefs_resolved(). I can't think of a reason not to return a
> new object.
> 
> I'd be happy to address this is a follow-up CL.

Good point about consistency;
my concern (hence optimization) was that I didn't want to create new objects
every time (note the "not in typedefs") for every type in an IDL fragment!
(Given that typedefs are pretty rare in many cases.)
...but it got complicated and confusing.

I think it's fine to replace this method with:
def typedefs_resolved(self, typedefs):
    return typedefs.get(self.base_type, self)
...which avoids any copying or mutation.

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_dictionary.py:87: if idl_type.is_interface_type else
None),
v8_type() is a Perl legacy;
I kept it around as "mostly harmless",
but since it can be replaced by templates
(and we have "V8" hardcoded throughout templates),
we should kill it.

I think the reasoning was to encapsulate the "V8" prefix,
perhaps by analogy with other targets (JSC?),
but this was never done consistently, it's too heavy-weight anyway,
and a simple sed on /V8/ works pretty well ;)

Thanks for the investigation!

https://codereview.chromium.org/470063003/diff/120001/Source/bindings/scripts...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/470063003/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/idl_types.py:391: self.is_array = True
On 2014/08/14 16:03:18, Jens Widell wrote:
> That had no effect on code generation, evidently. Good to know. :-)

Actually, could you try removing is_array and is_sequence?
Now that you have separate classes and have patched the necessary methods,
you may not need these flags!

https://codereview.chromium.org/470063003/diff/140001/Source/bindings/scripts...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/470063003/diff/140001/Source/bindings/scripts...
Source/bindings/scripts/idl_types.py:10: IdlArrayOrSequenceType
nit:
 IdlArrayOrSequenceType
   IdlArrayType
   IdlSequenceType

https://codereview.chromium.org/470063003/diff/140001/Source/bindings/scripts...
Source/bindings/scripts/idl_types.py:316: # object unmodified and returns a
different object.
Want to put a lil' FIXME here?
(Probably a short fix, but just as a reminder.)

https://codereview.chromium.org/470063003/diff/140001/Source/bindings/scripts...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/470063003/diff/140001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:629: extended_attributes =
extended_attributes or {}
Minor nit: I prefer putting these "default empty values for dicts/lists"
at the top, for legibility (standard recipe), even if not used until later.

[Jens Widell, 2014-08-14 18:16:43]

PS#6 gets rid of is_{array,sequence} (and {array,sequence}_element_type, that
were unused.)

PS#7 addresses nits.

PS#10 simplifies the new cpp_type_has_null_value(), which no longer needs to
have "unless it's an array or sequence" logic.

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:314: def resolve_typedefs(self, typedefs):
On 2014/08/14 16:18:55, Nils Barth (inactive) wrote:
> I think it's fine to replace this method with:
> def typedefs_resolved(self, typedefs):
>     return typedefs.get(self.base_type, self)
> ...which avoids any copying or mutation.

With that exact approach you'd lose the referencing type's is_nullable.

This was actually the one place I thought it would be an advantage to have
IdlNullableType, since it could just have replaced its inner type like
IdlArrayOrSequenceType does. In all other places, having a separate
IdlNullableType would just complicate things, I think.

[Nils Barth (inactive), 2014-08-14 19:26:05]

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/470063003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:314: def resolve_typedefs(self, typedefs):
On 2014/08/14 18:16:43, Jens Widell wrote:
> With that exact approach you'd lose the referencing type's is_nullable.
> 
> This was actually the one place I thought it would be an advantage to have
> IdlNullableType, since it could just have replaced its inner type like
> IdlArrayOrSequenceType does. In all other places, having a separate
> IdlNullableType would just complicate things, I think.

Got it.
Yeah, in principle we should have a separate IdlNullableType
(would simplify IdlTypeBase a little too), but if it complicates
things otherwise (esp. CG), fine to not do so.

In followup, could you check if having IdlNullableType really makes
things more complex?
(Glancing at code it looks like it simplifies matters, with one possible
exception in the CG.)

Thanks!

[Jens Widell, 2014-08-14 20:29:45]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-08-14 20:30:57]

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

[commit-bot: I haz the power, 2014-08-14 20:38:06]

Committed patchset #10 (240001) as 180298