Make [OriginTrialEnabled] and [RuntimeEnabled] mutually exclusive

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

Index: third_party/WebKit/Source/bindings/scripts/v8_utilities.py
diff --git a/third_party/WebKit/Source/bindings/scripts/v8_utilities.py b/third_party/WebKit/Source/bindings/scripts/v8_utilities.py
index 723bb55c77a94accf20f7910f852acb16beb121a..5a782707389c81d3c1e985c93ebdbbe72c71bbdc 100644
--- a/third_party/WebKit/Source/bindings/scripts/v8_utilities.py
+++ b/third_party/WebKit/Source/bindings/scripts/v8_utilities.py
@@ -366,27 +366,31 @@ def measure_as(definition_or_member, interface):
     return None
 
 
-def runtime_feature_name(definition_or_member):
+# [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
-    if 'RuntimeEnabled' not in extended_attributes:
-        return None
-    return extended_attributes['RuntimeEnabled']
+    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))
 
-def is_origin_trial_enabled(definition_or_member):
-    return 'OriginTrialEnabled' in definition_or_member.extended_attributes
+    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)
 
-def origin_trial_name(definition_or_member):
-    return definition_or_member.extended_attributes['OriginTrialEnabled'] if is_origin_trial_enabled(definition_or_member) else None
+    return 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)
+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']
 
 
 # [RuntimeEnabled]
@@ -404,9 +408,11 @@ def runtime_enabled_function_name(definition_or_member):
     # 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)