Use feature architecture as kill switch for WebAPKs instead of field trial

Use feature architecture as kill switch for WebAPKs instead of field trial

This CL converts the existing WebAPK finch field trial to use the feature
architecture. This is in preparation to adding "configurable WebAPK feature
parameters" in https://codereview.chromium.org/2441203002/

BUG=651561
Committed: https://crrev.com/d77e53258203f8df83aa26045b05107d1d82dd00
Cr-Commit-Position: refs/heads/master@{#427494}

[pkotwicz, 2016-10-24 03:41:07]

Description was changed from

==========
Use feature architecture as kill switch for WebAPKs instead of field trial

This CL converts the existing WebAPK finch field trial to use the feature
architecture. This is in preparation to adding "configurable WebAPK feature
parameters" in https://codereview.chromium.org/2441203002/

BUG=651561
==========

to

==========
Use feature architecture as kill switch for WebAPKs instead of field trial

This CL converts the existing WebAPK finch field trial to use the feature
architecture. This is in preparation to adding "configurable WebAPK feature
parameters" in https://codereview.chromium.org/2441203002/

BUG=651561
==========

[pkotwicz, 2016-10-24 03:46:29]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2016-10-24 03:46:29]

Dominick, can you please take a look?

Answering some CL comments from https://codereview.chromium.org/2441203002/

I chose to name the feature "WebAPK" instead of "WebApk" because we use the
"WebAPK" capitalization in comments and design docs. We use the "WebApk"
capitalization in variables. I know that this is correct for Java variable names
https://google.github.io/styleguide/javaguide.html#s5.3-camel-case not sure
about C++ variable names.

I chose to name the feature "WebAPK" instead of "WebAPKs" to make things easier
to remember. I am bound to forget the 's'

[dominickn, 2016-10-24 04:04:18]

Thanks, this will help clean things up a lot.

On 2016/10/24 03:46:29, pkotwicz wrote:
> Dominick, can you please take a look?
> 
> Answering some CL comments from https://codereview.chromium.org/2441203002/
> 
> I chose to name the feature "WebAPK" instead of "WebApk" because we use the
> "WebAPK" capitalization in comments and design docs. We use the "WebApk"
> capitalization in variables. I know that this is correct for Java variable
names
> https://google.github.io/styleguide/javaguide.html#s5.3-camel-case not sure
> about C++ variable names.

In chrome/common/chrome_features.cc, most acronyms are titlecase (e.g. ARC ->
Arc, HTML -> Html, PIN -> Pin, SyzyASAN - > Syzyasan), which is why I made this
suggestion.

> > I chose to name the feature "WebAPK" instead of "WebAPKs" to make things
easier
> to remember. I am bound to forget the 's'

Again, the comment was for consistency with the other features
(PreferHtmlOverPlugins) - the tense of "WebAPK" feels wrong because the feature
enables multiple WebAPKs, not just one. :)

https://codereview.chromium.org/2441243003/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2441243003/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_feature_list.cc:109: // For WebAPKs to be enabled,
the WebAPK feature must be enabled and the
Is it strictly necessary for the feature to be enabled and --enable-webapk to be
specified as well?

I believe you should be able to make the --enable-webapk flag enable this
feature, so you can probably remove the final two lines of this comment. 
Disabled features should take precedence over enabled features, so if you
kill-switch WebAPKs via Finch the disable should win even if the flag is on.

[pkotwicz, 2016-10-24 04:22:37]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2016-10-24 04:24:28]

Dominick, can you please take a look?

I have renamed things as you have requested

https://codereview.chromium.org/2441243003/diff/40001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2441243003/diff/40001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:110: // --enable-webapk command
line flag must be set.
The reason that I added the comment about the --enable-webapk command line flag
is to explain the default value. It is a bit confusing that the feature is
"default-on" even though it is not launched

[dominickn, 2016-10-24 04:35:47]

https://codereview.chromium.org/2441243003/diff/40001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2441243003/diff/40001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:110: // --enable-webapk command
line flag must be set.
On 2016/10/24 04:24:28, pkotwicz wrote:
> The reason that I added the comment about the --enable-webapk command line
flag
> is to explain the default value. It is a bit confusing that the feature is
> "default-on" even though it is not launched

