Move methods about indexed/named getters/setters/deleters to v8_utilities.py

Move methods about indexed/named getters/setters/deleters to v8_utilities.py

This is a split from https://codereview.chromium.org/447523003.
We don't have a plan to land the original CL soon but multiple people are applying
the CL locally, so it is important to make the CL size smaller.
This CL helps decrease the original CL.

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

[haraken, 2015-01-23 11:58:42]

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

[haraken, 2015-01-23 11:58:43]

PTAL

[Jens Widell, 2015-01-23 12:05:12]

jl@opera.com changed reviewers:
+ jl@opera.com

[Jens Widell, 2015-01-23 12:05:13]

I might make some sense to move the logic for finding the indexed/named
getter/setter/deleter in an interface to v8_utilities.py, but keep the functions
calculating the template context in v8_interface.py.

That is, have, in v8_interface.py,

  def property_getter(...):
    # same as today
    return { ... }

  def indexed_property_getter(interface, ...):
    getter = v8_utilities.indexed_property_getter(interface)
    if not getter:
      return None
    return property_getter(...)

[Yuki, 2015-01-23 12:10:40]

lgtm.  I'm okay with either of the current CL or jl's idea.

[haraken, 2015-01-23 12:14:51]

On 2015/01/23 12:05:13, Jens Widell wrote:
> I might make some sense to move the logic for finding the indexed/named
> getter/setter/deleter in an interface to v8_utilities.py, but keep the
functions
> calculating the template context in v8_interface.py.
> 
> That is, have, in v8_interface.py,
> 
>   def property_getter(...):
>     # same as today
>     return { ... }
> 
>   def indexed_property_getter(interface, ...):
>     getter = v8_utilities.indexed_property_getter(interface)
>     if not getter:
>       return None
>     return property_getter(...)

I'd slightly prefer the current CL since I don't want to create a dependency
from v8_utilities.py to v8_interface.py. Currently v8_utilities don't depend on
v8_interface/attributes/methods etc. WDYT?

[Jens Widell, 2015-01-23 12:25:47]

On 2015/01/23 12:14:51, haraken wrote:
> On 2015/01/23 12:05:13, Jens Widell wrote:
> > I might make some sense to move the logic for finding the indexed/named
> > getter/setter/deleter in an interface to v8_utilities.py, but keep the
> functions
> > calculating the template context in v8_interface.py.
> > 
> > That is, have, in v8_interface.py,
> > 
> >   def property_getter(...):
> >     # same as today
> >     return { ... }
> > 
> >   def indexed_property_getter(interface, ...):
> >     getter = v8_utilities.indexed_property_getter(interface)
> >     if not getter:
> >       return None
> >     return property_getter(...)
> 
> I'd slightly prefer the current CL since I don't want to create a dependency
> from v8_utilities.py to v8_interface.py. Currently v8_utilities don't depend
on
> v8_interface/attributes/methods etc. WDYT?

Not sure I see how my proposal introduces that dependency. The helper that finds
the getter would only depend on idl_definitions.py, I think?

[Jens Widell, 2015-01-23 13:03:34]

On 2015/01/23 12:25:47, Jens Widell wrote:
> On 2015/01/23 12:14:51, haraken wrote:
> > On 2015/01/23 12:05:13, Jens Widell wrote:
> > > I might make some sense to move the logic for finding the indexed/named
> > > getter/setter/deleter in an interface to v8_utilities.py, but keep the
> > functions
> > > calculating the template context in v8_interface.py.
> > > 
> > > That is, have, in v8_interface.py,
> > > 
> > >   def property_getter(...):
> > >     # same as today
> > >     return { ... }
> > > 
> > >   def indexed_property_getter(interface, ...):
> > >     getter = v8_utilities.indexed_property_getter(interface)
> > >     if not getter:
> > >       return None
> > >     return property_getter(...)
> > 
> > I'd slightly prefer the current CL since I don't want to create a dependency
> > from v8_utilities.py to v8_interface.py. Currently v8_utilities don't depend
> on
> > v8_interface/attributes/methods etc. WDYT?
> 
> Not sure I see how my proposal introduces that dependency. The helper that
finds
> the getter would only depend on idl_definitions.py, I think?

To clarify; the code I propose we add to v8_utilities.py instead is:

def indexed_property_getter(interface):
    try:
        # Find indexed property getter, if present; has form:
        # getter TYPE [OPTIONAL_IDENTIFIER](unsigned long ARG1)
        return next(
            method
            for method in interface.operations
            if ('getter' in method.specials and
                len(method.arguments) == 1 and
                str(method.arguments[0].idl_type) == 'unsigned long'))
    except StopIteration:
        return None

(or some variation thereof.) This is all that's actually needed by
https://codereview.chromium.org/447523003/, right?

[Jens Widell, 2015-01-23 13:05:36]

