Componentize parts of minidump uploading for use from WebView.

Componentize parts of minidump uploading for use from WebView.

We want to be able to upload minidumps for WebView. To do this we will
reuse some of the code in Chrome that handles minidump files on disk and
the file for uploading minidump files.
The current CL only moves minor utility classes to the new component -
the rest of the componentization will be performed in another CL.

BUG=652719
Committed: https://crrev.com/157e49c9cbbfc0691c3cd2440c6d9b3d50a63e87
Cr-Commit-Position: refs/heads/master@{#424395}

[gsennton, 2016-10-06 13:44:03]

gsennton@chromium.org changed reviewers:
+ blundell@chromium.org, isherman@chromium.org, mariakhomenko@chromium.org

[gsennton, 2016-10-06 13:44:03]

blundell@chromium.org for adding a new component (the changes in components/)

mariakhomenko@chromium.org for chrome/android/
(and note that I'm adding you in components/minidump_uploader/OWNERS)

isherman: note that I'm adding you to components/minidump_uploader/OWNERS

I have a doc explaining what we (WebView) want to do here, it should be shared
with all of you now.

[blundell, 2016-10-07 06:58:19]

As I noted on the design doc, I would like to see either:

(1) We only move files used by WebView, or
(2) We move the full functionality

I would guess that (1) is more appealing than (2) from your perspective. Are the
files you're moving in this CL used by WebView?

[Ilya Sherman, 2016-10-07 07:08:39]

It looks like there are a number of stray diffs introduced on lines that you're
not otherwise editing, due to running clang format.  I'm personally fine with
mass clang formatting the crash files, but I'd prefer if you could do that in a
separate CL.  It's distracting to see a bunch of diffs that are not actually
related to the changes that you're making, and it will be perhaps even more
distracting, when looking through code history.

https://codereview.chromium.org/2394763004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2394763004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:231:
public static final int MAX_TRIES_ALLOWED = 3;
This concept seems more appropriate for the minidump upload code.  What's the
reason for moving it to the crash file manager?

Also, if you do move it here, please move it to the top of the file, with the
other constants.

https://codereview.chromium.org/2394763004/diff/20001/chrome/android/javatest...
File chrome/android/javatests/DEPS (right):

https://codereview.chromium.org/2394763004/diff/20001/chrome/android/javatest...
chrome/android/javatests/DEPS:9: "+components/minidump_uploader",
nit: Please alphabetize.

https://codereview.chromium.org/2394763004/diff/20001/components/minidump_upl...
File components/minidump_uploader/OWNERS (right):

https://codereview.chromium.org/2394763004/diff/20001/components/minidump_upl...
components/minidump_uploader/OWNERS:2: isherman@chromium.org
Please add some comments indicating the review expectations -- I'm happy to
review for crash upload mechanics changes, but am not comfortable approving for
Java "readability".  Maria is more inclined toward reviewing the Java best
practices, and I think less inclined toward reviewing the crash upload mechanics
(though I'm not sure if I'm correctly capturing her preferences).

[gsennton, 2016-10-07 09:58:13]

WebView needs CrashFileManager and MinidumpUploadCallable.
MinidumpUploadCallable uses HttpURLConnectionFactory[Impl] so the thought with
this CL is to just move that 'leaf-node' (HttpURLConnectionFactory) to the new
component (to make this change as simple as possible - and not really affect the
actual implementation).

As isherman@ pointed out I'm doing some formatting changes as well, and doing
minor changes in CrashFileManager. I agree that this should just be done in a
separate CL to keep the changes as separated as possible.

https://codereview.chromium.org/2394763004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2394763004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:231:
public static final int MAX_TRIES_ALLOWED = 3;
On 2016/10/07 07:08:39, Ilya Sherman wrote:
> This concept seems more appropriate for the minidump upload code.  What's the
> reason for moving it to the crash file manager?
> 
> Also, if you do move it here, please move it to the top of the file, with the
> other constants.

Alright, so I chatted a bit with Toby (tobiasjs@) about this and we think it
makes more sense to not use the MAX_TRIES_ALLOWED in this file at all. The
reason I moved it here because we use it in one place (getMatchingFiles) to
ensure files that have been retried too many times aren't returned in minidump
queries.

We think it makes more sense to - instead of incrementing the tries for a file
that we have tried to upload MAX_TRIES_ALLOWED number of times - rename the file
so that it won't be included in the queries for files to upload. In this way the
CrashFileManager doesn't need to know MAX_TRIES_ALLOWED, and any retry limit can
instead be handled by the classes using CrashFileManager (in a way
CrashFileManager shouldn't depend on the rest of the services in this folder
IMO, so removing the dependency on MinidumpUploadService.MAX_TRIES_ALLOWED is
good for the overall code health here :)).

[blundell, 2016-10-07 13:16:28]

new component lgtm

Could you add a README.md to the new component explaining its purpose and use
case(s)?

[gsennton, 2016-10-10 18:25:06]

On 2016/10/07 13:16:28, blundell wrote:
> new component lgtm
> 
> Could you add a README.md to the new component explaining its purpose and use
> case(s)?

Added a README - not super descriptive.. let me know if you want more info in
there.

Maria and Ilya, could you ptal again? :)
(Maria for chrome/android/ + component/minidump_uploader/OWNERS and Ilya since
you will also be an OWNER)

