[Crash Reporting] Only upload Chrome crash reports over unmetered networks.

[Crash Reporting] Only upload Chrome crash reports over unmetered networks.

BUG=702454
TEST=none
R=gsennton@chromium.org, mariakhomenko@chromium.org

Review-Url: https://codereview.chromium.org/2751333004
Cr-Commit-Position: refs/heads/master@{#460227}
Committed: https://chromium.googlesource.com/chromium/src/+/ba20b44face1a92875fab6a62959f68faa400aa6

[Ilya Sherman, 2017-03-17 01:11:40]

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

[commit-bot: I haz the power, 2017-03-17 01:12:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-03-17 01:14:46]

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

[commit-bot: I haz the power, 2017-03-17 01:15:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-03-17 01:15:56]

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

[commit-bot: I haz the power, 2017-03-17 01:16:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-03-17 01:18:51]

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

[Ilya Sherman, 2017-03-17 01:19:02]

https://codereview.chromium.org/2751333004/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java
(right):

https://codereview.chromium.org/2751333004/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java:60:
return CrashReportingNetworkPermissionManager.isNetworkUnmetered(
This class name is a bit unwieldy.  WDYT -- any suggested alternate names?

[commit-bot: I haz the power, 2017-03-17 01:19:23]

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, 2017-03-17 01:43:03]

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

[commit-bot: I haz the power, 2017-03-17 01:43:04]

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

[Maria, 2017-03-17 04:04:29]

https://codereview.chromium.org/2751333004/diff/60001/components/minidump_upl...
File
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/CrashReportingNetworkPermissionManager.java
(right):

https://codereview.chromium.org/2751333004/diff/60001/components/minidump_upl...
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/CrashReportingNetworkPermissionManager.java:14:
public class CrashReportingNetworkPermissionManager {
given that there's nothing specific to crash reporting in this code, you can
just call it NetworkPermissionUtil instead

https://codereview.chromium.org/2751333004/diff/60001/components/minidump_upl...
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/CrashReportingNetworkPermissionManager.java:14:
public class CrashReportingNetworkPermissionManager {
FYI, you are missing a BUILD file change to add this class

[gsennton, 2017-03-20 17:29:01]

lgtm (after changing the CrashReportingNetworkPermissionManager name to
something shorter).

https://codereview.chromium.org/2751333004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java
(left):

https://codereview.chromium.org/2751333004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:78:
NetworkInfo networkInfo = mConnectivityManager.getActiveNetworkInfo();
Huh, strange that this wasn't using
PrivacyPreferencesManager.isWiFiOrEthernetNetwork.
Ah well 
__       __
  \(o.O)/

https://codereview.chromium.org/2751333004/diff/60001/components/minidump_upl...
File
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/CrashReportingNetworkPermissionManager.java
(right):

https://codereview.chromium.org/2751333004/diff/60001/components/minidump_upl...
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/CrashReportingNetworkPermissionManager.java:14:
public class CrashReportingNetworkPermissionManager {
On 2017/03/17 04:04:28, Maria wrote:
> given that there's nothing specific to crash reporting in this code, you can
> just call it NetworkPermissionUtil instead

+1 for the name change

[Ilya Sherman, 2017-03-20 23:03:22]

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

[commit-bot: I haz the power, 2017-03-20 23:04:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-03-20 23:08:18]

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

[commit-bot: I haz the power, 2017-03-20 23:09:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-03-20 23:10:20]

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

[commit-bot: I haz the power, 2017-03-20 23:11:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-03-20 23:11:21]

https://codereview.chromium.org/2751333004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java
(left):

https://codereview.chromium.org/2751333004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:78:
NetworkInfo networkInfo = mConnectivityManager.getActiveNetworkInfo();
On 2017/03/20 17:29:01, gsennton wrote:
> Huh, strange that this wasn't using
> PrivacyPreferencesManager.isWiFiOrEthernetNetwork.
> Ah well 
> __       __
>   \(o.O)/

Yeah, a confused ilya thought that it made more sense to temporarily duplicate
the code, than to risk something going wrong with PrivacyPreferencesManager's
lifetime.  A (hopefully) now less confused ilya also raises an eyebrow =P

https://codereview.chromium.org/2751333004/diff/60001/components/minidump_upl...
File
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/CrashReportingNetworkPermissionManager.java
(right):

https://codereview.chromium.org/2751333004/diff/60001/components/minidump_upl...
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/CrashReportingNetworkPermissionManager.java:14:
public class CrashReportingNetworkPermissionManager {
On 2017/03/17 04:04:28, Maria wrote:
> FYI, you are missing a BUILD file change to add this class

Done.

https://codereview.chromium.org/2751333004/diff/60001/components/minidump_upl...
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/CrashReportingNetworkPermissionManager.java:14:
public class CrashReportingNetworkPermissionManager {
On 2017/03/17 04:04:28, Maria wrote:
> given that there's nothing specific to crash reporting in this code, you can
> just call it NetworkPermissionUtil instead

Done.

[Maria, 2017-03-20 23:23:39]

mariakhomenko@chromium.org changed reviewers:
+ yolandyan@google.com

[Maria, 2017-03-20 23:23:40]

lgtm

Yoland, a question for you. Can parameter sets be used in roboelectric unit
tests today? I thought this test might be a good candidate.

https://codereview.chromium.org/2751333004/diff/120001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java
(right):

https://codereview.chromium.org/2751333004/diff/120001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java:52:
runTest(CONNECTED, UNMETERED, METRICS_REPORTING_ENABLED,
METRICS_UPLOAD_PERMITTED,
So I recently saw a presentation on junit4 and using ParameterSet to do exactly
this (permutation of function calls into a helper). It was in the context of
instrumentation, but I wonder if that works in roboelectric too. I am going to
add +yolandyan@chromium.org here and he should be able to tell you the syntax if
it's available. Makes these sorts of tests look nicer.

[Maria, 2017-03-20 23:24:11]

mariakhomenko@chromium.org changed reviewers:
+ yolandyan@chromium.org
- yolandyan@google.com

[commit-bot: I haz the power, 2017-03-20 23:47:14]

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

[commit-bot: I haz the power, 2017-03-20 23:47:15]

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

[Ilya Sherman, 2017-03-21 00:33:26]

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

[commit-bot: I haz the power, 2017-03-21 00:34:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[the real yoland, 2017-03-21 00:35:02]

On 2017/03/20 at 23:23:40, mariakhomenko wrote:
> lgtm
> 
> Yoland, a question for you. Can parameter sets be used in roboelectric unit
tests today? I thought this test might be a good candidate.
> 
>
https://codereview.chromium.org/2751333004/diff/120001/chrome/android/junit/s...
> File
chrome/android/junit/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java
(right):
> 
>
https://codereview.chromium.org/2751333004/diff/120001/chrome/android/junit/s...
>
chrome/android/junit/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java:52:
runTest(CONNECTED, UNMETERED, METRICS_REPORTING_ENABLED,
METRICS_UPLOAD_PERMITTED,
> So I recently saw a presentation on junit4 and using ParameterSet to do
exactly this (permutation of function calls into a helper). It was in the
context of instrumentation, but I wonder if that works in roboelectric too. I am
going to add +yolandyan@chromium.org here and he should be able to tell you the
syntax if it's available. Makes these sorts of tests look nicer.

Yes, it can, it was designed to work with any Junit4 runners

The new parameter test framework can help in this case by changing this one
monolithic into multiple test case and make sure they all run.

The parameter CL is still under review and I can help to convert this using
parameter testing once it lands (https://codereview.chromium.org/2568633002).
Feel free to add me as a TODO!

Thanks a lot for looping me in!

[commit-bot: I haz the power, 2017-03-21 02:15:58]

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

[commit-bot: I haz the power, 2017-03-21 02:15:59]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-03-21 22:10:50]

I'm going to hold off on submitting this CL until the previous implementation is
merged to M58, so as to not cloud the data.

On 2017/03/21 00:35:02, the real yoland wrote:
> On 2017/03/20 at 23:23:40, mariakhomenko wrote:
> > lgtm
> > 
> > Yoland, a question for you. Can parameter sets be used in roboelectric unit
> tests today? I thought this test might be a good candidate.
> > 
> >
>
https://codereview.chromium.org/2751333004/diff/120001/chrome/android/junit/s...
> > File
>
chrome/android/junit/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java
> (right):
> > 
> >
>
https://codereview.chromium.org/2751333004/diff/120001/chrome/android/junit/s...
> >
>
chrome/android/junit/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerTest.java:52:
> runTest(CONNECTED, UNMETERED, METRICS_REPORTING_ENABLED,
> METRICS_UPLOAD_PERMITTED,
> > So I recently saw a presentation on junit4 and using ParameterSet to do
> exactly this (permutation of function calls into a helper). It was in the
> context of instrumentation, but I wonder if that works in roboelectric too. I
am
> going to add mailto:+yolandyan@chromium.org here and he should be able to tell
you the
> syntax if it's available. Makes these sorts of tests look nicer.
> 
> Yes, it can, it was designed to work with any Junit4 runners
> 
> The new parameter test framework can help in this case by changing this one
> monolithic into multiple test case and make sure they all run.
> 
> The parameter CL is still under review and I can help to convert this using
> parameter testing once it lands (https://codereview.chromium.org/2568633002).
> Feel free to add me as a TODO!
> 
> Thanks a lot for looping me in!

Thanks!  I've added a TODO for you in the latest patch set.

[Ilya Sherman, 2017-03-28 21:27:26]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2017-03-28 21:27:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
gsennton@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2751333004/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-28 21:27:38]

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, 2017-03-28 21:37:31]

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

[commit-bot: I haz the power, 2017-03-28 21:37:33]

Exceeded global retry quota

[Ilya Sherman, 2017-03-28 21:43:38]

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

[commit-bot: I haz the power, 2017-03-28 21:43:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-03-28 21:43:55]

isherman@chromium.org changed reviewers:
+ tobiasjs@chromium.org

[Ilya Sherman, 2017-03-28 21:43:56]

+Toby for android_webview OWNERS review

[commit-bot: I haz the power, 2017-03-28 21:53:36]

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

[commit-bot: I haz the power, 2017-03-28 21:53:37]

Dry run: Exceeded global retry quota

[Tobias Sargeant, 2017-03-28 21:59:25]

On 2017/03/28 21:43:56, Ilya Sherman wrote:
> +Toby for android_webview OWNERS review

android_webview LGTM.

[Ilya Sherman, 2017-03-28 22:12:18]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2017-03-28 22:12:56]

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, 2017-03-28 22:44:55]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490739138609400,
"parent_rev": "feb94cb92b94f6cba3a8ba53d1b30c2bd617ae74", "commit_rev":
"ba20b44face1a92875fab6a62959f68faa400aa6"}

[commit-bot: I haz the power, 2017-03-28 22:45:52]

Description was changed from

==========
[Crash Reporting] Only upload Chrome crash reports over unmetered networks.

BUG=702454
TEST=none
R=gsennton@chromium.org, mariakhomenko@chromium.org
==========

to

==========
[Crash Reporting] Only upload Chrome crash reports over unmetered networks.

BUG=702454
TEST=none
R=gsennton@chromium.org, mariakhomenko@chromium.org

Review-Url: https://codereview.chromium.org/2751333004
Cr-Commit-Position: refs/heads/master@{#460227}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba20b44face1a92875fab6a62959...
==========

[commit-bot: I haz the power, 2017-03-28 22:45:53]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/ba20b44face1a92875fab6a62959...