Move OptionalBool out of study.proto

Support class type in json_to_struct and use C++ class base::Optional<bool>
for is_low_end_device instead of OptionalBool.

Bug: 987717
Change-Id: Ica6b475af788bbe9842c8630ef96e2ad6ad7f80c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1973491Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Weilun Shi <sweilun@chromium.org>
Auto-Submit: Weilun Shi <sweilun@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729594}

components/variations/field_trial_config/field_trial_testing_config_schema.json

@@ -4,7 +4,7 @@
 
 {
   "type_name": "FieldTrialTestingConfig",
-  "headers": ["components/variations/proto/study.pb.h"],
+  "headers": ["base/optional.h", "components/variations/proto/study.pb.h"],
   "schema": [{
     "field": "studies",
     "type": "array",
@@ -33,9 +33,9 @@
               },
               {
                 "field": "is_low_end_device",
-                "type": "enum",
-                "ctype": "Study::OptionalBool",
-                "default": "Study::OPTIONAL_BOOL_MISSING",
+                "type": "class",
+                "ctype": "base::Optional<bool>",
+                "default": "base::nullopt",
                 "optional": "True"
               },
               {

components/variations/field_trial_config/field_trial_util.cc

@@ -16,6 +16,7 @@
 #include "base/feature_list.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/field_trial_params.h"
+#include "base/optional.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/system/sys_info.h"
 #include "components/variations/field_trial_config/fieldtrial_testing_config.h"
@@ -39,13 +40,11 @@ bool HasPlatform(const FieldTrialTestingExperiment& experiment,
 // is_low_end_device than the current system value does.
 // If experiment has is_low_end_device missing, then it is False.
 bool HasDeviceLevelMismatch(const FieldTrialTestingExperiment& experiment) {
-  if (experiment.is_low_end_device == Study::OPTIONAL_BOOL_MISSING) {
+  if (!experiment.is_low_end_device.has_value()) {
     return false;
   }
-  if (base::SysInfo::IsLowEndDevice()) {
-    return experiment.is_low_end_device == Study::OPTIONAL_BOOL_FALSE;
-  }
-  return experiment.is_low_end_device == Study::OPTIONAL_BOOL_TRUE;
+  return experiment.is_low_end_device.value() !=
+         base::SysInfo::IsLowEndDevice();
 }
 
 // Gets current form factor and converts it from enum DeviceFormFactor to enum

tools/json_to_struct/element_generator.py

@@ -111,7 +111,7 @@ def GenerateFieldContent(element_name, field_info, content, lines, indent,
   if content is None:
     content = field_info.get('default', None)
   type = field_info['type']
-  if type == 'int' or type == 'enum':
+  if type in ('int', 'enum', 'class'):
     lines.append('%s%s,' % (indent, content))
   elif type == 'string':
     _GenerateString(content, lines, indent)

tools/json_to_struct/element_generator_test.py

@@ -67,6 +67,19 @@ class ElementGeneratorTest(unittest.TestCase):
     GenerateFieldContent('', {'type': 'enum'}, 'BLUE', lines, '  ', {})
     self.assertEquals(['  BLUE,'], lines)
 
+  def testGenerateClassFieldContent(self):
+    lines = []
+    GenerateFieldContent('', {'type': 'class', 'default': 'base::nullopt'},
+                         None, lines, '  ', {})
+    self.assertEquals(['  base::nullopt,'], lines)
+    lines = []
+    GenerateFieldContent('', {'type': 'class', 'default': 'base::nullopt'},
+                         'true', lines, '  ', {})
+    self.assertEquals(['  true,'], lines)
+    lines = []
+    GenerateFieldContent('', {'type': 'class'}, 'false', lines, '  ', {})
+    self.assertEquals(['  false,'], lines)
+
   def testGenerateArrayFieldContent(self):
     lines = ['STRUCT BEGINS'];
     GenerateFieldContent('test', {'type': 'array', 'contents': {'type': 'int'}},

tools/json_to_struct/struct_generator.py

@@ -24,7 +24,7 @@ def GenerateField(field_info):
     return 'const char* const %s' % field
   elif type == 'string16':
     return 'const wchar_t* const %s' % field
-  elif type == 'enum':
+  elif type == 'enum' or type == 'class':
     return 'const %s %s' % (field_info['ctype'], field)
   elif type == 'array':
     return _GenerateArrayField(field_info)

tools/json_to_struct/struct_generator_test.py

@@ -33,6 +33,12 @@ class StructGeneratorTest(unittest.TestCase):
                        'field': 'bar_bar',
                        'contents': {'type': 'int'}}))
 
+  def testGenerateClassField(self):
+    self.assertEquals('const base::Optional<bool> bar',
+        GenerateField({'type': 'class',
+                       'field': 'bar',
+                       'ctype': 'base::Optional<bool>'}))
+
   def testGenerateStruct(self):
     schema = [
       {'type': 'int', 'field': 'foo_bar'},

tools/variations/fieldtrial_to_struct.py

@@ -58,8 +58,7 @@ def _FormFactorEnumValue(form_factor):
   return 'Study::' + form_factor.upper()
 
 def _Load(filename):
-  """Loads a JSON file into a Python object and return this object.
-  """
+  """Loads a JSON file into a Python object and return this object."""
   with open(filename, 'r') as handle:
     result = json.loads(json_comment_eater.Nom(handle.read()))
   return result
@@ -82,14 +81,29 @@ def _ConvertOverrideUIStrings(override_ui_strings):
 
 def _CreateExperiment(experiment_data,
                       platforms,
-                      is_low_end_device,
-                      form_factors):
+                      form_factors,
+                      is_low_end_device):
+  """Creates an experiment dictionary with all necessary information.
+
+  Args:
+    experiment_data: An experiment json config.
+    platforms: A list of platforms for this trial. This should be
+      a subset of |_platforms|.
+    form_factors: A list of form factors for this trial. This should be
+      a subset of |_form_factors|.
+    is_low_end_device: An optional parameter. This can either be True or
+      False. None if not specified.
+
+  Returns:
+    An experiment dict.
+  """
   experiment = {
     'name': experiment_data['name'],
     'platforms': [_PlatformEnumValue(p) for p in platforms],
-    'is_low_end_device': is_low_end_device,
     'form_factors': [_FormFactorEnumValue(f) for f in form_factors],
   }
+  if is_low_end_device is not None:
+    experiment['is_low_end_device'] = str(is_low_end_device).lower()
   forcing_flags_data = experiment_data.get('forcing_flag')
   if forcing_flags_data:
     experiment['forcing_flag'] = forcing_flags_data
@@ -110,26 +124,22 @@ def _CreateExperiment(experiment_data,
   return experiment
 
 def _CreateTrial(study_name, experiment_configs, platforms):
-  """Returns the applicable experiments for |study_name| and |platforms|. This
-  iterates through all of the experiment_configs for |study_name| and picks out
-  the applicable experiments based off of the valid platforms and device
-  type settings if specified.
+  """Returns the applicable experiments for |study_name| and |platforms|.
+
+  This iterates through all of the experiment_configs for |study_name|
+  and picks out the applicable experiments based off of the valid platforms
+  and device type settings if specified.
   """
   experiments = []
   for config in experiment_configs:
     platform_intersection = [p for p in platforms if p in config['platforms']]
-    is_low_end_device = 'Study::OPTIONAL_BOOL_MISSING'
-    if 'is_low_end_device' in config:
-      is_low_end_device = ('Study::OPTIONAL_BOOL_TRUE'
-                           if config['is_low_end_device']
-                           else 'Study::OPTIONAL_BOOL_FALSE')
 
     if platform_intersection:
       experiments += [_CreateExperiment(
                           e,
                           platform_intersection,
-                          is_low_end_device,
-                          config.get('form_factors', []))
+                          config.get('form_factors', []),
+                          config.get('is_low_end_device'))
                       for e in config['experiments']]
   return {
     'name': study_name,

tools/variations/fieldtrial_to_struct_unittest.py

@@ -80,7 +80,6 @@ class FieldTrialToStruct(unittest.TestCase):
                   ],
                   'enable_features': ['A', 'B'],
                   'disable_features': ['C'],
-                  'is_low_end_device': 'Study::OPTIONAL_BOOL_MISSING',
                   'form_factors': [],
                 },
                 {
@@ -92,7 +91,6 @@ class FieldTrialToStruct(unittest.TestCase):
                   ],
                   'enable_features': ['D', 'E'],
                   'disable_features': ['F'],
-                  'is_low_end_device': 'Study::OPTIONAL_BOOL_MISSING',
                   'form_factors': [],
                 },
               ],
@@ -103,7 +101,6 @@ class FieldTrialToStruct(unittest.TestCase):
                 {
                   'name': 'OtherGroup',
                   'platforms': ['Study::PLATFORM_WINDOWS'],
-                  'is_low_end_device': 'Study::OPTIONAL_BOOL_MISSING',
                   'form_factors': [],
                 }
               ]
@@ -115,7 +112,6 @@ class FieldTrialToStruct(unittest.TestCase):
                     'name': 'ForcedGroup',
                     'platforms': ['Study::PLATFORM_WINDOWS'],
                     'forcing_flag': "my-forcing-flag",
-                    'is_low_end_device': 'Study::OPTIONAL_BOOL_MISSING',
                     'form_factors': [],
                   }
               ]
@@ -131,7 +127,7 @@ class FieldTrialToStruct(unittest.TestCase):
     'Trial1': [
       {
         'platforms': ['windows', 'ios'],
-        'is_low_end_device': True,
+        'is_low_end_device': 'true',
         'experiments': [
           {
             'name': 'Group1',
@@ -189,7 +185,7 @@ class FieldTrialToStruct(unittest.TestCase):
                   ],
                   'enable_features': ['A', 'B'],
                   'disable_features': ['C'],
-                  'is_low_end_device': 'Study::OPTIONAL_BOOL_TRUE',
+                  'is_low_end_device': 'true',
                   'form_factors': [],
                 },
                 {
@@ -201,13 +197,12 @@ class FieldTrialToStruct(unittest.TestCase):
                   ],
                   'enable_features': ['D', 'E'],
                   'disable_features': ['F'],
-                  'is_low_end_device': 'Study::OPTIONAL_BOOL_TRUE',
+                  'is_low_end_device': 'true',
                   'form_factors': [],
                 },
                 {
                   'name': 'IOSOnly',
                   'platforms': ['Study::PLATFORM_IOS'],
-                  'is_low_end_device': 'Study::OPTIONAL_BOOL_MISSING',
                   'form_factors': [],
                 },
               ],
@@ -232,7 +227,6 @@ class FieldTrialToStruct(unittest.TestCase):
                 {
                   'name': 'OtherGroup',
                   'platforms': ['Study::PLATFORM_MAC'],
-                  'is_low_end_device': 'Study::OPTIONAL_BOOL_MISSING',
                   'form_factors': [],
                 },
               ],
@@ -274,7 +268,6 @@ class FieldTrialToStruct(unittest.TestCase):
                 {
                   'name': 'Group1',
                   'platforms': ['Study::PLATFORM_WINDOWS'],
-                  'is_low_end_device': 'Study::OPTIONAL_BOOL_MISSING',
                   'form_factors': ['Study::DESKTOP', 'Study::PHONE'],
                 },
               ],
@@ -285,7 +278,6 @@ class FieldTrialToStruct(unittest.TestCase):
                 {
                   'name': 'OtherGroup',
                   'platforms': ['Study::PLATFORM_WINDOWS'],
-                  'is_low_end_device': 'Study::OPTIONAL_BOOL_MISSING',
                   'form_factors': ['Study::TABLET'],
                 },
               ],
@@ -352,7 +344,6 @@ class FieldTrialToStruct(unittest.TestCase):
                       'value': 'test2'
                     },
                   ],
-                  'is_low_end_device': 'Study::OPTIONAL_BOOL_MISSING',
                   'form_factors': [],
                 },
               ],
@@ -370,7 +361,6 @@ class FieldTrialToStruct(unittest.TestCase):
                       'value': 'test3'
                     },
                   ],
-                  'is_low_end_device': 'Study::OPTIONAL_BOOL_MISSING',
                   'form_factors': [],
                 },
               ],

tools/variations/unittest_data/DEPS

 specific_include_rules = {
-  "expected_output.h": ["+components/variations/proto/study.pb.h"]
+  "expected_output.h": [
+      "+base/optional.h",
+      "+components/variations/proto/study.pb.h"
+  ]
 }

tools/variations/unittest_data/expected_output.cc

@@ -28,7 +28,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_5[] = {
       1,
       array_kFieldTrialConfig_form_factors_8,
       0,
-      Study::OPTIONAL_BOOL_MISSING,
+      base::nullopt,
       NULL,
       0,
       NULL,
@@ -60,7 +60,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_4[] = {
       1,
       array_kFieldTrialConfig_form_factors_6,
       2,
-      Study::OPTIONAL_BOOL_MISSING,
+      base::nullopt,
       NULL,
       0,
       NULL,
@@ -77,7 +77,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_4[] = {
       1,
       array_kFieldTrialConfig_form_factors_7,
       1,
-      Study::OPTIONAL_BOOL_MISSING,
+      base::nullopt,
       NULL,
       0,
       NULL,
@@ -101,7 +101,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_3[] = {
       1,
       array_kFieldTrialConfig_form_factors_5,
       0,
-      Study::OPTIONAL_BOOL_MISSING,
+      base::nullopt,
       NULL,
       0,
       NULL,
@@ -130,7 +130,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_2[] = {
       1,
       array_kFieldTrialConfig_form_factors_3,
       0,
-      Study::OPTIONAL_BOOL_TRUE,
+      true,
       NULL,
       0,
       NULL,
@@ -147,7 +147,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_2[] = {
       1,
       array_kFieldTrialConfig_form_factors_4,
       0,
-      Study::OPTIONAL_BOOL_FALSE,
+      false,
       NULL,
       0,
       NULL,
@@ -174,7 +174,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_1[] = {
       1,
       array_kFieldTrialConfig_form_factors_2,
       0,
-      Study::OPTIONAL_BOOL_MISSING,
+      base::nullopt,
       NULL,
       0,
       array_kFieldTrialConfig_enable_features_1,
@@ -237,7 +237,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_0[] = {
       1,
       array_kFieldTrialConfig_form_factors_0,
       0,
-      Study::OPTIONAL_BOOL_MISSING,
+      base::nullopt,
       array_kFieldTrialConfig_params,
       2,
       array_kFieldTrialConfig_enable_features,
@@ -254,7 +254,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_0[] = {
       1,
       array_kFieldTrialConfig_form_factors_1,
       0,
-      Study::OPTIONAL_BOOL_MISSING,
+      base::nullopt,
       array_kFieldTrialConfig_params_0,
       2,
       array_kFieldTrialConfig_enable_features_0,
@@ -278,7 +278,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = {
       1,
       array_kFieldTrialConfig_form_factors,
       0,
-      Study::OPTIONAL_BOOL_MISSING,
+      base::nullopt,
       NULL,
       0,
       NULL,

tools/variations/unittest_data/expected_output.h

@@ -12,6 +12,7 @@
 
 #include <cstddef>
 
+#include "base/optional.h"
 #include "components/variations/proto/study.pb.h"
 
 struct OverrideUIString {
@@ -30,7 +31,7 @@ struct FieldTrialTestingExperiment {
   const size_t platforms_size;
   const Study::FormFactor * form_factors;
   const size_t form_factors_size;
-  const Study::OptionalBool is_low_end_device;
+  const base::Optional<bool> is_low_end_device;
   const FieldTrialTestingExperimentParams * params;
   const size_t params_size;
   const char* const * enable_features;