Make chrome://flags control whether the WebAPK feature is on

Make chrome://flags control whether the WebAPK feature is on

This CL removes the need for special code in variations to allow users to
experiment with WebAPKs. Since "variations experiments" are time limited, this
is a good thing.

This CL removes the ability to disable the WebAPK feature through variations.
This is OK because very few users currently use WebAPKs. In order to use
WebAPKs users need to turn the feature on via chrome://flags Once we start
rolling out WebAPKs to all users we will need to add a variations parameter as
a kill switch

BUG=None

Committed: https://crrev.com/8bd5759627bc6ea0ab775efa1cdf0281b08065b6
Cr-Commit-Position: refs/heads/master@{#438425}

[pkotwicz, 2016-12-02 01:05:04]

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

[pkotwicz, 2016-12-02 01:05:05]

Dominick can you please take a look?

[pkotwicz, 2016-12-02 01:07:55]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2016-12-02 01:07:59]

Patchset #1 (id:1) has been deleted

[dominickn, 2016-12-02 01:15:13]

I don't think this should be enabled by default until WebAPKs have passed
launch review and are approved for launch. Has launch approval happened yet?

On Fri., 2 Dec. 2016 at 12:05, <pkotwicz@chromium.org> wrote:

>
>
>
>
>
>
>
>
>
>
> Reviewers: dominickn
> CL: https://codereview.chromium.org/2544973002/
>
> Message:
> Dominick can you please take a look?
>
> Description:
> Set WebAPK feature to be on by default.
>
> This better reflects the feature's purpose as a kill switch
>
> BUG=None
>
> Affected files (+3, -1 lines):
> M chrome/browser/android/chrome_feature_list.cc
>
>
> Index: chrome/browser/android/chrome_feature_list.cc
> diff --git a/chrome/browser/android/chrome_feature_list.cc
> b/chrome/browser/android/chrome_feature_list.cc
> index
>
9342c8a6fd456cfd9748625c8e7337ac3f496b49..233e4cfe2afb37208557f92414c3c48480ac6b69
> 100644
> --- a/chrome/browser/android/chrome_feature_list.cc
> +++ b/chrome/browser/android/chrome_feature_list.cc
> @@ -118,7 +118,9 @@ const base::Feature kUserMediaScreenCapturing{
>
> // Makes "Add to Home screen" in the app menu generate an APK for the
> shortcut
> // URL which opens Chrome in fullscreen.
> -const base::Feature kWebApks{"WebApks",
> base::FEATURE_DISABLED_BY_DEFAULT};
> +// Note: The WebAPK feature is only on if both the feature is enabled and
> the
> +// command line flag is set.
> +const base::Feature kWebApks{"WebApks", base::FEATURE_ENABLED_BY_DEFAULT};
>
> static jboolean IsEnabled(JNIEnv* env,
> const JavaParamRef<jclass>& clazz,
>
>
>
>
>
>
>
>
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pkotwicz, 2016-12-02 03:16:57]

Description was changed from

==========
Set WebAPK feature to be on by default.

This better reflects the feature's purpose as a kill switch

BUG=None
==========

to

==========
Set WebAPK feature to be on by default.

This better reflects the feature's purpose as a kill switch

Although this CL turns the "feature" on by default, this CL does not turn the
WebAPK functionality on by default. In order for the WebAPK functionality to be
enabled, both the "feature" must be enabled and the --enable-improved-a2hs
command line flag must be set.

BUG=None
==========

[pkotwicz, 2016-12-02 16:57:08]

Dominick, can you please take another look?

I have updated the CL description to be clearer. This CL does not "enable the
WebAPK functionality by default" because the WebAPK functionality is still
behind an off-by-default command line flag (--enable-improved-a2hs).

This CL enables the WebAPK "feature" by default so that users can try out
WebAPKs IF AND ONLY IF they flip the --enable-improved-a2hs flag in
chrome://flags

I know that FEATURE_VALUE_TYPE can be used in about_flags.cc However, I don't
know of a way of giving "disabling the feature via variations" precedence over
"enabling the feature via --enable-features".

I want the following behavior:
WebAPK functionality is off if
- Functionality is disabled via variations
OR
- Functionality is disabled via chrome://flags
Using FEATURE_VALUE_TYPE() in about_flags.cc does not provide this behavior

[pkotwicz, 2016-12-03 23:56:44]

The CQ bit was checked by pkotwicz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 23:56:53]

