DownloadProtection: Add more graceful handling of verdict=UNKNOWN

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 2381394d23b89fff2dd471b930f8ddf92471a992..2f2414bdd3a17d82b23dde44241f6d5392e48bcc 100644
--- a/chrome/browser/safe_browsing/download_protection_service.cc
+++ b/chrome/browser/safe_browsing/download_protection_service.cc
@@ -471,37 +471,44 @@ class DownloadProtectionService::CheckClientDownloadRequest
         // Ignore the verdict because we were just reporting a sampled file.
         reason = REASON_SAMPLED_UNSUPPORTED_FILE;
         result = UNKNOWN;
-      } else if (response.verdict() == ClientDownloadResponse::SAFE) {
-        reason = REASON_DOWNLOAD_SAFE;
-        result = SAFE;
-      } else if (service_ && !service_->IsSupportedDownload(
-          *item_, item_->GetTargetFilePath())) {
-        // TODO(nparker): Remove this check since it should be impossible.
-        reason = REASON_DOWNLOAD_NOT_SUPPORTED;
-        result = UNKNOWN;
-      } else if (response.verdict() == ClientDownloadResponse::DANGEROUS) {
-        reason = REASON_DOWNLOAD_DANGEROUS;
-        result = DANGEROUS;
-        token = response.token();
-      } else if (response.verdict() == ClientDownloadResponse::UNCOMMON) {
-        reason = REASON_DOWNLOAD_UNCOMMON;
-        result = UNCOMMON;
-        token = response.token();
-      } else if (response.verdict() == ClientDownloadResponse::DANGEROUS_HOST) {
-        reason = REASON_DOWNLOAD_DANGEROUS_HOST;
-        result = DANGEROUS_HOST;
-        token = response.token();
-      } else if (
-          response.verdict() == ClientDownloadResponse::POTENTIALLY_UNWANTED) {
-        reason = REASON_DOWNLOAD_POTENTIALLY_UNWANTED;
-        result = POTENTIALLY_UNWANTED;
-        token = response.token();
       } else {
-        LOG(DFATAL) << "Unknown download response verdict: "
-                    << response.verdict();
-        reason = REASON_INVALID_RESPONSE_VERDICT;
-        result = UNKNOWN;
+        switch (response.verdict()) {
+          case ClientDownloadResponse::SAFE:
+            reason = REASON_DOWNLOAD_SAFE;
+            result = SAFE;
+            break;
+          case ClientDownloadResponse::DANGEROUS:
+            reason = REASON_DOWNLOAD_DANGEROUS;
+            result = DANGEROUS;
+            token = response.token();
+            break;
+          case ClientDownloadResponse::UNCOMMON:
+            reason = REASON_DOWNLOAD_UNCOMMON;
+            result = UNCOMMON;
+            token = response.token();
+            break;
+          case ClientDownloadResponse::DANGEROUS_HOST:
+            reason = REASON_DOWNLOAD_DANGEROUS_HOST;
+            result = DANGEROUS_HOST;
+            token = response.token();
+            break;
+          case ClientDownloadResponse::POTENTIALLY_UNWANTED:
+            reason = REASON_DOWNLOAD_POTENTIALLY_UNWANTED;
+            result = POTENTIALLY_UNWANTED;
+            token = response.token();
+            break;
+          case ClientDownloadResponse::UNKNOWN:
+            reason = REASON_VERDICT_UNKNOWN;
+            result = UNKNOWN;
+            break;
+          default:

[asanka, 2016/07/07 20:46:59]

Nit: Shall we go the route of eliminating the default label? That way the
compiler will throw an error if a new verdict is added that's not handled here.
If the incoming proto has an unrecognized verdict, it should fail to parse,
right?

[Nathan Parker, 2016/07/07 21:02:55]

I don't think it'd fail to parse... hence I'm catching it here. Though I can't
find docs to that effect.

I think the no-default style works well only when the value is set solely within
the same C++ binary, so the compiler can enforce the type.

I was relying on it _not_ failing to parse, so existing chrome binaries will be
able to handle the new "UNKNOWN" value with the old code that doesn't know about
it.

[asanka, 2016/07/07 21:15:11]

Ah. TIL. Acknowledged.

[Nathan Parker, 2016/07/11 20:26:22]

Apparently I was wrong.  The old code apparently can't parse the proto with the
new enum value, so I'm seeing a histogram value of INVALID_RESPONSE_PROTO.  The
result (unknown) is the same.

+            LOG(DFATAL) << "Unknown download response verdict: "
+                        << response.verdict();
+            reason = REASON_INVALID_RESPONSE_VERDICT;
+            result = UNKNOWN;
+        }
       }
+
       if (!token.empty())
         SetDownloadPingToken(item_, token);
 
@@ -1394,6 +1401,8 @@ class DownloadProtectionService::PPAPIDownloadRequest
         return DANGEROUS;
       case ClientDownloadResponse::DANGEROUS_HOST:
         return DANGEROUS_HOST;
+      case ClientDownloadResponse::UNKNOWN:
+        return UNKNOWN;
     }
     return UNKNOWN;
   }