[Sync::Cleanup] Exclude deprecated model types from presubmit check.

1) Exclude deprecated model types (e.g. DEPRECATED_SUPERVISED_USERS and DEPRECATED_SUPERVISED_USER_SHARED_SETTINGS) from kModelTypeInfoMap presubmit check.

2) Improve a couple of comments in model_type.cc.

Change-Id: I68fd166396804c69ca567686b671ee9d19c624ac
Reviewed-on: https://chromium-review.googlesource.com/1054875Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557858}

components/sync/PRESUBMIT.py

@@ -13,19 +13,22 @@ import re
 
 # Some definitions don't follow all the conventions we want to enforce.
 # It's either difficult or impossible to fix this, so we ignore the problem(s).
-GRANDFATHERED_MODEL_TYPES = [
+EXCEPTION_MODEL_TYPES = [
+  # Grandfathered types:
   'UNSPECIFIED',  # Doesn't have a root tag or notification type.
   'TOP_LEVEL_FOLDER',  # Doesn't have a root tag or notification type.
   'AUTOFILL_WALLET_DATA',  # Root tag and model type string lack DATA suffix.
   'APP_SETTINGS',  # Model type string has inconsistent capitalization.
   'EXTENSION_SETTINGS',  # Model type string has inconsistent capitalization.
+  'PROXY_TABS',  # Doesn't have a root tag or notification type.
+  'NIGORI',  # Model type string is 'encryption keys'.
   'SUPERVISED_USER_SETTINGS',  # Root tag and model type string replace
-                 # 'Supervised' with 'Managed'
-  'SUPERVISED_USERS',  # See previous.
+                               # 'Supervised' with 'Managed'
   'SUPERVISED_USER_WHITELISTS',  # See previous.
-  'SUPERVISED_USER_SHARED_SETTINGS',  # See previous.
-  'PROXY_TABS',  # Doesn't have a root tag or notification type.
-  'NIGORI']  # Model type string is 'encryption keys'.
+
+  # Deprecated types:
+  'DEPRECATED_SUPERVISED_USERS',
+  'DEPRECATED_SUPERVISED_USER_SHARED_SETTINGS']
 
 # Root tags are used as prefixes when creating storage keys, so certain strings
 # are blacklisted in order to prevent prefix collision.
@@ -108,7 +111,7 @@ def CheckModelTypeInfoMap(input_api, output_api, model_type_file):
         output_api, map_entry, proto_field_definitions))
     entry_problems.extend(CheckRootTagNotInBlackList(output_api, map_entry))
 
-    if map_entry.model_type not in GRANDFATHERED_MODEL_TYPES:
+    if map_entry.model_type not in EXCEPTION_MODEL_TYPES:
       entry_problems.extend(
         CheckModelTypeStringMatchesModelType(output_api, map_entry))
       entry_problems.extend(
@@ -164,6 +167,9 @@ def ParseModelTypeEntries(input_api, model_type_cc_path):
   inside_enum = False
   current_line_number = 1
   for line in file_contents.splitlines():
+    if line.strip().startswith('//'):
+        # Ignore comments.
+        continue
     if start_pattern.match(line):
       inside_enum = True
       continue

components/sync/PRESUBMIT_test.py

@@ -76,6 +76,8 @@ MOCK_PROTOFILE_CONTENTS = ('\n'
   'optional AppSpecifics app = 456;\n'
   'optional AppSettingSpecifics app_setting = 789;\n'
   'optional ExtensionSettingSpecifics extension_setting = 910;\n'
+  'optional ManagedUserSharedSettingSpecifics managed_user_shared_setting'
+                                                                    ' = 915;\n'
   '//comment\n'
   '}\n')
 
@@ -84,6 +86,7 @@ MOCK_PROTOFILE_CONTENTS = ('\n'
 # in order to test presubmit parsing of the ModelTypeInfoMap in that file.
 MOCK_MODELTYPE_CONTENTS =('\n'
   'const ModelTypeInfo kModelTypeInfoMap[] = {\n'
+  '// Some comment \n'
   '{APP_SETTINGS, "APP_SETTING", "app_settings", "App settings",\n'
   'sync_pb::EntitySpecifics::kAppSettingFieldNumber, 13},\n'
   '%s\n'
@@ -107,6 +110,13 @@ class ModelTypeInfoChangeTest(unittest.TestCase):
     results = self._testChange('{PROXY_TABS, "", "", "Tabs", -1, 25},')
     self.assertEqual(0, len(results))
 
+  def testValidChangeDeprecatedEntry(self):
+    results = self._testChange('{DEPRECATED_SUPERVISED_USER_SHARED_SETTINGS,'
+    '"MANAGED_USER_SHARED_SETTING", "managed_user_shared_settings",'
+    '"Managed User Shared Settings",'
+    'sync_pb::EntitySpecifics::kManagedUserSharedSettingFieldNumber, 30},')
+    self.assertEqual(0, len(results))
+
   def testInvalidChangeMismatchedNotificationType(self):
     results = self._testChange('{AUTOFILL, "AUTOFILL_WRONG", "autofill",\n'
      '"Autofill",sync_pb::EntitySpecifics::kAutofillFieldNumber, 6},')

components/sync/syncable/model_type.cc

@@ -41,11 +41,10 @@ struct ModelTypeInfo {
   // This should be the same as the model type but space separated and the
   // first letter of every word capitalized.
   const char* model_type_string;
-  // SpecificsFieldNumber for Model Type
+  // Field number of the model type specifics in EntitySpecifics.
   int specifics_field_number;
-  // Histogram value should be unique for the Model Type, Existing histogram
-  // values should never be modified without updating "SyncModelTypes" enum in
-  // histograms.xml to maintain backward compatibility.
+  // Model type value from SyncModelTypes enum in enums.xml. Must always be in
+  // sync with the enum.
   int model_type_histogram_val;
 };
 
@@ -140,7 +139,9 @@ const ModelTypeInfo kModelTypeInfoMap[] = {
      sync_pb::EntitySpecifics::kReadingListFieldNumber, 38},
     {USER_EVENTS, "USER_EVENT", "user_events", "User Events",
      sync_pb::EntitySpecifics::kUserEventFieldNumber, 39},
+    // ---- Proxy types ----
     {PROXY_TABS, "", "", "Tabs", -1, 25},
+    // ---- Control Types ----
     {NIGORI, "NIGORI", "nigori", "Encryption Keys",
      sync_pb::EntitySpecifics::kNigoriFieldNumber, 17},
     {EXPERIMENTS, "EXPERIMENTS", "experiments", "Experiments",
@@ -476,8 +477,8 @@ const char* ModelTypeToString(ModelType model_type) {
 // the list, and be careful to not reuse integer values that have already been
 // assigned.
 //
-// Don't forget to update the "SyncModelTypes" enum in histograms.xml when you
-// make changes to this list.
+// Don't forget to update the "SyncModelTypes" enum in enums.xml when you make
+// changes to this list.
 int ModelTypeToHistogramInt(ModelType model_type) {
   if (model_type >= UNSPECIFIED && model_type < MODEL_TYPE_COUNT)
     return kModelTypeInfoMap[model_type].model_type_histogram_val;