Support per-overload [RuntimeEnabled] extended attribute

Source/bindings/scripts/v8_interface.py

 # Copyright (C) 2013 Google Inc. All rights reserved.
 # coding=utf-8
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
@@ skipping 336 matching lines @@
 def generate_overloads(overloads):
     """Returns |overloads| template values for a single name.
 
     Sets |method.overload_index| in place for |method| in |overloads|
     and returns dict of overall overload template values.
     """
     assert len(overloads) > 1  # only apply to overloaded names
     for index, method in enumerate(overloads, 1):
         method['overload_index'] = index
 
+    runtime_enabled_features = set(method.get('runtime_enabled_function')

[Nils Barth (inactive), 2014/05/27 06:33:20]

You don't need dict.get() here, do you?

[Jens Widell, 2014/05/27 06:46:20]

Needed for overloaded constructors currently; generate_constructor() doesn't set
'runtime_enabled_function'. Maybe it should (i.e. maybe we should support
runtime enabled constructor overloads too) and either way, it could of course at
least set it to None to allow us to assume it's there.

Which do you prefer? :-)

[Nils Barth (inactive), 2014/05/27 07:24:17]

Oh, good point.
get() is a bit better, since having long lists of default None/False for the
constructor context is fragile and gross.
Thanks!

+                                   for method in overloads)
+    if len(runtime_enabled_features) > 1:

[Nils Barth (inactive), 2014/05/27 06:33:20]

Shouldn't you use the utility common_value() function instead?

[Jens Widell, 2014/05/27 06:46:20]

It returns None both if there is no common value and when the common value is
None. I handle those two cases differently here.

[Nils Barth (inactive), 2014/05/27 07:24:17]

Oh, good point.

I'd prefer consistency with the other [DeprecateAs] and [MeasureAs] code,
however you do it.
Those treat it by having a separate *_all_as variable and branching in Jinja,
instead of changing the variables in Python.

I think you can handle these uniformly via something like:
Python:
'runtime_enabled_function_all': common_value(overloads,
'runtime_enabled_function'),

Jinja:
{% filter runtime_enabled(not overloads.runtime_enabled_function_all and
method.runtime_enabled_function) %}

WDYT?

[Jens Widell, 2014/05/27 07:39:17]

That works nicely in the overload_resolution_method() macro, but in the
configure_class_template block, where the method is registered, I don't have the
'overloads' dict accessible. Almost, though. It's stored on the last method, and
the method registration is generated for the first method:

  {% if not method.overload_index or method.overload_index == 1 %}
  {# For overloaded methods, only generate one accessor #}

I could change that condition to

  {% if not method.overload_index or method.overloads %}

I think, and then use

  {% filter runtime_enabled(method.overloads.runtime_enabled_function_all) %}

to make the registration runtime enabled.

Good, you think? (I haven't tested it.)

[Jens Widell, 2014/05/27 07:42:15]

But that of course doesn't work for non-overloaded methods.

[Nils Barth (inactive), 2014/05/27 07:50:42]

That sounds great!

[Nils Barth (inactive), 2014/05/27 07:50:42]

...why not? (confused)

+        # More than one different value for [RuntimeEnabled] among the

[Nils Barth (inactive), 2014/05/27 06:33:20]

Could you clarify that this also works for *missing* values?

E.g.,
f();
[RuntimeEnabled=Foo] f(long x);

You have a test case, but None makes it a bit weird.

[Jens Widell, 2014/05/27 06:46:20]

Yeah, I'm essentially treating None as any other value for the RuntimeEnabled
attribute. In your example, 'runtime_enabled_features' is set([None, 'Foo'])
which looks like more than one different value and triggers per-overload
handling.

I can add a comment about it. I could also rewrite the code to handle None
explicitly, if you like.

[Nils Barth (inactive), 2014/05/27 07:24:17]

None is confusing enough that I'd appreciate a comment, if you'd be so kind :)
(The value of no value is itself a value. It's so Zen.)
I think you can avoid special-casing None by using a more Jinja-y style, as
outlined above.

+        # overloads; need to handle as part of overload resoultion.
+        for method in overloads:
+            method['overload_runtime_enabled_function'] = method['runtime_enabled_function']
+            method['runtime_enabled_function'] = None
+
     effective_overloads_by_length = effective_overload_set_by_length(overloads)
     lengths = [length for length, _ in effective_overloads_by_length]
 
     return {
         'deprecate_all_as': common_value(overloads, 'deprecate_as'),  # [DeprecateAs]
         'length_tests_methods': length_tests_methods(effective_overloads_by_length),
         'minarg': lengths[0],
         # 1. Let maxarg be the length of the longest type list of the
         # entries in S.
         'maxarg': lengths[-1],
@@ skipping 323 matching lines @@
     # that’s the end, and no other tests are needed.)
 
     # 11. Otherwise: if there is an entry in S that has one of the following
     # types at position i of its type list,
     # • DOMString
     # • an enumeration type
     try:
         method = next(method for idl_type, method in idl_types_methods
                       if idl_type.name == 'String' or idl_type.is_enum)
         yield 'true', method
-        return
+        # Stop here unless this overload (and not all overloads) is runtime

[Jens Widell, 2014/05/23 14:55:16]

A simpler fix here would of course be to just remove the 'return' here and let
the compiler eliminate the resulting dead code that would be produced in cases
where no [RuntimeEnabled] is involved and there are string+number overloads:

  if (true) {
    foo1Method(info); // string
    return;
  }
  if (true) {
    foo2Method(info); // number
    return;
  }

[Nils Barth (inactive), 2014/05/27 06:33:20]

That's fine to do: CG simplicity is a higher priority than generated source code
legibility, per:
http://www.chromium.org/developers/design-documents/idl-compiler#TOC-Goals

Could you remove the early returns here and below, and add a note about the dead
code elimination in the comment beginning:
# (Perform automatic type conversion, in order.

Something like: "We rely on compiler dead code elimination to remove redundant
branches."

+        # enabled.
+        if not method.get('overload_runtime_enabled_function'):
+            return
     except StopIteration:
         pass
 
     # 12. Otherwise: if there is an entry in S that has one of the following
     # types at position i of its type list,
     # • a numeric type
     try:
         method = next(method for idl_type, method in idl_types_methods
                       if idl_type.is_numeric_type)
         yield 'true', method
@@ skipping 296 matching lines @@
         deleter = next(
             method
             for method in interface.operations
             if ('deleter' in method.specials and
                 len(method.arguments) == 1 and
                 str(method.arguments[0].idl_type) == 'DOMString'))
     except StopIteration:
         return None
 
     return property_deleter(deleter)