Add disabled origin trial feature list

Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding component manifest key, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

The manifest data from the component will be persisted in browser local state,
as a list of strings under the key "origin_trials.disabled_features".

BUG=589830, 614454
Committed: https://crrev.com/79237e966353560115a92eb802aa7da459cdaf9f
Cr-Commit-Position: refs/heads/master@{#401583}

[iclelland, 2016-02-26 15:25:41]

Description was changed from

==========
[Experimental Framework] Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding field trial parameter, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, but will stop them from being enabled by remote origins through
origin trials.

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

to

==========
[Experimental Framework] Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding field trial parameter, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

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

[iclelland, 2016-02-26 15:34:13]

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

[iclelland, 2016-02-26 15:34:13]

+r chasej, PTAL, thanks

[chasej, 2016-02-29 19:23:26]

A few minor comments.

https://codereview.chromium.org/1741783002/diff/20001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1741783002/diff/20001/content/public/renderer...
content/public/renderer/content_renderer_client.h:336: // origin trials are not
enabled in this context.
Comment needs to be updated (from copy/paste I presume).

https://codereview.chromium.org/1741783002/diff/20001/content/renderer/origin...
File content/renderer/origin_trials/trial_token_validator.cc (right):

https://codereview.chromium.org/1741783002/diff/20001/content/renderer/origin...
content/renderer/origin_trials/trial_token_validator.cc:30: return false;
Move this check before getting the public key? Minor optimization to avoid
getting the key if won't be used.

[iclelland, 2016-05-24 19:36:28]

Description was changed from

==========
[Experimental Framework] Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding field trial parameter, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

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

to

==========
[Experimental Framework] Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding field trial parameter, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

BUG=589830,614454
==========

[iclelland, 2016-05-24 19:38:43]

https://codereview.chromium.org/1741783002/diff/20001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1741783002/diff/20001/content/public/renderer...
content/public/renderer/content_renderer_client.h:336: // origin trials are not
enabled in this context.
On 2016/02/29 19:23:26, chasej wrote:
> Comment needs to be updated (from copy/paste I presume).

Done.

https://codereview.chromium.org/1741783002/diff/20001/content/renderer/origin...
File content/renderer/origin_trials/trial_token_validator.cc (right):

https://codereview.chromium.org/1741783002/diff/20001/content/renderer/origin...
content/renderer/origin_trials/trial_token_validator.cc:30: return false;
On 2016/02/29 19:23:26, chasej wrote:
> Move this check before getting the public key? Minor optimization to avoid
> getting the key if won't be used.

Done.

[iclelland, 2016-05-27 13:53:55]

Description was changed from

==========
[Experimental Framework] Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding field trial parameter, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

BUG=589830,614454
==========

to

==========
[Experimental Framework] Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding component manifest key, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

BUG=589830,614454
==========

[iclelland, 2016-05-27 14:30:50]

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

[iclelland, 2016-05-27 14:30:50]

+r waffles for component updater integration

[waffles, 2016-05-27 17:10:07]

https://codereview.chromium.org/1741783002/diff/120001/chrome/browser/compone...
File chrome/browser/component_updater/origin_trials_component_installer.cc
(right):

https://codereview.chromium.org/1741783002/diff/120001/chrome/browser/compone...
chrome/browser/component_updater/origin_trials_component_installer.cc:91:
override_disabled_features);
Hm, are you sure this will run in time? ComponentReady is called asynchronously
during browser start-up, so I'm not sure if your changes in main_delegate will
pick it up or not.

[iclelland, 2016-05-30 18:54:06]

https://codereview.chromium.org/1741783002/diff/120001/chrome/browser/compone...
File chrome/browser/component_updater/origin_trials_component_installer.cc
(right):

https://codereview.chromium.org/1741783002/diff/120001/chrome/browser/compone...
chrome/browser/component_updater/origin_trials_component_installer.cc:91:
override_disabled_features);
On 2016/05/27 17:10:06, waffles wrote:
> Hm, are you sure this will run in time? ComponentReady is called
asynchronously
> during browser start-up, so I'm not sure if your changes in main_delegate will
> pick it up or not.

That's a good question -- It appears to be run in time, at least in all of my
testing. I added a sleep(20) to this method for debugging, and logged the
renderer command line. In that case, the browser window doesn't start until this
method returns, and the command line is always set correctly.

So something is ensuring that the initial window startup is blocking on this
method, and it appears to be working as expected. (I haven't traced through all
of the code to see exactly why that is the case, and if or how it could be
broken.)

