Refactor OriginTrialPolicy to be an abstract interface in content/public/common

Refactor OriginTrialPolicy to be an abstract interface in content/public/common

This also changes the initialization of the OriginTrialPolicy object: it is now lazily instantiated on the ChromeContentClient when first requested. Because the object is only requested by the validator, when tokens are actually received, this means that the constructor is certain to have access to any policy provided by the browser through the command line.

BUG=589830
Committed: https://crrev.com/186a896dc1d498ab89c879e795fc101a35cf9d46
Cr-Commit-Position: refs/heads/master@{#401296}

[iclelland, 2016-06-08 15:41:44]

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

[iclelland, 2016-06-08 15:41:45]

+r chasej: PTAL; this was a bigger refactor than I expected, but if should make
adding future policy (like revoked tokens) cleaner.

[chasej, 2016-06-08 16:25:20]

lgtm, with nits.

https://codereview.chromium.org/2049783002/diff/1/content/shell/common/shell_...
File content/shell/common/shell_origin_trial_policy.cc (right):

https://codereview.chromium.org/2049783002/diff/1/content/shell/common/shell_...
content/shell/common/shell_origin_trial_policy.cc:13: // TODO(iclelland): Update
this comment with the location of the public and
Nit: This comment can be updated/removed now?

[iclelland, 2016-06-08 17:44:27]

https://codereview.chromium.org/2049783002/diff/1/content/shell/common/shell_...
File content/shell/common/shell_origin_trial_policy.cc (right):

https://codereview.chromium.org/2049783002/diff/1/content/shell/common/shell_...
content/shell/common/shell_origin_trial_policy.cc:13: // TODO(iclelland): Update
this comment with the location of the public and
On 2016/06/08 16:25:20, chasej wrote:
> Nit: This comment can be updated/removed now?

Done.

[iclelland, 2016-06-09 16:22:43]

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

[iclelland, 2016-06-09 16:22:44]

jochen, can you PTAL? (Sorry for the flurry of CLs)

[jochen (gone - plz use gerrit), 2016-06-13 15:26:43]

lgtm

https://codereview.chromium.org/2049783002/diff/40001/content/public/common/o...
File content/public/common/origin_trial_policy.h (right):

https://codereview.chromium.org/2049783002/diff/40001/content/public/common/o...
content/public/common/origin_trial_policy.h:15: OriginTrialPolicy() = default;
not needed?

[iclelland, 2016-06-15 09:43:47]

I've rebased this onto master, and removed the code from the CL it was depending
on (except for the InitializeOriginTrials() method from
https://codereview.chromium.org/2047683002 which is now needed so that the
public key can be set without an explicit cast from the abstract
OriginTrialPolicy to a concrete ChromeOriginTrialPolicy)

https://codereview.chromium.org/2049783002/diff/40001/content/public/common/o...
File content/public/common/origin_trial_policy.h (right):

https://codereview.chromium.org/2049783002/diff/40001/content/public/common/o...
content/public/common/origin_trial_policy.h:15: OriginTrialPolicy() = default;
On 2016/06/13 15:26:43, jochen (slow) wrote:
> not needed?

Not needed. Removed.

[jochen (gone - plz use gerrit), 2016-06-20 11:44:56]

https://codereview.chromium.org/2049783002/diff/80001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/2049783002/diff/80001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:639:
chrome_content_client_.InitializeOriginTrialPolicy();
chrome shouldn't call content's embedder APIs. Why aren't you just invoking a
method on ChromeOriginTrialPolicy?

[iclelland, 2016-06-20 18:08:40]

https://codereview.chromium.org/2049783002/diff/80001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/2049783002/diff/80001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:639:
chrome_content_client_.InitializeOriginTrialPolicy();
On 2016/06/20 11:44:56, jochen wrote:
> chrome shouldn't call content's embedder APIs. Why aren't you just invoking a
> method on ChromeOriginTrialPolicy?

This was a temporary situation (as a result of refactoring the three CLs to make
this one first)

You can see in https://codereview.chromium.org/2047683002/patch/60001/70011 that
this call is moved to render_thread_impl.cc, and called from content/.

I can fix the layering violation here, and update the next CL to undo that, or I
can merge the two CLs so it never happens. I had split them up to (hopefully)
make review easier, but I can always re-combine them.

[iclelland, 2016-06-21 03:33:34]

https://codereview.chromium.org/2049783002/diff/80001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/2049783002/diff/80001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:639:
chrome_content_client_.InitializeOriginTrialPolicy();
On 2016/06/20 18:08:40, iclelland wrote:
> On 2016/06/20 11:44:56, jochen wrote:
> > chrome shouldn't call content's embedder APIs. Why aren't you just invoking
a
> > method on ChromeOriginTrialPolicy?
> 
> This was a temporary situation (as a result of refactoring the three CLs to
make
> this one first)
> 
> You can see in https://codereview.chromium.org/2047683002/patch/60001/70011
that
> this call is moved to render_thread_impl.cc, and called from content/.
> 
> I can fix the layering violation here, and update the next CL to undo that, or
I
> can merge the two CLs so it never happens. I had split them up to (hopefully)
> make review easier, but I can always re-combine them.

I think the only reason I hadn't made InitializeOriginTrialPolicy a method on
OriginTrialPolicy before is that I was making everything about OriginTrialPolicy
const, by Sorin's suggestion, and so the RenderThreadImpl didn't have a way to
get a non-const OriginTrialPolicy object to initialize it. I've added a patch to
this CL that adds a non-const GetOriginTrialPolicy() method, which can be used
for initialization. The TrialTokenValidator still uses the const version, to
make it clear that it's not modifying either the ContentClient or the
OriginTrialPolicy.

[jochen (gone - plz use gerrit), 2016-06-21 07:48:18]

On 2016/06/21 at 03:33:34, iclelland wrote:
>
https://codereview.chromium.org/2049783002/diff/80001/chrome/app/chrome_main_...
> File chrome/app/chrome_main_delegate.cc (right):
> 
>
https://codereview.chromium.org/2049783002/diff/80001/chrome/app/chrome_main_...
> chrome/app/chrome_main_delegate.cc:639:
chrome_content_client_.InitializeOriginTrialPolicy();
> On 2016/06/20 18:08:40, iclelland wrote:
> > On 2016/06/20 11:44:56, jochen wrote:
> > > chrome shouldn't call content's embedder APIs. Why aren't you just
invoking a
> > > method on ChromeOriginTrialPolicy?
> > 
> > This was a temporary situation (as a result of refactoring the three CLs to
make
> > this one first)
> > 
> > You can see in https://codereview.chromium.org/2047683002/patch/60001/70011
that
> > this call is moved to render_thread_impl.cc, and called from content/.
> > 
> > I can fix the layering violation here, and update the next CL to undo that,
or I
> > can merge the two CLs so it never happens. I had split them up to
(hopefully)
> > make review easier, but I can always re-combine them.
> 
> I think the only reason I hadn't made InitializeOriginTrialPolicy a method on
OriginTrialPolicy before is that I was making everything about OriginTrialPolicy
const, by Sorin's suggestion, and so the RenderThreadImpl didn't have a way to
get a non-const OriginTrialPolicy object to initialize it. I've added a patch to
this CL that adds a non-const GetOriginTrialPolicy() method, which can be used
for initialization. The TrialTokenValidator still uses the const version, to
make it clear that it's not modifying either the ContentClient or the
OriginTrialPolicy.

The other patch where you move the call to RenderThreadImpl also looks like a
layering violation.

For this CL, you could just read out the command line flag in
ChromeOriginTrialPolicy's ctor, no?

I'd also drop the const getter

[iclelland, 2016-06-21 13:46:20]

On 2016/06/21 07:48:18, jochen wrote:
> On 2016/06/21 at 03:33:34, iclelland wrote:
> >
>
https://codereview.chromium.org/2049783002/diff/80001/chrome/app/chrome_main_...
> > File chrome/app/chrome_main_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/2049783002/diff/80001/chrome/app/chrome_main_...
> > chrome/app/chrome_main_delegate.cc:639:
> chrome_content_client_.InitializeOriginTrialPolicy();
> > On 2016/06/20 18:08:40, iclelland wrote:
> > > On 2016/06/20 11:44:56, jochen wrote:
> > > > chrome shouldn't call content's embedder APIs. Why aren't you just
> invoking a
> > > > method on ChromeOriginTrialPolicy?
> > > 
> > > This was a temporary situation (as a result of refactoring the three CLs
to
> make
> > > this one first)
> > > 
> > > You can see in
https://codereview.chromium.org/2047683002/patch/60001/70011
> that
> > > this call is moved to render_thread_impl.cc, and called from content/.
> > > 
> > > I can fix the layering violation here, and update the next CL to undo
that,
> or I
> > > can merge the two CLs so it never happens. I had split them up to
> (hopefully)
> > > make review easier, but I can always re-combine them.
> > 
> > I think the only reason I hadn't made InitializeOriginTrialPolicy a method
on
> OriginTrialPolicy before is that I was making everything about
OriginTrialPolicy
> const, by Sorin's suggestion, and so the RenderThreadImpl didn't have a way to
> get a non-const OriginTrialPolicy object to initialize it. I've added a patch
to
> this CL that adds a non-const GetOriginTrialPolicy() method, which can be used
> for initialization. The TrialTokenValidator still uses the const version, to
> make it clear that it's not modifying either the ContentClient or the
> OriginTrialPolicy.
> 
> The other patch where you move the call to RenderThreadImpl also looks like a
> layering violation.
> 
> For this CL, you could just read out the command line flag in
> ChromeOriginTrialPolicy's ctor, no?

For this CL, that's easy enough, I think. The command line isn't actually
available when ChromeMainDelegate is constructed, so I can't construct the
ChromeOriginTrialPolicy at the same time, but that's okay, because I'll have to
delay constructing the policy object anyway when we start reading policy from
prefs.

> 
> I'd also drop the const getter
Done.

[jochen (gone - plz use gerrit), 2016-06-21 14:16:54]

did you mean to upload a new version?

[iclelland, 2016-06-21 14:30:28]

On 2016/06/21 14:16:54, jochen wrote:
> did you mean to upload a new version?

No, I just forgot that replying in the message stream doesn't save as draft
first; I'm still working out a couple of timing issues before I upload it.

[iclelland, 2016-06-21 15:28:49]

On 2016/06/21 14:30:28, iclelland wrote:
> On 2016/06/21 14:16:54, jochen wrote:
> > did you mean to upload a new version?
> 
> No, I just forgot that replying in the message stream doesn't save as draft
> first; I'm still working out a couple of timing issues before I upload it.

So the issue that I'm facing is the same one that I figured I had resolved in
https://codereview.chromium.org/2047683002 -- on Linux, the command line string
is not passed to the zygote process, but the content client is created in the
zygote. We don't have access to the renderer command line (which we need to get
the key from the browser process) until we create the renderer thread (which is
why I had moved initialization to RenderThreadImpl::Init).

