Use contiguous enum values for favicon_base::IconType

This simplifies the integration with UMA and improves Mojo verification,
which cannot otherwise detect invalid enum values. It should also help
prevent Undefined Behavior cases in the future, with C++17.

We update ThumbnailDatabase for backward compatibility because the
values get persisted. There should be no other persistence of these
enums since we have recently adopted enum classes to surface such cases.

Bug: 778551
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I69fe20d64f30701a00cd798338ab74147b356b7f
Reviewed-on: https://chromium-review.googlesource.com/756737
Reviewed-by: Brett Wilson <brettw@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516844}

components/favicon_base/favicon_types.cc

@@ -50,25 +50,4 @@ LargeIconImageResult::LargeIconImageResult(
 
 LargeIconImageResult::~LargeIconImageResult() {}
 
-int GetUmaFaviconType(IconType icon_type) {
-  // These values must stay in sync with the FaviconType enum in
-  // histograms/enums.xml.
-  switch (icon_type) {
-    case IconType::kInvalid:
-      break;
-    case IconType::kFavicon:
-      return 1;
-    case IconType::kTouchIcon:
-      return 2;
-    case IconType::kTouchPrecomposedIcon:
-      return 3;
-    case IconType::kWebManifestIcon:
-      return 4;
-  }
-  // Unexpected values including multiple bits (accidentally) being set in the
-  // input contribute to the zero bucket in release mode.
-  NOTREACHED();
-  return 0;
-}
-
 }  // namespace favicon_base

components/favicon_base/favicon_types.h

@@ -21,8 +21,10 @@ struct FallbackIconStyle;
 
 typedef int64_t FaviconID;
 
-// Defines the icon types. They are also stored in icon_type field of favicons
-// table.
+// Defines the icon types.
+//
+// IMPORTANT: these values must stay in sync with the FaviconType enum in
+// tools/metrics/histograms/enum.xml.
 //
 // The values of the IconTypes are used to select the priority in which favicon
 // data is returned in HistoryBackend and ThumbnailDatabase.
@@ -31,11 +33,12 @@ typedef int64_t FaviconID;
 // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.favicon
 enum class IconType {
   kInvalid = 0,
-  kFavicon = 1 << 0,
-  kTouchIcon = 1 << 1,
-  kTouchPrecomposedIcon = 1 << 2,
-  kWebManifestIcon = 1 << 3,
-  kMax = kWebManifestIcon
+  kFavicon,
+  kTouchIcon,
+  kTouchPrecomposedIcon,
+  kWebManifestIcon,
+  kMax = kWebManifestIcon,
+  kCount
 };
 
 using IconTypeSet = base::flat_set<IconType>;
@@ -165,10 +168,6 @@ enum class GoogleFaviconServerRequestStatus {
   COUNT
 };
 
-// Returns a value corresponding to the UMA enum values for FaviconType in
-// histograms/enums.xml.
-int GetUmaFaviconType(IconType icon_type);
-
 }  // namespace favicon_base
 
 #endif  // COMPONENTS_FAVICON_BASE_FAVICON_TYPES_H_

components/history/core/browser/thumbnail_database.cc

@@ -11,6 +11,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/bits.h"
 #include "base/debug/alias.h"
 #include "base/files/file_util.h"
 #include "base/memory/ref_counted_memory.h"
