Add an about:flag to control WebApk enablement.

Add an about:flag to control WebApk enablement.

- Moves it out from being only a command line flag.
- Adds a finch override (kill switch)
- Centralizes logic in ChromeWebApkHost and caches configuration
in a shared pref so that it can be read before native is loaded.
BUG=633803
Committed: https://crrev.com/33d6dc519bd369a51953e47cf072e93282ba5a4f
Cr-Commit-Position: refs/heads/master@{#412372}

[Yaron, 2016-08-11 17:05:46]

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

[commit-bot: I haz the power, 2016-08-11 17:06:13]

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-08-11 17:08:33]

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

[commit-bot: I haz the power, 2016-08-11 17:08:34]

Dry run: Try jobs failed on following builders:
  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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yaron, 2016-08-11 17:26:25]

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

[commit-bot: I haz the power, 2016-08-11 17:26: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-08-11 20:15:43]

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

[commit-bot: I haz the power, 2016-08-11 20:15:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yaron, 2016-08-12 21:34:11]

Description was changed from

==========
Add an about:flag to control WebApk enablement.

Moves it out from being only a command line flag.
Also adds a finch override (kill switch)
Centralizes logic in ChromeWebApkHost and caches configuration
in a shared pref so that it can be read before native is loaded.
BUG=633803
==========

to

==========
Add an about:flag to control WebApk enablement.

- Moves it out from being only a command line flag.
- Adds a finch override (kill switch)
- Centralizes logic in ChromeWebApkHost and caches configuration
in a shared pref so that it can be read before native is loaded.
BUG=633803
==========

[Yaron, 2016-08-12 21:34:11]

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

[Yaron, 2016-08-12 21:36:29]

what do you think about this? There's still a command line flag defined in c++
but that's only so the about-flag can set it.

We're in this weird place where we need to know whether the feature is enabled
before native is loaded but the configuration can only be read in native. That's
why I followed a pattern elsewhere in chorme of saving to a preference for next
launch

[pkotwicz, 2016-08-15 15:27:53]

LGTM with nits

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java
(right):

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:78:
/** Finch experiment group which forces webapks off. */
Nit: webapks -> WebAPKs

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:85:
private static Boolean sEnabledForTesting;
For my general knowledge, why is this static not with the other statics up top?

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:116:
* state to see if we should enable WebApks.
Nit: "WebApks" -> "WebAPKs". We use WebApks capitalization in Java code but
WebAPKs capitalization in Java comments which is confusing. This is however what
the Google Java style guide recomments
https://google.github.io/styleguide/javaguide.html#s5.3-camel-case

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:122:
String experiment = FieldTrialList.findFullName(WEBAPK_DISABLE_EXPERIMENT_NAME);
Nit: I think that it might be clearer if you condensed the if-else ladder

boolean isEnabled = (!WEBAPK_RUNTIME_DISABLED.equals(experiment) &&
instance.hasSwitch(ChromeSwitches.ENABLE_WEBAPK));

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:130:
Log.d("TAG", "WebApk setting changed (%s => %s)", wasEnabled, isEnabled);
Nit: Did you intend to use a different tag name than "TAG"?

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java
(right):

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:107:
assertEquals(new HashSet<>(Arrays.asList("first")), ids);
The changes in this file seem unrelated to your CL

https://codereview.chromium.org/2242473002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2242473002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15071: +        Enable WebApks
Nit ⬆️case. "Enable WebApks" -> "Enable WebAPKs"

https://codereview.chromium.org/2242473002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15074: +        Enables packaging PWAs into
an Android WebApk. Requires sending data to a chrome server to generate the
application.
"WebApk" -> "WebAPK"

[Yaron, 2016-08-16 01:08:41]

yfriedman@chromium.org changed reviewers:
+ mpearson@chromium.org, peter@chromium.org

[Yaron, 2016-08-16 01:08:42]

Peter (Beverloo): notifications
mpearson: histograms change. You may also note that I didn't use the new finch
feature flags. We don't intend to launch via finch but thought to have a
backstop to disable the feature since it affects our server. If it's still
recommended (perhaps for tracking of metrics?) I could do it but would
regardless want a kill switch option which overrides the about:flag

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java
(right):

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:78:
/** Finch experiment group which forces webapks off. */
On 2016/08/15 15:27:52, pkotwicz wrote:
> Nit: webapks -> WebAPKs

Done.

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:85:
private static Boolean sEnabledForTesting;
On 2016/08/15 15:27:52, pkotwicz wrote:
> For my general knowledge, why is this static not with the other statics up
top?

no good reason, moved.

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:116:
* state to see if we should enable WebApks.
On 2016/08/15 15:27:52, pkotwicz wrote:
> Nit: "WebApks" -> "WebAPKs". We use WebApks capitalization in Java code but
> WebAPKs capitalization in Java comments which is confusing. This is however
what
> the Google Java style guide recomments
> https://google.github.io/styleguide/javaguide.html#s5.3-camel-case

