[BlobAsync] Adding better error reporting and some new tests.

[BlobAsync] Adding better error reporting and some new tests.

R=michaeln@chromium.org,kinuko@chromium.org
BUG=599416
Committed: https://crrev.com/e13cd7dcf1264b164c7a9e7cd463ff1885e4b68b
Cr-Commit-Position: refs/heads/master@{#385254}

[dmurph, 2016-04-01 22:21:20]

Description was changed from

==========
[Blob] Adding better error reporting and some new tests.

R=michaeln@chromium.org
BUG=599416
==========

to

==========
[Blob] Adding better error reporting and some new tests.

R=michaeln@chromium.org,kinuko@chromium.org
BUG=599416
==========

[dmurph, 2016-04-01 22:22:15]

Hello!

I need some better error reporting in UMA to help debug some flaky failures (and
reporting correct failures as well to http). Can you PTAL?

Thanks,
Daniel

[dmurph, 2016-04-01 22:44:09]

dmurph@chromium.org changed reviewers:
+ kinuko@chromium.org

[dmurph, 2016-04-01 22:44:25]

Added Kinuko for content/browser/blob_storage/*

[dmurph, 2016-04-04 17:51:14]

dmurph@chromium.org changed reviewers:
+ jsbell@chromium.org

[dmurph, 2016-04-04 17:51:15]

+jsbell, if you have a free second can you PTAL? I'd like to get this patch in
ASAP so I can get better UMA metrics and error reporting on the perf bots.

[jsbell, 2016-04-04 20:41:20]

https://codereview.chromium.org/1846363002/diff/20001/content/browser/blob_st...
File content/browser/blob_storage/blob_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/1846363002/diff/20001/content/browser/blob_st...
content/browser/blob_storage/blob_dispatcher_host_unittest.cc:381:
EXPECT_TRUE(built) << static_cast<int>(error_code);
Maybe add `<< "Error code: "` to explain what this # is in the output?

(here and below)

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
storage/browser/blob/blob_async_builder_host.cc:48: // For most cases we
propogate the error.
Typo: propagate

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
storage/browser/blob/blob_async_builder_host.cc:55: // why (the
BLOB_DEREFERENCED_WHILE_BUILDING should never happen, as we hold
Do we want to DCHECK so we'd notice this during development?

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
storage/browser/blob/blob_async_builder_host.cc:411: IPCBlobCreationCancelCode
reason) {
ISTM we should combine these, e.g. ResultCode w/ OK/SUCCESS as an option. Is
there any reason not to?

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
File storage/browser/blob/blob_data_handle.cc (right):

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
storage/browser/blob/blob_data_handle.cc:135: done.Run(false,
IPCBlobCreationCancelCode::UNKNOWN);
Add a code for this case?

https://codereview.chromium.org/1846363002/diff/20001/storage/common/blob_sto...
File storage/common/blob_storage/blob_storage_constants.h (right):

https://codereview.chromium.org/1846363002/diff/20001/storage/common/blob_sto...
storage/common/blob_storage/blob_storage_constants.h:37: // We couldn't create
or write to a file. File system error, like a full hdd.
nit: just say "disk"

[dmurph, 2016-04-04 22:07:46]

Description was changed from

==========
[Blob] Adding better error reporting and some new tests.

R=michaeln@chromium.org,kinuko@chromium.org
BUG=599416
==========

to

==========
[BlobAsync] Adding better error reporting and some new tests.

R=michaeln@chromium.org,kinuko@chromium.org
BUG=599416
==========

[dmurph, 2016-04-04 22:17:43]

Thanks!

https://codereview.chromium.org/1846363002/diff/20001/content/browser/blob_st...
File content/browser/blob_storage/blob_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/1846363002/diff/20001/content/browser/blob_st...
content/browser/blob_storage/blob_dispatcher_host_unittest.cc:381:
EXPECT_TRUE(built) << static_cast<int>(error_code);
On 2016/04/04 at 20:41:19, jsbell wrote:
> Maybe add `<< "Error code: "` to explain what this # is in the output?
> 
> (here and below)

Done.

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
storage/browser/blob/blob_async_builder_host.cc:48: // For most cases we
propogate the error.
On 2016/04/04 at 20:41:19, jsbell wrote:
> Typo: propagate

Done.

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
storage/browser/blob/blob_async_builder_host.cc:55: // why (the
BLOB_DEREFERENCED_WHILE_BUILDING should never happen, as we hold
On 2016/04/04 at 20:41:19, jsbell wrote:
> Do we want to DCHECK so we'd notice this during development?

Done.

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
storage/browser/blob/blob_async_builder_host.cc:411: IPCBlobCreationCancelCode
reason) {
On 2016/04/04 at 20:41:19, jsbell wrote:
> ISTM we should combine these, e.g. ResultCode w/ OK/SUCCESS as an option. Is
there any reason not to?

YES this is definitely better. However, this would be a much larger change, so
I'd like to do that in a followup CL. Bug here:
crbug.com/600528

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
File storage/browser/blob/blob_data_handle.cc (right):

https://codereview.chromium.org/1846363002/diff/20001/storage/browser/blob/bl...
storage/browser/blob/blob_data_handle.cc:135: done.Run(false,
IPCBlobCreationCancelCode::UNKNOWN);
On 2016/04/04 at 20:41:19, jsbell wrote:
> Add a code for this case?

This is a crazy edge case, where the context is even around (so our system is
shutting down). I'm not sure if this warrants an enum value. Michael, WDYT?

https://codereview.chromium.org/1846363002/diff/20001/storage/common/blob_sto...
File storage/common/blob_storage/blob_storage_constants.h (right):

https://codereview.chromium.org/1846363002/diff/20001/storage/common/blob_sto...
storage/common/blob_storage/blob_storage_constants.h:37: // We couldn't create
or write to a file. File system error, like a full hdd.
On 2016/04/04 at 20:41:19, jsbell wrote:
> nit: just say "disk"

Done.

[kinuko, 2016-04-05 09:42:35]

lgtm

https://codereview.chromium.org/1846363002/diff/100001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1846363002/diff/100001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:47: IPCBlobCreationCancelCode
TransformReferencedBlobErrorToConstructingError(
nit: I feel 'Convert' is more common than 'Transform' for converting error codes

https://codereview.chromium.org/1846363002/diff/100001/storage/browser/blob/b...
File storage/browser/blob/blob_reader.cc (right):

https://codereview.chromium.org/1846363002/diff/100001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:43: int
TransformBlobErrorToNetError(IPCBlobCreationCancelCode reason) {
nit: ditto, ConvertBlobError?

[dmurph, 2016-04-05 18:15:23]

https://codereview.chromium.org/1846363002/diff/100001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/1846363002/diff/100001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:47: IPCBlobCreationCancelCode
TransformReferencedBlobErrorToConstructingError(
On 2016/04/05 at 09:42:35, kinuko wrote:
> nit: I feel 'Convert' is more common than 'Transform' for converting error
codes

Done.

https://codereview.chromium.org/1846363002/diff/100001/storage/browser/blob/b...
File storage/browser/blob/blob_reader.cc (right):

https://codereview.chromium.org/1846363002/diff/100001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:43: int
TransformBlobErrorToNetError(IPCBlobCreationCancelCode reason) {
On 2016/04/05 at 09:42:35, kinuko wrote:
> nit: ditto, ConvertBlobError?

Done.

[dmurph, 2016-04-05 18:16:14]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2016-04-05 18:16:14]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1846363002/#ps120001
(title: "comments")

[commit-bot: I haz the power, 2016-04-05 18:16:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1846363002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1846363002/120001

[commit-bot: I haz the power, 2016-04-05 19:27:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-05 19:29:25]

Description was changed from

==========
[BlobAsync] Adding better error reporting and some new tests.

R=michaeln@chromium.org,kinuko@chromium.org
BUG=599416
==========

to

==========
[BlobAsync] Adding better error reporting and some new tests.

R=michaeln@chromium.org,kinuko@chromium.org
BUG=599416

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

[commit-bot: I haz the power, 2016-04-05 19:29:26]

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