Dry run: 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-12-04 01:33:58]

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

[commit-bot: I haz the power, 2016-12-04 01:33:59]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-12-04 22:55:12]

On 2016/12/02 16:57:08, pkotwicz wrote:
> Dominick, can you please take another look?
> 
> I have updated the CL description to be clearer. This CL does not "enable the
> WebAPK functionality by default" because the WebAPK functionality is still
> behind an off-by-default command line flag (--enable-improved-a2hs).
> 
> This CL enables the WebAPK "feature" by default so that users can try out
> WebAPKs IF AND ONLY IF they flip the --enable-improved-a2hs flag in
> chrome://flags
> 
> I know that FEATURE_VALUE_TYPE can be used in about_flags.cc However, I don't
> know of a way of giving "disabling the feature via variations" precedence over
> "enabling the feature via --enable-features".
> 
> I want the following behavior:
> WebAPK functionality is off if
> - Functionality is disabled via variations
> OR
> - Functionality is disabled via chrome://flags
> Using FEATURE_VALUE_TYPE() in about_flags.cc does not provide this behavior

WebAPKs have a server component - could we simply neuter the feature by
disabling the WebAPK server if necessary? What benefit is there to forcibly
overriding the flag set by a developer? They know that it's experimental and
could stop working at any point. I'm still in favour of using FEATURE_VALUE_TYPE
here and reducing the confusion inherent in needing two separate switches to
enable WebAPKs.

Again, I'm very hesitant to have an enabled by default feature (even if there's
another flag required to turn it on) when it hasn't yet passed launch review.

[pkotwicz, 2016-12-07 21:12:57]

Description was changed from

==========
Set WebAPK feature to be on by default.

This better reflects the feature's purpose as a kill switch

Although this CL turns the "feature" on by default, this CL does not turn the
WebAPK functionality on by default. In order for the WebAPK functionality to be
enabled, both the "feature" must be enabled and the --enable-improved-a2hs
command line flag must be set.

BUG=None
==========

to

==========
Make chrome://flags control whether the WebAPK feature is on

This CL removes the need for special code in variations to allow users to
experiment with WebAPKs. Since "variations experiments" are time limited, this
is a good thing.

This CL removes the ability to disable the WebAPK feature through variations.
This is OK because very few users currently use WebAPKs. In order to use
WebAPKs users need to turn the feature on via chrome://flags Once we start
rolling out WebAPKs to all users we will need to add a variations parameter as
a kill switch

BUG=None
==========

[pkotwicz, 2016-12-07 21:13:19]

I have switched to using FEATURE_VALUE_TYPE as you have requested

[dominickn, 2016-12-08 02:12:13]

lgtm

This is much nicer, thanks very much for doing it.

[pkotwicz, 2016-12-08 02:52:21]

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

[pkotwicz, 2016-12-08 02:52:21]

Dan for everything except ChromeWebApkHost.java

[gone, 2016-12-08 19:58:16]

lgtm

[pkotwicz, 2016-12-08 22:59:21]

The CQ bit was checked by pkotwicz@chromium.org

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

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-12-08 23:12:06]

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

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

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pkotwicz, 2016-12-09 15:52:09]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-12-09 15:52:25]

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-12-09 17:19:48]

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

[commit-bot: I haz the power, 2016-12-09 17:19:49]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pkotwicz, 2016-12-09 21:37:09]

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

[pkotwicz, 2016-12-09 21:37:09]

isherman@ for changes to histograms.xml

[Ilya Sherman, 2016-12-09 23:01:52]

histograms.xml lgtm

[pkotwicz, 2016-12-09 23:03:58]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-12-09 23:03:59]

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

