Make [OriginTrialEnabled] and [RuntimeEnabled] mutually exclusive

third_party/WebKit/Source/bindings/scripts/v8_utilities.py

 # Copyright (C) 2013 Google Inc. All rights reserved.
 #
 # 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
 # copyright notice, this list of conditions and the following disclaimer
@@ skipping 348 matching lines @@
         return lambda suffix: extended_attributes['MeasureAs']
     if 'Measure' in extended_attributes:
         includes.add('core/frame/UseCounter.h')
         measure_as_name = capitalize(definition_or_member.name)
         if interface is not None:
             measure_as_name = '%s_%s' % (capitalize(interface.name), measure_as_name)
         return lambda suffix: 'V8%s_%s' % (measure_as_name, suffix)
     return None
 
 
+# [OriginTrialEnabled]
+def origin_trial_enabled(definition_or_member, interface):

[iclelland, 2016/04/05 13:38:58]

This function's name suggests that it is a boolean value; it actually returns a
method to call, or None if origin trials are not enabled for a particular
feature. (And, by side effect, adds the appropriate #includes). Is there a
better name for this?

(I like the function, it simplifies a lot of other code, but the final result is
that I had to go looking here to see what it actually *did*)

[chasej, 2016/04/05 19:50:22]

The function is implemented similar to the deprecate_as() and measure_as()
functions. Those functions seem to be named after the extended attribute
(without any prefix/suffix), so I used similar naming for this function.
Admittedly, the fact that the attribute name includes "enabled" confuses things.
 
Looking at all the functions in this file, there doesn't seem to be a majority
convention for extended attributes. The runtime_enabled_function_name() is an
obvious counter-example to the approach I took.

Other than adding the results of these functions to the bindings context, the
usage seems rather separated (i.e. you have to look in the *.cpp template
files). Given that, I'm not sure what naming approach is best. Suggestions for a
better name are welcome.

[iclelland, 2016/04/07 14:00:31]

With the obvious parallels to RuntimeEnabled code, I think I'd be happy with
(the slightly longer) origin_trial_enabled_function_name

+    extended_attributes = definition_or_member.extended_attributes
+    is_origin_trial_enabled = 'OriginTrialEnabled' in extended_attributes
+
+    if (is_origin_trial_enabled and 'RuntimeEnabled' in extended_attributes):

[iclelland, 2016/04/05 13:38:58]

Is there a compelling reason for this check to be here (as opposed to in
runtime_enabled, or both) Or could it be checked at a higher level?

[chasej, 2016/04/05 19:50:22]

I had the check here because it really belongs to [OriginTrialEnabled], not to
[RuntimeEnabled]. This could be checked at a higher level, but then it's not
encapsulated as nicely. Every caller that uses origin_trial_enabled() would have
to know/remember to add the check.

[iclelland, 2016/04/07 14:00:32]

It just seems like an obvious symmetry, that it could be checked in either
place, and the only difference is that this method is *newer*.

If it doesn't makes sense to put this check in v8_attributes.py, where the
includes.add lines were removed, then I'm okay with it being here. (And see the
comments in V8TestInterface2.cpp as well)

+        raise Exception('[OriginTrialEnabled] and [RuntimeEnabled] must '

[iclelland, 2016/04/05 13:38:58]

I'd expect a more specific exception type to be raised, but that might be a
consequence of where this code is right now.

[chasej, 2016/04/05 19:50:22]

Other than a few specific scenarios, most of the bindings script seem to raise
either Exception or ValueError. I didn't notice a specific pattern to either.
I'm open to suggestions here.

[iclelland, 2016/04/07 14:00:32]

Acknowledged. All I had seen (without looking *too* hard) was ValueError, which
doesn't seem to fit here. If there's nothing else in common use here, and no
user-defined exceptions, then adding them here seems out-of-scope. 

+                        'not be specified on the same definition: '
+                        '%s.%s' % (definition_or_member.idl_name, definition_or_member.name))
+
+    if is_origin_trial_enabled:
+        includes.add('core/inspector/ConsoleMessage.h')
+        includes.add('core/origin_trials/OriginTrials.h')
+
+        trial_name = extended_attributes['OriginTrialEnabled']
+        return 'OriginTrials::%sEnabled' % uncapitalize(trial_name)
+
+    return None
+
+
 def runtime_feature_name(definition_or_member):
     extended_attributes = definition_or_member.extended_attributes
     if 'RuntimeEnabled' not in extended_attributes:
         return None
     return extended_attributes['RuntimeEnabled']
 
 
-def is_origin_trial_enabled(definition_or_member):
-    return 'OriginTrialEnabled' in definition_or_member.extended_attributes
-
-
-def origin_trial_name(definition_or_member):
-    return definition_or_member.extended_attributes['OriginTrialEnabled'] if is_origin_trial_enabled(definition_or_member) else None
-
-
-def origin_trial_enabled_function(definition_or_member):
-    trial_name = origin_trial_name(definition_or_member)
-    feature_name = runtime_feature_name(definition_or_member)
-    if not feature_name or not trial_name:
-        return
-    return 'OriginTrials::%sEnabled' % uncapitalize(feature_name)
-
-
 # [RuntimeEnabled]
 def runtime_enabled_function_name(definition_or_member):
     """Returns the name of the RuntimeEnabledFeatures function.

[iclelland, 2016/04/05 13:38:58]

Document the new side effect

[chasej, 2016/04/05 19:50:22]

Done.

 
     The returned function checks if a method/attribute is enabled.
     Given extended attribute RuntimeEnabled=FeatureName, return:
         RuntimeEnabledFeatures::{featureName}Enabled
     """
     feature_name = runtime_feature_name(definition_or_member)
 
     # If an origin trial is on the method/attribute, it overrides the runtime
     # enabled status. For now, we are unconditionally installing these
     # attributes/methods, so we are acting as though the runtime enabled
     # function doesn't exist. (It is checked in the generated OriginTrials
     # function, instead)
-    trial_name = origin_trial_name(definition_or_member)
-    if not feature_name or trial_name:
+    trial_enabled = origin_trial_enabled(definition_or_member, None)
+    if not feature_name or trial_enabled:

[haraken, 2016/04/05 01:23:18]

Do we still need this check?

[chasej, 2016/04/05 19:50:22]

You're right, we probably don't need this check anymore, given the
check/exception in the origin_trial_enabled() function.

Removed.

         return
+
+    includes.add('platform/RuntimeEnabledFeatures.h')
     return 'RuntimeEnabledFeatures::%sEnabled' % uncapitalize(feature_name)
 
 
 # [Unforgeable]
 def is_unforgeable(interface, member):
     return (('Unforgeable' in interface.extended_attributes or
              'Unforgeable' in member.extended_attributes) and
             not member.is_static)
 
 
@@ skipping 173 matching lines @@
     except StopIteration:
         return None
 
 
 IdlInterface.indexed_property_getter = property(indexed_property_getter)
 IdlInterface.indexed_property_setter = property(indexed_property_setter)
 IdlInterface.indexed_property_deleter = property(indexed_property_deleter)
 IdlInterface.named_property_getter = property(named_property_getter)
 IdlInterface.named_property_setter = property(named_property_setter)
 IdlInterface.named_property_deleter = property(named_property_deleter)