@@ -978,12 +979,22 @@ bool ThumbnailDatabase::RetainDataForPageUrls(
 
 // static
 int ThumbnailDatabase::ToPersistedIconType(favicon_base::IconType icon_type) {
-  return static_cast<int>(icon_type);
+  if (icon_type == favicon_base::IconType::kInvalid)
+    return 0;
+
+  return 1 << (static_cast<int>(icon_type) - 1);
 }
 
 // static
 favicon_base::IconType ThumbnailDatabase::FromPersistedIconType(int icon_type) {
-  return static_cast<favicon_base::IconType>(icon_type);
+  if (icon_type == 0)
+    return favicon_base::IconType::kInvalid;
+
+  int val = 1 + base::bits::Log2Floor(icon_type);
+  if (val > static_cast<int>(favicon_base::IconType::kMax))
+    return favicon_base::IconType::kInvalid;
+
+  return static_cast<favicon_base::IconType>(val);
 }
 
 sql::InitStatus ThumbnailDatabase::OpenDatabase(sql::Connection* db,

components/history/core/browser/thumbnail_database_unittest.cc

@@ -1282,6 +1282,8 @@ TEST(ThumbnailDatabaseIconTypeTest, ShouldBeBackwardCompatible) {
             ThumbnailDatabase::FromPersistedIconType(4));
   EXPECT_EQ(favicon_base::IconType::kWebManifestIcon,
             ThumbnailDatabase::FromPersistedIconType(8));
+  EXPECT_EQ(favicon_base::IconType::kInvalid,
+            ThumbnailDatabase::FromPersistedIconType(16));
 }
 
 }  // namespace history

components/ntp_tiles/metrics.cc

@@ -143,18 +143,16 @@ void RecordTileImpression(const NTPTileImpression& impression,
         impression.index, kMaxNumTiles);
 
     if (impression.icon_type != favicon_base::IconType::kInvalid) {
-      base::UmaHistogramExactLinear(
+      base::UmaHistogramEnumeration(
           base::StringPrintf("NewTabPage.TileFaviconType.%s", tile_type_suffix),
-          favicon_base::GetUmaFaviconType(impression.icon_type),
-          favicon_base::GetUmaFaviconType(favicon_base::IconType::kMax) + 1);
+          impression.icon_type, favicon_base::IconType::kCount);
     }
   }
 
   if (impression.icon_type != favicon_base::IconType::kInvalid) {
-    base::UmaHistogramExactLinear(
-        "NewTabPage.TileFaviconType",
-        favicon_base::GetUmaFaviconType(impression.icon_type),
-        favicon_base::GetUmaFaviconType(favicon_base::IconType::kMax) + 1);
+    base::UmaHistogramEnumeration("NewTabPage.TileFaviconType",
+                                  impression.icon_type,
+                                  favicon_base::IconType::kCount);
   }
 }
 
@@ -183,19 +181,17 @@ void RecordTileClick(const NTPTileImpression& impression) {
         impression.index, kMaxNumTiles);
 
     if (impression.icon_type != favicon_base::IconType::kInvalid) {
-      base::UmaHistogramExactLinear(
+      base::UmaHistogramEnumeration(
           base::StringPrintf("NewTabPage.TileFaviconTypeClicked.%s",
                              tile_type_suffix),
-          favicon_base::GetUmaFaviconType(impression.icon_type),
-          favicon_base::GetUmaFaviconType(favicon_base::IconType::kMax) + 1);
+          impression.icon_type, favicon_base::IconType::kCount);
     }
   }
 
   if (impression.icon_type != favicon_base::IconType::kInvalid) {
-    base::UmaHistogramExactLinear(
-        "NewTabPage.TileFaviconTypeClicked",
-        favicon_base::GetUmaFaviconType(impression.icon_type),
-        favicon_base::GetUmaFaviconType(favicon_base::IconType::kMax) + 1);
+    base::UmaHistogramEnumeration("NewTabPage.TileFaviconTypeClicked",
+                                  impression.icon_type,
+                                  favicon_base::IconType::kCount);
   }
 
   UMA_HISTOGRAM_ENUMERATION("NewTabPage.TileTitleClicked",

ios/web/public/favicon_url.h

@@ -18,11 +18,10 @@ struct FaviconURL {
   // favicon_base::IconType.
   enum class IconType {
     kInvalid = 0,
-    kFavicon = 1 << 0,
-    kTouchIcon = 1 << 1,
-    kTouchPrecomposedIcon = 1 << 2,
-    kWebManifestIcon = 1 << 3,
-    kMax = kWebManifestIcon
+    kFavicon,
+    kTouchIcon,
+    kTouchPrecomposedIcon,
+    kWebManifestIcon,
   };
 
   FaviconURL();