[commit-bot: I haz the power, 2016-12-09 23:05:21]

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-12-10 01:08:27]

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

[commit-bot: I haz the power, 2016-12-10 01:08:28]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[pkotwicz, 2016-12-11 02:19:08]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-12-11 02:19:14]

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-12-11 04:06:40]

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

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

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pkotwicz, 2016-12-12 00:36:21]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-12-12 00:36:37]

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-12-12 02:30:31]

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

[commit-bot: I haz the power, 2016-12-12 02:30:32]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pkotwicz, 2016-12-12 15:20:41]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-12-12 15:21:05]

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-12-12 16:33:35]

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

[commit-bot: I haz the power, 2016-12-12 16:33:37]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dominickn, 2016-12-12 23:47:03]

On 2016/12/12 16:33:37, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

linux_android_rel_ng seems to be super flaky this week....

[pkotwicz, 2016-12-13 15:34:44]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-12-13 15:35:03]

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-12-13 16:18:09]

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

[commit-bot: I haz the power, 2016-12-13 16:18:11]

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...)

[pkotwicz, 2016-12-14 02:50:27]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-12-14 02:50:28]

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

[commit-bot: I haz the power, 2016-12-14 02:51:04]

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-12-14 04:12:53]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481683827560840,
"parent_rev": "258c81875965638e63d52a894f536b7db4128399", "commit_rev":
"30a6022d0bc2ac7f0fc0f5e8d4bb2b16871d5e76"}

[commit-bot: I haz the power, 2016-12-14 04:13:50]

Description was changed from

==========
Make chrome://flags control whether the WebAPK feature is on

This CL removes the need for special code in variations to allow users to
experiment with WebAPKs. Since "variations experiments" are time limited, this
is a good thing.

This CL removes the ability to disable the WebAPK feature through variations.
This is OK because very few users currently use WebAPKs. In order to use
WebAPKs users need to turn the feature on via chrome://flags Once we start
rolling out WebAPKs to all users we will need to add a variations parameter as
a kill switch

BUG=None
==========

to

==========
Make chrome://flags control whether the WebAPK feature is on

This CL removes the need for special code in variations to allow users to
experiment with WebAPKs. Since "variations experiments" are time limited, this
is a good thing.

This CL removes the ability to disable the WebAPK feature through variations.
This is OK because very few users currently use WebAPKs. In order to use
WebAPKs users need to turn the feature on via chrome://flags Once we start
rolling out WebAPKs to all users we will need to add a variations parameter as
a kill switch

BUG=None

Review-Url: https://codereview.chromium.org/2544973002
==========

[commit-bot: I haz the power, 2016-12-14 04:13:51]

Committed patchset #4 (id:100001)

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

Description was changed from

==========
Make chrome://flags control whether the WebAPK feature is on

This CL removes the need for special code in variations to allow users to
experiment with WebAPKs. Since "variations experiments" are time limited, this
is a good thing.

This CL removes the ability to disable the WebAPK feature through variations.
This is OK because very few users currently use WebAPKs. In order to use
WebAPKs users need to turn the feature on via chrome://flags Once we start
rolling out WebAPKs to all users we will need to add a variations parameter as
a kill switch

BUG=None

Review-Url: https://codereview.chromium.org/2544973002
==========

to

==========
Make chrome://flags control whether the WebAPK feature is on

This CL removes the need for special code in variations to allow users to
experiment with WebAPKs. Since "variations experiments" are time limited, this
is a good thing.

This CL removes the ability to disable the WebAPK feature through variations.
This is OK because very few users currently use WebAPKs. In order to use
WebAPKs users need to turn the feature on via chrome://flags Once we start
rolling out WebAPKs to all users we will need to add a variations parameter as
a kill switch

BUG=None

Committed: https://crrev.com/8bd5759627bc6ea0ab775efa1cdf0281b08065b6
Cr-Commit-Position: refs/heads/master@{#438425}
==========

[commit-bot: I haz the power, 2016-12-14 04:17:08]

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