[Experimental Framework] Move the trial token public key out of content and into the embedder.

[Experimental Framework] Move the trial token public key out of content and into the embedder.

In order to make the code changes easier to reason about, this is now part 2
of a two-part commit.

Part 1: https://codereview.chromium.org/1680873002/

BUG=543220
Committed: https://crrev.com/17b143af6e640744583640b405531b410569dd73
Cr-Commit-Position: refs/heads/master@{#375244}

[iclelland, 2016-02-04 20:55:53]

iclelland@chromium.org changed reviewers:
+ chasej@chromium.org

[iclelland, 2016-02-04 20:55:54]

+r chasej, PTAL, thanks!

[iclelland, 2016-02-05 19:05:47]

+r jochen -- Can you take a look?

I've run this past a couple of people here, but I want to be sure that the
separation of this into chrome/common and content/common makes sense, and that
attaching this functionality to the ContentClient is the right thing to do here.

Thanks!

[iclelland, 2016-02-05 19:05:58]

iclelland@chromium.org changed reviewers:
+ jochen@chromium.org

[iclelland, 2016-02-05 19:10:55]

iclelland@chromium.org changed reviewers:
+ jam@chromium.org
- jochen@chromium.org

[iclelland, 2016-02-05 19:10:55]

-r jochen - n/m, Sorry, I didn't see your OOO status until too late

+r jam -- Do you mind taking a look? Thanks!

[chasej, 2016-02-05 19:35:28]

https://codereview.chromium.org/1653263005/diff/60001/chrome/common/origin_tr...
File chrome/common/origin_trials/origin_trial_key_manager.cc (right):

https://codereview.chromium.org/1653263005/diff/60001/chrome/common/origin_tr...
chrome/common/origin_trials/origin_trial_key_manager.cc:20: base::StringPiece
ChromeOriginTrialKeyManager::GetPublicKey() {
The file name doesn't match the class name. I would have expected the file name
to be chrome_origin_trial_key_manager.cc.

https://codereview.chromium.org/1653263005/diff/60001/chrome/common/origin_tr...
File chrome/common/origin_trials/origin_trial_key_manager.h (right):

https://codereview.chromium.org/1653263005/diff/60001/chrome/common/origin_tr...
chrome/common/origin_trials/origin_trial_key_manager.h:10: class
ChromeOriginTrialKeyManager {
The file name doesn't match the class name. I would have expected the file name
to be chrome_origin_trial_key_manager.h.

https://codereview.chromium.org/1653263005/diff/60001/content/child/origin_tr...
File content/child/origin_trials/trial_token_validator_unittest.cc (right):

https://codereview.chromium.org/1653263005/diff/60001/content/child/origin_tr...
content/child/origin_trials/trial_token_validator_unittest.cc:88:
test_content_client_.SetOriginTrialPublicKey(kTestPublicKey);
Nit: Should this call setPublicKey(), to avoid some duplication?

https://codereview.chromium.org/1653263005/diff/60001/content/common/origin_t...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1653263005/diff/60001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:87: // active keys here.
https://crbug.com/543220
Nit: This is the wrong bug number (543220 is for supporting a single signing
key).  I've added a new bug for multiple keys: 584737.

[iclelland, 2016-02-05 20:18:09]

https://codereview.chromium.org/1653263005/diff/60001/chrome/common/origin_tr...
File chrome/common/origin_trials/origin_trial_key_manager.cc (right):

https://codereview.chromium.org/1653263005/diff/60001/chrome/common/origin_tr...
chrome/common/origin_trials/origin_trial_key_manager.cc:20: base::StringPiece
ChromeOriginTrialKeyManager::GetPublicKey() {
On 2016/02/05 19:35:28, chasej wrote:
> The file name doesn't match the class name. I would have expected the file
name
> to be chrome_origin_trial_key_manager.cc.

Renamed class to match filename instead.

https://codereview.chromium.org/1653263005/diff/60001/chrome/common/origin_tr...
File chrome/common/origin_trials/origin_trial_key_manager.h (right):

https://codereview.chromium.org/1653263005/diff/60001/chrome/common/origin_tr...
chrome/common/origin_trials/origin_trial_key_manager.h:10: class
ChromeOriginTrialKeyManager {
On 2016/02/05 19:35:28, chasej wrote:
> The file name doesn't match the class name. I would have expected the file
name
> to be chrome_origin_trial_key_manager.h.

Now that this doesn't implement a content/ interface, I've just dropped the
"Chrome" prefix instead.

https://codereview.chromium.org/1653263005/diff/60001/content/child/origin_tr...
File content/child/origin_trials/trial_token_validator_unittest.cc (right):

https://codereview.chromium.org/1653263005/diff/60001/content/child/origin_tr...
content/child/origin_trials/trial_token_validator_unittest.cc:88:
test_content_client_.SetOriginTrialPublicKey(kTestPublicKey);
On 2016/02/05 19:35:28, chasej wrote:
> Nit: Should this call setPublicKey(), to avoid some duplication?

I think I'm just a bit wary of calling instance methods inside a constructor,
but you're probably right here.

Done.

https://codereview.chromium.org/1653263005/diff/60001/content/common/origin_t...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1653263005/diff/60001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:87: // active keys here.
https://crbug.com/543220
On 2016/02/05 19:35:28, chasej wrote:
> Nit: This is the wrong bug number (543220 is for supporting a single signing
> key).  I've added a new bug for multiple keys: 584737.

Thanks! Done.

[chasej, 2016-02-05 21:52:55]

LGTM.

[jam, 2016-02-08 17:20:24]

why is this code in content/common? i don't see it hooked up yet. from the docs,
it seems like this is a web platform feature. so i assume it'll be called from
content/renderer? or at most also content/child?

if so, please put this in content/renderer (if it's only used from there)

[jam, 2016-02-08 17:20:36]

On 2016/02/08 17:20:24, jam wrote:
> why is this code in content/common? i don't see it hooked up yet. from the
docs,
> it seems like this is a web platform feature. so i assume it'll be called from
> content/renderer? or at most also content/child?
> 
> if so, please put this in content/renderer (if it's only used from there)

(to be clear, i'm only talking about code in content)

[iclelland, 2016-02-08 18:09:15]

On 2016/02/08 17:20:36, jam wrote:
> On 2016/02/08 17:20:24, jam wrote:
> > why is this code in content/common? i don't see it hooked up yet. from the
> docs,
> > it seems like this is a web platform feature. so i assume it'll be called
from
> > content/renderer? or at most also content/child?
> > 
> > if so, please put this in content/renderer (if it's only used from there)
> 
> (to be clear, i'm only talking about code in content)

Yes, it is currently only being used in the renderer. My intention originally
was to allow the trial tokens to also be validated (and cached, and persisted if
necessary,) from the browser process as well-- that's why trial_token.* ended up
where it is, in content/common. That code hasn't been written yet, though, so at
the moment, this is renderer only.

[jam, 2016-02-08 18:40:27]

On 2016/02/08 18:09:15, iclelland wrote:
> On 2016/02/08 17:20:36, jam wrote:
> > On 2016/02/08 17:20:24, jam wrote:
> > > why is this code in content/common? i don't see it hooked up yet. from the
> > docs,
> > > it seems like this is a web platform feature. so i assume it'll be called
> from
> > > content/renderer? or at most also content/child?
> > > 
> > > if so, please put this in content/renderer (if it's only used from there)
> > 
> > (to be clear, i'm only talking about code in content)
> 
> Yes, it is currently only being used in the renderer. My intention originally
> was to allow the trial tokens to also be validated (and cached, and persisted
if
> necessary,) from the browser process as well-- that's why trial_token.* ended
up
> where it is, in content/common. That code hasn't been written yet, though, so
at
> the moment, this is renderer only.

in that case, the contentclient implementation would probably be different for
renderer vs browser code right? i.e. content/renderer would do IPCs to browser
and maybe caching. while browser would do validation/persistence. so that's why
it feels odd that the embedder API is on content_client.h instead of
content_renderer_client.h

since it's only called from the renderer now, can we put it in content/renderer
until we have more code in the browser?

[iclelland, 2016-02-08 18:44:30]

On 2016/02/08 18:40:27, jam wrote:
> On 2016/02/08 18:09:15, iclelland wrote:
> > On 2016/02/08 17:20:36, jam wrote:
> > > On 2016/02/08 17:20:24, jam wrote:
> > > > why is this code in content/common? i don't see it hooked up yet. from
the
> > > docs,
> > > > it seems like this is a web platform feature. so i assume it'll be
called
> > from
> > > > content/renderer? or at most also content/child?
> > > > 
> > > > if so, please put this in content/renderer (if it's only used from
there)
> > > 
> > > (to be clear, i'm only talking about code in content)
> > 
> > Yes, it is currently only being used in the renderer. My intention
originally
> > was to allow the trial tokens to also be validated (and cached, and
persisted
> if
> > necessary,) from the browser process as well-- that's why trial_token.*
ended
> up
> > where it is, in content/common. That code hasn't been written yet, though,
so
> at
> > the moment, this is renderer only.
> 
> in that case, the contentclient implementation would probably be different for
> renderer vs browser code right? i.e. content/renderer would do IPCs to browser
> and maybe caching. while browser would do validation/persistence.

Yes; this was pretty much exactly the original design. I made some compromises
to avoid synchronous IPC, and the current code was the result.

> so that's why
> it feels odd that the embedder API is on content_client.h instead of
> content_renderer_client.h
> 
> since it's only called from the renderer now, can we put it in
content/renderer
> until we have more code in the browser?

Sounds like a good plan; I'll move the code around, and update the issue.

Thanks for the sanity-check

[iclelland, 2016-02-08 21:29:13]

Description was changed from

==========
[Experimental Framework] Move the trial token public key out of content and into
the embedder.

BUG=543220
==========

to

==========
[Experimental Framework] Move the trial token public key out of content and into
the embedder.

In order to make the code changes easier to reason about, this is now part 2
of a two-part commit.

Part 1: https://codereview.chromium.org/1680873002/

BUG=543220
==========

[iclelland, 2016-02-09 15:34:39]

jam -- can you PTAL again? I've moved all of the common code into renderer/ (in
the Part 1 commit), so this just separates the responsibility for providing the
signing public key out to the embedder.

[chasej, 2016-02-10 05:24:24]

I noticed a few nits.

https://codereview.chromium.org/1653263005/diff/100001/chrome/renderer/origin...
File chrome/renderer/origin_trials/origin_trial_key_manager.cc (right):

https://codereview.chromium.org/1653263005/diff/100001/chrome/renderer/origin...
chrome/renderer/origin_trials/origin_trial_key_manager.cc:11: //
https://crbug.com/543220
Nit: This is the wrong bug number (543220 is for supporting a single signing
key), the bug for multiple keys is 584737.

https://codereview.chromium.org/1653263005/diff/100001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1653263005/diff/100001/content/public/rendere...
content/public/renderer/content_renderer_client.h:324: // Returns the public key
to be used for origin trials, or an empty srting if
Typo, should be "string", not "srting"

https://codereview.chromium.org/1653263005/diff/100001/content/renderer/origi...
File content/renderer/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1653263005/diff/100001/content/renderer/origi...
content/renderer/origin_trials/trial_token.cc:87: // active keys here.
https://crbug.com/543220
Nit: This is the wrong bug number (543220 is for supporting a single signing
key), the bug for multiple keys is 584737.

[iclelland, 2016-02-10 20:31:00]

Thanks for catching those.

https://codereview.chromium.org/1653263005/diff/100001/chrome/renderer/origin...
File chrome/renderer/origin_trials/origin_trial_key_manager.cc (right):

https://codereview.chromium.org/1653263005/diff/100001/chrome/renderer/origin...
chrome/renderer/origin_trials/origin_trial_key_manager.cc:11: //
https://crbug.com/543220
On 2016/02/10 05:24:23, chasej wrote:
> Nit: This is the wrong bug number (543220 is for supporting a single signing
> key), the bug for multiple keys is 584737.

Done.

https://codereview.chromium.org/1653263005/diff/100001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1653263005/diff/100001/content/public/rendere...
content/public/renderer/content_renderer_client.h:324: // Returns the public key
to be used for origin trials, or an empty srting if
On 2016/02/10 05:24:23, chasej wrote:
> Typo, should be "string", not "srting"

Done.

https://codereview.chromium.org/1653263005/diff/100001/content/renderer/origi...
File content/renderer/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1653263005/diff/100001/content/renderer/origi...
content/renderer/origin_trials/trial_token.cc:87: // active keys here.
https://crbug.com/543220
On 2016/02/10 05:24:23, chasej wrote:
> Nit: This is the wrong bug number (543220 is for supporting a single signing
> key), the bug for multiple keys is 584737.

Done.

[jam, 2016-02-11 21:50:24]

lgtm

https://codereview.chromium.org/1653263005/diff/120001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/1653263005/diff/120001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.h:160: 
nit: no blank line

https://codereview.chromium.org/1653263005/diff/120001/content/shell/renderer...
File content/shell/renderer/shell_content_renderer_client.h (right):

https://codereview.chromium.org/1653263005/diff/120001/content/shell/renderer...
content/shell/renderer/shell_content_renderer_client.h:31: 
nit: no blank line

[iclelland, 2016-02-12 19:11:33]

https://codereview.chromium.org/1653263005/diff/120001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/1653263005/diff/120001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.h:160: 
On 2016/02/11 21:50:24, jam wrote:
> nit: no blank line

Done.

https://codereview.chromium.org/1653263005/diff/120001/content/shell/renderer...
File content/shell/renderer/shell_content_renderer_client.h (right):

https://codereview.chromium.org/1653263005/diff/120001/content/shell/renderer...
content/shell/renderer/shell_content_renderer_client.h:31: 
On 2016/02/11 21:50:24, jam wrote:
> nit: no blank line

Done.

[iclelland, 2016-02-12 19:11:49]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-02-12 19:11:49]

The patchset sent to the CQ was uploaded after l-g-t-m from chasej@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1653263005/#ps140001
(title: "Rebase, and update for recent token changes")

[commit-bot: I haz the power, 2016-02-12 19:13:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1653263005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1653263005/140001

[commit-bot: I haz the power, 2016-02-12 20:22:30]

Description was changed from

==========
[Experimental Framework] Move the trial token public key out of content and into
the embedder.

In order to make the code changes easier to reason about, this is now part 2
of a two-part commit.

Part 1: https://codereview.chromium.org/1680873002/

BUG=543220
==========

to

==========
[Experimental Framework] Move the trial token public key out of content and into
the embedder.

In order to make the code changes easier to reason about, this is now part 2
of a two-part commit.

Part 1: https://codereview.chromium.org/1680873002/

BUG=543220
==========

[commit-bot: I haz the power, 2016-02-12 20:22:31]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-16 22:44:49]

Description was changed from

==========
[Experimental Framework] Move the trial token public key out of content and into
the embedder.

In order to make the code changes easier to reason about, this is now part 2
of a two-part commit.

Part 1: https://codereview.chromium.org/1680873002/

BUG=543220
==========

to

==========
[Experimental Framework] Move the trial token public key out of content and into
the embedder.

In order to make the code changes easier to reason about, this is now part 2
of a two-part commit.

Part 1: https://codereview.chromium.org/1680873002/

BUG=543220

Committed: https://crrev.com/17b143af6e640744583640b405531b410569dd73
Cr-Commit-Position: refs/heads/master@{#375244}
==========

[commit-bot: I haz the power, 2016-02-16 22:44:50]

Patchset 8 (id:??) landed as
https://crrev.com/17b143af6e640744583640b405531b410569dd73
Cr-Commit-Position: refs/heads/master@{#375244}