Is it reasonable to have RenderThreadImpl call methods in the content client,
implemented by ChromeContentClient? You mentioned that it looked like a layering
violation, but I'm not seeing it.

[jochen (gone - plz use gerrit), 2016-06-22 13:25:05]

On 2016/06/21 at 15:28:49, iclelland wrote:
> On 2016/06/21 14:30:28, iclelland wrote:
> > On 2016/06/21 14:16:54, jochen wrote:
> > > did you mean to upload a new version?
> > 
> > No, I just forgot that replying in the message stream doesn't save as draft
> > first; I'm still working out a couple of timing issues before I upload it.
> 
> So the issue that I'm facing is the same one that I figured I had resolved in
https://codereview.chromium.org/2047683002 -- on Linux, the command line string
is not passed to the zygote process, but the content client is created in the
zygote. We don't have access to the renderer command line (which we need to get
the key from the browser process) until we create the renderer thread (which is
why I had moved initialization to RenderThreadImpl::Init).
> 
> Is it reasonable to have RenderThreadImpl call methods in the content client,
implemented by ChromeContentClient? You mentioned that it looked like a layering
violation, but I'm not seeing it.

The layering violation is that now content has to know when a chrome class needs
to be configured.

If you lazily construct the policy in
ChromeContentClient::GetOriginTrialPolicy() shouldn't the command line be
available? If not, chrome's render thread observer should initialize the trial
policy.

