Move usage and crash reporting to opt-out by default.

Move usage and crash reporting to opt-out by default.

BUG=622445
Committed: https://crrev.com/40c72ce3887b344aeff02d8117c209a8156064f0
Cr-Commit-Position: refs/heads/master@{#414777}

[gayane -on leave until 09-2017, 2016-08-23 21:12:08]

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

[commit-bot: I haz the power, 2016-08-23 21:13:27]

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-23 21:37:46]

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

[commit-bot: I haz the power, 2016-08-23 21:37:48]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[gayane -on leave until 09-2017, 2016-08-24 17:20:08]

Patchset #2 (id:20001) has been deleted

[gayane -on leave until 09-2017, 2016-08-25 15:43:45]

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

[gayane -on leave until 09-2017, 2016-08-25 15:43:45]

Please have a look

[gayane -on leave until 09-2017, 2016-08-25 15:54:24]

Description was changed from

==========
Move usage and crash reporting to opt-out by default.

BUG=
==========

to

==========
Move usage and crash reporting to opt-out by default.

Also did some cleanup.

BUG=622445
==========

[Ilya Sherman, 2016-08-25 19:56:51]

Thanks, Gayane!  LGTM % nits:

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java
(right):

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:71:
public MinidumpUploadCallable(File fileToUpload, File logfile) {
Optional nit: This change looks fine, but doesn't seem particularly related to
the current CL.  Mebbe makes sense to split it off to a separate cleanup CL?

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java
(right):

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:58:
public static final boolean DEFAULT_USAGE_CRASH_REPORTING = true;
nit: "USAGE_CRASH" is very much confusing my brain.  WDYT of something like
"DEFAULT_METRICS_AND_CRASH_REPORTING"?

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java
(right):

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:123:
}
Hmm, it seems like it would make sense to inline this method at this point, and
update any tests to check the actual preferences state, rather than whether or
not this method was called.  Maybe a good cleanup task for a follow-up CL?

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:154:
// Initialize usage and crash reporting according to default value.
nit: s/to default/to the default

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:155:
// The user can change it on the Welcome page.
nit: I'd phrase this as "The user can explicitly enable or disable the reporting
on the Welcome page".  As currently written, I'm tempted to read the sentence as
meaning "The user can change _the default value_ ...", which is confusing.

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

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:304:
* Check whether crash dump upload preference is set to allow uploads or is set
to NEVER only.
Hmm, I don't really understand what "is set to NEVER only" means.  Could you
please rephrase this comment?

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:395:
return isUploadCrashDumpEnabled();
I'm confused by the changes here.  What is the distinction between
isUploadCrashDumpEnabled() and isUsageAndCrashReportingEnabled()?  What is the
purpose of this method, isUploadUserPermitted -- how does it differ from the
other two?

I *think* that the answers to my questions are: isUploadCrashDumpEnabled()
checks the old tri-state preference, isUsageAndCrashReportingEnabled() checks
the new bi-state preference, and isUploadUserPermitted() is a wrapper that
should always be used in place of the other two.  But, in that case, should the
other two be private?  Hopefully this will all get a lot clearer once you have a
chance to deprecate the tri-state preference, and we only have one preference to
worry about.

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencerTest.java
(right):

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencerTest.java:113:
}
It looks like there are no tests where this is called.  Is that intentional?

[gayane -on leave until 09-2017, 2016-08-25 20:50:02]

Description was changed from

==========
Move usage and crash reporting to opt-out by default.

Also did some cleanup.

BUG=622445
==========

to

==========
Move usage and crash reporting to opt-out by default.

BUG=622445
==========

[gayane -on leave until 09-2017, 2016-08-25 20:50:15]

Thanks Ilya

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java
(right):

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:71:
public MinidumpUploadCallable(File fileToUpload, File logfile) {
On 2016/08/25 19:56:51, Ilya Sherman wrote:
> Optional nit: This change looks fine, but doesn't seem particularly related to
> the current CL.  Mebbe makes sense to split it off to a separate cleanup CL?

reverted unrelated cleanup

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java
(right):

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:58:
public static final boolean DEFAULT_USAGE_CRASH_REPORTING = true;
On 2016/08/25 19:56:51, Ilya Sherman wrote:
> nit: "USAGE_CRASH" is very much confusing my brain.  WDYT of something like
> "DEFAULT_METRICS_AND_CRASH_REPORTING"?

Done.

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java
(right):

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:123:
}
On 2016/08/25 19:56:51, Ilya Sherman wrote:
> Hmm, it seems like it would make sense to inline this method at this point,
and
> update any tests to check the actual preferences state, rather than whether or
> not this method was called.  Maybe a good cleanup task for a follow-up CL?

