Collect UMA data for Origin Trials

content/common/origin_trials/trial_token_validator.cc

Index: content/common/origin_trials/trial_token_validator.cc
diff --git a/content/common/origin_trials/trial_token_validator.cc b/content/common/origin_trials/trial_token_validator.cc
index 6eef75a5e9a40be7f7cf5b3a3d594dc8e789f1b7..f9261fe19a30804e5711e6f5dc971ec3870666cb 100644
--- a/content/common/origin_trials/trial_token_validator.cc
+++ b/content/common/origin_trials/trial_token_validator.cc
@@ -7,23 +7,30 @@
 #include "base/time/time.h"
 #include "content/common/origin_trials/trial_token.h"
 #include "content/public/common/content_client.h"
+#include "third_party/WebKit/public/platform/WebOriginTrialTokenStatus.h"
 
 namespace content {
 
-bool TrialTokenValidator::ValidateToken(const std::string& token,
-                                        const url::Origin& origin,
-                                        base::StringPiece feature_name) {
+blink::WebOriginTrialTokenStatus TrialTokenValidator::ValidateToken(
+    const std::string& token,
+    const url::Origin& origin,
+    base::StringPiece feature_name) {
   // TODO(iclelland): Allow for multiple signing keys, and iterate over all
   // active keys here. https://crbug.com/543220
   ContentClient* content_client = GetContentClient();
   base::StringPiece public_key = content_client->GetOriginTrialPublicKey();
   if (public_key.empty()) {
-    return false;
+    return blink::WebOriginTrialTokenStatus::NotSupported;
   }
-  std::unique_ptr<TrialToken> trial_token = TrialToken::From(token, public_key);
 
-  return trial_token &&
-         trial_token->IsValidForFeature(origin, feature_name,
+  blink::WebOriginTrialTokenStatus status;
+  std::unique_ptr<TrialToken> trial_token =
+      TrialToken::From(token, public_key, &status);
+  if (status != blink::WebOriginTrialTokenStatus::Success) {
+    return status;
+  }
+
+  return trial_token->IsValidForFeature(origin, feature_name,

[iclelland, 2016/04/28 17:00:58]

I'd be happier with a DCHECK(trial_token) here before we dereference it. I can
see by inspection that you can't get Success with a nullptr, but an assertion
would be good to ensure it.

[chasej, 2016/05/02 15:53:11]

Does a DCHECK buy you much here? The pointer is provided by an internal method,
not from an unknown caller. A unit test would give you equivalent/better
assurances, and the documentation benefit seems minimal.

[iclelland, 2016/05/02 16:17:55]

Fair enough; a crash is a crash :)

Documentation-wise, I guess issue could only come up if a future change (bug) is
somehow made to ::From that causes the return to be null with a Success status.
In that case, any documentation should really be on ::From, that it must return
a valid object on success.

                                         base::Time::Now());
 }