[iclelland, 2016-06-22 13:31:13]

On 2016/06/22 13:25:05, jochen wrote:
> On 2016/06/21 at 15:28:49, iclelland wrote:
> > On 2016/06/21 14:30:28, iclelland wrote:
> > > On 2016/06/21 14:16:54, jochen wrote:
> > > > did you mean to upload a new version?
> > > 
> > > No, I just forgot that replying in the message stream doesn't save as
draft
> > > first; I'm still working out a couple of timing issues before I upload it.
> > 
> > So the issue that I'm facing is the same one that I figured I had resolved
in
> https://codereview.chromium.org/2047683002 -- on Linux, the command line
string
> is not passed to the zygote process, but the content client is created in the
> zygote. We don't have access to the renderer command line (which we need to
get
> the key from the browser process) until we create the renderer thread (which
is
> why I had moved initialization to RenderThreadImpl::Init).
> > 
> > Is it reasonable to have RenderThreadImpl call methods in the content
client,
> implemented by ChromeContentClient? You mentioned that it looked like a
layering
> violation, but I'm not seeing it.
> 
> The layering violation is that now content has to know when a chrome class
needs
> to be configured.

That's why I had declared it as a generic ContentClient::InitializeOTC(); trying
not to be chrome-specific, but rather saying "here is the initialization hook;
it will be called during renderer startup". However:

