Introduce a new enum ModelTypeForHistograms

This is a variant of ModelType that fulfills the requirements for
recording in histograms (i.e. no reordering/renumbering). It's a
type-safe replacement for the current system based on ints.
The actual values stay identical to the current ints, so the histograms
themselves won't be affected.

A follow-up CL will update all histogram recording sites to use the new
enum.

Bug: 1007293
Change-Id: I3e36c97303ba08e34baccffafbab8b55d8b9288b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1821787Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699663}

components/sync/base/model_type.cc

@@ -47,7 +47,7 @@ struct ModelTypeInfo {
   const int specifics_field_number;
   // Model type value from SyncModelTypes enum in enums.xml. Must always be in
   // sync with the enum.
-  const int model_type_histogram_val;
+  const ModelTypeForHistograms model_type_histogram_val;
 };
 
 // Below struct entries are in the same order as their definition in the
@@ -57,113 +57,151 @@ struct ModelTypeInfo {
 //  - update the SyncModelType histogram suffix in histograms.xml.
 // Struct field values should be unique across the entire map.
 const ModelTypeInfo kModelTypeInfoMap[] = {
-    {UNSPECIFIED, "", "", "Unspecified", -1, 0},
-    {TOP_LEVEL_FOLDER, "", "", "Top Level Folder", -1, 1},
+    {UNSPECIFIED, "", "", "Unspecified", -1,
+     ModelTypeForHistograms::kUnspecified},
+    {TOP_LEVEL_FOLDER, "", "", "Top Level Folder", -1,
+     ModelTypeForHistograms::kTopLevelFolder},
     {BOOKMARKS, "BOOKMARK", "bookmarks", "Bookmarks",
-     sync_pb::EntitySpecifics::kBookmarkFieldNumber, 2},
+     sync_pb::EntitySpecifics::kBookmarkFieldNumber,
+     ModelTypeForHistograms::kBookmarks},
     {PREFERENCES, "PREFERENCE", "preferences", "Preferences",
-     sync_pb::EntitySpecifics::kPreferenceFieldNumber, 3},
+     sync_pb::EntitySpecifics::kPreferenceFieldNumber,
+     ModelTypeForHistograms::kPreferences},
     {PASSWORDS, "PASSWORD", "passwords", "Passwords",
-     sync_pb::EntitySpecifics::kPasswordFieldNumber, 4},
+     sync_pb::EntitySpecifics::kPasswordFieldNumber,
+     ModelTypeForHistograms::kPasswords},
     {AUTOFILL_PROFILE, "AUTOFILL_PROFILE", "autofill_profiles",
      "Autofill Profiles", sync_pb::EntitySpecifics::kAutofillProfileFieldNumber,
-     5},
+     ModelTypeForHistograms::kAutofillProfile},
     {AUTOFILL, "AUTOFILL", "autofill", "Autofill",
-     sync_pb::EntitySpecifics::kAutofillFieldNumber, 6},
+     sync_pb::EntitySpecifics::kAutofillFieldNumber,
+     ModelTypeForHistograms::kAutofill},
     {AUTOFILL_WALLET_DATA, "AUTOFILL_WALLET", "autofill_wallet",
      "Autofill Wallet", sync_pb::EntitySpecifics::kAutofillWalletFieldNumber,
-     34},
+     ModelTypeForHistograms::kAutofillWalletData},
     {AUTOFILL_WALLET_METADATA, "WALLET_METADATA", "autofill_wallet_metadata",
      "Autofill Wallet Metadata",
-     sync_pb::EntitySpecifics::kWalletMetadataFieldNumber, 35},
+     sync_pb::EntitySpecifics::kWalletMetadataFieldNumber,
+     ModelTypeForHistograms::kAutofillWalletMetadata},
     {THEMES, "THEME", "themes", "Themes",
-     sync_pb::EntitySpecifics::kThemeFieldNumber, 7},
+     sync_pb::EntitySpecifics::kThemeFieldNumber,
+     ModelTypeForHistograms::kThemes},
     {TYPED_URLS, "TYPED_URL", "typed_urls", "Typed URLs",
-     sync_pb::EntitySpecifics::kTypedUrlFieldNumber, 8},
+     sync_pb::EntitySpecifics::kTypedUrlFieldNumber,
+     ModelTypeForHistograms::kTypedUrls},
     {EXTENSIONS, "EXTENSION", "extensions", "Extensions",
-     sync_pb::EntitySpecifics::kExtensionFieldNumber, 9},
+     sync_pb::EntitySpecifics::kExtensionFieldNumber,
+     ModelTypeForHistograms::kExtensions},
     {SEARCH_ENGINES, "SEARCH_ENGINE", "search_engines", "Search Engines",
-     sync_pb::EntitySpecifics::kSearchEngineFieldNumber, 10},
+     sync_pb::EntitySpecifics::kSearchEngineFieldNumber,
+     ModelTypeForHistograms::kSearchEngines},
     {SESSIONS, "SESSION", "sessions", "Sessions",
-     sync_pb::EntitySpecifics::kSessionFieldNumber, 11},
+     sync_pb::EntitySpecifics::kSessionFieldNumber,
+     ModelTypeForHistograms::kSessions},
     {APPS, "APP", "apps", "Apps", sync_pb::EntitySpecifics::kAppFieldNumber,
-     12},
+     ModelTypeForHistograms::kApps},
     {APP_SETTINGS, "APP_SETTING", "app_settings", "App settings",
-     sync_pb::EntitySpecifics::kAppSettingFieldNumber, 13},
+     sync_pb::EntitySpecifics::kAppSettingFieldNumber,
+     ModelTypeForHistograms::kAppSettings},
     {EXTENSION_SETTINGS, "EXTENSION_SETTING", "extension_settings",
      "Extension settings",
-     sync_pb::EntitySpecifics::kExtensionSettingFieldNumber, 14},
+     sync_pb::EntitySpecifics::kExtensionSettingFieldNumber,
+     ModelTypeForHistograms::kExtensionSettings},
     {DEPRECATED_APP_NOTIFICATIONS, "APP_NOTIFICATION", "app_notifications",
      "App Notifications", sync_pb::EntitySpecifics::kAppNotificationFieldNumber,
-     15},
+     ModelTypeForHistograms::kDeprecatedAppNotifications},
     {HISTORY_DELETE_DIRECTIVES, "HISTORY_DELETE_DIRECTIVE",
      "history_delete_directives", "History Delete Directives",
-     sync_pb::EntitySpecifics::kHistoryDeleteDirectiveFieldNumber, 16},
+     sync_pb::EntitySpecifics::kHistoryDeleteDirectiveFieldNumber,
+     ModelTypeForHistograms::kHistoryDeleteDirectices},
     {DEPRECATED_SYNCED_NOTIFICATIONS, "SYNCED_NOTIFICATION",
      "synced_notifications", "Synced Notifications",
-     sync_pb::EntitySpecifics::kSyncedNotificationFieldNumber, 20},
+     sync_pb::EntitySpecifics::kSyncedNotificationFieldNumber,
+     ModelTypeForHistograms::kDeprecatedSyncedNotifications},
     {DEPRECATED_SYNCED_NOTIFICATION_APP_INFO, "SYNCED_NOTIFICATION_APP_INFO",
      "synced_notification_app_info", "Synced Notification App Info",
-     sync_pb::EntitySpecifics::kSyncedNotificationAppInfoFieldNumber, 31},
+     sync_pb::EntitySpecifics::kSyncedNotificationAppInfoFieldNumber,
+     ModelTypeForHistograms::kDeprecatedSyncedNotificationAppInfo},
     {DICTIONARY, "DICTIONARY", "dictionary", "Dictionary",
-     sync_pb::EntitySpecifics::kDictionaryFieldNumber, 22},
+     sync_pb::EntitySpecifics::kDictionaryFieldNumber,
+     ModelTypeForHistograms::kDictionary},
     {FAVICON_IMAGES, "FAVICON_IMAGE", "favicon_images", "Favicon Images",
-     sync_pb::EntitySpecifics::kFaviconImageFieldNumber, 23},
+     sync_pb::EntitySpecifics::kFaviconImageFieldNumber,
+     ModelTypeForHistograms::kFaviconImages},
     {FAVICON_TRACKING, "FAVICON_TRACKING", "favicon_tracking",
      "Favicon Tracking", sync_pb::EntitySpecifics::kFaviconTrackingFieldNumber,
-     24},
+     ModelTypeForHistograms::kFaviconTracking},
     {DEVICE_INFO, "DEVICE_INFO", "device_info", "Device Info",
-     sync_pb::EntitySpecifics::kDeviceInfoFieldNumber, 18},
+     sync_pb::EntitySpecifics::kDeviceInfoFieldNumber,
+     ModelTypeForHistograms::kDeviceInfo},
     {PRIORITY_PREFERENCES, "PRIORITY_PREFERENCE", "priority_preferences",
      "Priority Preferences",
-     sync_pb::EntitySpecifics::kPriorityPreferenceFieldNumber, 21},
+     sync_pb::EntitySpecifics::kPriorityPreferenceFieldNumber,
+     ModelTypeForHistograms::kPriorityPreferences},
     {SUPERVISED_USER_SETTINGS, "MANAGED_USER_SETTING", "managed_user_settings",
      "Managed User Settings",
-     sync_pb::EntitySpecifics::kManagedUserSettingFieldNumber, 26},
+     sync_pb::EntitySpecifics::kManagedUserSettingFieldNumber,
+     ModelTypeForHistograms::kSupervisedUserSettings},
     {DEPRECATED_SUPERVISED_USERS, "MANAGED_USER", "managed_users",
-     "Managed Users", sync_pb::EntitySpecifics::kManagedUserFieldNumber, 27},
+     "Managed Users", sync_pb::EntitySpecifics::kManagedUserFieldNumber,
+     ModelTypeForHistograms::kDeprecatedSupervisedUsers},
     {DEPRECATED_SUPERVISED_USER_SHARED_SETTINGS, "MANAGED_USER_SHARED_SETTING",
      "managed_user_shared_settings", "Managed User Shared Settings",
-     sync_pb::EntitySpecifics::kManagedUserSharedSettingFieldNumber, 30},
+     sync_pb::EntitySpecifics::kManagedUserSharedSettingFieldNumber,
+     ModelTypeForHistograms::kDeprecatedSupervisedUserSharedSettings},
     {DEPRECATED_ARTICLES, "ARTICLE", "articles", "Articles",
-     sync_pb::EntitySpecifics::kArticleFieldNumber, 28},
+     sync_pb::EntitySpecifics::kArticleFieldNumber,
+     ModelTypeForHistograms::kDeprecatedArticles},
     {APP_LIST, "APP_LIST", "app_list", "App List",
-     sync_pb::EntitySpecifics::kAppListFieldNumber, 29},
+     sync_pb::EntitySpecifics::kAppListFieldNumber,
+     ModelTypeForHistograms::kAppList},
     {DEPRECATED_WIFI_CREDENTIALS, "WIFI_CREDENTIAL", "wifi_credentials",
      "WiFi Credentials", sync_pb::EntitySpecifics::kWifiCredentialFieldNumber,
-     32},
+     ModelTypeForHistograms::kDeprecatedWifiCredentials},
     {SUPERVISED_USER_WHITELISTS, "MANAGED_USER_WHITELIST",
      "managed_user_whitelists", "Managed User Whitelists",
-     sync_pb::EntitySpecifics::kManagedUserWhitelistFieldNumber, 33},
+     sync_pb::EntitySpecifics::kManagedUserWhitelistFieldNumber,
+     ModelTypeForHistograms::kSupervisedUserWhitelists},
     {ARC_PACKAGE, "ARC_PACKAGE", "arc_package", "Arc Package",
-     sync_pb::EntitySpecifics::kArcPackageFieldNumber, 36},
+     sync_pb::EntitySpecifics::kArcPackageFieldNumber,
+     ModelTypeForHistograms::kArcPackage},
     {PRINTERS, "PRINTER", "printers", "Printers",
-     sync_pb::EntitySpecifics::kPrinterFieldNumber, 37},
+     sync_pb::EntitySpecifics::kPrinterFieldNumber,
+     ModelTypeForHistograms::kPrinters},
     {READING_LIST, "READING_LIST", "reading_list", "Reading List",
-     sync_pb::EntitySpecifics::kReadingListFieldNumber, 38},
+     sync_pb::EntitySpecifics::kReadingListFieldNumber,
+     ModelTypeForHistograms::kReadingList},
     {USER_EVENTS, "USER_EVENT", "user_events", "User Events",
-     sync_pb::EntitySpecifics::kUserEventFieldNumber, 39},
+     sync_pb::EntitySpecifics::kUserEventFieldNumber,
+     ModelTypeForHistograms::kUserEvents},
     {MOUNTAIN_SHARES, "MOUNTAIN_SHARE", "mountain_shares", "Mountain Shares",
-     sync_pb::EntitySpecifics::kMountainShareFieldNumber, 40},
+     sync_pb::EntitySpecifics::kMountainShareFieldNumber,
+     ModelTypeForHistograms::kMountainShares},
     {USER_CONSENTS, "USER_CONSENT", "user_consent", "User Consents",
-     sync_pb::EntitySpecifics::kUserConsentFieldNumber, 41},
+     sync_pb::EntitySpecifics::kUserConsentFieldNumber,
+     ModelTypeForHistograms::kUserConsents},
     {SEND_TAB_TO_SELF, "SEND_TAB_TO_SELF", "send_tab_to_self",
      "Send Tab To Self", sync_pb::EntitySpecifics::kSendTabToSelfFieldNumber,
-     42},
+     ModelTypeForHistograms::kSendTabToSelf},
     {SECURITY_EVENTS, "SECURITY_EVENT", "security_events", "Security Events",
-     sync_pb::EntitySpecifics::kSecurityEventFieldNumber, 43},
+     sync_pb::EntitySpecifics::kSecurityEventFieldNumber,
+     ModelTypeForHistograms::kSecurityEvents},
     {WIFI_CONFIGURATIONS, "WIFI_CONFIGURATION", "wifi_configurations",
      "Wifi Configurations",
-     sync_pb::EntitySpecifics::kWifiConfigurationFieldNumber, 44},
+     sync_pb::EntitySpecifics::kWifiConfigurationFieldNumber,
+     ModelTypeForHistograms::kWifiConfigurations},
     {WEB_APPS, "WEB_APP", "web_apps", "Web Apps",
-     sync_pb::EntitySpecifics::kWebAppFieldNumber, 45},
+     sync_pb::EntitySpecifics::kWebAppFieldNumber,
+     ModelTypeForHistograms::kWebApps},
     // ---- Proxy types ----
