Rename Experimental Framework classes and concepts

Rename Experimental Framework classes and concepts

"Experiments" is far too generic a term to use for the trials enabled by the experimental framework, so this patch renames everything to (hopefully) more precise terms.

These changes are most prominent:
  core::ExperimentalFeatures class => OriginTrials
  core::Experiments class => OriginTrialContext
  API Key => Trial Token (or "Token" where unambiguous)
  API => Feature
  /experiments/ directories => /origin_trials/
  api_name (RuntimeEnabledFeatures) => origin_trial_feature_name

BUG=575808
Committed: https://crrev.com/5b27fa016f2736e2186984e5f737e805c2836d9c
Cr-Commit-Position: refs/heads/master@{#372943}

[iclelland, 2016-01-26 03:47:12]

Description was changed from

==========
Rename Experimental Framework classes and concepts

BUG=575808
==========

to

==========
Rename Experimental Framework classes and concepts

"Experiments" is far too generic a term to use for the trials enabled by the
experimental framework, so this patch renames everything to (hopefully) more
precise terms.

These changes are most prominent:
  core::ExperimentalFeatures class => OriginTrials
  core::Experiments class => OriginTrialContext
  API Key => Trial Token (or "Token" where unambiguous)
  API => Feature
  /experiments/ directories => /origin_trials/
  api_name (RuntimeEnabledFeatures) => experimental_feature_name

BUG=575808
==========

[iclelland, 2016-01-26 03:47:12]

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

[iclelland, 2016-01-26 03:47:43]

+r chasej -- can you PTAL? Thanks

[chasej, 2016-01-26 16:56:53]

I've raised an issue around naming for "experimental_feature_name" in
RuntimeEnabledFeatures.in.

Otherwise, most of the comments are about cleaning up little things missed in
the giant rename effort.

https://codereview.chromium.org/1635593004/diff/20001/content/common/origin_t...
File content/common/origin_trials/trial_token.h (right):

https://codereview.chromium.org/1635593004/diff/20001/content/common/origin_t...
content/common/origin_trials/trial_token.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
Should this be 2016? Are file renames considered new files for the copyright?

https://codereview.chromium.org/1635593004/diff/20001/content/common/origin_t...
content/common/origin_trials/trial_token.h:24: // code does not maintain an enum
or constant list for experiment names.
Use "feature name", instead of "experiment name".

https://codereview.chromium.org/1635593004/diff/20001/content/common/origin_t...
File content/common/origin_trials/trial_token_unittest.cc (right):

https://codereview.chromium.org/1635593004/diff/20001/content/common/origin_t...
content/common/origin_trials/trial_token_unittest.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
Should be 2016?

https://codereview.chromium.org/1635593004/diff/20001/content/common/origin_t...
content/common/origin_trials/trial_token_unittest.cc:131: TEST_F(TrialTokenTest,
ParseValidKeyString) {
This test should probably be named ParseValidTokenString.

Similarly for the following tests with "Key" in the name.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_runtime_features.py (right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_runtime_features.py:46:
'experimental_feature_name': None,
Subject to change, based on comment on naming for RuntimeEnabledFeatures.in.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl
(right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl:11: {%
if feature.experimental_feature_name %}
Subject to change, based on comment on naming for RuntimeEnabledFeatures.in. 
Applies to any other uses in this template.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl
(right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl:20: {% if
feature.experimental_feature_name %}
Subject to change, based on comment on naming for RuntimeEnabledFeatures.in.
Applies to any other uses in this template.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:30: return
"The '" + featureName + "' API is currently enabled in limited experiments.
Please see [Chrome experiments website URL] for information on enabling this
experiment on your site.";
The message should refer to features and trials.  Something like:
"The '" + featureName + "' feature is currently enabled in limited trials.
Please see [Phosphor console URL] for information on enabling a trial for your
site.";

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:35: bool
foundAnyKey = false;
Should be foundAnyToken.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:55:
*errorMessage = "The provided token(s) are not valid for the '" + featureName +
"' API.";
The error message should say "feature", instead of "API".

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:102:
DOMException* OriginTrialContext::createApiDisabledException(const String&
featureName)
To be renamed or deleted, as per my comment in the header file.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:17: // The
Experimental Framework (EF) provides limited access to experimental,
Nit, I think the comma after experimental should be removed.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:27: // feature
names. Instead, the EF validates the name provided by the API
Should be "feature", instead of "API".

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:39: //
external developers why the API is disabled and how to join origin
Should be "feature", instead of "API".

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:41: static
DOMException* createApiDisabledException(const String& apiName);
Should be renamed to createFeatureDisabledException(featureName).

By the way, I'm not convinced this method is still needed. I implemented it
originally, before the isXEnabled method was changed to return an error message.
I believe it's only used by tests, so it would be reasonable to remove it until
we know it's needed.  I'm fine to defer a final decision, and keep this as a
rename-only CL.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp
(right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:121:
void addTrialToken(const String& keyValue)
Should be tokenValue.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:160:
EXPECT_EQ(("The provided token(s) are not valid for the 'This feature does not
exist' API."), errorMessage);
Error message should have "feature", not "API".

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:188:
TEST_F(OriginTrialContextTest, InvalidKeyResponseFromPlatform)
Should be InvalidTokenResponseFromPlatform.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:197:
EXPECT_EQ(("The provided token(s) are not valid for the 'Frobulate' API."),
errorMessage);
Error message should have "feature", not "API".

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:23: //
experimental_feature_name=FEATURE_NAME is used to integrate the feature with the
experimental framework, so that the
Earlier, I liked the "experimental_feature_name" for use as the label.  However,
after seeing it in use in this file, I think a different name might be needed. 
I think there could be confusion with "status=experimental". Often, there will
be a number of entries here with "status=experimental", but without integration
with the experimental framework (e.g. features in early development).  So, there
could be a mix of entries with status=experimental with, or without,
"experimental_feature_name=XXX".  Even with the explanatory comments, it could
appear random to developers as to which features should have the additional
configuration.

Perhaps we should use something like "origin_trial_feature_name".  That suggests
that there are origin trials associated with the feature, and the specified
feature name is used in those trials.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebTrialTokenValidator.h (right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebTrialTokenValidator.h:16: // document's
head. The details of determining whether a token is valid for a
I'd suggest a more generic comment about where tokens come from.  It's unlikely
anyone would remember to update this comment if/when support is added for HTTP
headers, etc.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebTrialTokenValidator.h:17: // particular
feature is left up to the embedder. An implementation can
Given that the token validation happens in the renderer now, is this comment
accurate about the embedder making the determination?

[iclelland, 2016-01-26 19:05:49]

Thanks for the thorough review! I've addressed most of the issues -- everything
except for RuntimeFeatures, I believe.

https://codereview.chromium.org/1635593004/diff/20001/content/common/origin_t...
File content/common/origin_trials/trial_token.h (right):

https://codereview.chromium.org/1635593004/diff/20001/content/common/origin_t...
content/common/origin_trials/trial_token.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/01/26 16:56:52, chasej wrote:
> Should this be 2016? Are file renames considered new files for the copyright?

Renames? I don't know what the policy is there. I suppose it's *technically* a
new file, according to git...

https://codereview.chromium.org/1635593004/diff/20001/content/common/origin_t...
content/common/origin_trials/trial_token.h:24: // code does not maintain an enum
or constant list for experiment names.
On 2016/01/26 16:56:52, chasej wrote:
> Use "feature name", instead of "experiment name".

Done.

https://codereview.chromium.org/1635593004/diff/20001/content/common/origin_t...
File content/common/origin_trials/trial_token_unittest.cc (right):

https://codereview.chromium.org/1635593004/diff/20001/content/common/origin_t...
content/common/origin_trials/trial_token_unittest.cc:131: TEST_F(TrialTokenTest,
ParseValidKeyString) {
On 2016/01/26 16:56:52, chasej wrote:
> This test should probably be named ParseValidTokenString.
> 
> Similarly for the following tests with "Key" in the name.

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_runtime_features.py (right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_runtime_features.py:46:
'experimental_feature_name': None,
On 2016/01/26 16:56:52, chasej wrote:
> Subject to change, based on comment on naming for RuntimeEnabledFeatures.in.

Acknowledged.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl
(right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl:11: {%
if feature.experimental_feature_name %}
On 2016/01/26 16:56:52, chasej wrote:
> Subject to change, based on comment on naming for RuntimeEnabledFeatures.in. 
> Applies to any other uses in this template.

Acknowledged.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl
(right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl:20: {% if
feature.experimental_feature_name %}
On 2016/01/26 16:56:52, chasej wrote:
> Subject to change, based on comment on naming for RuntimeEnabledFeatures.in.
> Applies to any other uses in this template.

Acknowledged.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:30: return
"The '" + featureName + "' API is currently enabled in limited experiments.
Please see [Chrome experiments website URL] for information on enabling this
experiment on your site.";
On 2016/01/26 16:56:52, chasej wrote:
> The message should refer to features and trials.  Something like:
> "The '" + featureName + "' feature is currently enabled in limited trials.
> Please see [Phosphor console URL] for information on enabling a trial for your
> site.";

I like it -- done, thanks!

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:35: bool
foundAnyKey = false;
On 2016/01/26 16:56:52, chasej wrote:
> Should be foundAnyToken.

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:55:
*errorMessage = "The provided token(s) are not valid for the '" + featureName +
"' API.";
On 2016/01/26 16:56:53, chasej wrote:
> The error message should say "feature", instead of "API".

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:102:
DOMException* OriginTrialContext::createApiDisabledException(const String&
featureName)
On 2016/01/26 16:56:52, chasej wrote:
> To be renamed or deleted, as per my comment in the header file.

Deleted.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:17: // The
Experimental Framework (EF) provides limited access to experimental,
On 2016/01/26 16:56:53, chasej wrote:
> Nit, I think the comma after experimental should be removed.

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:27: // feature
names. Instead, the EF validates the name provided by the API
On 2016/01/26 16:56:53, chasej wrote:
> Should be "feature", instead of "API".

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:41: static
DOMException* createApiDisabledException(const String& apiName);
On 2016/01/26 16:56:53, chasej wrote:
> Should be renamed to createFeatureDisabledException(featureName).
> 
> By the way, I'm not convinced this method is still needed. I implemented it
> originally, before the isXEnabled method was changed to return an error
message.
> I believe it's only used by tests, so it would be reasonable to remove it
until
> we know it's needed.  I'm fine to defer a final decision, and keep this as a
> rename-only CL.  

Happy to delete it, and avoid creating an awkward name that we don't really
need.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp
(right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:121:
void addTrialToken(const String& keyValue)
On 2016/01/26 16:56:53, chasej wrote:
> Should be tokenValue.

Renamed to 'token' for consistency with the method below.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:160:
EXPECT_EQ(("The provided token(s) are not valid for the 'This feature does not
exist' API."), errorMessage);
On 2016/01/26 16:56:53, chasej wrote:
> Error message should have "feature", not "API".

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:188:
TEST_F(OriginTrialContextTest, InvalidKeyResponseFromPlatform)
On 2016/01/26 16:56:53, chasej wrote:
> Should be InvalidTokenResponseFromPlatform.

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:197:
EXPECT_EQ(("The provided token(s) are not valid for the 'Frobulate' API."),
errorMessage);
On 2016/01/26 16:56:53, chasej wrote:
> Error message should have "feature", not "API".

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:23: //
experimental_feature_name=FEATURE_NAME is used to integrate the feature with the
experimental framework, so that the
On 2016/01/26 16:56:53, chasej wrote:
> Earlier, I liked the "experimental_feature_name" for use as the label. 
However,
> after seeing it in use in this file, I think a different name might be needed.

> I think there could be confusion with "status=experimental". Often, there will
> be a number of entries here with "status=experimental", but without
integration
> with the experimental framework (e.g. features in early development).  So,
there
> could be a mix of entries with status=experimental with, or without,
> "experimental_feature_name=XXX".  Even with the explanatory comments, it could
> appear random to developers as to which features should have the additional
> configuration.
> 
> Perhaps we should use something like "origin_trial_feature_name".  That
suggests
> that there are origin trials associated with the feature, and the specified
> feature name is used in those trials.

I like that name -- I used 'experimental_feature_name' because it was in the
bug, and also suggested in
https://codereview.chromium.org/1541983003/patch/80001/90008.
Do we need more discussion on this?

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebTrialTokenValidator.h (right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebTrialTokenValidator.h:16: // document's
head. The details of determining whether a token is valid for a
On 2016/01/26 16:56:53, chasej wrote:
> I'd suggest a more generic comment about where tokens come from.  It's
unlikely
> anyone would remember to update this comment if/when support is added for HTTP
> headers, etc.

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebTrialTokenValidator.h:17: // particular
feature is left up to the embedder. An implementation can
On 2016/01/26 16:56:53, chasej wrote:
> Given that the token validation happens in the renderer now, is this comment
> accurate about the embedder making the determination?

... it's unfortunate, perhaps, but true for the time being. The validation
happens in the content layer, outside of blink code, which was the reason I left
this in. On the other hand, it was mentioned previously that Blink isn't the
embeddable interface, content is. I'll remove the comment so it's not confusing
to anyone looking here.

[iclelland, 2016-01-27 17:30:30]

Description was changed from

==========
Rename Experimental Framework classes and concepts

"Experiments" is far too generic a term to use for the trials enabled by the
experimental framework, so this patch renames everything to (hopefully) more
precise terms.

These changes are most prominent:
  core::ExperimentalFeatures class => OriginTrials
  core::Experiments class => OriginTrialContext
  API Key => Trial Token (or "Token" where unambiguous)
  API => Feature
  /experiments/ directories => /origin_trials/
  api_name (RuntimeEnabledFeatures) => experimental_feature_name

BUG=575808
==========

to

==========
Rename Experimental Framework classes and concepts

"Experiments" is far too generic a term to use for the trials enabled by the
experimental framework, so this patch renames everything to (hopefully) more
precise terms.

These changes are most prominent:
  core::ExperimentalFeatures class => OriginTrials
  core::Experiments class => OriginTrialContext
  API Key => Trial Token (or "Token" where unambiguous)
  API => Feature
  /experiments/ directories => /origin_trials/
  api_name (RuntimeEnabledFeatures) => origin_trial_feature_name

BUG=575808
==========

[iclelland, 2016-01-27 18:56:46]

RuntimeEnabledFeatures flag updated.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_runtime_features.py (right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_runtime_features.py:46:
'experimental_feature_name': None,
On 2016/01/26 19:05:48, iclelland wrote:
> On 2016/01/26 16:56:52, chasej wrote:
> > Subject to change, based on comment on naming for RuntimeEnabledFeatures.in.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl
(right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl:11: {%
if feature.experimental_feature_name %}
On 2016/01/26 19:05:48, iclelland wrote:
> On 2016/01/26 16:56:52, chasej wrote:
> > Subject to change, based on comment on naming for RuntimeEnabledFeatures.in.

> > Applies to any other uses in this template.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl
(right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl:20: {% if
feature.experimental_feature_name %}
On 2016/01/26 19:05:48, iclelland wrote:
> On 2016/01/26 16:56:52, chasej wrote:
> > Subject to change, based on comment on naming for RuntimeEnabledFeatures.in.
> > Applies to any other uses in this template.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1635593004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:23: //
experimental_feature_name=FEATURE_NAME is used to integrate the feature with the
experimental framework, so that the
On 2016/01/26 19:05:49, iclelland wrote:
> On 2016/01/26 16:56:53, chasej wrote:
> > Earlier, I liked the "experimental_feature_name" for use as the label. 
> However,
> > after seeing it in use in this file, I think a different name might be
needed.
> 
> > I think there could be confusion with "status=experimental". Often, there
will
> > be a number of entries here with "status=experimental", but without
> integration
> > with the experimental framework (e.g. features in early development).  So,
> there
> > could be a mix of entries with status=experimental with, or without,
> > "experimental_feature_name=XXX".  Even with the explanatory comments, it
could
> > appear random to developers as to which features should have the additional
> > configuration.
> > 
> > Perhaps we should use something like "origin_trial_feature_name".  That
> suggests
> > that there are origin trials associated with the feature, and the specified
> > feature name is used in those trials.
> 
> I like that name -- I used 'experimental_feature_name' because it was in the
> bug, and also suggested in
> https://codereview.chromium.org/1541983003/patch/80001/90008.
> Do we need more discussion on this?

Done now. Renamed to origin_trial_feature_name in PS#4

[chasej, 2016-01-27 19:31:28]

lgtm

[iclelland, 2016-01-27 19:31:54]

iclelland@chromium.org changed reviewers:
+ davidben@chromium.org, rbyers@chromium.org

[iclelland, 2016-01-27 19:33:06]

+r rbyers for /third_party/WebKit/,
+r davidben for /content/

Sorry it's so long, guys; it's mostly a mechanical rename, though.

Thanks

[davidben, 2016-01-27 19:38:33]

content lgtm

https://codereview.chromium.org/1635593004/diff/80001/content/child/origin_tr...
File content/child/origin_trials/trial_token_validator.cc (right):

https://codereview.chromium.org/1635593004/diff/80001/content/child/origin_tr...
content/child/origin_trials/trial_token_validator.cc:24: return result;
Optional nit: I would probably have written this as

  scoped_ptr<TrialToken> trialToken = TrialToken::Parse(token.utf8());
  return trialToken &&
         trialToken->IsAppropriate(origin.utf8(), featureName.utf8()) &&
         trialToken->IsValid(base::Time::Now());

Doesn't actually matter. It's just slightly shorter.

https://codereview.chromium.org/1635593004/diff/80001/content/common/origin_t...
File content/common/origin_trials/trial_token.h (right):

https://codereview.chromium.org/1635593004/diff/80001/content/common/origin_t...
content/common/origin_trials/trial_token.h:18: // The Experimental Framework
(EF) provides limited access to experimental
Is this still called "Experimental Framework"? (Origin Trial Framework? I guess
OTF is a kind of font...)

[iclelland, 2016-01-27 19:47:12]

On 2016/01/27 19:38:33, davidben (slow) wrote:
> content lgtm
> 
>
https://codereview.chromium.org/1635593004/diff/80001/content/child/origin_tr...
> File content/child/origin_trials/trial_token_validator.cc (right):
> 
>
https://codereview.chromium.org/1635593004/diff/80001/content/child/origin_tr...
> content/child/origin_trials/trial_token_validator.cc:24: return result;
> Optional nit: I would probably have written this as
> 
>   scoped_ptr<TrialToken> trialToken = TrialToken::Parse(token.utf8());
>   return trialToken &&
>          trialToken->IsAppropriate(origin.utf8(), featureName.utf8()) &&
>          trialToken->IsValid(base::Time::Now());
> 
> Doesn't actually matter. It's just slightly shorter.
> 
>
https://codereview.chromium.org/1635593004/diff/80001/content/common/origin_t...
> File content/common/origin_trials/trial_token.h (right):
> 
>
https://codereview.chromium.org/1635593004/diff/80001/content/common/origin_t...
> content/common/origin_trials/trial_token.h:18: // The Experimental Framework
> (EF) provides limited access to experimental
> Is this still called "Experimental Framework"? (Origin Trial Framework? I
guess
> OTF is a kind of font...)

Yeah, until we come up with a better name, "Experimental Framework" is still the
name for the whole project (I guess that includes all this code, as well as
stuff like the developer console, and the feedback process)

[haraken, 2016-01-27 23:55:24]

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

[haraken, 2016-01-27 23:55:25]

https://codereview.chromium.org/1635593004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/1635593004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:97: if
'APIExperimentEnabled' in extended_attributes:

You need to rename [APIExperimentEnabled] as well.

https://codereview.chromium.org/1635593004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:108:
'api_experiment_name': extended_attributes.get('APIExperimentEnabled'),  #
[APIExperimentEnabled]

And these "*_experiment_*" family too.

[iclelland, 2016-01-28 05:06:44]

On 2016/01/27 23:55:25, haraken wrote:
>
https://codereview.chromium.org/1635593004/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):
> 
>
https://codereview.chromium.org/1635593004/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/scripts/v8_attributes.py:97: if
> 'APIExperimentEnabled' in extended_attributes:
> 
> You need to rename [APIExperimentEnabled] as well.
> 
>
https://codereview.chromium.org/1635593004/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/scripts/v8_attributes.py:108:
> 'api_experiment_name': extended_attributes.get('APIExperimentEnabled'),  #
> [APIExperimentEnabled]
> 
> And these "*_experiment_*" family too.

We *do* need to rename that, I think -- there hasn't been any discussion on what
the right name for the IDL attribute should be; I'll ask that on the bug. I
think we can make that a separate CL; there's a clean separation between the IDL
attribute name and the rest of the code here.

[haraken, 2016-01-28 05:12:55]

On 2016/01/28 05:06:44, iclelland wrote:
> On 2016/01/27 23:55:25, haraken wrote:
> >
>
https://codereview.chromium.org/1635593004/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):
> > 
> >
>
https://codereview.chromium.org/1635593004/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/scripts/v8_attributes.py:97: if
> > 'APIExperimentEnabled' in extended_attributes:
> > 
> > You need to rename [APIExperimentEnabled] as well.
> > 
> >
>
https://codereview.chromium.org/1635593004/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/scripts/v8_attributes.py:108:
> > 'api_experiment_name': extended_attributes.get('APIExperimentEnabled'),  #
> > [APIExperimentEnabled]
> > 
> > And these "*_experiment_*" family too.
> 
> We *do* need to rename that, I think -- there hasn't been any discussion on
what
> the right name for the IDL attribute should be; I'll ask that on the bug. I
> think we can make that a separate CL; there's a clean separation between the
IDL
> attribute name and the rest of the code here.

A separate CL is fine.

WebKit/Source/ LGTM.

[iclelland, 2016-01-28 12:18:12]

Thanks, haraken!

rbyers -- can you just take a look at WebKit/public/platform? Thanks

https://codereview.chromium.org/1635593004/diff/80001/content/child/origin_tr...
File content/child/origin_trials/trial_token_validator.cc (right):

https://codereview.chromium.org/1635593004/diff/80001/content/child/origin_tr...
content/child/origin_trials/trial_token_validator.cc:24: return result;
On 2016/01/27 19:38:33, davidben (slow) wrote:
> Optional nit: I would probably have written this as
> 
>   scoped_ptr<TrialToken> trialToken = TrialToken::Parse(token.utf8());
>   return trialToken &&
>          trialToken->IsAppropriate(origin.utf8(), featureName.utf8()) &&
>          trialToken->IsValid(base::Time::Now());
> 
> Doesn't actually matter. It's just slightly shorter.

Thanks, I'll do that; I like the look of it.

[Rick Byers, 2016-02-02 04:50:12]

Sorry for the delay - I'm travelling and forgot to update my code review name.

WebKit/public LGTM

https://codereview.chromium.org/1635593004/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebTrialTokenValidator.h (right):

https://codereview.chromium.org/1635593004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebTrialTokenValidator.h:18: // More
documentation on the design of the experimental framework is at
nit: did you want to update the use of "experimental framework" in this comment
as well?

[iclelland, 2016-02-02 14:12:53]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-02-02 14:12:54]

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

[commit-bot: I haz the power, 2016-02-02 14:13:29]

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

[commit-bot: I haz the power, 2016-02-02 15:00:49]

Description was changed from

==========
Rename Experimental Framework classes and concepts

"Experiments" is far too generic a term to use for the trials enabled by the
experimental framework, so this patch renames everything to (hopefully) more
precise terms.

These changes are most prominent:
  core::ExperimentalFeatures class => OriginTrials
  core::Experiments class => OriginTrialContext
  API Key => Trial Token (or "Token" where unambiguous)
  API => Feature
  /experiments/ directories => /origin_trials/
  api_name (RuntimeEnabledFeatures) => origin_trial_feature_name

BUG=575808
==========

to

==========
Rename Experimental Framework classes and concepts

"Experiments" is far too generic a term to use for the trials enabled by the
experimental framework, so this patch renames everything to (hopefully) more
precise terms.

These changes are most prominent:
  core::ExperimentalFeatures class => OriginTrials
  core::Experiments class => OriginTrialContext
  API Key => Trial Token (or "Token" where unambiguous)
  API => Feature
  /experiments/ directories => /origin_trials/
  api_name (RuntimeEnabledFeatures) => origin_trial_feature_name

BUG=575808
==========

[commit-bot: I haz the power, 2016-02-02 15:00:50]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-02 15:02:08]

Description was changed from

==========
Rename Experimental Framework classes and concepts

"Experiments" is far too generic a term to use for the trials enabled by the
experimental framework, so this patch renames everything to (hopefully) more
precise terms.

These changes are most prominent:
  core::ExperimentalFeatures class => OriginTrials
  core::Experiments class => OriginTrialContext
  API Key => Trial Token (or "Token" where unambiguous)
  API => Feature
  /experiments/ directories => /origin_trials/
  api_name (RuntimeEnabledFeatures) => origin_trial_feature_name

BUG=575808
==========

to

==========
Rename Experimental Framework classes and concepts

"Experiments" is far too generic a term to use for the trials enabled by the
experimental framework, so this patch renames everything to (hopefully) more
precise terms.

These changes are most prominent:
  core::ExperimentalFeatures class => OriginTrials
  core::Experiments class => OriginTrialContext
  API Key => Trial Token (or "Token" where unambiguous)
  API => Feature
  /experiments/ directories => /origin_trials/
  api_name (RuntimeEnabledFeatures) => origin_trial_feature_name

BUG=575808

Committed: https://crrev.com/5b27fa016f2736e2186984e5f737e805c2836d9c
Cr-Commit-Position: refs/heads/master@{#372943}
==========

[commit-bot: I haz the power, 2016-02-02 15:02:09]

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