It's not critical that the renderer be informed of any updates immediately; if a
new version of the component is downloaded, it's okay if the browser sees the
old version until the next restart. At that point, though, I'd want it to always
use the version on disk, even for the first window that gets instantiated.

[iclelland, 2016-05-31 14:50:13]

https://codereview.chromium.org/1741783002/diff/120001/chrome/browser/compone...
File chrome/browser/component_updater/origin_trials_component_installer.cc
(right):

https://codereview.chromium.org/1741783002/diff/120001/chrome/browser/compone...
chrome/browser/component_updater/origin_trials_component_installer.cc:91:
override_disabled_features);
On 2016/05/30 18:54:06, iclelland wrote:
> On 2016/05/27 17:10:06, waffles wrote:
> > Hm, are you sure this will run in time? ComponentReady is called
> asynchronously
> > during browser start-up, so I'm not sure if your changes in main_delegate
will
> > pick it up or not.
> 
> That's a good question -- It appears to be run in time, at least in all of my
> testing. I added a sleep(20) to this method for debugging, and logged the
> renderer command line. In that case, the browser window doesn't start until
this
> method returns, and the command line is always set correctly.
> 
> So something is ensuring that the initial window startup is blocking on this
> method, and it appears to be working as expected. (I haven't traced through
all
> of the code to see exactly why that is the case, and if or how it could be
> broken.)
> 
> It's not critical that the renderer be informed of any updates immediately; if
a
> new version of the component is downloaded, it's okay if the browser sees the
> old version until the next restart. At that point, though, I'd want it to
always
> use the version on disk, even for the first window that gets instantiated.

So the StartupTaskRunner in BrowserMainLoop is blocking on the
ComponentUpdateService, and so the renderer processes don't start until CUS is
finished. However, this is desktop only; on Android, it appears that the startup
tasks run asynchronously, so we may need another way of doing this (for both
this CL, and for the existing public key override)

[iclelland, 2016-06-07 13:47:57]

Description was changed from

==========
[Experimental Framework] Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding component manifest key, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

BUG=589830,614454
==========

to

==========
[Experimental Framework] Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding component manifest key, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

The manifest data from the component will be persisted in browser local state,
as a list of strings under the key "origin_trials.disabled_features".

BUG=589830,614454
==========

[iclelland, 2016-06-07 13:52:02]

After much back-and-forth on email, it was determined that
 -- Yes, the race is real
 -- We need a different persistence mechanism; component updater will work as a
delivery mechanism, but if we want to read the state to configure the renderers,
we need to store it somewhere else and read it on startup.

I added the code to persist all of the origin trial policy data, in PS#8, but
for the sake of review, and making smaller incremental changes for single bugs,
I've split it into two CLs:

https://codereview.chromium.org/2047683002/ now just deals with storing the
existing data (public key) in prefs.
This CL now just extends that mechanism to add the disabled feature list, like
the description claims.

[chasej, 2016-06-07 15:10:27]

Description was changed from

==========
[Experimental Framework] Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding component manifest key, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

The manifest data from the component will be persisted in browser local state,
as a list of strings under the key "origin_trials.disabled_features".

BUG=589830,614454
==========

to

==========
Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding component manifest key, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

The manifest data from the component will be persisted in browser local state,
as a list of strings under the key "origin_trials.disabled_features".

BUG=589830,614454
==========

[chasej, 2016-06-07 15:51:31]

