[Android] Wire up manual crash uploads.

[Android] Wire up manual crash uploads.

BUG=641628
R=gayane@chromium.org

Committed: https://crrev.com/4fb2ead4ab711de980cad0f0a1fcae906ebd5c52
Cr-Commit-Position: refs/heads/master@{#420227}

[Ilya Sherman, 2016-09-09 00:49:28]

Hi Gayane, PTAL.  I still need to update the tests to account for the revised
structure of the Java class, but otherwise I believe that this is functional and
ready for review.

One issue that I noticed, though, is that the chrome://crashes UI reloads before
the crash report has a chance to be uploaded.  As a result, it looks like the
manual upload fails, until the user refreshes the page again.  Do you have any
thoughts on how best to address this issue?

[gayane -on leave until 09-2017, 2016-09-09 18:18:29]

Looks good.

For addressing the issue for updating the crash UI. I think if we show forced
crash reports with correct UserRequested state then UI will show that the crash
report was marked for a forced upload and now "Send now" link would be shown for
that. 
Currently as I understand if you press "Send now" the report will just disappear
from the list right?

https://codereview.chromium.org/2324983002/diff/20001/chrome/browser/crash_up...
File chrome/browser/crash_upload_list/crash_upload_list_android.cc (right):

https://codereview.chromium.org/2324983002/diff/20001/chrome/browser/crash_up...
chrome/browser/crash_upload_list/crash_upload_list_android.cc:42: const char
skipped_uploads[] = ".skipped";
We should also show .forced crashes here it should have 
UploadList::UploadInfo::State::Userrequested state instead of
UploadList::UploadInfo::State::NotUploaded

[Ilya Sherman, 2016-09-09 23:02:56]

Thanks, Gayane =)

https://codereview.chromium.org/2324983002/diff/20001/chrome/browser/crash_up...
File chrome/browser/crash_upload_list/crash_upload_list_android.cc (right):

https://codereview.chromium.org/2324983002/diff/20001/chrome/browser/crash_up...
chrome/browser/crash_upload_list/crash_upload_list_android.cc:42: const char
skipped_uploads[] = ".skipped";
On 2016/09/09 18:18:29, gayane wrote:
> We should also show .forced crashes here it should have 
> UploadList::UploadInfo::State::Userrequested state instead of
> UploadList::UploadInfo::State::NotUploaded

Done.

[Ilya Sherman, 2016-09-09 23:04:21]

isherman@chromium.org changed reviewers:
+ dbeam@chromium.org, mariakhomenko@chromium.org, mark@chromium.org

[Ilya Sherman, 2016-09-09 23:04:22]

Mark, could you PTAL at the *upload_list* files?
Maria, could you PTAL at the Java files, and possibly also the
crash_upload_list_android.cc changes?
Dan, could you PTAL at the webui handler changes?

[Dan Beam, 2016-09-09 23:05:36]

webui lgtm

[gayane -on leave until 09-2017, 2016-09-09 23:49:46]

Thanks.

[gayane -on leave until 09-2017, 2016-09-09 23:49:59]

lgtm

[Ilya Sherman, 2016-09-14 19:53:02]

Mark and Maria, could you PTAL?  Thanks.

[Mark Mentovai, 2016-09-15 00:02:30]

