[SafeBrowsing] Send pingbacks for additional file types.

chrome/browser/safe_browsing/download_protection_service.cc

Index: chrome/browser/safe_browsing/download_protection_service.cc
diff --git a/chrome/browser/safe_browsing/download_protection_service.cc b/chrome/browser/safe_browsing/download_protection_service.cc
index 4773c8cbaa9d638cb71520af210ab788c1c6dc67..94963956f49373b0dec591f2f6eda6155adf7abf 100644
--- a/chrome/browser/safe_browsing/download_protection_service.cc
+++ b/chrome/browser/safe_browsing/download_protection_service.cc
@@ -62,72 +62,61 @@ const char DownloadProtectionService::kDownloadRequestUrl[] =
     "https://sb-ssl.google.com/safebrowsing/clientreport/download";
 
 namespace {
-// List of extensions for which we track some UMA stats.
-enum MaliciousExtensionType {
-  EXTENSION_EXE,
-  EXTENSION_MSI,
-  EXTENSION_CAB,
-  EXTENSION_SYS,
-  EXTENSION_SCR,
-  EXTENSION_DRV,
-  EXTENSION_BAT,
-  EXTENSION_ZIP,
-  EXTENSION_RAR,
-  EXTENSION_DLL,
-  EXTENSION_PIF,
-  EXTENSION_COM,
-  EXTENSION_JAR,
-  EXTENSION_CLASS,
-  EXTENSION_PDF,
-  EXTENSION_VB,
-  EXTENSION_REG,
-  EXTENSION_GRP,
-  EXTENSION_OTHER,  // Groups all other extensions into one bucket.
-  EXTENSION_CRX,
-  EXTENSION_APK,
-  EXTENSION_DMG,
-  EXTENSION_PKG,
-  EXTENSION_TORRENT,
-  EXTENSION_WEBSITE,
-  EXTENSION_URL,
-  EXTENSION_MAX,
+// List of extensions for which we track some UMA stats. The position of the
+// extension in kDangerousFileTypes is considered to be the UMA enumeration
+// value. Naturally, new values should only be added at the end.
+const base::FilePath::CharType* const kDangerousFileTypes[] = {
+    FILE_PATH_LITERAL(".exe"),
+    FILE_PATH_LITERAL(".msi"),
+    FILE_PATH_LITERAL(".cab"),
+    FILE_PATH_LITERAL(".sys"),
+    FILE_PATH_LITERAL(".scr"),
+    FILE_PATH_LITERAL(".drv"),
+    FILE_PATH_LITERAL(".bat"),
+    FILE_PATH_LITERAL(".zip"),
+    FILE_PATH_LITERAL(".rar"),
+    FILE_PATH_LITERAL(".dll"),
+    FILE_PATH_LITERAL(".pif"),
+    FILE_PATH_LITERAL(".com"),
+    FILE_PATH_LITERAL(".jar"),
+    FILE_PATH_LITERAL(".class"),
+    FILE_PATH_LITERAL(".pdf"),
+    FILE_PATH_LITERAL(".vb"),
+    FILE_PATH_LITERAL(".reg"),
+    FILE_PATH_LITERAL(".grp"),
+    nullptr,  // The "Other" bucket. This is in the middle of the array due to
+              // historical reasons.
+    FILE_PATH_LITERAL(".crx"),
+    FILE_PATH_LITERAL(".apk"),
+    FILE_PATH_LITERAL(".dmg"),
+    FILE_PATH_LITERAL(".pkg"),
+    FILE_PATH_LITERAL(".torrent"),
+    FILE_PATH_LITERAL(".website"),
+    FILE_PATH_LITERAL(".url"),
+    FILE_PATH_LITERAL(".vbe"),
+    FILE_PATH_LITERAL(".vbs"),
+    FILE_PATH_LITERAL(".js"),
+    FILE_PATH_LITERAL(".jse"),
+    FILE_PATH_LITERAL(".mht"),
+    FILE_PATH_LITERAL(".mhtml"),

[moheeb1, 2015/07/08 20:25:29]

Would be great to expand this list to cover all dangerous file we know about, so
we get a sense of prevalance of these file types.

 };
 
-MaliciousExtensionType GetExtensionType(const base::FilePath& f) {
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".exe"))) return EXTENSION_EXE;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".msi"))) return EXTENSION_MSI;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".cab"))) return EXTENSION_CAB;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".sys"))) return EXTENSION_SYS;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".scr"))) return EXTENSION_SCR;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".drv"))) return EXTENSION_DRV;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".bat"))) return EXTENSION_BAT;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".zip"))) return EXTENSION_ZIP;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".rar"))) return EXTENSION_RAR;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".dll"))) return EXTENSION_DLL;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".pif"))) return EXTENSION_PIF;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".com"))) return EXTENSION_COM;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".jar"))) return EXTENSION_JAR;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".class"))) return EXTENSION_CLASS;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".pdf"))) return EXTENSION_PDF;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".vb"))) return EXTENSION_VB;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".reg"))) return EXTENSION_REG;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".grp"))) return EXTENSION_GRP;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".crx"))) return EXTENSION_CRX;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".apk"))) return EXTENSION_APK;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".dmg"))) return EXTENSION_DMG;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".pkg"))) return EXTENSION_PKG;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".torrent")))
-    return EXTENSION_TORRENT;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".website")))
-    return EXTENSION_WEBSITE;
-  if (f.MatchesExtension(FILE_PATH_LITERAL(".url"))) return EXTENSION_URL;
-  return EXTENSION_OTHER;
-}
+const int EXTENSION_OTHER = 18;

[Nathan Parker, 2015/07/08 20:11:08]

Nit: Add comment or make the name indicate that this is an index into the array.

 
 void RecordFileExtensionType(const base::FilePath& file) {
+  DCHECK_EQ(static_cast<base::FilePath::CharType*>(nullptr),
+            kDangerousFileTypes[EXTENSION_OTHER]);
+
+  int extension_type = EXTENSION_OTHER;
+  for (const auto& extension : kDangerousFileTypes) {
+    if (extension && file.MatchesExtension(extension)) {
+      extension_type = &extension - kDangerousFileTypes;
+      break;
+    }
+  }
+
   UMA_HISTOGRAM_ENUMERATION("SBClientDownload.DownloadExtensions",
-                            GetExtensionType(file),
-                            EXTENSION_MAX);
+                            extension_type, arraysize(kDangerousFileTypes));
 }
 
 // Enumerate for histogramming purposes.
@@ -627,6 +616,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
     }
 
     const GURL& url = url_chain_.back();
+    // TODO(asanka): This may acquire a lock on the SB DB on the IO thread.
     if (url.is_valid() && database_manager_->MatchDownloadWhitelistUrl(url)) {
       DVLOG(2) << url << " is on the download whitelist.";
       RecordCountOfSignedOrWhitelistedDownload();