Rename flags for Origin Trials framework

Rename flags for Origin Trials framework

The Blink runtime flags and browser feature are still using the
"Experimental Framework" naming. This CL renames those artifacts to use
the origin trials naming.

BUG=610733
Committed: https://crrev.com/d55c6f409a41a26eac7a2bd1bb0ed5bfc4026d71
Cr-Commit-Position: refs/heads/master@{#393909}

[chasej, 2016-05-11 15:16:11]

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

[chasej, 2016-05-11 15:16:12]

iclelland, pfeldman please take a look.

[iclelland, 2016-05-11 16:23:44]

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp
(left):

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:80: :
m_frameworkWasEnabled(RuntimeEnabledFeatures::experimentalFrameworkEnabled())
This is more than a rename -- is it also unnecessary now to track the state of
the flag during test runs?

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebRuntimeFeatures.h:67: BLINK_EXPORT static void
enableOriginTrials(bool);
Just curious -- why are these declarations in a different order than the
implementation? You moved them in the .cpp file to be still roughly
alphabetical, but not here.

[chasej, 2016-05-11 19:05:39]

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp
(left):

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:80: :
m_frameworkWasEnabled(RuntimeEnabledFeatures::experimentalFrameworkEnabled())
On 2016/05/11 16:23:44, iclelland wrote:
> This is more than a rename -- is it also unnecessary now to track the state of
> the flag during test runs?

It might be premature to remove this now, but the flag is set to stable. Also, I
can't find it now, but I seem to recall someone implementing support to
automatically reset flags for tests.

Anyway, as the trybots are green, I thought it made sense to remove the manual
tracking, vs rename. I can certainly defer to a later cleanup CL if desired.

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebRuntimeFeatures.h:67: BLINK_EXPORT static void
enableOriginTrials(bool);
On 2016/05/11 16:23:44, iclelland wrote:
> Just curious -- why are these declarations in a different order than the
> implementation? You moved them in the .cpp file to be still roughly
> alphabetical, but not here.

I was trying to match the existing order in both files. The source file isn't
following the convention to match the declaration order. So, the order is
different in each file.

As far as I can tell, it looks like the header file groups flags that have a
getter (or other custom accessor) separately from those that only have a setter.
Within those groups, the order looks alphabetical. Based on that order (group,
then alphabetical), the renamed methods are actually (still) in the right place.


On the other hand, the source file appears to be mostly ordered alphabetically,
by the name of the setter. So, I moved the methods to what seemed like the right
place.

[iclelland, 2016-05-11 19:14:50]

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp
(left):

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:80: :
m_frameworkWasEnabled(RuntimeEnabledFeatures::experimentalFrameworkEnabled())
On 2016/05/11 19:05:39, chasej wrote:
> On 2016/05/11 16:23:44, iclelland wrote:
> > This is more than a rename -- is it also unnecessary now to track the state
of
> > the flag during test runs?
> 
> It might be premature to remove this now, but the flag is set to stable. Also,
I
> can't find it now, but I seem to recall someone implementing support to
> automatically reset flags for tests.
> 
> Anyway, as the trybots are green, I thought it made sense to remove the manual
> tracking, vs rename. I can certainly defer to a later cleanup CL if desired.

I don't think it's premature -- I'd quickly LGTM both CLs if it were split up.
As it is, this is a strange CL to find this change in. It looks like it could
have been an accident. You could update the commit message to include both
changes, but I think it's cleaner to just split it. (Commits are pretty cheap)

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebRuntimeFeatures.h:67: BLINK_EXPORT static void
enableOriginTrials(bool);
On 2016/05/11 19:05:39, chasej wrote:
> On 2016/05/11 16:23:44, iclelland wrote:
> > Just curious -- why are these declarations in a different order than the
> > implementation? You moved them in the .cpp file to be still roughly
> > alphabetical, but not here.
> 
> I was trying to match the existing order in both files. The source file isn't
> following the convention to match the declaration order. So, the order is
> different in each file.
> 
> As far as I can tell, it looks like the header file groups flags that have a
> getter (or other custom accessor) separately from those that only have a
setter.
> Within those groups, the order looks alphabetical. Based on that order (group,
> then alphabetical), the renamed methods are actually (still) in the right
place.
> 
> 
> On the other hand, the source file appears to be mostly ordered
alphabetically,
> by the name of the setter. So, I moved the methods to what seemed like the
right
> place.

Acknowledged. I'm satisfied with this :)

[chasej, 2016-05-11 19:33:13]

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp
(left):

https://codereview.chromium.org/1968653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:80: :
m_frameworkWasEnabled(RuntimeEnabledFeatures::experimentalFrameworkEnabled())
On 2016/05/11 19:14:50, iclelland wrote:
> On 2016/05/11 19:05:39, chasej wrote:
> > On 2016/05/11 16:23:44, iclelland wrote:
> > > This is more than a rename -- is it also unnecessary now to track the
state
> of
> > > the flag during test runs?
> > 
> > It might be premature to remove this now, but the flag is set to stable.
Also,
> I
> > can't find it now, but I seem to recall someone implementing support to
> > automatically reset flags for tests.
> > 
> > Anyway, as the trybots are green, I thought it made sense to remove the
manual
> > tracking, vs rename. I can certainly defer to a later cleanup CL if desired.
> 
> I don't think it's premature -- I'd quickly LGTM both CLs if it were split up.
> As it is, this is a strange CL to find this change in. It looks like it could
> have been an accident. You could update the commit message to include both
> changes, but I think it's cleaner to just split it. (Commits are pretty cheap)

Changed to rename, instead of cleanup.

[iclelland, 2016-05-11 19:34:47]

lgtm

[chasej, 2016-05-16 18:16:34]

pfeldman, please take a look.

[pfeldman, 2016-05-16 18:18:21]

lgtm

[chasej, 2016-05-16 18:20:39]

The CQ bit was checked by chasej@chromium.org

[commit-bot: I haz the power, 2016-05-16 18:21:20]

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

[commit-bot: I haz the power, 2016-05-16 20:59:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-16 21:01:09]

Description was changed from

==========
Rename flags for Origin Trials framework

The Blink runtime flags and browser feature are still using the
"Experimental Framework" naming. This CL renames those artifacts to use
the origin trials naming.

BUG=610733
==========

to

==========
Rename flags for Origin Trials framework

The Blink runtime flags and browser feature are still using the
"Experimental Framework" naming. This CL renames those artifacts to use
the origin trials naming.

BUG=610733

Committed: https://crrev.com/d55c6f409a41a26eac7a2bd1bb0ed5bfc4026d71
Cr-Commit-Position: refs/heads/master@{#393909}
==========

[commit-bot: I haz the power, 2016-05-16 21:01:10]

Patchset 3 (id:??) landed as
https://crrev.com/d55c6f409a41a26eac7a2bd1bb0ed5bfc4026d71
Cr-Commit-Position: refs/heads/master@{#393909}