Cool, LGTM.

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/crash_up...
File chrome/browser/crash_upload_list/crash_upload_list_android.cc (right):

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/crash_up...
chrome/browser/crash_upload_list/crash_upload_list_android.cc:53: if
(file.value().find(manually_forced_uploads) != std::string::npos) {
base::EndsWith() from base/strings/string_util.h? Oh, I guess not, given the
.try0 stuff mentioned below.

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/crash_up...
chrome/browser/crash_upload_list/crash_upload_list_android.cc:59: continue;
What are these, no upload attempted yet? I guess this is OK as preserving the
status quo, but we would still list those for Crashpad-using platforms.

We do have UploadList::UploadInfo::State::Pending for this state. But the 20 or
so lines that follow probably aren’t totally right for those files, so it’d take
a little more work.

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/crashes_ui.cc (left):

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/crashes_ui.cc:96: #if defined(OS_WIN) ||
defined(OS_MACOSX)
Is it helpful to get rid of these rather than add OS_ANDROID? It’s dead weight
on, say, Chrome OS.

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/crashes_ui.cc (right):

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/crashes_ui.cc:176: // Maunal uploads currently are
supported only for Crashpad-using platforms or
and

https://codereview.chromium.org/2324983002/diff/40001/components/upload_list/...
File components/upload_list/upload_list.cc (left):

https://codereview.chromium.org/2324983002/diff/40001/components/upload_list/...
components/upload_list/upload_list.cc:142: #if defined(OS_WIN) ||
defined(OS_MACOSX)
Ditto

https://codereview.chromium.org/2324983002/diff/40001/components/upload_list/...
File components/upload_list/upload_list.cc (right):

https://codereview.chromium.org/2324983002/diff/40001/components/upload_list/...
components/upload_list/upload_list.cc:149: // Manual uploads for not yet
uploaded crash reports are only available for
not-yet

[Maria, 2016-09-15 21:46:33]

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

https://codereview.chromium.org/2324983002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:367:
public static void tryUploadCrashDumpWithLocalId(Context context, String
localId) {
I don't understand why you use an intent to do this rather than just performing
the action.  That's generally slower... If you want it to be asynchronous you
can do that without intents as well.

Do you expect other apps on the system to be sending MinidumpUploadService
commands to do a manual dump?

[Maria, 2016-09-15 21:46:59]

On 2016/09/15 21:46:33, Maria wrote:
>
https://codereview.chromium.org/2324983002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java
> (right):
> 
>
https://codereview.chromium.org/2324983002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:367:
> public static void tryUploadCrashDumpWithLocalId(Context context, String
> localId) {
> I don't understand why you use an intent to do this rather than just
performing
> the action.  That's generally slower... If you want it to be asynchronous you
> can do that without intents as well.
> 
> Do you expect other apps on the system to be sending MinidumpUploadService
> commands to do a manual dump?

(Also sorry for the delay in the code review... It got lost in my inbox)

[Ilya Sherman, 2016-09-21 22:50:03]

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

https://codereview.chromium.org/2324983002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:367:
public static void tryUploadCrashDumpWithLocalId(Context context, String
localId) {
On 2016/09/15 21:46:33, Maria wrote:
> I don't understand why you use an intent to do this rather than just
performing
> the action.  That's generally slower... If you want it to be asynchronous you
> can do that without intents as well.
> 
> Do you expect other apps on the system to be sending MinidumpUploadService
> commands to do a manual dump?

I don't know why this file is implemented entirely using intents, but it is. 
I'm happy to implement a better solution, but will need some more guidance to
get there.

Does it make sense to you why the rest of this file uses intents for all of the
actions?

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/crash_up...
File chrome/browser/crash_upload_list/crash_upload_list_android.cc (right):

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/crash_up...
chrome/browser/crash_upload_list/crash_upload_list_android.cc:53: if
(file.value().find(manually_forced_uploads) != std::string::npos) {
On 2016/09/15 00:02:30, Mark Mentovai wrote:
> base::EndsWith() from base/strings/string_util.h? Oh, I guess not, given the
> .try0 stuff mentioned below.

Acknowledged.

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/crash_up...
chrome/browser/crash_upload_list/crash_upload_list_android.cc:59: continue;
On 2016/09/15 00:02:29, Mark Mentovai wrote:
> What are these, no upload attempted yet? I guess this is OK as preserving the
> status quo, but we would still list those for Crashpad-using platforms.
> 
> We do have UploadList::UploadInfo::State::Pending for this state. But the 20
or
> so lines that follow probably aren’t totally right for those files, so it’d
take
> a little more work.

There's various other junk in the directory, such as logcat files.  Added a
comment.

.dmp files are pending files -- corrected the label for those as well.  Thanks
for catching that =)

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/crashes_ui.cc (left):

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/crashes_ui.cc:96: #if defined(OS_WIN) ||
defined(OS_MACOSX)
On 2016/09/15 00:02:30, Mark Mentovai wrote:
> Is it helpful to get rid of these rather than add OS_ANDROID? It’s dead weight
> on, say, Chrome OS.

I'm kind of assuming the compiler can strip dead code well enough to where we
don't need this sort of micro-optimization.  Am I wrong?  It was a bit tedious
to find all of the spots that needed updating, and it seemed preferable to me to
have only a single preprocessor block that needed to updated.  But, if it's
important to reduce dead weight on other platforms, I can restore all of these
checks.

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/crashes_ui.cc (right):

https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/crashes_ui.cc:176: // Maunal uploads currently are
supported only for Crashpad-using platforms or
On 2016/09/15 00:02:30, Mark Mentovai wrote:
> and

Done.

https://codereview.chromium.org/2324983002/diff/40001/components/upload_list/...
File components/upload_list/upload_list.cc (right):

https://codereview.chromium.org/2324983002/diff/40001/components/upload_list/...
components/upload_list/upload_list.cc:149: // Manual uploads for not yet
uploaded crash reports are only available for
On 2016/09/15 00:02:30, Mark Mentovai wrote:
> not-yet

Done.

[Maria, 2016-09-21 23:04:39]

On 2016/09/21 22:50:03, Ilya Sherman wrote:
>
https://codereview.chromium.org/2324983002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java
> (right):
> 
>
https://codereview.chromium.org/2324983002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:367:
> public static void tryUploadCrashDumpWithLocalId(Context context, String
> localId) {
> On 2016/09/15 21:46:33, Maria wrote:
> > I don't understand why you use an intent to do this rather than just
> performing
> > the action.  That's generally slower... If you want it to be asynchronous
you
> > can do that without intents as well.
> > 
> > Do you expect other apps on the system to be sending MinidumpUploadService
> > commands to do a manual dump?
> 
> I don't know why this file is implemented entirely using intents, but it is. 
> I'm happy to implement a better solution, but will need some more guidance to
> get there.
> 
> Does it make sense to you why the rest of this file uses intents for all of
the
> actions?

Okay, I looked at this file some more and I think I understand. Since this is a
service,
it runs as a separate process. So when we do any of the requests, we load the
code to
create the intent within the browser process and then dispatch the intent, so
that the action
is processed by the service in its own process.

I think it's a bit weird that the code to construct the intent lives in the
service class and that's what confused me here. But what you did is actually
correct.


> 
>
https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/crash_up...
> File chrome/browser/crash_upload_list/crash_upload_list_android.cc (right):
> 
>
https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/crash_up...
> chrome/browser/crash_upload_list/crash_upload_list_android.cc:53: if
> (file.value().find(manually_forced_uploads) != std::string::npos) {
> On 2016/09/15 00:02:30, Mark Mentovai wrote:
> > base::EndsWith() from base/strings/string_util.h? Oh, I guess not, given the
> > .try0 stuff mentioned below.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/crash_up...
> chrome/browser/crash_upload_list/crash_upload_list_android.cc:59: continue;
> On 2016/09/15 00:02:29, Mark Mentovai wrote:
> > What are these, no upload attempted yet? I guess this is OK as preserving
the
> > status quo, but we would still list those for Crashpad-using platforms.
> > 
> > We do have UploadList::UploadInfo::State::Pending for this state. But the 20
> or
> > so lines that follow probably aren’t totally right for those files, so it’d
> take
> > a little more work.
> 
> There's various other junk in the directory, such as logcat files.  Added a
> comment.
> 
> .dmp files are pending files -- corrected the label for those as well.  Thanks
> for catching that =)
> 
>
https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/crashes_ui.cc (left):
> 
>
https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/crashes_ui.cc:96: #if defined(OS_WIN) ||
> defined(OS_MACOSX)
> On 2016/09/15 00:02:30, Mark Mentovai wrote:
> > Is it helpful to get rid of these rather than add OS_ANDROID? It’s dead
weight
> > on, say, Chrome OS.
> 
> I'm kind of assuming the compiler can strip dead code well enough to where we
> don't need this sort of micro-optimization.  Am I wrong?  It was a bit tedious
> to find all of the spots that needed updating, and it seemed preferable to me
to
> have only a single preprocessor block that needed to updated.  But, if it's
> important to reduce dead weight on other platforms, I can restore all of these
> checks.
> 
>
https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/crashes_ui.cc (right):
> 
>
https://codereview.chromium.org/2324983002/diff/40001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/crashes_ui.cc:176: // Maunal uploads currently are
> supported only for Crashpad-using platforms or
> On 2016/09/15 00:02:30, Mark Mentovai wrote:
> > and
> 
> Done.
> 
>
https://codereview.chromium.org/2324983002/diff/40001/components/upload_list/...
> File components/upload_list/upload_list.cc (right):
> 
>
https://codereview.chromium.org/2324983002/diff/40001/components/upload_list/...
> components/upload_list/upload_list.cc:149: // Manual uploads for not yet
> uploaded crash reports are only available for
> On 2016/09/15 00:02:30, Mark Mentovai wrote:
> > not-yet
> 
> Done.

[Maria, 2016-09-21 23:10:51]

lgtm

[Ilya Sherman, 2016-09-21 23:19:01]

Okay, thanks Maria =)

Mark, did you want to take another look, or should I go ahead and commit this
CL?

[Mark Mentovai, 2016-09-21 23:26:14]

Nope, it was LGTM with those changes, so since you’ve made ’em…LGTM.

[Ilya Sherman, 2016-09-21 23:42:08]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-09-21 23:42:08]

The patchset sent to the CQ was uploaded after l-g-t-m from gayane@chromium.org,
dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2324983002/#ps100001
(title: "Fix pending upload handling, and nit fixes")

[commit-bot: I haz the power, 2016-09-21 23:42:36]

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-09-22 01:21:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-22 01:24:12]

Description was changed from

==========
[Android] Wire up manual crash uploads.

BUG=641628
R=gayane@chromium.org
==========

to

==========
[Android] Wire up manual crash uploads.

BUG=641628
R=gayane@chromium.org

Committed: https://crrev.com/4fb2ead4ab711de980cad0f0a1fcae906ebd5c52
Cr-Commit-Position: refs/heads/master@{#420227}
==========

[commit-bot: I haz the power, 2016-09-22 01:24:13]

Patchset 6 (id:??) landed as
https://crrev.com/4fb2ead4ab711de980cad0f0a1fcae906ebd5c52
Cr-Commit-Position: refs/heads/master@{#420227}