Lets do that in follow-up CL, because I also need to do some other changes which
will change both initCrashUploadPreference and the preferences that need to be
checked.

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:154:
// Initialize usage and crash reporting according to default value.
On 2016/08/25 19:56:51, Ilya Sherman wrote:
> nit: s/to default/to the default

Done.

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:155:
// The user can change it on the Welcome page.
On 2016/08/25 19:56:51, Ilya Sherman wrote:
> nit: I'd phrase this as "The user can explicitly enable or disable the
reporting
> on the Welcome page".  As currently written, I'm tempted to read the sentence
as
> meaning "The user can change _the default value_ ...", which is confusing.

Done.

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

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:304:
* Check whether crash dump upload preference is set to allow uploads or is set
to NEVER only.
On 2016/08/25 19:56:51, Ilya Sherman wrote:
> Hmm, I don't really understand what "is set to NEVER only" means.  Could you
> please rephrase this comment?

Done.

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:395:
return isUploadCrashDumpEnabled();
On 2016/08/25 19:56:51, Ilya Sherman wrote:
> I'm confused by the changes here.  What is the distinction between
> isUploadCrashDumpEnabled() and isUsageAndCrashReportingEnabled()?  What is the
> purpose of this method, isUploadUserPermitted -- how does it differ from the
> other two?
> 
> I *think* that the answers to my questions are: isUploadCrashDumpEnabled()
> checks the old tri-state preference, isUsageAndCrashReportingEnabled() checks
> the new bi-state preference, and isUploadUserPermitted() is a wrapper that
> should always be used in place of the other two.  But, in that case, should
the
> other two be private?  Hopefully this will all get a lot clearer once you have
a
> chance to deprecate the tri-state preference, and we only have one preference
to
> worry about.

Yes, you got it right. I am about to remove a lot of code and deprecate
tri-state preferences. For now I will add a short comments. Also cannot simply
make these functions private as they are called from other places. But cleanup
is coming!

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencerTest.java
(right):

https://codereview.chromium.org/2275513003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencerTest.java:113:
}
On 2016/08/25 19:56:51, Ilya Sherman wrote:
> It looks like there are no tests where this is called.  Is that intentional?

fixed the tests, should be call in all tests expect when isFirstRunFlowComplete
is true.

[Ilya Sherman, 2016-08-26 00:25:03]

(Still lgtm)

https://codereview.chromium.org/2275513003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java
(right):

https://codereview.chromium.org/2275513003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:59:
public static final boolean DEFAULT_METRICS_CRASH_REPORTING = true;
nit: I think "METRICS_AND_CRASH" is clearer than "METRICS_CRASH". 
("USAGE_AND_CRASH" would be okay too, if that better matches the terms used
elsewhere.  I think the missing "and" is the most confusing part of this name.)

https://codereview.chromium.org/2275513003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java
(right):

https://codereview.chromium.org/2275513003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:139:
System.out.println("first flow complete");
nit: Please remember to remove the debug code =)

[gayane -on leave until 09-2017, 2016-08-26 00:32:18]

Thanks Ilya

https://codereview.chromium.org/2275513003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java
(right):

https://codereview.chromium.org/2275513003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:59:
public static final boolean DEFAULT_METRICS_CRASH_REPORTING = true;
On 2016/08/26 00:25:03, Ilya Sherman wrote:
> nit: I think "METRICS_AND_CRASH" is clearer than "METRICS_CRASH". 
> ("USAGE_AND_CRASH" would be okay too, if that better matches the terms used
> elsewhere.  I think the missing "and" is the most confusing part of this
name.)
Added the "AND"

https://codereview.chromium.org/2275513003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java
(right):

https://codereview.chromium.org/2275513003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:139:
System.out.println("first flow complete");
On 2016/08/26 00:25:03, Ilya Sherman wrote:
> nit: Please remember to remove the debug code =)

Done.

[gone, 2016-08-26 18:15:01]

lgtm

[gayane -on leave until 09-2017, 2016-08-26 18:16:00]

The CQ bit was checked by gayane@chromium.org

[gayane -on leave until 09-2017, 2016-08-26 18:16:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2275513003/#ps100001
(title: "sync")

[commit-bot: I haz the power, 2016-08-26 18:17:23]

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-26 19:30:35]

Description was changed from

==========
Move usage and crash reporting to opt-out by default.

BUG=622445
==========

to

==========
Move usage and crash reporting to opt-out by default.

BUG=622445
==========

[commit-bot: I haz the power, 2016-08-26 19:30:36]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-08-26 19:32:54]

Description was changed from

==========
Move usage and crash reporting to opt-out by default.

BUG=622445
==========

to

==========
Move usage and crash reporting to opt-out by default.

BUG=622445

Committed: https://crrev.com/40c72ce3887b344aeff02d8117c209a8156064f0
Cr-Commit-Position: refs/heads/master@{#414777}
==========

[commit-bot: I haz the power, 2016-08-26 19:32:56]

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