[Ilya Sherman, 2016-10-10 23:55:27]

LGTM, thanks!

https://codereview.chromium.org/2394763004/diff/60001/components/minidump_upl...
File components/minidump_uploader/README.md (right):

https://codereview.chromium.org/2394763004/diff/60001/components/minidump_upl...
components/minidump_uploader/README.md:3: The minidump\_uploader component
provides functionality for uploading
nit: Probably remove the backslash on this line -- unless it's needed for MD
parsing?

[Maria, 2016-10-11 04:15:11]

lgtm

https://codereview.chromium.org/2394763004/diff/60001/components/minidump_upl...
File components/minidump_uploader/README.md (right):

https://codereview.chromium.org/2394763004/diff/60001/components/minidump_upl...
components/minidump_uploader/README.md:5: that is useful when debugging.
Slight rewording suggestion: The minidump_uploader component provides
functionality for uploading crash information to Google servers. Minidumps
contain crash stack traces and other metadata useful for debugging.

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

https://codereview.chromium.org/2394763004/diff/60001/components/minidump_upl...
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/HttpURLConnectionFactory.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Update copyright year when moving files.

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

https://codereview.chromium.org/2394763004/diff/60001/components/minidump_upl...
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/HttpURLConnectionFactoryImpl.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
2016

[gsennton, 2016-10-11 08:27:57]

Alright! Committing :)

https://codereview.chromium.org/2394763004/diff/60001/components/minidump_upl...
File components/minidump_uploader/README.md (right):

https://codereview.chromium.org/2394763004/diff/60001/components/minidump_upl...
components/minidump_uploader/README.md:3: The minidump\_uploader component
provides functionality for uploading
On 2016/10/10 23:55:27, Ilya Sherman wrote:
> nit: Probably remove the backslash on this line -- unless it's needed for MD
> parsing?

Done (my editor is highlighting underscores in an ugly way unless I backslash
them ;)).

https://codereview.chromium.org/2394763004/diff/60001/components/minidump_upl...
components/minidump_uploader/README.md:5: that is useful when debugging.
On 2016/10/11 04:15:11, Maria wrote:
> Slight rewording suggestion: The minidump_uploader component provides
> functionality for uploading crash information to Google servers. Minidumps
> contain crash stack traces and other metadata useful for debugging.

Done.

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

https://codereview.chromium.org/2394763004/diff/60001/components/minidump_upl...
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/HttpURLConnectionFactory.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/10/11 04:15:11, Maria wrote:
> Update copyright year when moving files.

Done.

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

https://codereview.chromium.org/2394763004/diff/60001/components/minidump_upl...
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/HttpURLConnectionFactoryImpl.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/10/11 04:15:11, Maria wrote:
> 2016

Done.

[gsennton, 2016-10-11 08:28:11]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2016-10-11 08:28:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, isherman@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2394763004/#ps80001
(title: "Minor changes - README and copyrights.")

[commit-bot: I haz the power, 2016-10-11 08:28:25]

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-10-11 09:24:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-11 09:25:51]

Description was changed from

==========
Componentize parts of minidump uploading for use from WebView.

We want to be able to upload minidumps for WebView. To do this we will
reuse some of the code in Chrome that handles minidump files on disk and
the file for uploading minidump files.
The current CL only moves minor utility classes to the new component -
the rest of the componentization will be performed in another CL.

BUG=652719
==========

to

==========
Componentize parts of minidump uploading for use from WebView.

We want to be able to upload minidumps for WebView. To do this we will
reuse some of the code in Chrome that handles minidump files on disk and
the file for uploading minidump files.
The current CL only moves minor utility classes to the new component -
the rest of the componentization will be performed in another CL.

BUG=652719

Committed: https://crrev.com/157e49c9cbbfc0691c3cd2440c6d9b3d50a63e87
Cr-Commit-Position: refs/heads/master@{#424395}
==========

[commit-bot: I haz the power, 2016-10-11 09:25:53]

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