Separate out experimental flag behavior.

Separate out experimental flag behavior for minidump upload.

This sets the default for minidump upload to being disabled from command
line flag (if enabled, then depends on user and network preferences).
Also, separates out the case of command line disabled uploads, such that
the associated minidumps are not uploaded, and also not deleted.

BUG=538340
Committed: https://crrev.com/83fdc052f41a78ac6290b3a204535fff590fb788
Cr-Commit-Position: refs/heads/master@{#364454}

[jchinlee, 2015-11-19 23:59:51]

jchinlee@chromium.org changed reviewers:
+ acleung@chromium.org, mimee@chromium.org, yfriedman@chromium.org

[jchinlee, 2015-11-19 23:59:51]

This is needed to retain minidump files that experiments disabling upload might
need.

(Will need refactoring when we address e.g. comments in
https://codereview.chromium.org/1445233002/ but retention needed ASAP for
Clusterfuzz.)

[acleung1, 2015-11-20 01:13:36]

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

https://codereview.chromium.org/1461883003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:44:
mCrashUploadingExperimentalDisabled = false;
If Chrome crashed before onDeferredStartup. Would we ended up uploading?

[jchinlee, 2015-11-20 01:45:57]

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

https://codereview.chromium.org/1461883003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:44:
mCrashUploadingExperimentalDisabled = false;
On 2015/11/20 01:13:36, acleung1 wrote:
> If Chrome crashed before onDeferredStartup. Would we ended up uploading?

That seems to be the case. The main concern here is not to delete the minidump
if disabling was disabled by command line switch though, which is currently the
case because PrivacyPreferencesManager can't tell the difference between
standard user-disabling and command line-disabling, and which needs to be fixed
ASAP. There is definitely a question of where (and potentially how) to indicate
what to upload and when, and whether or not to delete the minidump file, and
that ties back to the discussion on https://codereview.chromium.org/1445233002/.

[acleung1, 2015-11-21 01:05:16]

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

https://codereview.chromium.org/1461883003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:44:
mCrashUploadingExperimentalDisabled = false;
On 2015/11/20 01:45:57, jchinlee wrote:
> On 2015/11/20 01:13:36, acleung1 wrote:
> > If Chrome crashed before onDeferredStartup. Would we ended up uploading?
> 
> That seems to be the case. The main concern here is not to delete the minidump
> if disabling was disabled by command line switch though, which is currently
the
> case because PrivacyPreferencesManager can't tell the difference between
> standard user-disabling and command line-disabling, and which needs to be
fixed
> ASAP. There is definitely a question of where (and potentially how) to
indicate
> what to upload and when, and whether or not to delete the minidump file, and
> that ties back to the discussion on
https://codereview.chromium.org/1445233002/.

I think there is no way for this to work 100%. It's a circular dependency:

-I want to upload *before* deferredStartup
-You want to have a flag that controls upload that is only available *after*
deferredStartup.

This is an alternative:

1. mCrashUploadingExperimentalDisabled defaults to be true;
2. Check
CommandLine.getInstance().hasSwitch(ChromeSwitches.DISABLE_CRASH_DUMP_UPLOAD) on
deferrred start up. If it isn't there, toggle
mCrashUploadingExperimentalDisabled to false.

Lets look at the impact:

This CL: If Chrome crash before deferredStartup, we upload ignoring the disable
flag.
My Alternative: If Chrome crash before deferredStartup, don't upload.

I would not mind just uploading regardless. Since that case is rather rare.

[Yaron, 2015-11-23 17:47:36]

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

https://codereview.chromium.org/1461883003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:44:
mCrashUploadingExperimentalDisabled = false;
On 2015/11/21 01:05:16, acleung1 wrote:
> On 2015/11/20 01:45:57, jchinlee wrote:
> > On 2015/11/20 01:13:36, acleung1 wrote:
> > > If Chrome crashed before onDeferredStartup. Would we ended up uploading?
> > 
> > That seems to be the case. The main concern here is not to delete the
minidump
> > if disabling was disabled by command line switch though, which is currently
> the
> > case because PrivacyPreferencesManager can't tell the difference between
> > standard user-disabling and command line-disabling, and which needs to be
> fixed
> > ASAP. There is definitely a question of where (and potentially how) to
> indicate
> > what to upload and when, and whether or not to delete the minidump file, and
> > that ties back to the discussion on
> https://codereview.chromium.org/1445233002/.
> 
> I think there is no way for this to work 100%. It's a circular dependency:
> 
> -I want to upload *before* deferredStartup
> -You want to have a flag that controls upload that is only available *after*
> deferredStartup.
> 
> This is an alternative:
> 
> 1. mCrashUploadingExperimentalDisabled defaults to be true;
> 2. Check
> CommandLine.getInstance().hasSwitch(ChromeSwitches.DISABLE_CRASH_DUMP_UPLOAD)
on
> deferrred start up. If it isn't there, toggle
> mCrashUploadingExperimentalDisabled to false.
> 
> Lets look at the impact:
> 
> This CL: If Chrome crash before deferredStartup, we upload ignoring the
disable
> flag.
> My Alternative: If Chrome crash before deferredStartup, don't upload.
> 
> I would not mind just uploading regardless. Since that case is rather rare.

we should never upload if unsure about consent so I'd prefer your alternative.

[jchinlee, 2015-11-23 18:03:29]

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

https://codereview.chromium.org/1461883003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:44:
mCrashUploadingExperimentalDisabled = false;
On 2015/11/23 17:47:36, Yaron wrote:
> On 2015/11/21 01:05:16, acleung1 wrote:
> > On 2015/11/20 01:45:57, jchinlee wrote:
> > > On 2015/11/20 01:13:36, acleung1 wrote:
> > > > If Chrome crashed before onDeferredStartup. Would we ended up uploading?
> > > 
> > > That seems to be the case. The main concern here is not to delete the
> minidump
> > > if disabling was disabled by command line switch though, which is
currently
> > the
> > > case because PrivacyPreferencesManager can't tell the difference between
> > > standard user-disabling and command line-disabling, and which needs to be
> > fixed
> > > ASAP. There is definitely a question of where (and potentially how) to
> > indicate
> > > what to upload and when, and whether or not to delete the minidump file,
and
> > > that ties back to the discussion on
> > https://codereview.chromium.org/1445233002/.
> > 
> > I think there is no way for this to work 100%. It's a circular dependency:
> > 
> > -I want to upload *before* deferredStartup
> > -You want to have a flag that controls upload that is only available *after*
> > deferredStartup.
> > 
> > This is an alternative:
> > 
> > 1. mCrashUploadingExperimentalDisabled defaults to be true;
> > 2. Check
> >
CommandLine.getInstance().hasSwitch(ChromeSwitches.DISABLE_CRASH_DUMP_UPLOAD)
> on
> > deferrred start up. If it isn't there, toggle
> > mCrashUploadingExperimentalDisabled to false.
> > 
> > Lets look at the impact:
> > 
> > This CL: If Chrome crash before deferredStartup, we upload ignoring the
> disable
> > flag.
> > My Alternative: If Chrome crash before deferredStartup, don't upload.
> > 
> > I would not mind just uploading regardless. Since that case is rather rare.
> 
> we should never upload if unsure about consent so I'd prefer your alternative.

Done.

[acleung1, 2015-11-23 19:41:22]

some minor nits

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

https://codereview.chromium.org/1461883003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:181:
* Provides a way to remove disabliing crash uploading entirely.
*disabling

https://codereview.chromium.org/1461883003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:182:
* If command line flag does not explicitly disable uploads, then there is a
chance of uploading
I usually prefer a less passive tone in documentation to make the point less
ambiguous.

"Enable crash uploading based on user's preference when an overriding flag does
not exist in commandline"

[jchinlee, 2015-11-23 20:51:25]

Patchset #3 (id:40001) has been deleted

[jchinlee, 2015-11-23 22:12:26]

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

https://codereview.chromium.org/1461883003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:181:
* Provides a way to remove disabliing crash uploading entirely.
On 2015/11/23 19:41:21, acleung1 wrote:
> *disabling

Done.

https://codereview.chromium.org/1461883003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:182:
* If command line flag does not explicitly disable uploads, then there is a
chance of uploading
On 2015/11/23 19:41:22, acleung1 wrote:
> I usually prefer a less passive tone in documentation to make the point less
> ambiguous.
> 
> "Enable crash uploading based on user's preference when an overriding flag
does
> not exist in commandline"
> 

Done.

[mimee, 2015-11-23 22:46:05]

https://codereview.chromium.org/1461883003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1461883003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:73:
if (!crashDumpUploadingDisabled) {
Sorry for yet another naming comment. If this is only used for command line, can
we make this extra clear in this method? Something like
crashDumpUploadingDisabled -> crashUploadCommandLineDisabled. In this way, those
changing this part of code in the future can easily see that it is not a
preference setter for normal chrome.

[jchinlee, 2015-12-01 18:14:32]

On 2015/11/24 00:41:59, jchinlee wrote:

bump

[mimee1, 2015-12-01 18:25:44]

mimee@google.com changed reviewers:
+ mimee@google.com

[mimee1, 2015-12-01 18:25:45]

lgtm, suggested a comment.

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

https://codereview.chromium.org/1461883003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:44:
mCrashUploadingCommandLineDisabled = true;
Comment: // default flag to disable uploads 
         // unless altered at deferred handler
         // to prevent unwanted uploads at startup.

[jchinlee, 2015-12-03 22:40:26]

On 2015/12/01 18:25:45, mimee1 wrote:
> lgtm, suggested a comment.
> 
>
https://codereview.chromium.org/1461883003/diff/80001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java
> (right):
> 
>
https://codereview.chromium.org/1461883003/diff/80001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:44:
> mCrashUploadingCommandLineDisabled = true;
> Comment: // default flag to disable uploads 
>          // unless altered at deferred handler
>          // to prevent unwanted uploads at startup.

PTAL

[jchinlee, 2015-12-03 22:40:38]

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

https://codereview.chromium.org/1461883003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:44:
mCrashUploadingCommandLineDisabled = true;
On 2015/12/01 18:25:45, mimee1 wrote:
> Comment: // default flag to disable uploads 
>          // unless altered at deferred handler
>          // to prevent unwanted uploads at startup.

Done.

[acleung1, 2015-12-03 22:51:53]

lgtm

[jchinlee, 2015-12-04 06:20:11]

The CQ bit was checked by jchinlee@chromium.org

[jchinlee, 2015-12-04 06:20:11]

The patchset sent to the CQ was uploaded after l-g-t-m from mimee@google.com
Link to the patchset: https://codereview.chromium.org/1461883003/#ps100001
(title: "Add comment.")

[commit-bot: I haz the power, 2015-12-04 06:20:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461883003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461883003/100001

[commit-bot: I haz the power, 2015-12-04 06:28:18]

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

[commit-bot: I haz the power, 2015-12-04 06:28:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jchinlee, 2015-12-04 08:09:07]

On 2015/12/04 06:28:19, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Needs LGTM from owner---Yaron, would you take a quick look when you get a
chance? Thanks!

[Yaron, 2015-12-04 21:24:27]

lgtm - please associate with a bug and update description

[jchinlee, 2015-12-04 21:43:07]

Description was changed from

==========
Separate out experimental flag behavior.

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

to

==========
Separate out experimental flag behavior for minidump upload.

This sets the default for minidump upload to being disabled from command
line flag (if enabled, then depends on user and network preferences).
Also, separates out the case of command line disabled uploads, such that
the associated minidumps are not uploaded, and also not deleted.

BUG=538340
==========

[jchinlee, 2015-12-04 21:43:36]

On 2015/12/04 21:24:27, Yaron wrote:
> lgtm - please associate with a bug and update description

Done.

[jchinlee, 2015-12-05 00:05:59]

The CQ bit was checked by jchinlee@chromium.org

[commit-bot: I haz the power, 2015-12-05 00:11:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461883003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461883003/100001

[commit-bot: I haz the power, 2015-12-05 02:15:54]

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

[commit-bot: I haz the power, 2015-12-05 02:15:54]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[jchinlee, 2015-12-10 20:02:07]

The CQ bit was checked by jchinlee@chromium.org

[commit-bot: I haz the power, 2015-12-10 20:04:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461883003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461883003/100001

[commit-bot: I haz the power, 2015-12-10 20:16:58]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-12-10 20:17:50]

Description was changed from

==========
Separate out experimental flag behavior for minidump upload.

This sets the default for minidump upload to being disabled from command
line flag (if enabled, then depends on user and network preferences).
Also, separates out the case of command line disabled uploads, such that
the associated minidumps are not uploaded, and also not deleted.

BUG=538340
==========

to

==========
Separate out experimental flag behavior for minidump upload.

This sets the default for minidump upload to being disabled from command
line flag (if enabled, then depends on user and network preferences).
Also, separates out the case of command line disabled uploads, such that
the associated minidumps are not uploaded, and also not deleted.

BUG=538340

Committed: https://crrev.com/83fdc052f41a78ac6290b3a204535fff590fb788
Cr-Commit-Position: refs/heads/master@{#364454}
==========

[commit-bot: I haz the power, 2015-12-10 20:17:50]

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