Collect UMA data for Origin Trials

content/common/origin_trials/trial_token.h

Index: content/common/origin_trials/trial_token.h
diff --git a/content/common/origin_trials/trial_token.h b/content/common/origin_trials/trial_token.h
index fa05e3713b5b943cbd3f8fd774d09f1c5f8793cc..37785312475918465e8084e2063ce579de61d47c 100644
--- a/content/common/origin_trials/trial_token.h
+++ b/content/common/origin_trials/trial_token.h
@@ -11,6 +11,7 @@
 #include "base/strings/string_piece.h"
 #include "base/time/time.h"
 #include "content/common/content_export.h"
+#include "content/common/origin_trials/trial_token_status.h"
 #include "url/origin.h"
 
 namespace content {
@@ -32,19 +33,24 @@ class CONTENT_EXPORT TrialToken {
  public:
   ~TrialToken();
 
-  // Returns a token object if the string represents a signed well-formed token,
-  // or nullptr otherwise. (This does not mean that the token is currently
-  // valid, or appropriate for a given origin / feature, just that it is
+  // If the string represents a signed well-formed token, a token object is
+  // returned in the |out_token| parameter and success is returned. Otherwise,
+  // the return code indicates what was wrong with the string, and |out_token|
+  // is set to NULL.
+  // Note that success does not mean that the token is currently valid, or
+  // appropriate for a given origin / feature. It only means that it is
   // correctly formatted and signed by the supplied public key, and can be
-  // parsed.)
-  static std::unique_ptr<TrialToken> From(const std::string& token_text,
-                                          base::StringPiece public_key);
-
-  // Returns true if this token is appropriate for use by the given origin,
-  // for the given feature name, and has not yet expired.
-  bool IsValidForFeature(const url::Origin& origin,
-                         base::StringPiece feature_name,
-                         const base::Time& now) const;
+  // parsed.
+  static TrialTokenStatus From(const std::string& token_text,
+                               base::StringPiece public_key,
+                               std::unique_ptr<TrialToken>& out_token);

[Marijn Kruisselbrink, 2016/04/21 18:18:42]

Pass-by-(non-const)-reference is against the styleguide. If you want to return
something through an argument you should use a pointer.
I also wonder if since this method is still called "From" it might not make
sense to still return a std::unique_ptr, and have the status result through a
(pointer) out-parameter. That would at least avoid the somewhat weird situation
of having a pointer to a unique pointer.

It's also not entirely clear to me why TrialToken is still a class that we even
bother instantiating though. Maybe as an implementation detail of
TrialTokenValidator it makes sense, but TrialToken instances are always
short-lived, yet heap-allocated, and there doesn't even really seem to be any
benefit to try to keep one around. Of course if in the future we would somehow
cache parsed tokens rather than token strings there might be some point in
having a TrialToken instance, but even that wouldn't really work in the presence
of multiple public keys.

[chasej, 2016/04/22 18:50:06]

TL;DR - I made the result enum be the out parameter, and went back to returning
a unique_ptr.

You're correct, I missed the part in the Google style guide about reference
arguments. I didn't see anything about how to handle out parameters that were
smart pointers in the various docs on ownership and passing objects. Common
wisdom (outside of our style guides) suggested that a non-const ref was the best
way to have a unique_ptr as an out parameter.  As you say, maybe the best answer
is to swap the return and out values. I originally considered that, but it
seemed the convention was typically to return the result code, and have the
maybe-created object as the out parameter. In this case though, I think the swap
is justifiable, especially with the "From" naming.
The plan was to have some caching of parsed tokens, or at least the validation
results. Of course, we don't have any code like that as yet. I don't really
understand what you mean that caching wouldn't work with multiple public keys.

Regardless, if we want to get rid of TrialToken, I'd suggest that should be a
separate bug/CL.

[Marijn Kruisselbrink, 2016/04/22 21:51:06]

Never mind, you're right. I think I was thinking that somehow the validating the
signature before parsing would mean that the parsed tokens are now somehow tied
to the public key used to parse them. But even if that is the case, it doesn't
make it any less valid to cache them, as once the key is validated it really
doesn't matter anymore what key they were signed with in the first place.

+
+  // Returns success if this token is appropriate for use by the given origin
+  // and feature name, and has not yet expired. Otherwise, the return value
+  // indicates why the token is not valid.
+  TrialTokenStatus IsValidForFeature(const url::Origin& origin,
+                                     base::StringPiece feature_name,
+                                     const base::Time& now) const;
 
   url::Origin origin() { return origin_; }
   std::string feature_name() { return feature_name_; }
@@ -53,10 +59,13 @@ class CONTENT_EXPORT TrialToken {
  protected:
   friend class TrialTokenTest;
 
-  // Returns the payload of a signed token, or nullptr if the token is not
-  // properly signed, or is not well-formed.
-  static std::unique_ptr<std::string> Extract(const std::string& token_text,
-                                              base::StringPiece public_key);
+  // If the string represents a properly signed and well-formed token, the JSON
+  // payload is returned in the |out_token_json| parameter and success is
+  // returned. Otherwise,the return code indicates what was wrong with the
+  // string, and |out_token_json| is unchanged.
+  static TrialTokenStatus Extract(const std::string& token_text,
+                                  base::StringPiece public_key,
+                                  std::unique_ptr<std::string>& out_token_json);

[Marijn Kruisselbrink, 2016/04/21 18:18:42]

Here too, don't use non-const reference arguments. And in fact now you have an
explicit TrialTokenStatus result the out_token_json doesn't have to be a
unique_ptr anymore either. So just use std::string* out_token_json or something
like that.

[chasej, 2016/04/22 18:50:06]

Done.

 
   // Returns a token object if the string represents a well-formed JSON token
   // payload, or nullptr otherwise.