Oh, I didn't notice that ENABLED_BY_DEFAULT (features almost always land
DISABLED_BY_DEFAULT if they're under development). My instinct is that it really
should be DISABLED_BY_DEFAULT since it isn't launched and is still under very
heavy development (it's very rare for a feature to land ENABLED_BY_DEFAULT).
Then the flag to turn on WebAPKs is just --enable-features=WebApks, and if you
really want you can then make the --enable-webapk flag enable the feature as
well.

Again, this will consolidate everything for WebAPKs under this one feature,
which is desirable. The kill switch via Finch should still take precedence if
necessary.

[pkotwicz, 2016-10-24 15:33:26]

pkotwicz@chromium.org changed reviewers:
+ tedchoc@chromium.org

[pkotwicz, 2016-10-24 15:33:26]

tedchoc@ for OWNERS

I think I have addressed all of Dominick's comments. I am not asking Dominick
for another review till later tonight because of time zones

https://codereview.chromium.org/2441243003/diff/40001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2441243003/diff/40001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:110: // --enable-webapk command
line flag must be set.
I have disabled the feature by default as you have requested.

I am keeping the current logic of requiring both the feature to be enabled and
the command line switch. We will probably "enable the feature by default" prior
to "enabling the command line switch by default" because we will want there to
be a way for random developers to try WebAPKs out

[Ted C, 2016-10-24 17:46:04]

On 2016/10/24 15:33:26, pkotwicz wrote:
> tedchoc@ for OWNERS
> 
> I think I have addressed all of Dominick's comments. I am not asking Dominick
> for another review till later tonight because of time zones
> 
>
https://codereview.chromium.org/2441243003/diff/40001/chrome/browser/android/...
> File chrome/browser/android/chrome_feature_list.cc (right):
> 
>
https://codereview.chromium.org/2441243003/diff/40001/chrome/browser/android/...
> chrome/browser/android/chrome_feature_list.cc:110: // --enable-webapk command
> line flag must be set.
> I have disabled the feature by default as you have requested.
> 
> I am keeping the current logic of requiring both the feature to be enabled and
> the command line switch. We will probably "enable the feature by default"
prior
> to "enabling the command line switch by default" because we will want there to
> be a way for random developers to try WebAPKs out

owners lgtm

[pkotwicz, 2016-10-24 17:52:15]

Dominick, can you please take another look?

[dominickn, 2016-10-25 00:13:32]

lgtm

https://codereview.chromium.org/2441243003/diff/40001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2441243003/diff/40001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:110: // --enable-webapk command
line flag must be set.
On 2016/10/24 15:33:26, pkotwicz wrote:
> I have disabled the feature by default as you have requested.
> 
> I am keeping the current logic of requiring both the feature to be enabled and
> the command line switch. We will probably "enable the feature by default"
prior
> to "enabling the command line switch by default" because we will want there to
> be a way for random developers to try WebAPKs out

Random developers can try it out by passing --enable-features=WebApks on the
command line - that's what I meant by the Feature API being able to fully
replace the command line string too. :)

[pkotwicz, 2016-10-25 17:30:31]

By random developers, I meant random "Web Developers". Can --enable-features be
set by chrome://flags?

[pkotwicz, 2016-10-25 17:39:41]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-10-25 17:40:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 17:42:19]

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

[commit-bot: I haz the power, 2016-10-25 17:42:20]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pkotwicz, 2016-10-25 21:10:57]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-10-25 21:10:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2441243003/#ps80001
(title: "Merge branch 'master' into webapk_finch0")

[commit-bot: I haz the power, 2016-10-25 21:11:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 21:52:56]

Description was changed from

==========
Use feature architecture as kill switch for WebAPKs instead of field trial

This CL converts the existing WebAPK finch field trial to use the feature
architecture. This is in preparation to adding "configurable WebAPK feature
parameters" in https://codereview.chromium.org/2441203002/

BUG=651561
==========

to

==========
Use feature architecture as kill switch for WebAPKs instead of field trial

This CL converts the existing WebAPK finch field trial to use the feature
architecture. This is in preparation to adding "configurable WebAPK feature
parameters" in https://codereview.chromium.org/2441203002/

BUG=651561
==========

[commit-bot: I haz the power, 2016-10-25 21:52:58]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-10-25 21:54:57]

Description was changed from

==========
Use feature architecture as kill switch for WebAPKs instead of field trial

This CL converts the existing WebAPK finch field trial to use the feature
architecture. This is in preparation to adding "configurable WebAPK feature
parameters" in https://codereview.chromium.org/2441203002/

BUG=651561
==========

to

==========
Use feature architecture as kill switch for WebAPKs instead of field trial

This CL converts the existing WebAPK finch field trial to use the feature
architecture. This is in preparation to adding "configurable WebAPK feature
parameters" in https://codereview.chromium.org/2441203002/

BUG=651561

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

[commit-bot: I haz the power, 2016-10-25 21:54:58]

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

[dominickn, 2016-10-25 21:58:04]

On 2016/10/25 17:30:31, pkotwicz wrote:
> By random developers, I meant random "Web Developers". Can --enable-features
be
> set by chrome://flags?

Yes, like I mentioned, you just wire up your existing web apk flag to enable the
feature. Things like material design history on desktop are set up like this.