bindings: Supports inheritance of [Unforgeable] attributes as accessor-type properties.

bindings: Supports inheritance of [Unforgeable] attributes as accessor-type properties.

This CL makes it possible that we define [Unforgeable] attributes as
accessor-type JS properties, and child-interfaces inherit them from
parent-interfaces.

Note that Window and Location's attributes are still data-type properties.

Except for Event.isTrusted, there should be no behavioral change.
Event.isTrusted becomes accessor-type properties.

BUG=43394, 491006, 497616
Committed: https://crrev.com/e56d75cefca6bff8bb96648e86f7fba30740e5bb
git-svn-id: svn://svn.chromium.org/blink/trunk@199658 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[Yuki, 2015-07-27 07:40:56]

yukishiino@chromium.org changed reviewers:
+ bashi@chromium.org, haraken@chromium.org

[Yuki, 2015-07-27 07:40:56]

Could you guys review this CL?

[haraken, 2015-07-27 08:54:11]

LGTM, but bashi@ might want to take another look.

BTW, is Unforgeable attribute the only thing we need to copy into inherited
interfaces? What about other attributes defined on instance objects (e.g.,
static operations)?

[Yuki, 2015-07-27 09:04:12]

On 2015/07/27 08:54:11, haraken wrote:
> LGTM, but bashi@ might want to take another look.
> 
> BTW, is Unforgeable attribute the only thing we need to copy into inherited
> interfaces? What about other attributes defined on instance objects (e.g.,
> static operations)?

Yes, I want bashi's review, too.

There is no case other than [Unforgeable] that needs inheritance.
- static members are defined on the INTERFACE obejct, not on the instance
object.
- There are only two kinds of members which are defined on the instance object.
  1. [Unforgeable]
  2. [Global] / [PrimaryGlobal]
  and it's not allowed to inherit a [Global] or [PrimaryGlobal] interface
because
  it's global.

[haraken, 2015-07-27 09:09:04]

On 2015/07/27 09:04:12, Yuki wrote:
> On 2015/07/27 08:54:11, haraken wrote:
> > LGTM, but bashi@ might want to take another look.
> > 
> > BTW, is Unforgeable attribute the only thing we need to copy into inherited
> > interfaces? What about other attributes defined on instance objects (e.g.,
> > static operations)?
> 
> Yes, I want bashi's review, too.
> 
> There is no case other than [Unforgeable] that needs inheritance.
> - static members are defined on the INTERFACE obejct, not on the instance
> object.
> - There are only two kinds of members which are defined on the instance
object.
>   1. [Unforgeable]
>   2. [Global] / [PrimaryGlobal]
>   and it's not allowed to inherit a [Global] or [PrimaryGlobal] interface
> because
>   it's global.

Makes sense!

[bashi, 2015-07-28 00:38:47]

