Disable custom tabs if the feature is disabled in finch experiment

Disable custom tabs if the feature is disabled in finch experiment

Since we don't have access to native calls, we can't check the finch
experiment group from launch. This change uses a shared pref that is
updated on every deferred startup to handle the enabling/disabling
through the finch trial. This has a possibility of launching one
custom tab before the feature is disabled.

Also checks for the field trial group to disable
prerendering. This one we don't need to use the pref since
by the time we get to the mayLaunchUrl call native should
have been initialized.

BUG=499056
Committed: https://crrev.com/8bbe9f1f6330fa413176d5fec312b9a6f9442031
Cr-Commit-Position: refs/heads/master@{#335800}

[Yusuf, 2015-06-15 17:12:59]

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

[Ted C, 2015-06-15 17:56:02]

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:111:
ChromePreferenceManager.getInstance(application).setCustomTabsDisabled(true);
I think inverting the logic is more consistent with other places (i.e.
setCustomTabsEnabled).  Then calling would be isCustomTabsEnabled();

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java
(right):

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:24:
if (ChromePreferenceManager.getInstance(this).getCustomTabsDisabled()) return
null;
hmm...i was hoping that we could actually allow the clients to call warmup once
if the finch trial hadn't been initialized, but I'm not sure how feasible that
is (i was hoping that good clients would be able to avoid the one time startup
penalty.

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java
(right):

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:130:
* @return Whether custom.
custom what?

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:133:
return mSharedPreferences.getBoolean(DISABLE_CUSTOM_TABS, false);
should we default to true for disabled-ness?  I guess that would mess up clients
until we push the finch stuff?

I do think it should be disabled by default, but oh well.

[Yusuf, 2015-06-15 18:13:50]

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:111:
ChromePreferenceManager.getInstance(application).setCustomTabsDisabled(true);
On 2015/06/15 17:56:02, Ted C wrote:
> I think inverting the logic is more consistent with other places (i.e.
> setCustomTabsEnabled).  Then calling would be isCustomTabsEnabled();

Done.

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java
(right):

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:24:
if (ChromePreferenceManager.getInstance(this).getCustomTabsDisabled()) return
null;
On 2015/06/15 17:56:02, Ted C wrote:
> hmm...i was hoping that we could actually allow the clients to call warmup
once
> if the finch trial hadn't been initialized, but I'm not sure how feasible that
> is (i was hoping that good clients would be able to avoid the one time startup
> penalty.

If the finch trial isn't initialized, we default to enabled.

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java
(right):

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:130:
* @return Whether custom.
On 2015/06/15 17:56:02, Ted C wrote:
> custom what?

Done.

https://codereview.chromium.org/1181353002/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:133:
return mSharedPreferences.getBoolean(DISABLE_CUSTOM_TABS, false);
On 2015/06/15 17:56:02, Ted C wrote:
> should we default to true for disabled-ness?  I guess that would mess up
clients
> until we push the finch stuff?
> 
> I do think it should be disabled by default, but oh well.

Yes, I do agree it is weird to have a setting that says enabled and then to
default to enable. Seemed like the only way to use a kill switch for something
that launches before native load.

[Yusuf, 2015-06-23 21:22:34]

Updated with prerendering check and default to true.

PTAL

[Ted C, 2015-06-23 22:20:21]

https://codereview.chromium.org/1181353002/diff/50001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1181353002/diff/50001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:125:
} else if (customTabsTrialGroupName.equals("Enabled")) {
if it is DisabledPrerender or whatever, don't you want to set custom tabs
enabled to true?

https://codereview.chromium.org/1181353002/diff/50001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java
(right):

https://codereview.chromium.org/1181353002/diff/50001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:133:
return mSharedPreferences.getBoolean(ENABLE_CUSTOM_TABS, true);
I would add a comment that this is explicitly true as this is designed to be a
kill switch and not a gating feature.

[Ted C, 2015-06-23 22:20:34]

lgtm w/ my comments addressed

[Yusuf, 2015-06-23 22:27:45]

https://codereview.chromium.org/1181353002/diff/50001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1181353002/diff/50001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:125:
} else if (customTabsTrialGroupName.equals("Enabled")) {
On 2015/06/23 22:20:20, Ted C wrote:
> if it is DisabledPrerender or whatever, don't you want to set custom tabs
> enabled to true?

Done.

https://codereview.chromium.org/1181353002/diff/50001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java
(right):

https://codereview.chromium.org/1181353002/diff/50001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:133:
return mSharedPreferences.getBoolean(ENABLE_CUSTOM_TABS, true);
On 2015/06/23 22:20:20, Ted C wrote:
> I would add a comment that this is explicitly true as this is designed to be a
> kill switch and not a gating feature.

Done.

[Yusuf, 2015-06-23 22:31:02]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2015-06-23 22:31:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1181353002/#ps70001
(title: "Final comments addressed")

[commit-bot: I haz the power, 2015-06-23 22:33:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181353002/70001

[commit-bot: I haz the power, 2015-06-23 23:20:09]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2015-06-23 23:21:10]

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