Collect UMA data for Origin Trials

content/common/origin_trials/trial_token.cc

Index: content/common/origin_trials/trial_token.cc
diff --git a/content/common/origin_trials/trial_token.cc b/content/common/origin_trials/trial_token.cc
index 0a137edc68b43e780e572c1d083379c864921fc8..aaf653adf630e0b254f44fd717b5ab8f6884dad4 100644
--- a/content/common/origin_trials/trial_token.cc
+++ b/content/common/origin_trials/trial_token.cc
@@ -16,6 +16,7 @@
 #include "base/strings/string_piece.h"
 #include "base/time/time.h"
 #include "base/values.h"
+#include "third_party/WebKit/public/platform/WebOriginTrialTokenStatus.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
@@ -44,48 +45,68 @@ const uint8_t kVersion2 = 2;
 TrialToken::~TrialToken() {}
 
 // static
-std::unique_ptr<TrialToken> TrialToken::From(const std::string& token_text,
-                                             base::StringPiece public_key) {
-  std::unique_ptr<std::string> token_payload = Extract(token_text, public_key);
-  if (!token_payload) {
+std::unique_ptr<TrialToken> TrialToken::From(
+    const std::string& token_text,
+    base::StringPiece public_key,
+    blink::WebOriginTrialTokenStatus* out_status) {
+  DCHECK(out_status);
+  std::string token_payload;
+  *out_status = Extract(token_text, public_key, &token_payload);
+  if (*out_status != blink::WebOriginTrialTokenStatus::Success) {
     return nullptr;
   }
-  return Parse(*token_payload);
+  std::unique_ptr<TrialToken> token = Parse(token_payload);
+  *out_status = token ? blink::WebOriginTrialTokenStatus::Success
+                      : blink::WebOriginTrialTokenStatus::Malformed;
+  return token;
 }
 
-bool TrialToken::IsValidForFeature(const url::Origin& origin,
-                                   base::StringPiece feature_name,
-                                   const base::Time& now) const {
-  return ValidateOrigin(origin) && ValidateFeatureName(feature_name) &&
-         ValidateDate(now);
+blink::WebOriginTrialTokenStatus TrialToken::IsValidForFeature(
+    const url::Origin& origin,
+    base::StringPiece feature_name,
+    const base::Time& now) const {
+  // The order of these checks is intentional. For example, will only report a
+  // token as expired if it is valid for the origin + feature combination.
+  if (!ValidateOrigin(origin)) {
+    return blink::WebOriginTrialTokenStatus::WrongOrigin;
+  }
+  if (!ValidateFeatureName(feature_name)) {
+    return blink::WebOriginTrialTokenStatus::WrongFeature;
+  }
+  if (!ValidateDate(now)) {
+    return blink::WebOriginTrialTokenStatus::Expired;
+  }
+  return blink::WebOriginTrialTokenStatus::Success;
 }
 
-std::unique_ptr<std::string> TrialToken::Extract(
+// static
+blink::WebOriginTrialTokenStatus TrialToken::Extract(
     const std::string& token_payload,
-    base::StringPiece public_key) {
+    base::StringPiece public_key,
+    std::string* out_token_json) {
   if (token_payload.empty()) {
-    return nullptr;
+    return blink::WebOriginTrialTokenStatus::Malformed;

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

would an "Empty" status be more descriptive in this case?

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

It would be more descriptive, but I don't see the value in recording the
distinction in UMA. If a web developer makes the effort to add the meta tag, I'd
think it's unlikely they would forget/omit the actual token (especially if the
email provides the meta tag for copy/paste). Empty is a subset of "found a meta
tag, but it didn't have a well-formed token". I think we would handle empty vs
malformed most the same (e.g. guidance to make sure the token is copied as-is
into the meta tag). Do you have any reasons why we need to distinguish "Empty"
from the more general "Malformed" case?

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

I suppose it's just an interesting specific variant -- it seems like it would
have different fundamental causes than a general 'present-but-malformed' token.

If we consider this as the status of the whole tag, or the whole header, then
'malformed' would make sense here, it just doesn't strike me as the right fit
for the status of a single tag.

Another thing to consider is what we are going to do with the UMA data -- can we
use it to learn what kind of mistakes developers are making in the field, and
try to adapt the API or the token-delivery mechanism to fit the developers? If
all we have is 'malformed: xx%', then we don't get a lot of actionable data. If
we know the breakdown of missing token vs truncated token vs completely-garbled
(not-even-base64) token, then we might be able to do something with that data.

(Maybe we can we get that kind of data outside of UMA? Perhaps through crawling,
or httparchive?)

   }
 
   // Token is base64-encoded; decode first.
   std::string token_contents;
   if (!base::Base64Decode(token_payload, &token_contents)) {
-    return nullptr;
+    return blink::WebOriginTrialTokenStatus::Malformed;
   }
 
   // Only version 2 currently supported.
   if (token_contents.length() < (kVersionOffset + kVersionSize)) {
-    return nullptr;
+    return blink::WebOriginTrialTokenStatus::Malformed;
   }
   uint8_t version = token_contents[kVersionOffset];
   if (version != kVersion2) {
-    return nullptr;
+    return blink::WebOriginTrialTokenStatus::Malformed;

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

This should definitely be a different status code -- if we start issuing v3
tokens, then we could see a lot of "Malformed" responses in UMA from older
Chrome clients, when that's really not the case.

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

Done.

   }
 
   // Token must be large enough to contain a version, signature, and payload
   // length.
   if (token_contents.length() < (kPayloadLengthOffset + kPayloadLengthSize)) {
-    return nullptr;
+    return blink::WebOriginTrialTokenStatus::Malformed;
   }
 
   // Extract the length of the signed data (Big-endian).
@@ -94,7 +115,7 @@ std::unique_ptr<std::string> TrialToken::Extract(
 
   // Validate that the stated length matches the actual payload length.
   if (payload_length != token_contents.length() - kPayloadOffset) {
-    return nullptr;
+    return blink::WebOriginTrialTokenStatus::Malformed;
   }
 
   // Extract the version-specific contents of the token.
@@ -110,14 +131,15 @@ std::unique_ptr<std::string> TrialToken::Extract(
 
   // Validate the signature on the data before proceeding.
   if (!TrialToken::ValidateSignature(signature, signed_data, public_key)) {
-    return nullptr;
+    return blink::WebOriginTrialTokenStatus::InvalidSignature;
   }
 
   // Return just the payload, as a new string.
-  return base::WrapUnique(
-      new std::string(token_contents, kPayloadOffset, payload_length));
+  *out_token_json = token_contents.substr(kPayloadOffset, payload_length);
+  return blink::WebOriginTrialTokenStatus::Success;
 }
 
+// static
 std::unique_ptr<TrialToken> TrialToken::Parse(const std::string& token_json) {
   std::unique_ptr<base::DictionaryValue> datadict =
       base::DictionaryValue::From(base::JSONReader::Read(token_json));