New UMA settings fragment using Android prefs.

New UMA settings fragment using Android prefs.

This CL is part of new UMA upload logic for 3g uploads. Previous three-option model is replaced with a new binary on/off-switch for users assigned to experimental group.

Previous reverted CL (https://codereview.chromium.org/978623002/) used chromium preferences for the settings fragment, which caused problems. Crash reporting which also gets enabled/disabled by the same settings can not always support chromium preferences. Therefore this CL introduces new Android preferences for the new settings fragment.

As the new logic with be enabled for users assigned to a particular experiment group, that info is also cached into Android pref so that the experiment group info can be accessible anytime.

BUG=455847
Committed: https://crrev.com/a44343327c0174fbbf4f1125f3eee25977be1652
Cr-Commit-Position: refs/heads/master@{#326612}

[gayane -on leave until 09-2017, 2015-04-10 21:03:33]

Patchset #1 (id:1) has been deleted

[gayane -on leave until 09-2017, 2015-04-13 21:38:53]

Patchset #2 (id:40001) has been deleted

[gayane -on leave until 09-2017, 2015-04-13 21:40:05]

Patchset #1 (id:20001) has been deleted

[gayane -on leave until 09-2017, 2015-04-13 21:47:37]

gayane@chromium.org changed reviewers:
+ asvitkine@chromium.org

[gayane -on leave until 09-2017, 2015-04-13 21:47:38]

I have uploaded the previously reverted patch along with the new changes. 

I have also added a sync with the chromium preference for enabling metrics
reporting however currently chromium code will still read that info form the
previous crash reporting preference which is updated correctly as well.

[Alexei Svitkine (slow), 2015-04-14 15:14:44]

Sorry for also commenting on some code that hasn't changed since the previous CL
- but I guess I either missed it or forgot the answers to my questions back
then.

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java
(right):

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:166:
// Update cellular experiment preference.
Please expand comments to mention *why* this is being done.

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:169:
boolean cellularExperiment = groupName.equals("Enabled");
How about using a param instead?

via:
https://code.google.com/p/chromium/codesearch#chromium/src/components/variati...

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:175:
// If the chromium preference metrics reporting is has not been set, then
initialize it
Nit: "is has not been set" -> "has not been set".

Also, instead of saying "chromium preference", say "native preference" - since
this code is technically also Chromium code.

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:181:
// Set new android preference for usage and crash reporting.
Nit: capitalize Android

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java
(right):

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:66:
PreferenceScreen preferenceScreen = getPreferenceScreen();
Nit: Move it closer to where it's used, e.g. below networkPredictionOptions.

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:97:
preferenceScreen.removePreference(uploadCrashDumpPref);
Do we lose the original values of those prefs if we enable the experiment and
then decide to turn it off temporarily?

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:108:
mManagedPreferenceDelegate);
Wait, can you clarify the logic here?

Seems like with the new experiment enabled logic, we're no longer adding |this|
as a pref changed listener on the crash pref nor setting a managed pref delegate
on it. Is this correct? What are we losing as a result of it? (i.e. why was it
needed in the old codepath but no longer needed now?)

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java
(right):

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:252:
/*
Missing a * to make this Javadoc.

[gayane -on leave until 09-2017, 2015-04-14 21:09:45]

New settings fragment is managed by policy now + nits

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java
(right):

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:166:
// Update cellular experiment preference.
On 2015/04/14 15:14:44, Alexei Svitkine wrote:
> Please expand comments to mention *why* this is being done.

Done.

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:169:
boolean cellularExperiment = groupName.equals("Enabled");
On 2015/04/14 15:14:44, Alexei Svitkine wrote:
> How about using a param instead?
> 
> via:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/variati...

There is going to be only two groups, the default one and enabled, does it worth
really having a param? Do we in general prefer having params instead of
distinguishing according to the group ?

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:175:
// If the chromium preference metrics reporting is has not been set, then
initialize it
On 2015/04/14 15:14:44, Alexei Svitkine wrote:
> Nit: "is has not been set" -> "has not been set".
> 
> Also, instead of saying "chromium preference", say "native preference" - since
> this code is technically also Chromium code.

Done.

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:181:
// Set new android preference for usage and crash reporting.
On 2015/04/14 15:14:44, Alexei Svitkine wrote:
> Nit: capitalize Android

Done.

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java
(right):

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:66:
PreferenceScreen preferenceScreen = getPreferenceScreen();
On 2015/04/14 15:14:44, Alexei Svitkine wrote:
> Nit: Move it closer to where it's used, e.g. below networkPredictionOptions.

Done.

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:97:
preferenceScreen.removePreference(uploadCrashDumpPref);
On 2015/04/14 15:14:44, Alexei Svitkine wrote:
> Do we lose the original values of those prefs if we enable the experiment and
> then decide to turn it off temporarily?

In this case when it says preference it means the UI element of the settings
page. This code just removes UI element so that it is not displayed.

Added a comment to clarify that.

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:108:
mManagedPreferenceDelegate);
On 2015/04/14 15:14:44, Alexei Svitkine wrote:
> Wait, can you clarify the logic here?
> 
> Seems like with the new experiment enabled logic, we're no longer adding
|this|
> as a pref changed listener on the crash pref nor setting a managed pref
delegate
> on it. Is this correct? What are we losing as a result of it? (i.e. why was it
> needed in the old codepath but no longer needed now?)

We don't need to add a listener here because it is covered by
UsageAndCrashReportsPreferenceFragment, but I neglected the managed part. Added
in UsageAndCrashReportsPreferenceFragment.

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java
(right):

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:252:
/*
On 2015/04/14 15:14:44, Alexei Svitkine wrote:
> Missing a * to make this Javadoc.

Done.

[Alexei Svitkine (slow), 2015-04-14 21:41:12]

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java
(right):

https://codereview.chromium.org/1080723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:169:
boolean cellularExperiment = groupName.equals("Enabled");
On 2015/04/14 21:09:45, gayane wrote:
> On 2015/04/14 15:14:44, Alexei Svitkine wrote:
> > How about using a param instead?
> > 
> > via:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/variati...
> 
> There is going to be only two groups, the default one and enabled, does it
worth
> really having a param? Do we in general prefer having params instead of
> distinguishing according to the group ?

The advantage of a param is we can have multiple enabled groups (e.g. if we want
to see if an effect is real and we don't trust the data, we can spin up a second
enabled group to verify). Params gives us more flexibility.

https://codereview.chromium.org/1080723002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java
(right):

https://codereview.chromium.org/1080723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:91:
// of a device, by removing
?

[gayane -on leave until 09-2017, 2015-04-14 22:33:39]

Using experiment param

https://codereview.chromium.org/1080723002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java
(right):

https://codereview.chromium.org/1080723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferences.java:91:
// of a device, by removing
On 2015/04/14 21:41:12, Alexei Svitkine wrote:
> ?

Done.

[Alexei Svitkine (slow), 2015-04-15 16:31:58]

lgtm

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java
(right):

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:173:
"UMA_EnableCellularLogUpload", "Enabled") == "true";
Nit: I don't think this is correct indentation - the extra indentation should
relative to start of previous line (i.e. "boolean cellular") not start of the
call.

I don't recall, but maybe "git cl format" supports Java now?

[gayane -on leave until 09-2017, 2015-04-15 17:36:40]

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java
(right):

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:173:
"UMA_EnableCellularLogUpload", "Enabled") == "true";
On 2015/04/15 16:31:58, Alexei Svitkine wrote:
> Nit: I don't think this is correct indentation - the extra indentation should
> relative to start of previous line (i.e. "boolean cellular") not start of the
> call.
> 
> I don't recall, but maybe "git cl format" supports Java now?

This is weird because git cl format formats it this way. Even if it myself now,
it will be formatted this way.

[Alexei Svitkine (slow), 2015-04-15 17:38:13]

Fair enough - maybe Java style differs from C++. Stick to the "git cl
format" formatting then. :)

On Wed, Apr 15, 2015 at 1:36 PM, <gayane@chromium.org> wrote:

>
> https://codereview.chromium.org/1080723002/diff/120001/
> chrome/android/java/src/org/chromium/chrome/browser/
> metrics/UmaSessionStats.java
> File
> chrome/android/java/src/org/chromium/chrome/browser/
> metrics/UmaSessionStats.java
> (right):
>
> https://codereview.chromium.org/1080723002/diff/120001/
> chrome/android/java/src/org/chromium/chrome/browser/
> metrics/UmaSessionStats.java#newcode173
> chrome/android/java/src/org/chromium/chrome/browser/
> metrics/UmaSessionStats.java:173:
> "UMA_EnableCellularLogUpload", "Enabled") == "true";
> On 2015/04/15 16:31:58, Alexei Svitkine wrote:
>
>> Nit: I don't think this is correct indentation - the extra indentation
>>
> should
>
>> relative to start of previous line (i.e. "boolean cellular") not start
>>
> of the
>
>> call.
>>
>
>  I don't recall, but maybe "git cl format" supports Java now?
>>
>
> This is weird because git cl format formats it this way. Even if it
> myself now, it will be formatted this way.
>
> https://codereview.chromium.org/1080723002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gayane -on leave until 09-2017, 2015-04-15 17:41:33]

gayane@chromium.org changed reviewers:
+ dfalcantara@chromium.org, nyquist@chromium.org

[gayane -on leave until 09-2017, 2015-04-15 17:41:34]

I had to revert the previous CL that you reviewed. Please have a look on the new
CL.

dfalcantara@chromium.org: Please review changes in 
chrome/android/java/res/*
chrome/android/javatests/*

nyquist@chromium.org: Please review changes in 

chrome/android/java/src/*
chrome/android/java/strings/android_chrome_strings.grd
chrome/browser/android/preferences/pref_service_bridge.cc

[gone, 2015-04-15 23:34:06]

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java
(right):

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:173:
"UMA_EnableCellularLogUpload", "Enabled") == "true";
On 2015/04/15 17:36:40, gayane wrote:
> On 2015/04/15 16:31:58, Alexei Svitkine wrote:
> > Nit: I don't think this is correct indentation - the extra indentation
should
> > relative to start of previous line (i.e. "boolean cellular") not start of
the
> > call.
> > 
> > I don't recall, but maybe "git cl format" supports Java now?
> 
> This is weird because git cl format formats it this way. Even if it myself
now,
> it will be formatted this way.                  

Yeah, it's one of the weird cases that dissuades me from using git cl format :/ 
Are you doing a string comparison here, though?  You should be using either

boolean cellularExperiment =
"true".equals(VariationsAssociatedData.getVariationParamValue(
              "UMA_EnableCellularLogUpload", "Enabled"))

or

boolean cellularExperiment = TextUtils.equals("true",
VariationsAssociatedData.getVariationParamValue(
              "UMA_EnableCellularLogUpload", "Enabled")))

instead (with better formatting).

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java
(right):

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:35:
private static final String PREF_CELLULAR_EXPERIMENT = "cellular_experiment";
nit: group this with the other private static final prefs above.

[gayane -on leave until 09-2017, 2015-04-16 17:42:37]

fixed the string comparison + nit

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java
(right):

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:173:
"UMA_EnableCellularLogUpload", "Enabled") == "true";
On 2015/04/15 23:34:06, dfalcantara wrote:
> On 2015/04/15 17:36:40, gayane wrote:
> > On 2015/04/15 16:31:58, Alexei Svitkine wrote:
> > > Nit: I don't think this is correct indentation - the extra indentation
> should
> > > relative to start of previous line (i.e. "boolean cellular") not start of
> the
> > > call.
> > > 
> > > I don't recall, but maybe "git cl format" supports Java now?
> > 
> > This is weird because git cl format formats it this way. Even if it myself
> now,
> > it will be formatted this way.                  
> 
> Yeah, it's one of the weird cases that dissuades me from using git cl format
:/ 
> Are you doing a string comparison here, though?  You should be using either
> 
> boolean cellularExperiment =
> "true".equals(VariationsAssociatedData.getVariationParamValue(
>               "UMA_EnableCellularLogUpload", "Enabled"))
> 
> or
> 
> boolean cellularExperiment = TextUtils.equals("true",
> VariationsAssociatedData.getVariationParamValue(
>               "UMA_EnableCellularLogUpload", "Enabled")))
> 
> instead (with better formatting).

Done.

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java
(right):

https://codereview.chromium.org/1080723002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:35:
private static final String PREF_CELLULAR_EXPERIMENT = "cellular_experiment";
On 2015/04/15 23:34:06, dfalcantara wrote:
> nit: group this with the other private static final prefs above.

Done.

[gone, 2015-04-16 18:14:34]

Java bits lgtm

[gayane -on leave until 09-2017, 2015-04-16 18:28:06]

gayane@chromium.org changed reviewers:
+ noms@chromium.org

[gayane -on leave until 09-2017, 2015-04-16 18:28:07]

noms@chromium.org: Please review changes in 

chrome/browser/profiles/profile_io_data.cc

[noms (inactive), 2015-04-16 19:52:09]

profiles lgtm

[gayane -on leave until 09-2017, 2015-04-21 16:20:20]

nyquist@ please have a look. This is a new version of a CL that you previously
reviewed but it was reverted.

Please review changes in 

chrome/android/java/src/*
chrome/android/java/strings/android_chrome_strings.grd
chrome/browser/android/preferences/pref_service_bridge.cc

[nyquist, 2015-04-23 00:43:31]

lgtm

[gayane -on leave until 09-2017, 2015-04-23 19:44:56]

The CQ bit was checked by gayane@chromium.org

[gayane -on leave until 09-2017, 2015-04-23 19:44:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, nyquist@chromium.org, noms@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1080723002/#ps160001
(title: "merge")

[commit-bot: I haz the power, 2015-04-23 19:45:29]

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

[commit-bot: I haz the power, 2015-04-23 19:50:28]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-04-23 19:52:09]

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

[gayane -on leave until 09-2017, 2015-04-23 22:31:56]

A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/1101073003/ by gayane@chromium.org.

The reason for reverting is: breaks clank downstream bots.

[gayane -on leave until 09-2017, 2015-04-25 20:28:37]

Patchset #7 (id:180001) has been deleted