> If you lazily construct the policy in
> ChromeContentClient::GetOriginTrialPolicy() shouldn't the command line be
> available? If not, chrome's render thread observer should initialize the trial
> policy.

Ahh, that's an interesting idea -- I hadn't tried to construct lazily; it was
originally a fixed member of ChromeContentClient, so constructed with the
ChromeMainDelegate, and I was still thinking about early initialization. I'll
give that a try, thanks!

[iclelland, 2016-06-22 14:41:40]

Description was changed from

==========
Refactor OriginTrialPolicy to be an abstract interface in content/public/common

BUG=589830
==========

to

==========
Refactor OriginTrialPolicy to be an abstract interface in content/public/common

This also changes the initialization of the OriginTrialPolicy object: it is now
lazily instantiated on the ChromeContentClient when first requested. Because the
object is only requested by the validator, when tokens are actually received,
this means that the constructor is certain to have access to any policy provided
by the browser through the command line.

BUG=589830
==========

[iclelland, 2016-06-22 14:42:43]

On 2016/06/22 13:31:13, iclelland wrote:
> On 2016/06/22 13:25:05, jochen wrote:
> > On 2016/06/21 at 15:28:49, iclelland wrote:
> > > On 2016/06/21 14:30:28, iclelland wrote:
> > > > On 2016/06/21 14:16:54, jochen wrote:
> > > > > did you mean to upload a new version?
> > > > 
> > > > No, I just forgot that replying in the message stream doesn't save as
> draft
> > > > first; I'm still working out a couple of timing issues before I upload
it.
> > > 
> > > So the issue that I'm facing is the same one that I figured I had resolved
> in
> > https://codereview.chromium.org/2047683002 -- on Linux, the command line
> string
> > is not passed to the zygote process, but the content client is created in
the
> > zygote. We don't have access to the renderer command line (which we need to
> get
> > the key from the browser process) until we create the renderer thread (which
> is
> > why I had moved initialization to RenderThreadImpl::Init).
> > > 
> > > Is it reasonable to have RenderThreadImpl call methods in the content
> client,
> > implemented by ChromeContentClient? You mentioned that it looked like a
> layering
> > violation, but I'm not seeing it.
> > 
> > The layering violation is that now content has to know when a chrome class
> needs
> > to be configured.
> 
> That's why I had declared it as a generic ContentClient::InitializeOTC();
trying
> not to be chrome-specific, but rather saying "here is the initialization hook;
> it will be called during renderer startup". However:
> 
> > If you lazily construct the policy in
> > ChromeContentClient::GetOriginTrialPolicy() shouldn't the command line be
> > available? If not, chrome's render thread observer should initialize the
trial
> > policy.
> 
> Ahh, that's an interesting idea -- I hadn't tried to construct lazily; it was
> originally a fixed member of ChromeContentClient, so constructed with the
> ChromeMainDelegate, and I was still thinking about early initialization. I'll
> give that a try, thanks!