Thanks for working on this! I have some questions so it would be better to have
a chat :)

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

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:134: return
sorted(set(attribute for attribute in definition.attributes
nit: I slightly prefer:

if 'Unforgeable' in definition.extended_attributes:
  return sorted(definition.attributes)
return sorted(attribute for attribute in definition.attributes
              if 'Unforgeable' in attribute.extended_attributes)

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:218:
self.interfaces_info[definition.name] = {}
Why do you need this? Can't we just use |interface_info|?

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:222: #  
interfaces_info[interface_name] = {
(BTW, I'm not a big fun of {'core': ..., 'modules': ...} structure. I agree that
we need to store them in a component-aware way but a generic dict doesn't seem
to have enough capability to handle the complexity we need to solve. In my
opinion, we should replace interfaces_info with a class which provides
reasonable getter/setter methods if we have enough bandwidth to do refactoring.)

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:228: #   }
Can we factor out this block as a method?

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:233:
interface_basename, _ = os.path.splitext(os.path.basename(idl_filename))
idl_filename_to_interface_name(idl_filename)

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:235:
attr.extended_attributes['PartialInterfaceImplementedAs'] =
definition.extended_attributes.get('ImplementedAs', interface_basename)
We have similar logic in transfer_extended_attributes() which lives in
interface_dependency_resolver.py. This implies that we may want to share code.
That said, it's not clear to me whether it can be done easily. Could you add a
TODO comment if you think we could share code?

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:290:
'include_path': this_include_path,
Can we remove this?

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

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_overall.py:212: if
is_first_info:
Could you explain what you want to do by this change?

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

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/interface_dependency_resolver.py:337: return
collect_unforgeable_attributes_in_ancestors(
How about merge-and-return recursive results (unforgeable_attributes,
referenced_interfaces and cpp_includes) instead of passing them as arguments?

us, rs, cs =
collect_unforgeable_attributes_in_ancestors(interface.get('parent'), ...)
return us.extend(attrs), rs.extend(referenced),
cs.update(interface.get('cpp_includes', {}).get(component, {}))

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/interface_dependency_resolver.py:342: for
interface_name, interface in definitions.interfaces.iteritems():
Help me understand: len(definitions.interfaces) could be greater than one here?

[Yuki, 2015-07-29 08:16:25]

Could you take another look?

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

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:134: return
sorted(set(attribute for attribute in definition.attributes
On 2015/07/28 00:38:47, bashi1 wrote:
> nit: I slightly prefer:
> 
> if 'Unforgeable' in definition.extended_attributes:
>   return sorted(definition.attributes)
> return sorted(attribute for attribute in definition.attributes
>               if 'Unforgeable' in attribute.extended_attributes)

Done.

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:218:
self.interfaces_info[definition.name] = {}
On 2015/07/28 00:38:46, bashi1 wrote:
> Why do you need this? Can't we just use |interface_info|?

Before my change, we didn't update interfaces_info for partial interfaces.  With
this change, I'm updating interfaces_info for both of (non-partial) interfaces
and partial interfaces, so we cannot simply assign it and need to carefully
merge them.

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:222: #  
interfaces_info[interface_name] = {
On 2015/07/28 00:38:46, bashi1 wrote:
> (BTW, I'm not a big fun of {'core': ..., 'modules': ...} structure. I agree
that
> we need to store them in a component-aware way but a generic dict doesn't seem
> to have enough capability to handle the complexity we need to solve. In my
> opinion, we should replace interfaces_info with a class which provides
> reasonable getter/setter methods if we have enough bandwidth to do
refactoring.)

Acknowledged.

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:228: #   }
On 2015/07/28 00:38:47, bashi1 wrote:
> Can we factor out this block as a method?

Done.

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:233:
interface_basename, _ = os.path.splitext(os.path.basename(idl_filename))
On 2015/07/28 00:38:46, bashi1 wrote:
> idl_filename_to_interface_name(idl_filename)

Done.

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:235:
attr.extended_attributes['PartialInterfaceImplementedAs'] =
definition.extended_attributes.get('ImplementedAs', interface_basename)
On 2015/07/28 00:38:46, bashi1 wrote:
> We have similar logic in transfer_extended_attributes() which lives in
> interface_dependency_resolver.py. This implies that we may want to share code.
> That said, it's not clear to me whether it can be done easily. Could you add a
> TODO comment if you think we could share code?

Done.

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:290:
'include_path': this_include_path,
On 2015/07/28 00:38:46, bashi1 wrote:
> Can we remove this?

No, not at this moment.

'include_path' is required for V8Foo.cpp to #include "Foo.h".

'cpp_includes' above is required for V8Bar.cpp which inherits from Foo to
#include "Foo.h".

The current implementation treats them differently, so it's hard to remove
'include_path' at this moment.

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

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_overall.py:212: if
is_first_info:
On 2015/07/28 00:38:47, bashi1 wrote:
> Could you explain what you want to do by this change?

It turned out that it's totally unnecessary.  Removed.

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

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/interface_dependency_resolver.py:337: return
collect_unforgeable_attributes_in_ancestors(
On 2015/07/28 00:38:47, bashi1 wrote:
> How about merge-and-return recursive results (unforgeable_attributes,
> referenced_interfaces and cpp_includes) instead of passing them as arguments?
> 
> us, rs, cs =
> collect_unforgeable_attributes_in_ancestors(interface.get('parent'), ...)
> return us.extend(attrs), rs.extend(referenced),
> cs.update(interface.get('cpp_includes', {}).get(component, {}))

Done.

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/interface_dependency_resolver.py:342: for
interface_name, interface in definitions.interfaces.iteritems():
On 2015/07/28 00:38:47, bashi1 wrote:
> Help me understand: len(definitions.interfaces) could be greater than one
here?

For some interfaces, it's greater than one.
It seems there are interfaces that the main interface 'implements'.

> print definitions.interfaces
{'WindowBase64': <idl_definitions.IdlInterface object at 0x7f5ed9cd83d0>,
'WindowTimers': <idl_definitions.IdlInterface object at 0x7f5ed9897c90>,
'ImageBitmapFactories': <idl_definitions.IdlInterface object at 0x7f5ed98b02d0>,
'Window': <idl_definitions.IdlInterface object at 0x7f5ed9b7e690>,
'GlobalEventHandlers': <idl_definitions.IdlInterface object at 0x7f5ed98f0050>,
'WindowEventHandlers': <idl_definitions.IdlInterface object at 0x7f5ed9cd8190>}

[bashi, 2015-07-29 09:24:34]

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

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:218:
self.interfaces_info[definition.name] = {}
On 2015/07/29 08:16:24, Yuki wrote:
> On 2015/07/28 00:38:46, bashi1 wrote:
> > Why do you need this? Can't we just use |interface_info|?
> 
> Before my change, we didn't update interfaces_info for partial interfaces. 
With
> this change, I'm updating interfaces_info for both of (non-partial) interfaces
> and partial interfaces, so we cannot simply assign it and need to carefully
> merge them.

Got it.

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:290:
'include_path': this_include_path,
On 2015/07/29 08:16:24, Yuki wrote:
> On 2015/07/28 00:38:46, bashi1 wrote:
> > Can we remove this?
> 
> No, not at this moment.
> 
> 'include_path' is required for V8Foo.cpp to #include "Foo.h".
> 
> 'cpp_includes' above is required for V8Bar.cpp which inherits from Foo to
> #include "Foo.h".
> 
> The current implementation treats them differently, so it's hard to remove
> 'include_path' at this moment.

I see. Thanks for the explanation.

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

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/interface_dependency_resolver.py:342: for
interface_name, interface in definitions.interfaces.iteritems():
On 2015/07/29 08:16:24, Yuki wrote:
> On 2015/07/28 00:38:47, bashi1 wrote:
> > Help me understand: len(definitions.interfaces) could be greater than one
> here?
> 
> For some interfaces, it's greater than one.
> It seems there are interfaces that the main interface 'implements'.
> 
> > print definitions.interfaces
> {'WindowBase64': <idl_definitions.IdlInterface object at 0x7f5ed9cd83d0>,
> 'WindowTimers': <idl_definitions.IdlInterface object at 0x7f5ed9897c90>,
> 'ImageBitmapFactories': <idl_definitions.IdlInterface object at
0x7f5ed98b02d0>,
> 'Window': <idl_definitions.IdlInterface object at 0x7f5ed9b7e690>,
> 'GlobalEventHandlers': <idl_definitions.IdlInterface object at
0x7f5ed98f0050>,
> 'WindowEventHandlers': <idl_definitions.IdlInterface object at
0x7f5ed9cd8190>}

As chatted offline, this could be executed multiple times for right-hand
interfaces of 'implements'. It may be difficult to avoid multiple execution so
I'm fine with this if this doesn't cause unexpected inheritance. Could you check
it is safe?

[Yuki, 2015-07-29 09:32:38]

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

https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/interface_dependency_resolver.py:342: for
interface_name, interface in definitions.interfaces.iteritems():
On 2015/07/29 09:24:33, bashi1 wrote:
> On 2015/07/29 08:16:24, Yuki wrote:
> > On 2015/07/28 00:38:47, bashi1 wrote:
> > > Help me understand: len(definitions.interfaces) could be greater than one
> > here?
> > 
> > For some interfaces, it's greater than one.
> > It seems there are interfaces that the main interface 'implements'.
> > 
> > > print definitions.interfaces
> > {'WindowBase64': <idl_definitions.IdlInterface object at 0x7f5ed9cd83d0>,
> > 'WindowTimers': <idl_definitions.IdlInterface object at 0x7f5ed9897c90>,
> > 'ImageBitmapFactories': <idl_definitions.IdlInterface object at
> 0x7f5ed98b02d0>,
> > 'Window': <idl_definitions.IdlInterface object at 0x7f5ed9b7e690>,
> > 'GlobalEventHandlers': <idl_definitions.IdlInterface object at
> 0x7f5ed98f0050>,
> > 'WindowEventHandlers': <idl_definitions.IdlInterface object at
> 0x7f5ed9cd8190>}
> 
> As chatted offline, this could be executed multiple times for right-hand
> interfaces of 'implements'. It may be difficult to avoid multiple execution so
> I'm fine with this if this doesn't cause unexpected inheritance. Could you
check
> it is safe?

I've updated the code so that we don't add any duplicates when the loop process
the same interface multiple times.

[bashi, 2015-07-29 09:39:33]

On 2015/07/29 09:32:38, Yuki wrote:
>
https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
> File Source/bindings/scripts/interface_dependency_resolver.py (right):
> 
>
https://codereview.chromium.org/1257613003/diff/20001/Source/bindings/scripts...
> Source/bindings/scripts/interface_dependency_resolver.py:342: for
> interface_name, interface in definitions.interfaces.iteritems():
> On 2015/07/29 09:24:33, bashi1 wrote:
> > On 2015/07/29 08:16:24, Yuki wrote:
> > > On 2015/07/28 00:38:47, bashi1 wrote:
> > > > Help me understand: len(definitions.interfaces) could be greater than
one
> > > here?
> > > 
> > > For some interfaces, it's greater than one.
> > > It seems there are interfaces that the main interface 'implements'.
> > > 
> > > > print definitions.interfaces
> > > {'WindowBase64': <idl_definitions.IdlInterface object at 0x7f5ed9cd83d0>,
> > > 'WindowTimers': <idl_definitions.IdlInterface object at 0x7f5ed9897c90>,
> > > 'ImageBitmapFactories': <idl_definitions.IdlInterface object at
> > 0x7f5ed98b02d0>,
> > > 'Window': <idl_definitions.IdlInterface object at 0x7f5ed9b7e690>,
> > > 'GlobalEventHandlers': <idl_definitions.IdlInterface object at
> > 0x7f5ed98f0050>,
> > > 'WindowEventHandlers': <idl_definitions.IdlInterface object at
> > 0x7f5ed9cd8190>}
> > 
> > As chatted offline, this could be executed multiple times for right-hand
> > interfaces of 'implements'. It may be difficult to avoid multiple execution
so
> > I'm fine with this if this doesn't cause unexpected inheritance. Could you
> check
> > it is safe?
> 
> I've updated the code so that we don't add any duplicates when the loop
process
> the same interface multiple times.

Thanks! LGTM.

[Yuki, 2015-07-29 12:27:37]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2015-07-29 12:27:37]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1257613003/#ps80001
(title: "Made inherit_unforgeable_attributes more robust.")

[commit-bot: I haz the power, 2015-07-29 12:27:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257613003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257613003/80001

[commit-bot: I haz the power, 2015-07-29 12:32:11]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199658

[tasak, 2015-07-29 12:56:44]

tasak@google.com changed reviewers:
+ tasak@google.com

[tasak, 2015-07-29 12:56:45]

https://codereview.chromium.org/1257613003/diff/80001/Source/bindings/scripts...
File Source/bindings/scripts/interface_dependency_resolver.py (right):

https://codereview.chromium.org/1257613003/diff/80001/Source/bindings/scripts...
Source/bindings/scripts/interface_dependency_resolver.py:346:
interface_info['referenced_interfaces'] = sorted(set(referenced_interfaces))
Just my perfornal preference. I would like to avoid updating interface_info
while resolving interface_dependency.

Is it possible to add a new extended attribute like
[PartialInterfaceImplementedAs]?
i.e. add UnforguableAttributeImplementedAs to attributes' extended attributes.

[Yuki, 2015-08-03 02:02:13]

https://codereview.chromium.org/1257613003/diff/80001/Source/bindings/scripts...
File Source/bindings/scripts/interface_dependency_resolver.py (right):

https://codereview.chromium.org/1257613003/diff/80001/Source/bindings/scripts...
Source/bindings/scripts/interface_dependency_resolver.py:346:
interface_info['referenced_interfaces'] = sorted(set(referenced_interfaces))
On 2015/07/29 12:56:45, tasak wrote:
> Just my perfornal preference. I would like to avoid updating interface_info
> while resolving interface_dependency.
> 
> Is it possible to add a new extended attribute like
> [PartialInterfaceImplementedAs]?
> i.e. add UnforguableAttributeImplementedAs to attributes' extended attributes.
>  

Very good point.  It seems we may need redesign of IDL compiler, but it could be
a big work.  Let me leave this point as is and keep it in my mind, and when the
time come, let's work on the redesign.

[commit-bot: I haz the power, 2015-09-23 11:53:30]

Patchset 5 (id:??) landed as
https://crrev.com/e56d75cefca6bff8bb96648e86f7fba30740e5bb