https://codereview.chromium.org/1741783002/diff/180001/chrome/browser/compone...
File chrome/browser/component_updater/origin_trials_component_installer.cc
(right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/browser/compone...
chrome/browser/component_updater/origin_trials_component_installer.cc:83:
ListPrefUpdate update(local_state, "origin-trials.disabled-features");
Use prefs::kOriginTrialDisabledFeatures constant?

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
File chrome/common/origin_trials/origin_trial_key_manager.cc (right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.cc:51:
disabled_features_.insert(feature);
Is this intended to append to the existing set of disabled features? From the
SetXXX naming, I would expect this method to replace any existing contents.

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
File chrome/common/origin_trials/origin_trial_key_manager.h (right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.h:18: class
OriginTrialKeyManager {
This class now does more than manage keys. I think a rename might be in order.

https://codereview.chromium.org/1741783002/diff/180001/content/public/common/...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1741783002/diff/180001/content/public/common/...
content/public/common/content_client.h:182: virtual bool
IsOriginTrialFeatureDisabled(base::StringPiece feature);
With additional methods, would it make sense to collect them into a single class
(e.g. OriginTrialPolicy)? Then, could just expose the class (similar to
GetMediaClientAndroid below). The base class could provide the default
implementation (rather than making content_client.cc do so), and be overridden
as necessary.

I'm not sure that's the best thing to do, just asking the question.

[Sorin Jianu, 2016-06-07 17:20:23]

sorin@chromium.org changed reviewers:
+ sorin@chromium.org

[Sorin Jianu, 2016-06-07 17:20:25]

Thank you Ian. A bunch of const-correctness nits and suggestions.

https://codereview.chromium.org/1741783002/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:833: std::string
override_disabled_features =
can be const.

https://codereview.chromium.org/1741783002/diff/180001/chrome/browser/compone...
File chrome/browser/component_updater/origin_trials_component_installer.cc
(right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/browser/compone...
chrome/browser/component_updater/origin_trials_component_installer.cc:80:
base::ListValue* override_disabled_feature_list;
please initialize.

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
File chrome/common/origin_trials/origin_trial_key_manager.cc (right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.cc:47:
std::vector<std::string> features =
can be const.

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.cc:50: for (std::string
feature : features)
maybe use ref to const or const auto& for the range loop.

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
File chrome/common/origin_trials/origin_trial_key_manager.h (right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.h:26: bool
IsFeatureDisabled(base::StringPiece feature);
Most likely, StringPiece can be forward declared.

https://codereview.chromium.org/1741783002/diff/180001/content/common/origin_...
File content/common/origin_trials/trial_token_validator.cc (right):

https://codereview.chromium.org/1741783002/diff/180001/content/common/origin_...
content/common/origin_trials/trial_token_validator.cc:18: ContentClient*
content_client = GetContentClient();
can this be const?

https://codereview.chromium.org/1741783002/diff/180001/content/public/common/...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1741783002/diff/180001/content/public/common/...
content/public/common/content_client.h:182: virtual bool
IsOriginTrialFeatureDisabled(base::StringPiece feature);
can the virtual function be declared const?

[iclelland, 2016-06-08 13:37:56]

https://codereview.chromium.org/1741783002/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:833: std::string
override_disabled_features =
On 2016/06/07 17:20:24, Sorin Jianu wrote:
> can be const.

Done.

https://codereview.chromium.org/1741783002/diff/180001/chrome/browser/compone...
File chrome/browser/component_updater/origin_trials_component_installer.cc
(right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/browser/compone...
chrome/browser/component_updater/origin_trials_component_installer.cc:80:
base::ListValue* override_disabled_feature_list;
On 2016/06/07 17:20:24, Sorin Jianu wrote:
> please initialize.

Good catch, thanks. Done.

https://codereview.chromium.org/1741783002/diff/180001/chrome/browser/compone...
chrome/browser/component_updater/origin_trials_component_installer.cc:83:
ListPrefUpdate update(local_state, "origin-trials.disabled-features");
On 2016/06/07 15:51:31, chasej wrote:
> Use prefs::kOriginTrialDisabledFeatures constant?

Done.

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
File chrome/common/origin_trials/origin_trial_key_manager.cc (right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.cc:47:
std::vector<std::string> features =
On 2016/06/07 17:20:24, Sorin Jianu wrote:
> can be const.

Done.

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.cc:50: for (std::string
feature : features)
On 2016/06/07 17:20:24, Sorin Jianu wrote:
> maybe use ref to const or const auto& for the range loop.

Done.

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.cc:51:
disabled_features_.insert(feature);
On 2016/06/07 15:51:31, chasej wrote:
> Is this intended to append to the existing set of disabled features? From the
> SetXXX naming, I would expect this method to replace any existing contents.

That's a good point. It should probably clear the original list, if any. I don't
see any reason to require it to retain that. Done.

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
File chrome/common/origin_trials/origin_trial_key_manager.h (right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.h:26: bool
IsFeatureDisabled(base::StringPiece feature);
On 2016/06/07 17:20:24, Sorin Jianu wrote:
> Most likely, StringPiece can be forward declared.

I don't think so -- it's a typedef to a template; we could forward declare the
BasicStringPiece template class here, I guess, and then redefine the typedef,
but I don't see that anywhere else in the codebase.

https://codereview.chromium.org/1741783002/diff/180001/content/common/origin_...
File content/common/origin_trials/trial_token_validator.cc (right):

https://codereview.chromium.org/1741783002/diff/180001/content/common/origin_...
content/common/origin_trials/trial_token_validator.cc:18: ContentClient*
content_client = GetContentClient();
On 2016/06/07 17:20:24, Sorin Jianu wrote:
> can this be const?

Certainly -- I was thinking at the time that I didn't want to have to constrain
every implementation by making IsOriginTrialFeatureDisabled a const method.
That's not a super compelling reason, though, and the methods in the key manager
were already const anyway. Done.

https://codereview.chromium.org/1741783002/diff/180001/content/public/common/...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1741783002/diff/180001/content/public/common/...
content/public/common/content_client.h:182: virtual bool
IsOriginTrialFeatureDisabled(base::StringPiece feature);
On 2016/06/07 17:20:24, Sorin Jianu wrote:
> can the virtual function be declared const?

Yes, and GetOriginTrialPublicKey above it.

[iclelland, 2016-06-08 15:38:52]

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
File chrome/common/origin_trials/origin_trial_key_manager.h (right):

https://codereview.chromium.org/1741783002/diff/180001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.h:18: class
OriginTrialKeyManager {
On 2016/06/07 15:51:31, chasej wrote:
> This class now does more than manage keys. I think a rename might be in order.

Renamed in refactor-CL https://codereview.chromium.org/2049783002

https://codereview.chromium.org/1741783002/diff/180001/content/public/common/...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1741783002/diff/180001/content/public/common/...
content/public/common/content_client.h:182: virtual bool
IsOriginTrialFeatureDisabled(base::StringPiece feature);
On 2016/06/07 15:51:31, chasej wrote:
> With additional methods, would it make sense to collect them into a single
class
> (e.g. OriginTrialPolicy)? Then, could just expose the class (similar to
> GetMediaClientAndroid below). The base class could provide the default
> implementation (rather than making content_client.cc do so), and be overridden
> as necessary.
> 
> I'm not sure that's the best thing to do, just asking the question.

I've made that refactor in a follow-up CL; see the result here:
https://codereview.chromium.org/2049783002

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

Some additional comments that I probably should have found before (apologies for
the extra round).

https://codereview.chromium.org/1741783002/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1741783002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:825: std::vector<std::string>
disabled_features;
Nit: Should these be moved inside the if (override_disabled_feature_list)?
Because they are outside the if statement, I was looking to see them used
further down.

https://codereview.chromium.org/1741783002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:836:
base::CommandLine::ForCurrentProcess();
This should use the command_line variable from the top of the method? Any reason
to redefine the variable here?

https://codereview.chromium.org/1741783002/diff/220001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1741783002/diff/220001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:769: // origin trials. Names should be
separated by "|" characters.
Nit: This is really a list of features already exposed in trials which are now
disabled. As I read it, "should not be included" implies features that are not
eligible for trials. The comment doesn't seem to really match the name of the
constant.

https://codereview.chromium.org/1741783002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1741783002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:83: //   8.
The feature requested is disabled
I'd argue that FeatureDisabled has the highest priority over any token failures
(especially since the implementation checks this first, and ignores the token).

Taking it one step further, maybe should optimize checkFeatureEnabled() to stop
checking tokens if this failure is encountered.

[iclelland, 2016-06-08 17:43:45]

https://codereview.chromium.org/1741783002/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1741783002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:825: std::vector<std::string>
disabled_features;
On 2016/06/08 16:14:24, chasej wrote:
> Nit: Should these be moved inside the if (override_disabled_feature_list)?
> Because they are outside the if statement, I was looking to see them used
> further down.

Done.

https://codereview.chromium.org/1741783002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:836:
base::CommandLine::ForCurrentProcess();
On 2016/06/08 16:14:24, chasej wrote:
> This should use the command_line variable from the top of the method? Any
reason
> to redefine the variable here?

Done.

https://codereview.chromium.org/1741783002/diff/220001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1741783002/diff/220001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:769: // origin trials. Names should be
separated by "|" characters.
On 2016/06/08 16:14:24, chasej wrote:
> Nit: This is really a list of features already exposed in trials which are now
> disabled. As I read it, "should not be included" implies features that are not
> eligible for trials. The comment doesn't seem to really match the name of the
> constant.

Changed the wording to make it clear that we're disabling the origin trial
experiments, not the features.

https://codereview.chromium.org/1741783002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1741783002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:83: //   8.
The feature requested is disabled
On 2016/06/08 16:14:24, chasej wrote:
> I'd argue that FeatureDisabled has the highest priority over any token
failures
> (especially since the implementation checks this first, and ignores the
token).

That's not what this priority sequence represents, though, is it? No Tokens is
checked first, technically, and Expired is checked very late. The fact that a
token made it almost all the way to the end of the validation sequence means
that a failure at that point is the highest priority in terms of choosing a
message for the user.

In practice, though, it doesn't matter -- FeatureDisabled is never going to be
compared to any of the other validation failures.

> Taking it one step further, maybe should optimize checkFeatureEnabled() to
stop
> checking tokens if this failure is encountered.

The optimization is a good idea, since it's just a waste of time checking
further if the policy said to disable the feature.
Given that, I've been able to just remove FeatureDisabled from the priority list
above.

[chasej, 2016-06-08 19:43:20]

lgtm

[waffles, 2016-06-09 15:17:29]

lgtm

[iclelland, 2016-06-09 16:21:58]

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

[iclelland, 2016-06-09 16:21:59]

Thanks!

jochen, can you PTAL? Thanks!

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

can you rebase that on top of the origin trial policy CL?

[iclelland, 2016-06-15 07:50:11]

The dependency is in the other direction right now; if I pull out the
disabled-feature-list from the OriginTrialPolicy CL, then I can do that (and add
it back in this one)

[iclelland, 2016-06-15 09:48:34]

Rebased this on top of https://codereview.chromium.org/2047683002/, which now
depends on the OriginTrialPolicy CL
(https://codereview.chromium.org/2049783002/)

This code is now identical to the code in
https://codereview.chromium.org/2049783002/#ps40001 (with the unneeded
constructor removed as per comment) -- that is, the result of rebasing all three
CLs against each other in a different order is that no code is changed in the
end.

[iclelland, 2016-06-22 15:48:17]

On 2016/06/15 09:48:34, iclelland wrote:
> Rebased this on top of https://codereview.chromium.org/2047683002/, which now
> depends on the OriginTrialPolicy CL
> (https://codereview.chromium.org/2049783002/)
> 
> This code is now identical to the code in
> https://codereview.chromium.org/2049783002/#ps40001 (with the unneeded
> constructor removed as per comment) -- that is, the result of rebasing all
three
> CLs against each other in a different order is that no code is changed in the
> end.

Rebased again; jochen, can you PTAL? Thanks (last one today, I promise :) )

[jochen (gone - plz use gerrit), 2016-06-22 15:53:23]

lgtm

[Sorin Jianu, 2016-06-22 16:30:58]

lgtm

Thank you!

[iclelland, 2016-06-22 16:55:16]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-06-22 16:55:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
waffles@chromium.org, chasej@chromium.org
Link to the patchset: https://codereview.chromium.org/1741783002/#ps280001
(title: "Rebase")

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

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

[commit-bot: I haz the power, 2016-06-22 17:07:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-22 17:07:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[iclelland, 2016-06-22 17:16:10]

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

[iclelland, 2016-06-22 17:16:11]

+r rkaplow -- Can you PTAL at histograms.xml? (The corresponding enum is in
public/platform/WebOriginTrialTokenStatus.h)

Thanks!

[rkaplow, 2016-06-23 04:15:36]

lgtm

[iclelland, 2016-06-23 11:59:52]

The CQ bit was checked by iclelland@chromium.org

[commit-bot: I haz the power, 2016-06-23 12:00:08]

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

[commit-bot: I haz the power, 2016-06-23 12:04:12]

Description was changed from

==========
Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding component manifest key, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

The manifest data from the component will be persisted in browser local state,
as a list of strings under the key "origin_trials.disabled_features".

BUG=589830,614454
==========

to

==========
Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding component manifest key, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

The manifest data from the component will be persisted in browser local state,
as a list of strings under the key "origin_trials.disabled_features".

BUG=589830,614454
==========

[commit-bot: I haz the power, 2016-06-23 12:04:14]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-06-23 12:05:27]

Description was changed from

==========
Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding component manifest key, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

The manifest data from the component will be persisted in browser local state,
as a list of strings under the key "origin_trials.disabled_features".

BUG=589830,614454
==========

to

==========
Add disabled origin trial feature list

Adds both a command line option, "--origin-trial-disabled-features", and a
corresponding component manifest key, to exclude features from origin trials.
Disabling features in this manner will not stop them from being enabled via the
command line, or chrome://flags, but will stop them from being enabled by
remote origins through origin trials.

The manifest data from the component will be persisted in browser local state,
as a list of strings under the key "origin_trials.disabled_features".

BUG=589830,614454

Committed: https://crrev.com/79237e966353560115a92eb802aa7da459cdaf9f
Cr-Commit-Position: refs/heads/master@{#401583}
==========

[commit-bot: I haz the power, 2016-06-23 12:05:29]

Patchset 15 (id:??) landed as
https://crrev.com/79237e966353560115a92eb802aa7da459cdaf9f
Cr-Commit-Position: refs/heads/master@{#401583}