jochen - can you PTAL again? This seems to work perfectly; thanks a lot for your
patience.

[jochen (gone - plz use gerrit), 2016-06-22 14:48:32]

lgtm

thanks for your patience as well :)

[iclelland, 2016-06-22 16:00:38]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-06-22 16:00:39]

The patchset sent to the CQ was uploaded after l-g-t-m from chasej@chromium.org
Link to the patchset: https://codereview.chromium.org/2049783002/#ps120001
(title: "Switch to lazy initialization in ChromeContentClient")

[commit-bot: I haz the power, 2016-06-22 16:02:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2049783002/120001

[commit-bot: I haz the power, 2016-06-22 16:06:36]

Description was changed from

==========
Refactor OriginTrialPolicy to be an abstract interface in content/public/common

This also changes the initialization of the OriginTrialPolicy object: it is now
lazily instantiated on the ChromeContentClient when first requested. Because the
object is only requested by the validator, when tokens are actually received,
this means that the constructor is certain to have access to any policy provided
by the browser through the command line.

BUG=589830
==========

to

==========
Refactor OriginTrialPolicy to be an abstract interface in content/public/common

This also changes the initialization of the OriginTrialPolicy object: it is now
lazily instantiated on the ChromeContentClient when first requested. Because the
object is only requested by the validator, when tokens are actually received,
this means that the constructor is certain to have access to any policy provided
by the browser through the command line.

BUG=589830
==========

[commit-bot: I haz the power, 2016-06-22 16:06:37]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-22 16:08:21]

Description was changed from

==========
Refactor OriginTrialPolicy to be an abstract interface in content/public/common

This also changes the initialization of the OriginTrialPolicy object: it is now
lazily instantiated on the ChromeContentClient when first requested. Because the
object is only requested by the validator, when tokens are actually received,
this means that the constructor is certain to have access to any policy provided
by the browser through the command line.

BUG=589830
==========

to

==========
Refactor OriginTrialPolicy to be an abstract interface in content/public/common

This also changes the initialization of the OriginTrialPolicy object: it is now
lazily instantiated on the ChromeContentClient when first requested. Because the
object is only requested by the validator, when tokens are actually received,
this means that the constructor is certain to have access to any policy provided
by the browser through the command line.

BUG=589830

Committed: https://crrev.com/186a896dc1d498ab89c879e795fc101a35cf9d46
Cr-Commit-Position: refs/heads/master@{#401296}
==========

[commit-bot: I haz the power, 2016-06-22 16:08:23]

Patchset 7 (id:??) landed as
https://crrev.com/186a896dc1d498ab89c879e795fc101a35cf9d46
Cr-Commit-Position: refs/heads/master@{#401296}