Show crash report size in crashes UI for Android.

Show crash report size in crashes UI for Android.

As on Android manual crash uploads can happen on cellular
connection it is important to display the size of the crash reports.

BUG=641628
Committed: https://crrev.com/cc7d54eab860b6c17661ae641a6024514a511f0e
Cr-Commit-Position: refs/heads/master@{#418590}

[gayane -on leave until 09-2017, 2016-09-09 21:26:38]

Description was changed from

==========
Show crash report size for Android

BUG=620762
==========

to

==========
Show crash report size in crashes UI for Android.

As on Android manual crash uploads can happen on cellular connection it is
important to display the size of the crash reports.

BUG=641628
==========

[gayane -on leave until 09-2017, 2016-09-09 21:26:46]

Description was changed from

==========
Show crash report size in crashes UI for Android.

As on Android manual crash uploads can happen on cellular connection it is
important to display the size of the crash reports.

BUG=641628
==========

to

==========
Show crash report size in crashes UI for Android.

As on Android manual crash uploads can happen on cellular
connection it is important to display the size of the crash reports.

BUG=641628
==========

[gayane -on leave until 09-2017, 2016-09-09 21:31:47]

gayane@chromium.org changed reviewers:
+ mark@chromium.org

[gayane -on leave until 09-2017, 2016-09-09 21:31:48]

PTAL

[Mark Mentovai, 2016-09-13 16:13:25]

We should do this for all platforms, not just Android.

https://codereview.chromium.org/2330573002/diff/1/components/crash/core/brows...
File components/crash/core/browser/crashes_ui_util.cc (right):

https://codereview.chromium.org/2330573002/diff/1/components/crash/core/brows...
components/crash/core/browser/crashes_ui_util.cc:81:
crash->SetString("file_size", info.file_size == 0
A crash report’s might accidentally wind up as 0 bytes because of some kind of
problem. I think that we should pass it through and display it even in this case
to help troubleshooting.

We only wouldn’t want to show a size in cases where we can’t get the file size,
like non-Android platforms since you’re currently only implementing this for
Android.

https://codereview.chromium.org/2330573002/diff/1/components/crash_strings.grdp
File components/crash_strings.grdp (right):

https://codereview.chromium.org/2330573002/diff/1/components/crash_strings.gr...
components/crash_strings.grdp:44: Crash report is <ph
name="CRASH_SIZE">$1<ex>10KB</ex></ph>KB.
The way that these messages stack together gets kind of wordy and awkward:

Crash report captured on Tuesday, January 25, 2011 2:58:02 PM (not yet uploaded
or ignored)
Crash report is 10KB.

https://codereview.chromium.org/2330573002/diff/1/components/crash_strings.gr...
components/crash_strings.grdp:44: Crash report is <ph
name="CRASH_SIZE">$1<ex>10KB</ex></ph>KB.
The <ex> shouldn’t have KB in it since the message has KB outside of it.

But…

Don’t we have tools to localize things into kB? Having it be a strict suffix
like this might not always be right.

[gayane -on leave until 09-2017, 2016-09-13 17:39:53]

Patchset #2 (id:20001) has been deleted

[gayane -on leave until 09-2017, 2016-09-13 17:40:23]

Thanks. PTAL

https://codereview.chromium.org/2330573002/diff/1/components/crash/core/brows...
File components/crash/core/browser/crashes_ui_util.cc (right):

https://codereview.chromium.org/2330573002/diff/1/components/crash/core/brows...
components/crash/core/browser/crashes_ui_util.cc:81:
crash->SetString("file_size", info.file_size == 0
On 2016/09/13 16:13:25, Mark Mentovai wrote:
> A crash report’s might accidentally wind up as 0 bytes because of some kind of
> problem. I think that we should pass it through and display it even in this
case
> to help troubleshooting.
> 
> We only wouldn’t want to show a size in cases where we can’t get the file
size,
> like non-Android platforms since you’re currently only implementing this for
> Android.

Done.

https://codereview.chromium.org/2330573002/diff/1/components/crash_strings.grdp
File components/crash_strings.grdp (right):

https://codereview.chromium.org/2330573002/diff/1/components/crash_strings.gr...
components/crash_strings.grdp:44: Crash report is <ph
name="CRASH_SIZE">$1<ex>10KB</ex></ph>KB.
On 2016/09/13 16:13:25, Mark Mentovai wrote:
> The way that these messages stack together gets kind of wordy and awkward:
> 
> Crash report captured on Tuesday, January 25, 2011 2:58:02 PM (not yet
uploaded
> or ignored)
> Crash report is 10KB.

I cannot put them in one message as the crash report size doesn't always appear,
but for not repeating "crash report" two times I have reworded the second
message.

https://codereview.chromium.org/2330573002/diff/1/components/crash_strings.gr...
components/crash_strings.grdp:44: Crash report is <ph
name="CRASH_SIZE">$1<ex>10KB</ex></ph>KB.
On 2016/09/13 16:13:25, Mark Mentovai wrote:
> The <ex> shouldn’t have KB in it since the message has KB outside of it.
> 
> But…
> 
> Don’t we have tools to localize things into kB? Having it be a strict suffix
> like this might not always be right.
Removed KB from example.
I have searched for file size formatting with localization but didn't find
anything useful.

[gayane -on leave until 09-2017, 2016-09-13 17:42:16]

> We should do this for all platforms, not just Android.
Agree, we can do that separately as it requires separate changes to crashpad to
provide that information.

[Mark Mentovai, 2016-09-13 18:21:17]

https://codereview.chromium.org/2330573002/diff/1/components/crash_strings.grdp
File components/crash_strings.grdp (right):

https://codereview.chromium.org/2330573002/diff/1/components/crash_strings.gr...
components/crash_strings.grdp:44: Crash report is <ph
name="CRASH_SIZE">$1<ex>10KB</ex></ph>KB.
gayane wrote:
> On 2016/09/13 16:13:25, Mark Mentovai wrote:
> > The way that these messages stack together gets kind of wordy and awkward:
> > 
> > Crash report captured on Tuesday, January 25, 2011 2:58:02 PM (not yet
> uploaded
> > or ignored)
> > Crash report is 10KB.
> 
> I cannot put them in one message as the crash report size doesn't always
appear,
> but for not repeating "crash report" two times I have reworded the second
> message.

OK. I don’t like what this page is becoming (not your fault). I filed
https://crbug.com/646455 to improve it, and we can go with this for now.

https://codereview.chromium.org/2330573002/diff/1/components/crash_strings.gr...
components/crash_strings.grdp:44: Crash report is <ph
name="CRASH_SIZE">$1<ex>10KB</ex></ph>KB.
gayane wrote:
> I have searched for file size formatting with localization but didn't find
> anything useful.

Can we access ui/base from here? ui/base/text/bytes_formatting.h

https://codereview.chromium.org/2330573002/diff/40001/components/crash_string...
File components/crash_strings.grdp (right):

https://codereview.chromium.org/2330573002/diff/40001/components/crash_string...
components/crash_strings.grdp:44: The size on the local storage is <ph
name="CRASH_SIZE">$1<ex>10</ex></ph>KB.
I’m still not so sure about this string, but I guess we can see where things go
in the bug I filed.

[gayane -on leave until 09-2017, 2016-09-13 20:48:35]

Thanks for the pointer for file size formatting. Changed to use that.

[Mark Mentovai, 2016-09-13 21:05:54]

Great! LGTM

[gayane -on leave until 09-2017, 2016-09-13 21:09:46]

The CQ bit was checked by gayane@chromium.org

[commit-bot: I haz the power, 2016-09-13 21:10:16]

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-13 21:23:18]

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

[commit-bot: I haz the power, 2016-09-13 21:23:19]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[gayane -on leave until 09-2017, 2016-09-14 15:33:06]

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

[commit-bot: I haz the power, 2016-09-14 15:33:26]

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, 2016-09-14 16:42:48]

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

[commit-bot: I haz the power, 2016-09-14 16:42:49]

Dry run: This issue passed the CQ dry run.

[gayane -on leave until 09-2017, 2016-09-14 16:48:42]

The CQ bit was checked by gayane@chromium.org

[gayane -on leave until 09-2017, 2016-09-14 16:48:43]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2330573002/#ps80001
(title: "Add a copy constructor")

[commit-bot: I haz the power, 2016-09-14 16:49:02]

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-14 16:54:09]

Description was changed from

==========
Show crash report size in crashes UI for Android.

As on Android manual crash uploads can happen on cellular
connection it is important to display the size of the crash reports.

BUG=641628
==========

to

==========
Show crash report size in crashes UI for Android.

As on Android manual crash uploads can happen on cellular
connection it is important to display the size of the crash reports.

BUG=641628
==========

[commit-bot: I haz the power, 2016-09-14 16:54:10]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-14 16:56:06]

Description was changed from

==========
Show crash report size in crashes UI for Android.

As on Android manual crash uploads can happen on cellular
connection it is important to display the size of the crash reports.

BUG=641628
==========

to

==========
Show crash report size in crashes UI for Android.

As on Android manual crash uploads can happen on cellular
connection it is important to display the size of the crash reports.

BUG=641628

Committed: https://crrev.com/cc7d54eab860b6c17661ae641a6024514a511f0e
Cr-Commit-Position: refs/heads/master@{#418590}
==========

[commit-bot: I haz the power, 2016-09-14 16:56:07]

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