On 2015/01/23 13:03:34, Jens Widell wrote:
> On 2015/01/23 12:25:47, Jens Widell wrote:
> > On 2015/01/23 12:14:51, haraken wrote:
> > > On 2015/01/23 12:05:13, Jens Widell wrote:
> > > > I might make some sense to move the logic for finding the indexed/named
> > > > getter/setter/deleter in an interface to v8_utilities.py, but keep the
> > > functions
> > > > calculating the template context in v8_interface.py.
> > > > 
> > > > That is, have, in v8_interface.py,
> > > > 
> > > >   def property_getter(...):
> > > >     # same as today
> > > >     return { ... }
> > > > 
> > > >   def indexed_property_getter(interface, ...):
> > > >     getter = v8_utilities.indexed_property_getter(interface)
> > > >     if not getter:
> > > >       return None
> > > >     return property_getter(...)
> > > 
> > > I'd slightly prefer the current CL since I don't want to create a
dependency
> > > from v8_utilities.py to v8_interface.py. Currently v8_utilities don't
depend
> > on
> > > v8_interface/attributes/methods etc. WDYT?
> > 
> > Not sure I see how my proposal introduces that dependency. The helper that
> finds
> > the getter would only depend on idl_definitions.py, I think?
> 
> To clarify; the code I propose we add to v8_utilities.py instead is:
> 
> def indexed_property_getter(interface):
>     try:
>         # Find indexed property getter, if present; has form:
>         # getter TYPE [OPTIONAL_IDENTIFIER](unsigned long ARG1)
>         return next(
>             method
>             for method in interface.operations
>             if ('getter' in method.specials and
>                 len(method.arguments) == 1 and
>                 str(method.arguments[0].idl_type) == 'unsigned long'))
>     except StopIteration:
>         return None
> 
> (or some variation thereof.) This is all that's actually needed by
> https://codereview.chromium.org/447523003/, right?

And this could of course also be a extension property on IdlInterface, set by
v8_interface.py, by adding:

  IdlInterface.indexed_property_getter = property(indexed_property_getter)

in which case v8_attributes.py can use just "interface.indexed_property_getter"
and not care about where it's defined.

[haraken, 2015-01-26 02:12:09]

On 2015/01/23 13:05:36, Jens Widell wrote:
> On 2015/01/23 13:03:34, Jens Widell wrote:
> > On 2015/01/23 12:25:47, Jens Widell wrote:
> > > On 2015/01/23 12:14:51, haraken wrote:
> > > > On 2015/01/23 12:05:13, Jens Widell wrote:
> > > > > I might make some sense to move the logic for finding the
indexed/named
> > > > > getter/setter/deleter in an interface to v8_utilities.py, but keep the
> > > > functions
> > > > > calculating the template context in v8_interface.py.
> > > > > 
> > > > > That is, have, in v8_interface.py,
> > > > > 
> > > > >   def property_getter(...):
> > > > >     # same as today
> > > > >     return { ... }
> > > > > 
> > > > >   def indexed_property_getter(interface, ...):
> > > > >     getter = v8_utilities.indexed_property_getter(interface)
> > > > >     if not getter:
> > > > >       return None
> > > > >     return property_getter(...)
> > > > 
> > > > I'd slightly prefer the current CL since I don't want to create a
> dependency
> > > > from v8_utilities.py to v8_interface.py. Currently v8_utilities don't
> depend
> > > on
> > > > v8_interface/attributes/methods etc. WDYT?
> > > 
> > > Not sure I see how my proposal introduces that dependency. The helper that
> > finds
> > > the getter would only depend on idl_definitions.py, I think?
> > 
> > To clarify; the code I propose we add to v8_utilities.py instead is:
> > 
> > def indexed_property_getter(interface):
> >     try:
> >         # Find indexed property getter, if present; has form:
> >         # getter TYPE [OPTIONAL_IDENTIFIER](unsigned long ARG1)
> >         return next(
> >             method
> >             for method in interface.operations
> >             if ('getter' in method.specials and
> >                 len(method.arguments) == 1 and
> >                 str(method.arguments[0].idl_type) == 'unsigned long'))
> >     except StopIteration:
> >         return None
> > 
> > (or some variation thereof.) This is all that's actually needed by
> > https://codereview.chromium.org/447523003/, right?
> 
> And this could of course also be a extension property on IdlInterface, set by
> v8_interface.py, by adding:
> 
>   IdlInterface.indexed_property_getter = property(indexed_property_getter)
> 
> in which case v8_attributes.py can use just
"interface.indexed_property_getter"
> and not care about where it's defined.

Sounds nicer! Done.

[Jens Widell, 2015-01-26 06:05:04]

LGTM

[haraken, 2015-01-26 06:05:35]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-01-26 06:06:23]

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

[commit-bot: I haz the power, 2015-01-26 08:15:47]

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