Done.

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:122:
String experiment = FieldTrialList.findFullName(WEBAPK_DISABLE_EXPERIMENT_NAME);
On 2016/08/15 15:27:52, pkotwicz wrote:
> Nit: I think that it might be clearer if you condensed the if-else ladder
> 
> boolean isEnabled = (!WEBAPK_RUNTIME_DISABLED.equals(experiment) &&
> instance.hasSwitch(ChromeSwitches.ENABLE_WEBAPK));

Done.

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:130:
Log.d("TAG", "WebApk setting changed (%s => %s)", wasEnabled, isEnabled);
On 2016/08/15 15:27:52, pkotwicz wrote:
> Nit: Did you intend to use a different tag name than "TAG"?

Done.

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java
(right):

https://codereview.chromium.org/2242473002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:107:
assertEquals(new HashSet<>(Arrays.asList("first")), ids);
On 2016/08/15 15:27:52, pkotwicz wrote:
> The changes in this file seem unrelated to your CL

eclipse auto-fixes accordingly to its configuration. I prefer to let it apply
clean-ups as I go. I had originally made changes in this file but it's no longer
necessary so removed.

https://codereview.chromium.org/2242473002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2242473002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15071: +        Enable WebApks
On 2016/08/15 15:27:52, pkotwicz wrote:
> Nit ⬆️case. "Enable WebApks" -> "Enable WebAPKs"

Done.

https://codereview.chromium.org/2242473002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15074: +        Enables packaging PWAs into
an Android WebApk. Requires sending data to a chrome server to generate the
application.
On 2016/08/15 15:27:52, pkotwicz wrote:
> "WebApk" -> "WebAPK"

Done.

[Peter Beverloo, 2016-08-16 09:04:37]

lgtm, I only looked at the notification bits

[Mark P, 2016-08-16 17:46:02]

histograms.xml lgtm
Your usage of finch to disable the experiment looks fine to me too.  If you want
experiment metrics you'd want to set things up slightly differently, but this is
fine as a kill switch.

--mark

[Yaron, 2016-08-16 18:07:52]

The CQ bit was checked by yfriedman@chromium.org

[Yaron, 2016-08-16 18:07:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2242473002/#ps60001
(title: "Add an about:flag to control WebApk enablement.")

[commit-bot: I haz the power, 2016-08-16 18:08:18]

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-08-16 18:10:56]

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

[commit-bot: I haz the power, 2016-08-16 18:10:56]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yaron, 2016-08-16 18:23:54]

The CQ bit was checked by yfriedman@chromium.org

[Yaron, 2016-08-16 18:23:54]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
pkotwicz@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2242473002/#ps100001
(title: "Add an about:flag to control WebApk enablement.")

[commit-bot: I haz the power, 2016-08-16 18:24:15]

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-08-16 18:48:56]

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

[commit-bot: I haz the power, 2016-08-16 18:48:57]

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

[Yaron, 2016-08-16 19:59:29]

The CQ bit was checked by yfriedman@chromium.org

[Yaron, 2016-08-16 19:59:30]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
pkotwicz@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2242473002/#ps120001
(title: "fix merge")

[commit-bot: I haz the power, 2016-08-16 19:59:53]

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

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

[commit-bot: I haz the power, 2016-08-16 20:06:13]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yaron, 2016-08-16 20:14:55]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2016-08-16 20:15:11]

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-08-16 20:21:40]

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

[commit-bot: I haz the power, 2016-08-16 20:21:41]

Exceeded global retry quota

[Yaron, 2016-08-16 22:24:11]

The CQ bit was checked by yfriedman@chromium.org

[Yaron, 2016-08-16 22:24:11]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
pkotwicz@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2242473002/#ps140001
(title: "nbuild ch")

[commit-bot: I haz the power, 2016-08-16 22:24:42]

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-08-16 23:29:27]

Description was changed from

==========
Add an about:flag to control WebApk enablement.

- Moves it out from being only a command line flag.
- Adds a finch override (kill switch)
- Centralizes logic in ChromeWebApkHost and caches configuration
in a shared pref so that it can be read before native is loaded.
BUG=633803
==========

to

==========
Add an about:flag to control WebApk enablement.

- Moves it out from being only a command line flag.
- Adds a finch override (kill switch)
- Centralizes logic in ChromeWebApkHost and caches configuration
in a shared pref so that it can be read before native is loaded.
BUG=633803
==========

[commit-bot: I haz the power, 2016-08-16 23:29:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-16 23:31:45]

Description was changed from

==========
Add an about:flag to control WebApk enablement.

- Moves it out from being only a command line flag.
- Adds a finch override (kill switch)
- Centralizes logic in ChromeWebApkHost and caches configuration
in a shared pref so that it can be read before native is loaded.
BUG=633803
==========

to

==========
Add an about:flag to control WebApk enablement.

- Moves it out from being only a command line flag.
- Adds a finch override (kill switch)
- Centralizes logic in ChromeWebApkHost and caches configuration
in a shared pref so that it can be read before native is loaded.
BUG=633803

Committed: https://crrev.com/33d6dc519bd369a51953e47cf072e93282ba5a4f
Cr-Commit-Position: refs/heads/master@{#412372}
==========

[commit-bot: I haz the power, 2016-08-16 23:31:46]

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