Allow command-line arguments to override EF public key

chrome/renderer/origin_trials/origin_trial_key_manager.h

 // Copyright (c) 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_RENDERER_ORIGIN_TRIALS_ORIGIN_TRIAL_KEY_MANAGER_H_
 #define CHROME_RENDERER_ORIGIN_TRIALS_ORIGIN_TRIAL_KEY_MANAGER_H_
 
+#include <string>
+
+#include "base/macros.h"
 #include "base/strings/string_piece.h"
 
 class OriginTrialKeyManager {
  public:
-  base::StringPiece GetPublicKey();
+  OriginTrialKeyManager();
+  ~OriginTrialKeyManager();
+
+  bool SetPublicKeyFromASCIIString(const std::string& ascii_public_key);
+  base::StringPiece GetPublicKey() const;

[sky, 2016/03/03 20:55:30]

Is there a reason you don't return a const std::string& here?

[iclelland, 2016/03/07 14:48:19]

I was originally using uint8_t[] for key material everywhere, but it was
suggested (https://codereview.chromium.org/1522813002/) to use StringPiece
instead to pass them around, for safety.

I suspect that std::string would be just as suitable as a container type, and
possibly easier to work with. I'm not partial to StringPiece in any way.

[sky, 2016/04/14 16:19:51]

You still have StringPiece. Is there a reason you didn't change it to return
const std::string& ?

[iclelland, 2016/04/14 19:29:07]

I mistook your original request to change it to be a design question.

In an unrelated CL, palmer@ and I discussed the possibility of changing the
public keys to std::vector<uint8_t> instead. I mentioned there that it would be
a more substantial CL, since the keys are handled in a number of different
places in the code. (The same comment applies here as well, if I were to change
the type in this one)

(https://codereview.chromium.org/1858763003/#msg18)

All that really matters is that we have an 8-bit-safe container for ~32 bytes
which we can pass around, and which can eventually be converted to a const
uint8_t* for use by boringssl. std::string fits that bill, so does a vector of
uint8_t. I'd like to make that change everywhere (probably in a separate CL,
since the scope is bigger than this one) once I know which way to change it.

If you feel that std::string is the better way to go, I'll do that; otherwise,
I'll convert it all to std::vector.

[sky, 2016/04/14 20:58:04]

As you have a setter that takes a std::string I would inclined to make the
getter return a string. too. I'm ok with a vector too, as long as they are
consistent.

[iclelland, 2016/04/15 03:09:46]

I'm not sure I understand you here, sorry -- the setter is explicitly a
conversion, from the external representation of the key (a base64-encoded
string) to the internal representation that gets held and passed around and used
by boringssl (the raw 32 bytes). I don't think that tying the internal
representation to the same type as the external is the goal here.

Or am I misreading completely, and I should do the decoding from ASCII somewhere
else in the code?

[sky, 2016/04/15 16:08:46]

Fair enough.
Nope.
I still don't see you gaining anything by returning a StringPiece, but you said
you were going to revisit that. So I'll approve this.

+
+ private:
+  std::string public_key_;
+
+  DISALLOW_COPY_AND_ASSIGN(OriginTrialKeyManager);
 };
 
 #endif  // CHROME_RENDERER_ORIGIN_TRIALS_ORIGIN_TRIAL_KEY_MANAGER_H_