[Blob] BlobReader class & tests, and removal of all redundant reading.

[Blob] BlobReader class & tests, and removal of all redundant reading.

* New BlobReader class & tests
* Removal of other reading code, which now uses the BlobReader
* Removal of unnecessary UploadDiskCacheEntryElementReader
* Removal of blob expansion logic in UploadDataStreamBuilder

Now it's very easy for anyone in browserland to read blobs instead of
having to do url requests, and it's also easy for anyone to add new
blob backing storage.

This is a prerequisite for the new blob async transportation, see here:
goto/BlobPaging

BUG=138051, 375297
Committed: https://crrev.com/02561552a57ed8792a8ebb2676bad485e1d99605
Cr-Commit-Position: refs/heads/master@{#351470}

Committed: https://crrev.com/164f0b76c3a8156b95c36f17c95c452e8f3b9765
Cr-Commit-Position: refs/heads/master@{#351611}

[dmurph, 2015-09-11 20:18:24]

dmurph@chromium.org changed reviewers:
+ kinuko@chromium.org, michaeln@chromium.org, mmenke@chromium.org

[dmurph, 2015-09-11 20:18:25]

Hello all,

Please take a look at this blob reader patch. It does a lot of cleanup and
consolidation of reading code, and makes working with blobs much easier.

Matt: net/*
You recently reviewed Gavin's patch https://codereview.chromium.org/1108083002.
this patch is that reader we talked about. I'm able to clean up a lot here and
remove some of the redundant reading code that Gavin wrote.  Can you take a look
at the net changes?

Kinuko & Michael: content/browser/* & storage/browser/*
Can you guys take a look?  Let me know if you have any questions.

Thanks so much!
Daniel

patch gif: http://i.imgur.com/TiirqEb.gif

[michaeln, 2015-09-14 18:39:43]

> patch gif: http://i.imgur.com/TiirqEb.gif

Awesome... will take a look today!!

[mmenke, 2015-09-14 19:24:33]

net/ certainly looks good, I'll defer to the other reviewers on the other files.

https://codereview.chromium.org/1337153002/diff/60001/net/base/upload_element...
File net/base/upload_element_reader.h (right):

https://codereview.chromium.org/1337153002/diff/60001/net/base/upload_element...
net/base/upload_element_reader.h:31: AsUploadBlobElementReaderForTests() const;
This method is a layering violation.  Net shouldn't know what an upload blob
element reader is.  I'd suggest just casting instead, much as I dislike explicit
casting.

[dmurph, 2015-09-14 20:27:50]

https://codereview.chromium.org/1337153002/diff/60001/net/base/upload_element...
File net/base/upload_element_reader.h (right):

https://codereview.chromium.org/1337153002/diff/60001/net/base/upload_element...
net/base/upload_element_reader.h:31: AsUploadBlobElementReaderForTests() const;
On 2015/09/14 at 19:24:33, mmenke wrote:
> This method is a layering violation.  Net shouldn't know what an upload blob
element reader is.  I'd suggest just casting instead, much as I dislike explicit
casting.

Sounds good.  Done.

[mmenke, 2015-09-14 20:29:53]

On 2015/09/14 20:27:50, dmurph wrote:
>
https://codereview.chromium.org/1337153002/diff/60001/net/base/upload_element...
> File net/base/upload_element_reader.h (right):
> 
>
https://codereview.chromium.org/1337153002/diff/60001/net/base/upload_element...
> net/base/upload_element_reader.h:31: AsUploadBlobElementReaderForTests()
const;
> On 2015/09/14 at 19:24:33, mmenke wrote:
> > This method is a layering violation.  Net shouldn't know what an upload blob
> element reader is.  I'd suggest just casting instead, much as I dislike
explicit
> casting.
> 
> Sounds good.  Done.

net/ LGTM.  Thanks for doing this!

[michaeln, 2015-09-17 00:45:37]

Not a full review but a first pass at some comments. I really like this change a
lot, thank you for working on this stuff.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
File storage/browser/blob/blob_data_handle.cc (right):

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_data_handle.cc:56: FileSystemContext*
file_system_context_;
what guarantees the ptr isn't dangling? should this be a scoped_refptr<>?

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
File storage/browser/blob/blob_data_handle.h (right):

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_data_handle.h:48: base::SingleThreadTaskRunner*
file_task_runner) const;
Does this really need to be a SingleThreadTaskRunner or would a
SequencedTaskRunner be sufficient?

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

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:44: : blob_handle_(blob_handle),
rawptrs data members make me look twice. i don't think you really need the raw
blob_handle_ ptr data member. in the ctor, grab a snapshot then i think you can
test the snapshot for nullness thereafter.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:67: DCHECK(!total_size_calculated_);
not sure this matters, its not safe to call CalculateSize while a size
calculation is in progress and there's no dcheck about that kind of called twice
misuse.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:81: storage::FileStreamReader* const reader
= FileReaderAtIndex(i);
fyi: i was surprised that FileReaderAtIndex() created the reader if non was
found, the method name makes it sound like a getter and the existence of a
setter made me expect its use instead

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:114: size_done_ = done;
nit: these names sound like bool values, maybe append _callback on them?

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:202: bool BlobReader::IsInMemory() const {
if (!blob_data_) return true?

probably need to test for nullness at least in all public methods

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:269: if (result ==
net::ERR_UPLOAD_FILE_CHANGED) {
why translate this error code?

nit:

if (result = xxx)
  result = yyy;
if (result < 0) {
  InvalidateCallbacksAndDone(result, size_done_);
  return;
}

would make it more clear that the special case is just replacing
ERR_UPLOAD_FILE_CHANGED with ERR_FILE_NOT_FOUND

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:298: void BlobReader::DidCountSize() {
might make sense to have this method be responsible for invoking the size_done_
callback if it's not empty

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:509: uint64_t max_remaining =
std::numeric_limits<int>::max();
nit: maybe call this max_readable or better yet use numeric_limits<int>::max()
directly inline below

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:573: } else if (reader) {
looks possible to leave a nullptr in the map if we're replacing an exiting
reader which contradicts the DCHECK on line 527

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
File storage/browser/blob/blob_reader.h (right):

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:53: virtual FileStreamReader*
CreateForLocalFile(
probably should use scoped_ptr<FileStreamReader> as the return type

i know there are many places in filesystem code where a raw ptrs to newly
created objs are returned, but its more common now for use to be explicit about
the ownership xfer

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:75: //    total_size() value to query the
blob size, as it's uint64_t.
since the callback can't return the size if the value is out of range, i'd vote
to have this function not even try to return the size value via the callback.
Return SUCCESS and require callers to use the total_size() getter thereafter.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:85: // * bytes_read is populated only is
Status::DONE is returned. Otherwise the
typo: only if

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:147: BlobReader::Status
ReadDiskCacheEntryItem(const BlobDataItem& item,
BlobReader:: prefix not needed

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:161: const BlobDataHandle* blob_handle_;
why a rawptr? oh, see .cc comments, is this really needed at all?

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:52: blob_handle_(blob_handle),
rawptrs always make me look twice

I see the URLRequest owns the BlobDataHandle thru the user data mechanism. The
rawptr here is depending on that being the case. I think you should either...

1) DCHECK_EQ(blob_handle_, GetHandlePtrOwnedByRequest());

2) or spring for this class owning its own handle

The latter seems cleaner, it does create a new obj but that's not awful. The
copy ctor makes it easy and cheap to copy handles.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:211:
byte_range_.last_byte_position() + 1);
is the + 1 intentional? odd that first == last + 1 would be expected

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:315:
set_expected_content_size(blob_reader_.get() ? blob_reader_->remaining_bytes()
if status indicates an error, we're not going to return data even if
blob_reader_ is not null, expected size should be 0 in that case. maybe be
pessimistic, set it to 0 up top and reset it in the if (status_code) block to
what the blob_reader says?

[dmurph, 2015-09-19 00:33:44]

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
File storage/browser/blob/blob_data_handle.cc (right):

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_data_handle.cc:56: FileSystemContext*
file_system_context_;
On 2015/09/17 at 00:45:37, michaeln wrote:
> what guarantees the ptr isn't dangling? should this be a scoped_refptr<>?

Sounds good.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
File storage/browser/blob/blob_data_handle.h (right):

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_data_handle.h:48: base::SingleThreadTaskRunner*
file_task_runner) const;
On 2015/09/17 at 00:45:37, michaeln wrote:
> Does this really need to be a SingleThreadTaskRunner or would a
SequencedTaskRunner be sufficient?

Sure.

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

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:44: : blob_handle_(blob_handle),
On 2015/09/17 at 00:45:37, michaeln wrote:
> rawptrs data members make me look twice. i don't think you really need the raw
blob_handle_ ptr data member. in the ctor, grab a snapshot then i think you can
test the snapshot for nullness thereafter.

Sounds good.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:67: DCHECK(!total_size_calculated_);
On 2015/09/17 at 00:45:37, michaeln wrote:
> not sure this matters, its not safe to call CalculateSize while a size
calculation is in progress and there's no dcheck about that kind of called twice
misuse.

Done.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:81: storage::FileStreamReader* const reader
= FileReaderAtIndex(i);
On 2015/09/17 at 00:45:37, michaeln wrote:
> fyi: i was surprised that FileReaderAtIndex() created the reader if non was
found, the method name makes it sound like a getter and the existence of a
setter made me expect its use instead

Replaced with GetOrCreateFileReaderAtIndex

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:114: size_done_ = done;
On 2015/09/17 at 00:45:37, michaeln wrote:
> nit: these names sound like bool values, maybe append _callback on them?

Done.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:202: bool BlobReader::IsInMemory() const {
On 2015/09/17 at 00:45:37, michaeln wrote:
> if (!blob_data_) return true?
> 
> probably need to test for nullness at least in all public methods

Done.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:269: if (result ==
net::ERR_UPLOAD_FILE_CHANGED) {
On 2015/09/17 at 00:45:37, michaeln wrote:
> why translate this error code?
> 
> nit:
> 
> if (result = xxx)
>   result = yyy;
> if (result < 0) {
>   InvalidateCallbacksAndDone(result, size_done_);
>   return;
> }
> 
> would make it more clear that the special case is just replacing
ERR_UPLOAD_FILE_CHANGED with ERR_FILE_NOT_FOUND

Done.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:298: void BlobReader::DidCountSize() {
On 2015/09/17 at 00:45:37, michaeln wrote:
> might make sense to have this method be responsible for invoking the
size_done_ callback if it's not empty

This gets called at the end of both the async and sync pipelines, but I can just
check to see if it's set and only call it then.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:298: void BlobReader::DidCountSize() {
On 2015/09/17 at 00:45:37, michaeln wrote:
> might make sense to have this method be responsible for invoking the
size_done_ callback if it's not empty

This gets called at the end of both the async and sync pipelines, but I can just
check to see if it's set and only call it then.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:509: uint64_t max_remaining =
std::numeric_limits<int>::max();
On 2015/09/17 at 00:45:37, michaeln wrote:
> nit: maybe call this max_readable or better yet use numeric_limits<int>::max()
directly inline below

done.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:573: } else if (reader) {
On 2015/09/17 at 00:45:37, michaeln wrote:
> looks possible to leave a nullptr in the map if we're replacing an exiting
reader which contradicts the DCHECK on line 527

It should be impossible. Edited so that it can never add nullptr.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
File storage/browser/blob/blob_reader.h (right):

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:53: virtual FileStreamReader*
CreateForLocalFile(
On 2015/09/17 at 00:45:37, michaeln wrote:
> probably should use scoped_ptr<FileStreamReader> as the return type
> 
> i know there are many places in filesystem code where a raw ptrs to newly
created objs are returned, but its more common now for use to be explicit about
the ownership xfer

Done.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:75: //    total_size() value to query the
blob size, as it's uint64_t.
On 2015/09/17 at 00:45:37, michaeln wrote:
> since the callback can't return the size if the value is out of range, i'd
vote to have this function not even try to return the size value via the
callback. Return SUCCESS and require callers to use the total_size() getter
thereafter.

Sounds good, it'll just return net::OK when successful.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:85: // * bytes_read is populated only is
Status::DONE is returned. Otherwise the
On 2015/09/17 at 00:45:37, michaeln wrote:
> typo: only if

Done.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:147: BlobReader::Status
ReadDiskCacheEntryItem(const BlobDataItem& item,
On 2015/09/17 at 00:45:37, michaeln wrote:
> BlobReader:: prefix not needed

Done.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:161: const BlobDataHandle* blob_handle_;
On 2015/09/17 at 00:45:37, michaeln wrote:
> why a rawptr? oh, see .cc comments, is this really needed at all?

Done.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:52: blob_handle_(blob_handle),
On 2015/09/17 at 00:45:37, michaeln wrote:
> rawptrs always make me look twice
> 
> I see the URLRequest owns the BlobDataHandle thru the user data mechanism. The
rawptr here is depending on that being the case. I think you should either...
> 
> 1) DCHECK_EQ(blob_handle_, GetHandlePtrOwnedByRequest());
> 
> 2) or spring for this class owning its own handle
> 
> The latter seems cleaner, it does create a new obj but that's not awful. The
copy ctor makes it easy and cheap to copy handles.

Done.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:211:
byte_range_.last_byte_position() + 1);
On 2015/09/17 at 00:45:37, michaeln wrote:
> is the + 1 intentional? odd that first == last + 1 would be expected

Yes it is, notice the subtraction below.  When the size is 0, first byte is 0
and last byte is -1. They are both inclusive. HTML spec is weird.

https://codereview.chromium.org/1337153002/diff/120001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:315:
set_expected_content_size(blob_reader_.get() ? blob_reader_->remaining_bytes()
On 2015/09/17 at 00:45:37, michaeln wrote:
> if status indicates an error, we're not going to return data even if
blob_reader_ is not null, expected size should be 0 in that case. maybe be
pessimistic, set it to 0 up top and reset it in the if (status_code) block to
what the blob_reader says?

Sure.

[michaeln, 2015-09-23 18:52:46]

i noticed a couple small things to take a look at, otherwise lgtm!!

https://codereview.chromium.org/1337153002/diff/200001/content/browser/fileap...
File content/browser/fileapi/blob_reader_unittest.cc (right):

https://codereview.chromium.org/1337153002/diff/200001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:233: return
GetLengthImpl(net::Int64CompletionCallback());
What is the purpose of the GetLenghtImpl helper? Seems like the non-error case
that returns size_ could be handled more simply as is done in the block below
that returns net_error_.

https://codereview.chromium.org/1337153002/diff/200001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:265: return net::ERR_IO_PENDING;
looks odd to be calling the callback and returning IO_PENDING?

https://codereview.chromium.org/1337153002/diff/200001/content/browser/fileap...
File content/browser/fileapi/upload_blob_element_reader_unittest.cc (right):

https://codereview.chromium.org/1337153002/diff/200001/content/browser/fileap...
content/browser/fileapi/upload_blob_element_reader_unittest.cc:10:
TEST(UploadBlobElementReader, Test1) {
either add real tests or dont add the file

https://codereview.chromium.org/1337153002/diff/200001/content/browser/loader...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/1337153002/diff/200001/content/browser/loader...
content/browser/loader/upload_data_stream_builder.cc:105:
element_readers.push_back(new storage::UploadBlobElementReader(
Either comment about or dcheck assumptions about element.length an offset.

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

https://codereview.chromium.org/1337153002/diff/200001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:421: TRACE_EVENT_ASYNC_END1("Blob",
"BlobRequest::ReadFileItem", this, "uuid",
I think the only difference between DidReadFile() and DidReadDiskCacheEntry() is
the trace label. If thats right, it would be could to share the
DidReadCommon(result) code.

[dmurph, 2015-09-25 17:11:44]

https://codereview.chromium.org/1337153002/diff/200001/content/browser/fileap...
File content/browser/fileapi/blob_reader_unittest.cc (right):

https://codereview.chromium.org/1337153002/diff/200001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:233: return
GetLengthImpl(net::Int64CompletionCallback());
On 2015/09/23 at 18:52:45, michaeln wrote:
> What is the purpose of the GetLenghtImpl helper? Seems like the non-error case
that returns size_ could be handled more simply as is done in the block below
that returns net_error_.

Yeah I'll just return size.

https://codereview.chromium.org/1337153002/diff/200001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:265: return net::ERR_IO_PENDING;
On 2015/09/23 at 18:52:45, michaeln wrote:
> looks odd to be calling the callback and returning IO_PENDING?

Yeah, I'll only use this as a callback, and switch to void.

https://codereview.chromium.org/1337153002/diff/200001/content/browser/fileap...
File content/browser/fileapi/upload_blob_element_reader_unittest.cc (right):

https://codereview.chromium.org/1337153002/diff/200001/content/browser/fileap...
content/browser/fileapi/upload_blob_element_reader_unittest.cc:10:
TEST(UploadBlobElementReader, Test1) {
On 2015/09/23 at 18:52:46, michaeln wrote:
> either add real tests or dont add the file

Deleted. I forgot this was here.  Since the file is basically just delegating, I
don't think we need tests.

https://codereview.chromium.org/1337153002/diff/200001/content/browser/loader...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/1337153002/diff/200001/content/browser/loader...
content/browser/loader/upload_data_stream_builder.cc:105:
element_readers.push_back(new storage::UploadBlobElementReader(
On 2015/09/23 at 18:52:46, michaeln wrote:
> Either comment about or dcheck assumptions about element.length an offset.

Done.

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

https://codereview.chromium.org/1337153002/diff/200001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:421: TRACE_EVENT_ASYNC_END1("Blob",
"BlobRequest::ReadFileItem", this, "uuid",
On 2015/09/23 at 18:52:46, michaeln wrote:
> I think the only difference between DidReadFile() and DidReadDiskCacheEntry()
is the trace label. If thats right, it would be could to share the
DidReadCommon(result) code.

Done!

[dmurph, 2015-09-25 17:58:15]

Kinuko: Can you please take a look? It's been a week or so. I need you to take a
look at the content/browser/* code.

If you're busy, let me know, and I can send this to someone else.

[kinuko, 2015-09-26 15:14:26]

Sorry for really slow review, tokyo was having a very long weekend. Really glad
to see this change coming & looking good!

Mostly nit comments

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
File content/browser/fileapi/blob_reader_unittest.cc (right):

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:198: }
nit: While I can totally understand these naming, these would usually mean
'return error now', 'do (something) async now' per our naming convention.  I'd
probably name these 'SetReturnError', 'SetAsyncRunner' or something

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:200: ~FakeFileStreamReader()
override {}
nit: usually we put dtor after ctor

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:210: base::Unretained(this),
scoped_refptr<IOBuffer>(buf),
nit: make_scoped_refptr(buf)

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:221: return net_error_;
Hmm, I have a feeling that it might become slightly more readable if we swap the
outer if and inner if?

  // When async_task_runner_ is not set, return synchronously.
  if (!async_task_runner_.get()) {
    if (net_error_ == net::OK)
      return ReadImpl(buf, buf_length, net::CompletionCallback());
    else
      return net_error_;
  } 

  // Otherwise always return asynchronously.        
  if (net_error_ == net::OK) {
    async_task_runner_->PostTask(
        FROM_HERE, 
        base::Bind(base::IgnoreResult(&FakeFileStreamReader::ReadImpl),
                   base::Unretained(this), make_scoped_refptr(buf),
                   buf_length, done));
  } else {
    async_task_runner_->PostTask(FROM_HERE, base::Bind(done, net_error_));
  }
  return net::ERR_IO_PENDING;

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:224: int64 GetLength(const
net::Int64CompletionCallback& size_callback) override {
ditto for the method body structure

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:229: base::Unretained(this),
size_callback));
Doesn't base::Bind(size_callback, size_) work?

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

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:54: io_pending_(false),
nit: it might be easier to initialize some of these in header

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:126: return
ReportError(net::ERR_FILE_NOT_FOUND);
Is this error type right??

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:157: return
ReportError(net::ERR_FILE_NOT_FOUND);
Do we need to set *bytes_read = 0 in this case too?

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:171: }
It feels we can set *bytes_read = 0 at the beginning of this method and just do
'return ReportError(ERR_FAILED)' on line 164?

Also do we want to just do DCHECK(!total_size_calculated_) instead if no one
calls this before calculating size if everything's coded right?

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
File storage/browser/blob/blob_reader.h (right):

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:76: Status
CalculateSize(net::CompletionCallback done);
nit: would we want to use Int64CompletionCallback instead?

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:76: Status
CalculateSize(net::CompletionCallback done);
nit: const net::CompletionCallback& done

[dmurph, 2015-09-28 19:40:02]

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
File content/browser/fileapi/blob_reader_unittest.cc (right):

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:198: }
On 2015/09/26 at 15:14:26, kinuko wrote:
> nit: While I can totally understand these naming, these would usually mean
'return error now', 'do (something) async now' per our naming convention.  I'd
probably name these 'SetReturnError', 'SetAsyncRunner' or something

Done.

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:200: ~FakeFileStreamReader()
override {}
On 2015/09/26 at 15:14:26, kinuko wrote:
> nit: usually we put dtor after ctor

Done.

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:210: base::Unretained(this),
scoped_refptr<IOBuffer>(buf),
On 2015/09/26 at 15:14:26, kinuko wrote:
> nit: make_scoped_refptr(buf)

Done.

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:221: return net_error_;
On 2015/09/26 at 15:14:26, kinuko wrote:
> Hmm, I have a feeling that it might become slightly more readable if we swap
the outer if and inner if?
> 
>   // When async_task_runner_ is not set, return synchronously.
>   if (!async_task_runner_.get()) {
>     if (net_error_ == net::OK)
>       return ReadImpl(buf, buf_length, net::CompletionCallback());
>     else
>       return net_error_;
>   } 
> 
>   // Otherwise always return asynchronously.        
>   if (net_error_ == net::OK) {
>     async_task_runner_->PostTask(
>         FROM_HERE, 
>         base::Bind(base::IgnoreResult(&FakeFileStreamReader::ReadImpl),
>                    base::Unretained(this), make_scoped_refptr(buf),
>                    buf_length, done));
>   } else {
>     async_task_runner_->PostTask(FROM_HERE, base::Bind(done, net_error_));
>   }
>   return net::ERR_IO_PENDING;

ah, much better.

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:224: int64 GetLength(const
net::Int64CompletionCallback& size_callback) override {
On 2015/09/26 at 15:14:26, kinuko wrote:
> ditto for the method body structure

Done.

https://codereview.chromium.org/1337153002/diff/220001/content/browser/fileap...
content/browser/fileapi/blob_reader_unittest.cc:229: base::Unretained(this),
size_callback));
On 2015/09/26 at 15:14:26, kinuko wrote:
> Doesn't base::Bind(size_callback, size_) work?

Ah, yes, that's much better.

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

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:54: io_pending_(false),
On 2015/09/26 at 15:14:26, kinuko wrote:
> nit: it might be easier to initialize some of these in header

Done.

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:126: return
ReportError(net::ERR_FILE_NOT_FOUND);
On 2015/09/26 at 15:14:26, kinuko wrote:
> Is this error type right??

This is the same behavior as before.

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:157: return
ReportError(net::ERR_FILE_NOT_FOUND);
On 2015/09/26 at 15:14:26, kinuko wrote:
> Do we need to set *bytes_read = 0 in this case too?

Done.

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_reader.cc:171: }
On 2015/09/26 at 15:14:26, kinuko wrote:
> It feels we can set *bytes_read = 0 at the beginning of this method and just
do 'return ReportError(ERR_FAILED)' on line 164?
> 
> Also do we want to just do DCHECK(!total_size_calculated_) instead if no one
calls this before calculating size if everything's coded right?

Sure, I had it be an error case for tests, but I can DCHECK instead. It just
removes the test.

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
File storage/browser/blob/blob_reader.h (right):

https://codereview.chromium.org/1337153002/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:76: Status
CalculateSize(net::CompletionCallback done);
On 2015/09/26 at 15:14:26, kinuko wrote:
> nit: would we want to use Int64CompletionCallback instead?

Well, we want uint64_t, so the completion callback just says the error code. The
caller must use total_size() to query the size.

[michaeln, 2015-09-28 21:52:07]

still lgtm

https://codereview.chromium.org/1337153002/diff/240001/content/browser/loader...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/1337153002/diff/240001/content/browser/loader...
content/browser/loader/upload_data_stream_builder.cc:104: // needed.
The comment is a little confusing. It's describing what would have to happen to
specify a subset of the blob using .length and .offset. It'd be clearer to state
that specifying a subset is not supported, only whole blobs. But really, the
DECHECKs say that even better than a comment... I think you could delete the
comment.

[kinuko, 2015-09-29 01:39:31]

lgtm after michael's comment's addressed!

[kinuko, 2015-09-29 01:57:56]

https://codereview.chromium.org/1337153002/diff/240001/storage/browser/blob/b...
File storage/browser/blob/blob_reader.h (right):

https://codereview.chromium.org/1337153002/diff/240001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:168: int net_error_;
nit: this could be also initialized here

[dmurph, 2015-09-29 21:20:27]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2015-09-29 21:20:27]

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

[commit-bot: I haz the power, 2015-09-29 21:20:50]

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

[commit-bot: I haz the power, 2015-09-29 21:25:19]

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

[commit-bot: I haz the power, 2015-09-29 21:25:20]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[dmurph, 2015-09-29 21:29:22]

https://codereview.chromium.org/1337153002/diff/240001/content/browser/loader...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/1337153002/diff/240001/content/browser/loader...
content/browser/loader/upload_data_stream_builder.cc:104: // needed.
On 2015/09/28 at 21:52:07, michaeln wrote:
> The comment is a little confusing. It's describing what would have to happen
to specify a subset of the blob using .length and .offset. It'd be clearer to
state that specifying a subset is not supported, only whole blobs. But really,
the DECHECKs say that even better than a comment... I think you could delete the
comment.

Done.

https://codereview.chromium.org/1337153002/diff/240001/storage/browser/blob/b...
File storage/browser/blob/blob_reader.h (right):

https://codereview.chromium.org/1337153002/diff/240001/storage/browser/blob/b...
storage/browser/blob/blob_reader.h:168: int net_error_;
On 2015/09/29 at 01:57:56, kinuko wrote:
> nit: this could be also initialized here

I didn't want to include the net_errors.h file in the header to reduce size as
much as possible.  I guess I could just put 0, but that might be confusing.

[dmurph, 2015-09-29 21:40:30]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2015-09-29 21:40:31]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
michaeln@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1337153002/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-29 22:03:09]

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

[commit-bot: I haz the power, 2015-09-30 02:03:43]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-09-30 02:04:35]

Patchset 15 (id:??) landed as
https://crrev.com/02561552a57ed8792a8ebb2676bad485e1d99605
Cr-Commit-Position: refs/heads/master@{#351470}

[Lei Zhang, 2015-09-30 02:46:44]

There's a failure here, FYI:
http://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/8769

[ukai, 2015-09-30 03:37:19]

On 2015/09/30 02:46:44, Lei Zhang wrote:
> There's a failure here, FYI:
> http://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/8769

http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/31768

[kinuko, 2015-09-30 03:53:47]

On 2015/09/30 03:37:19, ukai wrote:
> On 2015/09/30 02:46:44, Lei Zhang wrote:
> > There's a failure here, FYI:
> > http://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/8769
> 
> http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/31768

Looks like failing on many / multiple bots.

[kinuko, 2015-09-30 03:54:36]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/1376123002/ by kinuko@chromium.org.

The reason for reverting is: BlobReaderTest.StateErrors failing on multiple
bots.

../../content/browser/fileapi/blob_reader_unittest.cc:985: Failure
Death test: reader_->Read(buffer.get(), 10, &bytes_read,
base::Bind(&SetValue<int>, &async_bytes_read))
    Result: failed to die.
 Error msg:
[  DEATH   ].

[dmurph, 2015-09-30 06:23:46]

Fixed issue with DCHECK not causing death in release builds, removed dcheck and
I just test for the error case now.

[dmurph, 2015-09-30 06:23:49]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2015-09-30 06:23:49]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
michaeln@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1337153002/#ps300001
(title: "Fixed prod/debug flakiness")

[commit-bot: I haz the power, 2015-09-30 06:24:01]

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

[phoglund_chromium, 2015-09-30 07:25:54]

On 2015/09/30 06:24:01, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1337153002/300001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1337153002/300001

I was about to revert this one but I see you're already aware of the StateErrors
test failure, so I'll assume you have fixed those.

[dmurph, 2015-09-30 07:26:32]

On 2015/09/30 at 07:25:54, phoglund wrote:
> On 2015/09/30 06:24:01, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1337153002/300001
> > View timeline at
> >  https://chromium-cq-status.appspot.com/patch-timeline/1337153002/300001
> 
> I was about to revert this one but I see you're already aware of the
StateErrors test failure, so I'll assume you have fixed those.

Yes, it was reverted and now re-committing.

[commit-bot: I haz the power, 2015-09-30 11:18:59]

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

[commit-bot: I haz the power, 2015-09-30 11:19:00]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[dmurph, 2015-09-30 17:37:38]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2015-09-30 17:38:30]

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

[commit-bot: I haz the power, 2015-09-30 18:49:10]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-09-30 18:50:10]

Patchset 16 (id:??) landed as
https://crrev.com/164f0b76c3a8156b95c36f17c95c452e8f3b9765
Cr-Commit-Position: refs/heads/master@{#351611}