Move minidump renaming from MinidumpUploadService.

Move minidump renaming from MinidumpUploadService.

Most of the minidump renaming happens in MinidumpUploadService but some
of it lives in MinidumpUploadCallable, move the rest of the calls into
the service class.

BUG=659938

[gsennton, 2016-10-31 11:56:53]

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

[gsennton, 2016-10-31 11:56:54]

ptal :)

[Ilya Sherman, 2016-10-31 19:48:13]

LGTM... though if you're not planning to componentize the Service, is this still
useful?  Won't WebView need to reimplement the logic if it's in the Service?

https://codereview.chromium.org/2464823002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java
(right):

https://codereview.chromium.org/2464823002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:245:
// Only update UMA stats if an intended and successful upload.
I'm a bit confused by this comment.  Do you know what it means?  (I know it's
unrelated to your CL -- just very near to the code you're modifying :-})

[gsennton, 2016-10-31 19:54:30]

Yeah, it's not strictly necessary for the componentization - but it seems like
it makes the MinidumpUploadCallable interface cleaner :)

https://codereview.chromium.org/2464823002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java
(right):

https://codereview.chromium.org/2464823002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:245:
// Only update UMA stats if an intended and successful upload.
On 2016/10/31 19:48:13, Ilya Sherman wrote:
> I'm a bit confused by this comment.  Do you know what it means?  (I know it's
> unrelated to your CL -- just very near to the code you're modifying :-})

IIUC we only have UMA stats for successful uploads, and for when we retry too
many times - I guess the comment is trying to imply that we don't update the UMA
stats unless we hit one of these two cases? (or at least the success-case).

[Maria, 2016-11-01 16:07:58]

lgtm

[gsennton, 2016-11-01 16:28:27]

The CQ bit was checked by gsennton@chromium.org

[commit-bot: I haz the power, 2016-11-01 16:28:52]

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-11-01 17:38:23]

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

[commit-bot: I haz the power, 2016-11-01 17:38:24]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gsennton, 2016-11-02 16:31:11]

Ah right, we have tests in MinidumpUploadCallableTest that ensures that we are
renaming files correctly, I'd have to replace these (or at least the
file-name-checking) and add some replacements in MinidumpUploadServiceTest
instead.

Not sure if it's worth it :P

[gsennton, 2016-11-04 09:37:16]

Given that this change needs new tests and is just a new potential for breakage
(if I miss to add some test) I think this CL should just be abandoned - if it
turns out that we will need this change for WebView componentization later on I
can just continue then.

I think we should just go ahead and land
https://codereview.chromium.org/2464943002/
instead so we get the componentization out of the way :)

[gsennton, 2016-11-04 09:37:29]

Description was changed from

==========
Move minidump renaming from MinidumpUploadService.

Most of the minidump renaming happens in MinidumpUploadService but some
of it lives in MinidumpUploadCallable, move the rest of the calls into
the service class.

BUG=659938
==========

to

==========
Move minidump renaming from MinidumpUploadService.

Most of the minidump renaming happens in MinidumpUploadService but some
of it lives in MinidumpUploadCallable, move the rest of the calls into
the service class.

BUG=659938
==========

[Ilya Sherman, 2016-11-05 01:21:38]

On 2016/11/04 09:37:16, gsennton wrote:
> Given that this change needs new tests and is just a new potential for
breakage
> (if I miss to add some test) I think this CL should just be abandoned - if it
> turns out that we will need this change for WebView componentization later on
I
> can just continue then.
> 
> I think we should just go ahead and land
> https://codereview.chromium.org/2464943002/
> instead so we get the componentization out of the way :)

SGTM -- thanks