-    {PROXY_TABS, "", "", "Tabs", -1, 25},
+    {PROXY_TABS, "", "", "Tabs", -1, ModelTypeForHistograms::kProxyTabs},
     // ---- Control Types ----
     {NIGORI, "NIGORI", "nigori", "Encryption Keys",
-     sync_pb::EntitySpecifics::kNigoriFieldNumber, 17},
+     sync_pb::EntitySpecifics::kNigoriFieldNumber,
+     ModelTypeForHistograms::kNigori},
     {DEPRECATED_EXPERIMENTS, "EXPERIMENTS", "experiments", "Experiments",
-     sync_pb::EntitySpecifics::kExperimentsFieldNumber, 19},
+     sync_pb::EntitySpecifics::kExperimentsFieldNumber,
+     ModelTypeForHistograms::kDeprecatedExperiments},
 };
 
 static_assert(base::size(kModelTypeInfoMap) == ModelType::NUM_ENTRIES,
@@ -526,6 +564,13 @@ const char* ModelTypeToHistogramSuffix(ModelType model_type) {
 // Don't forget to update the "SyncModelTypes" enum in enums.xml when you make
 // changes to this list.
 int ModelTypeToHistogramInt(ModelType model_type) {
+  DCHECK_GE(model_type, UNSPECIFIED);
+  DCHECK_LT(model_type, ModelType::NUM_ENTRIES);
+  return static_cast<int>(
+      kModelTypeInfoMap[model_type].model_type_histogram_val);
+}
+
+ModelTypeForHistograms ModelTypeHistogramValue(ModelType model_type) {
   DCHECK_GE(model_type, UNSPECIFIED);
   DCHECK_LT(model_type, ModelType::NUM_ENTRIES);
   return kModelTypeInfoMap[model_type].model_type_histogram_val;

components/sync/base/model_type.h

@@ -176,6 +176,64 @@ inline ModelType ModelTypeFromInt(int i) {
   return static_cast<ModelType>(i);
 }
 
+// A version of the ModelType enum for use in histograms. ModelType does not
+// have stable values (e.g. new ones may be inserted in the middle), so it can't
+// be recorded directly.
+// Instead of using entries from this enum directly, you'll usually want to get
+// them via ModelTypeHistogramValue(model_type).
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused. When you add a new entry, also update
+// SyncModelTypes in enums.xml
+enum class ModelTypeForHistograms {
+  kUnspecified = 0,
+  kTopLevelFolder = 1,
+  kBookmarks = 2,
+  kPreferences = 3,
+  kPasswords = 4,
+  kAutofillProfile = 5,
+  kAutofill = 6,
+  kThemes = 7,
+  kTypedUrls = 8,
+  kExtensions = 9,
+  kSearchEngines = 10,
+  kSessions = 11,
+  kApps = 12,
+  kAppSettings = 13,
+  kExtensionSettings = 14,
+  kDeprecatedAppNotifications = 15,
+  kHistoryDeleteDirectices = 16,
+  kNigori = 17,
+  kDeviceInfo = 18,
+  kDeprecatedExperiments = 19,
+  kDeprecatedSyncedNotifications = 20,
+  kPriorityPreferences = 21,
+  kDictionary = 22,
+  kFaviconImages = 23,
+  kFaviconTracking = 24,
+  kProxyTabs = 25,
+  kSupervisedUserSettings = 26,
+  kDeprecatedSupervisedUsers = 27,
+  kDeprecatedArticles = 28,
+  kAppList = 29,
+  kDeprecatedSupervisedUserSharedSettings = 30,
+  kDeprecatedSyncedNotificationAppInfo = 31,
+  kDeprecatedWifiCredentials = 32,
+  kSupervisedUserWhitelists = 33,
+  kAutofillWalletData = 34,
+  kAutofillWalletMetadata = 35,
+  kArcPackage = 36,
+  kPrinters = 37,
+  kReadingList = 38,
+  kUserEvents = 39,
+  kMountainShares = 40,
+  kUserConsents = 41,
+  kSendTabToSelf = 42,
+  kSecurityEvents = 43,
+  kWifiConfigurations = 44,
+  kWebApps = 45,
+  kMaxValue = kWebApps
+};
+
 // Used to mark the type of EntitySpecifics that has no actual data.
 void AddDefaultFieldValue(ModelType type, sync_pb::EntitySpecifics* specifics);
 
@@ -308,7 +366,10 @@ const char* ModelTypeToHistogramSuffix(ModelType model_type);
 // The mapping from ModelType to integer is defined here. It defines a
 // completely different order than the ModelType enum itself. The mapping should
 // match the SyncModelTypes mapping from integer to labels defined in enums.xml.
+// TODO(crbug.com/1007293): Update all histogram recording sites to use
+// ModelTypeHistogramValue() and remove ModelTypeToHistogramInt();
 int ModelTypeToHistogramInt(ModelType model_type);
+ModelTypeForHistograms ModelTypeHistogramValue(ModelType model_type);
 
 // Returns for every model_type a positive unique integer that is stable over
 // time and thus can be used when persisting data.

components/sync/base/model_type_unittest.cc

@@ -73,20 +73,17 @@ TEST_F(ModelTypeTest, ModelTypeOfInvalidSpecificsFieldNumber) {
 }
 
 TEST_F(ModelTypeTest, ModelTypeHistogramMapping) {
-  std::set<int> histogram_values;
+  std::set<ModelTypeForHistograms> histogram_values;
   ModelTypeSet all_types = ModelTypeSet::All();
   for (ModelType type : all_types) {
     SCOPED_TRACE(ModelTypeToString(type));
-    int histogram_value = ModelTypeToHistogramInt(type);
+    ModelTypeForHistograms histogram_value = ModelTypeHistogramValue(type);
 
     EXPECT_TRUE(histogram_values.insert(histogram_value).second)
         << "Expected histogram values to be unique";
 
-    // This is not necessary for the mapping to be valid, but most instances of
-    // UMA_HISTOGRAM that use this mapping specify ModelType::NUM_ENTRIES as the
-    // maximum possible value.  If you break this assumption, you should update
-    // those histograms.
-    EXPECT_LT(histogram_value, ModelType::NUM_ENTRIES);
+    EXPECT_LE(static_cast<int>(histogram_value),
+              static_cast<int>(ModelTypeForHistograms::kMaxValue));
   }
 }