Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
942633004:
IndexedDB: Fixed support for empty blobs. (Closed)
|
Created:
5 years, 10 months ago by cmumford Modified:
5 years, 9 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, kinuko+fileapi, nhiroki, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIndexedDB: Fixed support for empty blobs.
This solution allows empty blobs (and files) to be created.
Most tests for this change are Blink layout tests, so will land
in a separate CL.
BUG=399323
Committed: https://crrev.com/3a61739491730a1acd96c09a6ae7b79825eb445f
Cr-Commit-Position: refs/heads/master@{#320636}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Creating empty cursor to avoid null ptr deref. #
Total comments: 5
Patch Set 3 : Added back in some DCHECKs and simplified empty file write. #
Total comments: 14
Patch Set 4 : LocalFileStreamWriter always creates an output file. #Patch Set 5 : Deleted CREATE_NEW_FILE. Creating file before using FileStreamWriter. #
Total comments: 8
Patch Set 6 : Backing store now creating empty file + test. #
Total comments: 16
Patch Set 7 : Test cleanup & added blob delete to test. #
Total comments: 6
Patch Set 8 : Handling double read of empty blob, and allowing empty data to be added to the builder. #Patch Set 9 : Reordered test code and git cl format. #
Total comments: 3
Patch Set 10 : Created local var to improve readability #
Total comments: 14
Patch Set 11 : Removed IndexedDBContext::GetOriginBlobFileCount #
Total comments: 16
Patch Set 12 : Removed dead code in test. #Patch Set 13 : Removed no longer needed using base::File #Patch Set 14 : Early return in BlobDataBuilder::AppendData #
Total comments: 9
Patch Set 15 : More test cleanup. #
Total comments: 1
Patch Set 16 : Added back in handle1 (removed from wrong test). #Patch Set 17 : Re-removed handle1 #Patch Set 18 : Test using base::MessageLoopForIO #Patch Set 19 : Worked around Android feature/bug for test #Patch Set 20 : Added bug for test re: Android #Messages
Total messages: 60 (12 generated)
cmumford@chromium.org changed reviewers: + jsbell@chromium.org, michaeln@chromium.org
I know the discussion regarding saving empty files to disk is still ongoing (crrev.com/431143002). Still I want to put this up for review. If the consensus is to persist the file size/modification metadata to IDB then I will modify this CL. I guess my argument for this approach is that the filesystem is specifically designed for empty files, we wouldn't have as much conditions to test for, and wouldn't have to decide what to do if/when values (like lastModified) disagree - and surely they will.
https://codereview.chromium.org/942633004/diff/1/net/base/file_stream_context... File net/base/file_stream_context_posix.cc (right): https://codereview.chromium.org/942633004/diff/1/net/base/file_stream_context... net/base/file_stream_context_posix.cc:106: if (buf_len == 0) Can you explain why we need this special case? Does WriteAtCurrentPosNoBestEffort error out if the length is zero? If so, do we want to change the semantics of this API, or short-circuit closer to the Blob code? (This seems entirely reasonable, I'm just nervous...)
OK, found a (hopefully acceptable) further up the stack to write empty files eliminating the need to touch /net https://codereview.chromium.org/942633004/diff/1/net/base/file_stream_context... File net/base/file_stream_context_posix.cc (right): https://codereview.chromium.org/942633004/diff/1/net/base/file_stream_context... net/base/file_stream_context_posix.cc:106: if (buf_len == 0) On 2015/02/24 18:37:36, jsbell wrote: > Can you explain why we need this special case? Does > WriteAtCurrentPosNoBestEffort error out if the length is zero? If so, do we want > to change the semantics of this API, or short-circuit closer to the Blob code? > > (This seems entirely reasonable, I'm just nervous...) buf refers to a NULL IOBuffer in this case. So operator-> is what's failing without this check. I'd rather stay out of net/base, and will see if I can do that. However, going two levels up the callstack FileStream::Write allows for writing zero bytes (see DCHECK_GE), so maybe the solution should be in net/base. What do you think?
https://codereview.chromium.org/942633004/diff/1/net/base/file_stream_context... File net/base/file_stream_context_posix.cc (right): https://codereview.chromium.org/942633004/diff/1/net/base/file_stream_context... net/base/file_stream_context_posix.cc:106: if (buf_len == 0) Although you took a different approach, it is weird if FileStream::Write is effectively calling WriteFileImpl(nullptr, 0) ! https://codereview.chromium.org/942633004/diff/20001/storage/browser/fileapi/... File storage/browser/fileapi/file_writer_delegate.cc (right): https://codereview.chromium.org/942633004/diff/20001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:145: cursor_ = new net::DrainableIOBuffer(io_buffer_.get(), bytes_read_); Can you refactor to remove these duplicate bodies?
https://codereview.chromium.org/942633004/diff/20001/storage/browser/blob/blo... File storage/browser/blob/blob_data_builder.cc (left): https://codereview.chromium.org/942633004/diff/20001/storage/browser/blob/blo... storage/browser/blob/blob_data_builder.cc:18: DCHECK(length > 0); Are we also hitting this dcheck? Can you reduce it down to just relaxing those that we trip on.
fyi: VerifySnapshotTime() makes me wary of not using the actual timestamp on any underlying OS file used to back idb blobs.
https://codereview.chromium.org/942633004/diff/20001/storage/browser/blob/blo... File storage/browser/blob/blob_data_builder.cc (left): https://codereview.chromium.org/942633004/diff/20001/storage/browser/blob/blo... storage/browser/blob/blob_data_builder.cc:18: DCHECK(length > 0); On 2015/02/25 00:51:59, michaeln wrote: > Are we also hitting this dcheck? Can you reduce it down to just relaxing those > that we trip on. Looks like only one DCHECK was actually hit by the new tests so keeping the other three. https://codereview.chromium.org/942633004/diff/20001/storage/browser/fileapi/... File storage/browser/fileapi/file_writer_delegate.cc (right): https://codereview.chromium.org/942633004/diff/20001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:145: cursor_ = new net::DrainableIOBuffer(io_buffer_.get(), bytes_read_); On 2015/02/25 00:10:30, jsbell wrote: > Can you refactor to remove these duplicate bodies? Done.
This is using the tests in https://codereview.chromium.org/433983002/ correct? I don't see a case in there for using the File constructor e.g. `new File([], '')` - it may just predate that support landing. Can you sanity check that case too? https://codereview.chromium.org/942633004/diff/20001/storage/browser/blob/blo... File storage/browser/blob/blob_data_builder.cc (left): https://codereview.chromium.org/942633004/diff/20001/storage/browser/blob/blo... storage/browser/blob/blob_data_builder.cc:18: DCHECK(length > 0); On 2015/02/25 18:09:49, cmumford wrote: > On 2015/02/25 00:51:59, michaeln wrote: > > Are we also hitting this dcheck? Can you reduce it down to just relaxing those > > that we trip on. > > Looks like only one DCHECK was actually hit by the new tests so keeping the > other three. s/keeping/removing/ ? https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... File storage/browser/fileapi/file_writer_delegate.cc (right): https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:142: // If we received bytes then write them. Zero bytes signals EOF, but if no Excellent comment, thanks!
On 2015/02/25 19:53:27, jsbell wrote: > This is using the tests in https://codereview.chromium.org/433983002/ correct? > > I don't see a case in there for using the File constructor e.g. `new File([], > '')` - it may just predate that support landing. Can you sanity check that case > too? I wrote a content browsertest that tests new Blob([]) and new File([]), so this was tested against that as well as crrev.com/433983002. I will send that test to pwnall (test author) to include in that CL. If not then I will land as a new layout test. > > https://codereview.chromium.org/942633004/diff/20001/storage/browser/blob/blo... > File storage/browser/blob/blob_data_builder.cc (left): > > https://codereview.chromium.org/942633004/diff/20001/storage/browser/blob/blo... > storage/browser/blob/blob_data_builder.cc:18: DCHECK(length > 0); > On 2015/02/25 18:09:49, cmumford wrote: > > On 2015/02/25 00:51:59, michaeln wrote: > > > Are we also hitting this dcheck? Can you reduce it down to just relaxing > those > > > that we trip on. > > > > Looks like only one DCHECK was actually hit by the new tests so keeping the > > other three. > > s/keeping/removing/ ? Originally I took out four, but then put back in three, so the net change removes one. Confusing comment... > > https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... > File storage/browser/fileapi/file_writer_delegate.cc (right): > > https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... > storage/browser/fileapi/file_writer_delegate.cc:142: // If we received bytes > then write them. Zero bytes signals EOF, but if no > Excellent comment, thanks!
Ok, I see there's just too much hair on not producing zero len files in IDB for this. https://codereview.chromium.org/942633004/diff/40001/storage/browser/blob/blo... File storage/browser/blob/blob_data_builder.cc (left): https://codereview.chromium.org/942633004/diff/40001/storage/browser/blob/blo... storage/browser/blob/blob_data_builder.cc:28: DCHECK(length > 0); Out of curiosity, if (!length) return goes here instead, do tests pass? https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... File storage/browser/fileapi/file_writer_delegate.cc (right): https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:146: // file creation. Should we fix this problem directly in the FileStreamWriter class? It fails to create a file when kCreateFlagsForWrite is used but then 0 bytes are actually written, that seems like a bug in FileStreamWriter. https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:160: int64 bytes_to_write = bytes_read_ - bytes_written_; It looks like bytes_to_write here will go negative in some cases, will FileStreamWriter::Write be ok with that?
https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... File storage/browser/fileapi/file_writer_delegate.cc (right): https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:160: int64 bytes_to_write = bytes_read_ - bytes_written_; On 2015/02/25 20:35:43, michaeln wrote: > It looks like bytes_to_write here will go negative in some cases, will > FileStreamWriter::Write be ok with that? oh, nm it cant be < zero https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:162: file_stream_writer_->Write(cursor_.get(), I'm not sure if this is depending on a return value of 0 immediately or if its expecting IO_PENDING and a later call to OnDataWritten(0) , either type of return would be valid and FileStreamWriter is an abstract interface so different subclasses may handle this case differently.
https://codereview.chromium.org/942633004/diff/40001/storage/browser/blob/blo... File storage/browser/blob/blob_data_builder.cc (left): https://codereview.chromium.org/942633004/diff/40001/storage/browser/blob/blo... storage/browser/blob/blob_data_builder.cc:28: DCHECK(length > 0); On 2015/02/25 20:35:43, michaeln wrote: > Out of curiosity, if (!length) return goes here instead, do tests pass? Yes, the tests do pass. Does that give you enough confidence to want that change? https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... File storage/browser/fileapi/file_writer_delegate.cc (right): https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:146: // file creation. On 2015/02/25 20:35:43, michaeln wrote: > Should we fix this problem directly in the FileStreamWriter class? It fails to > create a file when kCreateFlagsForWrite is used but then 0 bytes are actually > written, that seems like a bug in FileStreamWriter. Was reading the comment above net::FileStream::Writer, and just saw this: "Zero byte writes are not allowed". I think I'm going to have to take a different approach where I don't call FileStreamWriter::Write() with zero bytes, and instead use base::File to create an empty file. https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:162: file_stream_writer_->Write(cursor_.get(), On 2015/02/25 21:55:42, michaeln wrote: > I'm not sure if this is depending on a return value of 0 immediately or if its > expecting IO_PENDING and a later call to OnDataWritten(0) , either type of > return would be valid and FileStreamWriter is an abstract interface so > different subclasses may handle this case differently. I think that now that I won't be calling Write() I'll create a new method (CreateEmpty???) which just creates the file, and then OnDataWritten(0), etc.
https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... File storage/browser/fileapi/file_writer_delegate.cc (right): https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:146: // file creation. On 2015/02/25 22:37:29, cmumford wrote: > On 2015/02/25 20:35:43, michaeln wrote: > > Should we fix this problem directly in the FileStreamWriter class? It fails to > > > create a file when kCreateFlagsForWrite is used but then 0 bytes are actually > > written, that seems like a bug in FileStreamWriter. > > Was reading the comment above net::FileStream::Writer, and just saw this: "Zero > byte writes are not allowed". I think I'm going to have to take a different > approach where I don't call FileStreamWriter::Write() with zero bytes, and > instead use base::File to create an empty file. On second thought, I'm back to the idea of fixing FileStreamWriter. Do you think that we might create problems elsewhere where a FileStreamWriter is created, but Write is never called?
https://codereview.chromium.org/942633004/diff/40001/storage/browser/blob/blo... File storage/browser/blob/blob_data_builder.cc (left): https://codereview.chromium.org/942633004/diff/40001/storage/browser/blob/blo... storage/browser/blob/blob_data_builder.cc:28: DCHECK(length > 0); On 2015/02/25 22:37:29, cmumford wrote: > On 2015/02/25 20:35:43, michaeln wrote: > > Out of curiosity, if (!length) return goes here instead, do tests pass? > > Yes, the tests do pass. Does that give you enough confidence to want that > change? A little uncomfortable without a better understanding of the purpose of the release callback IDB has hooked up to the file. https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... File storage/browser/fileapi/file_writer_delegate.cc (right): https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:146: // file creation. > On second thought, I'm back to the idea of fixing FileStreamWriter. Do you think > that we might create problems elsewhere where a FileStreamWriter is created, but > Write is never called? Maybe fix that class w/o altering behavior for existing callers. enum OpenOrCreate { OPEN_EXISTING_FILE, CREATE_NEW_FILE, CREATE_NEW_EMPTY_FILE }; Where the new 3rd enum means delete CreateForLocalFile(task_runner, ..., CREATE_NEW_EMPTY_FILE); should result in a task having been posted to the task_runner to create the file. https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... storage/browser/fileapi/file_writer_delegate.cc:162: file_stream_writer_->Write(cursor_.get(), > I think that now that I won't be calling Write() I'll create a new method > (CreateEmpty???) which just creates the file, and then OnDataWritten(0), etc. The lack of a file being created by FileStreamWriter definitely was unexpected behavior. I do think it's best to put the fix in that class. It would document the existing behavior of the existing CREATE enum and give callers an easy way to get the behavior they most likely want.
https://codereview.chromium.org/942633004/diff/40001/storage/browser/blob/blo... File storage/browser/blob/blob_storage_context.cc (left): https://codereview.chromium.org/942633004/diff/40001/storage/browser/blob/blo... storage/browser/blob/blob_storage_context.cc:312: DCHECK_GT(length, 0u); I think BlobURLRequestJob is relying on items have non-zero length, otherwise I think javascript could coerce the browser process to stack overflow. It's ReadItem() method can be hacked to recurse.
On 2015/02/26 02:05:37, michaeln wrote: > https://codereview.chromium.org/942633004/diff/40001/storage/browser/blob/blo... > File storage/browser/blob/blob_data_builder.cc (left): > > https://codereview.chromium.org/942633004/diff/40001/storage/browser/blob/blo... > storage/browser/blob/blob_data_builder.cc:28: DCHECK(length > 0); > On 2015/02/25 22:37:29, cmumford wrote: > > On 2015/02/25 20:35:43, michaeln wrote: > > > Out of curiosity, if (!length) return goes here instead, do tests pass? > > > > Yes, the tests do pass. Does that give you enough confidence to want that > > change? > > A little uncomfortable without a better understanding of the purpose of the > release callback IDB has hooked up to the file. > > https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... > File storage/browser/fileapi/file_writer_delegate.cc (right): > > https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... > storage/browser/fileapi/file_writer_delegate.cc:146: // file creation. > > On second thought, I'm back to the idea of fixing FileStreamWriter. Do you > think > > that we might create problems elsewhere where a FileStreamWriter is created, > but > > Write is never called? > > Maybe fix that class w/o altering behavior for existing callers. > > enum OpenOrCreate { OPEN_EXISTING_FILE, CREATE_NEW_FILE, CREATE_NEW_EMPTY_FILE > }; > > Where the new 3rd enum means > > delete CreateForLocalFile(task_runner, ..., CREATE_NEW_EMPTY_FILE); > > should result in a task having been posted to the task_runner to create the > file. > > https://codereview.chromium.org/942633004/diff/40001/storage/browser/fileapi/... > storage/browser/fileapi/file_writer_delegate.cc:162: > file_stream_writer_->Write(cursor_.get(), > > I think that now that I won't be calling Write() I'll create a new method > > (CreateEmpty???) which just creates the file, and then OnDataWritten(0), etc. > > The lack of a file being created by FileStreamWriter definitely was unexpected > behavior. I do think it's best to put the fix in that class. It would document > the existing behavior of the existing CREATE enum and give callers an easy way > to get the behavior they most likely want. I thought it was in wider use, but FileStreamWriter::Write() is only called with CREATE_NEW_FILE in WriteBlobToFileOnIOThread(). I think the right thing to do here is just to create the output file immediately in FileStreamWriter::CreateForLocalFile(), and not lazily when the first chunk of data is received. I don't see the need to create a new CREATE_NEW_EMPTY_FILE enum. I don't see any compatibility issues with modifying FileStreamWriter. Do you agree?
> I thought it was in wider use, but FileStreamWriter::Write() is only called with > CREATE_NEW_FILE in WriteBlobToFileOnIOThread(). I think the right thing to do > here is just to create the output file immediately in > FileStreamWriter::CreateForLocalFile(), and not lazily when the first chunk of > data is received. I don't see the need to create a new CREATE_NEW_EMPTY_FILE > enum. I don't see any compatibility issues with modifying FileStreamWriter. Do > you agree? yup, sgtm
a thought about the api? * remove the 'OpenOrCreate'flag altogether, it will only open if Write() is called * to support creating new files, add an async Create() method with a completion callback
cmumford@chromium.org changed reviewers: + kinaba@chromium.org
michaeln, kinaba for review jsbell: FYI
lgtm https://codereview.chromium.org/942633004/diff/80001/content/browser/fileapi/... File content/browser/fileapi/copy_or_move_operation_delegate_unittest.cc (right): https://codereview.chromium.org/942633004/diff/80001/content/browser/fileapi/... content/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:741: EXPECT_TRUE(file.created()); Ah, so there were some uses of the CREATE_NEW_FILE flag. nit: calling file.Close() instead of the nested scope might be more readable and self-documenting. https://codereview.chromium.org/942633004/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/942633004/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:2459: base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE); nit: ditto file.Close() https://codereview.chromium.org/942633004/diff/80001/storage/browser/blob/blo... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/942633004/diff/80001/storage/browser/blob/blo... storage/browser/blob/blob_data_builder.cc:29: return; ty! https://codereview.chromium.org/942633004/diff/80001/storage/browser/blob/blo... File storage/browser/blob/blob_storage_context.cc (left): https://codereview.chromium.org/942633004/diff/80001/storage/browser/blob/blo... storage/browser/blob/blob_storage_context.cc:312: DCHECK_GT(length, 0u); Does this need to be removed now?
I'm slightly worried about the "open file, close file, re-open file" pattern this now imposes on FileStreamWriter users, recalling that Windows virus scanners like to watch for files being closed and then grab an exclusive lock on the file to scan it. But I don't have any explicit data to back that up. https://codereview.chromium.org/942633004/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/942633004/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:2458: base::File file(path, Should we do this in WriteBlobToFileOnIOThread instead, to avoid stalling the IDB thread on this I/O?
LGTM to me, though I'm not quite confident about the concern in the open-close-open pattern jsbell@ mentioned.
Now back full circle and the backing store is creating the empty file, but only if necessary. Also, added a test. https://codereview.chromium.org/942633004/diff/80001/content/browser/fileapi/... File content/browser/fileapi/copy_or_move_operation_delegate_unittest.cc (right): https://codereview.chromium.org/942633004/diff/80001/content/browser/fileapi/... content/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:741: EXPECT_TRUE(file.created()); On 2015/03/03 00:02:51, michaeln wrote: > Ah, so there were some uses of the CREATE_NEW_FILE flag. > nit: calling file.Close() instead of the nested scope might be more readable and > self-documenting. Done. https://codereview.chromium.org/942633004/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/942633004/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:2458: base::File file(path, On 2015/03/03 01:01:48, jsbell wrote: > Should we do this in WriteBlobToFileOnIOThread instead, to avoid stalling the > IDB thread on this I/O? Can't do file I/O on that thread - only reason why it's here. https://codereview.chromium.org/942633004/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:2459: base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE); On 2015/03/03 00:02:51, michaeln wrote: > nit: ditto file.Close() Done.
lgtm! https://codereview.chromium.org/942633004/diff/100001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/942633004/diff/100001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:429: GetContext()->TaskRunner()->PostTask( Is this actually necessary? Is state persisted across tests? https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... File content/test/data/indexeddb/empty_blob.html (right): https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:34: var trans = db.transaction(store_name, 'readwrite'); It's probably personal preference, but WDYT about the order: var tx = db.transaction(...); // do stuff with tx here tx.onabort = ...; tx.oncomplete = function() { // do more stuff here }; I find that more readable since code order mimics execution order. But maybe that's just me. https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:59: lastModified: new Date('1999-12-31T23:59:59Z') Party like it's... https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:69: result = e.target.result; Just a consistency nit: in the blob case you have put_blob and get_blob, here you use file and result https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:80: function deleteFile() { Should we have a case for deleting blob as well? https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:84: put_tx.oncomplete = function() { could just say: oncomplete = done;
https://codereview.chromium.org/942633004/diff/100001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/942633004/diff/100001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:429: GetContext()->TaskRunner()->PostTask( On 2015/03/03 22:38:07, jsbell wrote: > Is this actually necessary? Is state persisted across tests? Yes, it is necessary. Do you think this should be moved to the SetUp method and done there? https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... File content/test/data/indexeddb/empty_blob.html (right): https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:34: var trans = db.transaction(store_name, 'readwrite'); On 2015/03/03 22:38:07, jsbell wrote: > It's probably personal preference, but WDYT about the order: > > var tx = db.transaction(...); > // do stuff with tx here > tx.onabort = ...; > tx.oncomplete = function() { > // do more stuff here > }; > > I find that more readable since code order mimics execution order. But maybe > that's just me. I think I see what you're talking about. Let me know if this is what you mean? https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:59: lastModified: new Date('1999-12-31T23:59:59Z') On 2015/03/03 22:38:07, jsbell wrote: > Party like it's... 1999 https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:69: result = e.target.result; On 2015/03/03 22:38:07, jsbell wrote: > Just a consistency nit: in the blob case you have put_blob and get_blob, here > you use file and result > Done. https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:80: function deleteFile() { On 2015/03/03 22:38:07, jsbell wrote: > Should we have a case for deleting blob as well? Done. https://codereview.chromium.org/942633004/diff/100001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:84: put_tx.oncomplete = function() { On 2015/03/03 22:38:07, jsbell wrote: > could just say: oncomplete = done; Done.
https://codereview.chromium.org/942633004/diff/100001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/942633004/diff/100001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:429: GetContext()->TaskRunner()->PostTask( On 2015/03/03 23:01:35, cmumford wrote: > On 2015/03/03 22:38:07, jsbell wrote: > > Is this actually necessary? Is state persisted across tests? > > Yes, it is necessary. Do you think this should be moved to the SetUp method and > done there? SetUp would probably be better, but involve a new class. Not a big deal. https://codereview.chromium.org/942633004/diff/100001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:438: EXPECT_EQ(1, new_count); Is this still valid if the Blob is deleted too? (sorry, I'd missed that this comment references the test behavior) https://codereview.chromium.org/942633004/diff/120001/content/test/data/index... File content/test/data/indexeddb/empty_blob.html (right): https://codereview.chromium.org/942633004/diff/120001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:43: get_tx.oncomplete = writeFile; Maybe move this to the end of the block as well. https://codereview.chromium.org/942633004/diff/120001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:67: get_tx.oncomplete = deleteFile; Ditto. https://codereview.chromium.org/942633004/diff/120001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:80: function deleteBlob() { Maybe move this below deleteFile(), to follow execution order?
cmumford@chromium.org changed reviewers: - kinaba@chromium.org
-kinaba as his code is no longer modified. Adding the CreateUploadDataStreamWithEmptyBlob test hit the DCHECK in BlobDataBuilder::AppendData(), and a null deref in DataElement::bytes(). DataElement already supports the concept of being empty, so I felt this was the right change. https://codereview.chromium.org/942633004/diff/120001/content/test/data/index... File content/test/data/indexeddb/empty_blob.html (right): https://codereview.chromium.org/942633004/diff/120001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:43: get_tx.oncomplete = writeFile; On 2015/03/03 23:37:57, jsbell wrote: > Maybe move this to the end of the block as well. Done. https://codereview.chromium.org/942633004/diff/120001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:67: get_tx.oncomplete = deleteFile; On 2015/03/03 23:37:57, jsbell wrote: > Ditto. Done. https://codereview.chromium.org/942633004/diff/120001/content/test/data/index... content/test/data/indexeddb/empty_blob.html:80: function deleteBlob() { On 2015/03/03 23:37:57, jsbell wrote: > Maybe move this below deleteFile(), to follow execution order? Done.
lgtm. This time for sure? https://codereview.chromium.org/942633004/diff/160001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/942633004/diff/160001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:434: // Test creates two blob's (but stores them to the same key), and then Nit: just blobs, no ' https://codereview.chromium.org/942633004/diff/160001/content/browser/loader/... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/942633004/diff/160001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:359: AreElementsEqual(*(*upload->GetElementReaders())[0], blob_element1)); Make a local reference to this to avoid the repeated (*upload->GetElementReaders()) ?
cmumford@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke for content/browser/loader* https://codereview.chromium.org/942633004/diff/160001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/942633004/diff/160001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:434: // Test creates two blob's (but stores them to the same key), and then On 2015/03/06 18:28:50, jsbell wrote: > Nit: just blobs, no ' done - yes, I keep doing that!
loader/ LGTM (deferring to other reviewers) https://codereview.chromium.org/942633004/diff/180001/content/browser/loader/... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/942633004/diff/180001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:341: base::FilePath(FILE_PATH_LITERAL("BlobFile.txt")), 0, 20, time1); The CL description indicates it supports empty files, too... Should we be using an empty file here, or a 0-length file range? If it's correct as-is, should the test be renamed to WithEmptyBlobElements? We aren't actually checking a completely empty blob, right?
mostly just nits about the tests https://codereview.chromium.org/942633004/diff/100001/content/browser/indexed... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/942633004/diff/100001/content/browser/indexed... content/browser/indexed_db/indexed_db_backing_store.cc:2344: // If none was then create one now. nit: the first line of this comment is enough https://codereview.chromium.org/942633004/diff/100001/content/browser/indexed... content/browser/indexed_db/indexed_db_backing_store.cc:2410: // Create an empty file. this comment probably isn't needed https://codereview.chromium.org/942633004/diff/180001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/942633004/diff/180001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:435: // a single file. Expect two blobs. This comment is confusing. At this point the test has run to completion has invoked IDB to delete the blob. An explanation of why the files still exists whould help. And maybe a test for some other artifact that the files are scheduled for deletion (read leveldb directly for that artifact?) https://codereview.chromium.org/942633004/diff/180001/content/browser/loader/... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/942633004/diff/180001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:334: base::FilePath(FILE_PATH_LITERAL("BlobFile.txt")), 0, 20, time1); This isn't an empty blob? https://codereview.chromium.org/942633004/diff/180001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:359: EXPECT_TRUE(AreElementsEqual(*readers[0], blob_element1)); Not sure about this test? It's making sure that blob_data_builder->AppendData("") actually creates a DataElement under the covers and adds it to the list. And its further ensuring that the emtpy elements are propagated into the UpdateDataStream. But neither of those internal details are important. It's an unoptimzation that those empty elements are not culled at some point in the process and probably shouldn't be a criteria for success. The test doesn't determine if UploadDataStream can properly functions given a list of empty elements. By inspection, it looks like it'll be ok, but at a glance, the existing tests for those classes only use non-empty elements. https://codereview.chromium.org/942633004/diff/180001/content/public/browser/... File content/public/browser/indexed_db_context.h (right): https://codereview.chromium.org/942633004/diff/180001/content/public/browser/... content/public/browser/indexed_db_context.h:33: virtual int GetOriginBlobFileCount(const GURL& origin_url) = 0; It looks like this virtual method may not be needed? I think this is only used for the browser test which knows all about the concrete impl class. https://codereview.chromium.org/942633004/diff/180001/storage/browser/blob/bl... File storage/browser/blob/blob_data_builder.cc (left): https://codereview.chromium.org/942633004/diff/180001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.cc:18: DCHECK(length > 0); i'd probably go for an early return here to avoid special cases and untested code paths downstream https://codereview.chromium.org/942633004/diff/180001/storage/browser/blob/bl... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/942633004/diff/180001/storage/browser/blob/bl... storage/browser/blob/blob_url_request_job.cc:337: return true; nice!
https://codereview.chromium.org/942633004/diff/180001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/942633004/diff/180001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:435: // a single file. Expect two blobs. On 2015/03/06 22:31:38, michaeln wrote: > This comment is confusing. At this point the test has run to completion has > invoked IDB to delete the blob. An explanation of why the files still exists > whould help. And maybe a test for some other artifact that the files are > scheduled for deletion (read leveldb directly for that artifact?) Yes, good point. Maybe I should just create these blob/file objects, write them, assert two files exist - but not delete them. The delete testing could happen in a layout test, but would not assert that the physical file was gone from disk as that is too coupled with the implementation. I could also split this test into two phases 1) create, and 2) delete. The EXPECT_EQ would be before phase 2, and phase 2 would have no c++ checks after the test ends. Do you have a preference? https://codereview.chromium.org/942633004/diff/180001/content/browser/loader/... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/942633004/diff/180001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:334: base::FilePath(FILE_PATH_LITERAL("BlobFile.txt")), 0, 20, time1); On 2015/03/06 22:31:38, michaeln wrote: > This isn't an empty blob? Acknowledged. https://codereview.chromium.org/942633004/diff/180001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:341: base::FilePath(FILE_PATH_LITERAL("BlobFile.txt")), 0, 20, time1); On 2015/03/06 20:54:40, mmenke wrote: > The CL description indicates it supports empty files, too... Should we be using > an empty file here, or a 0-length file range? > > If it's correct as-is, should the test be renamed to WithEmptyBlobElements? We > aren't actually checking a completely empty blob, right? Acknowledged. https://codereview.chromium.org/942633004/diff/180001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:359: EXPECT_TRUE(AreElementsEqual(*readers[0], blob_element1)); On 2015/03/06 22:31:38, michaeln wrote: > Not sure about this test? It's making sure that > blob_data_builder->AppendData("") actually creates a DataElement under the > covers and adds it to the list. And its further ensuring that the emtpy elements > are propagated into the UpdateDataStream. But neither of those internal details > are important. > > It's an unoptimzation that those empty elements are not culled at some point in > the process and probably shouldn't be a criteria for success. > > The test doesn't determine if UploadDataStream can properly functions given a > list of empty elements. By inspection, it looks like it'll be ok, but at a > glance, the existing tests for those classes only use non-empty elements. > Yes, I see your point. I've modified the test to use UploadDataStream (i.e read from it). https://codereview.chromium.org/942633004/diff/180001/content/public/browser/... File content/public/browser/indexed_db_context.h (right): https://codereview.chromium.org/942633004/diff/180001/content/public/browser/... content/public/browser/indexed_db_context.h:33: virtual int GetOriginBlobFileCount(const GURL& origin_url) = 0; On 2015/03/06 22:31:38, michaeln wrote: > It looks like this virtual method may not be needed? I think this is only used > for the browser test which knows all about the concrete impl class. Correct - fixed. https://codereview.chromium.org/942633004/diff/180001/storage/browser/blob/bl... File storage/browser/blob/blob_data_builder.cc (left): https://codereview.chromium.org/942633004/diff/180001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.cc:18: DCHECK(length > 0); On 2015/03/06 22:31:38, michaeln wrote: > i'd probably go for an early return here to avoid special cases and untested > code paths downstream It's actually back in now. Was only removed for the test, which as you pointed out was incorrect. https://codereview.chromium.org/942633004/diff/180001/storage/browser/blob/bl... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/942633004/diff/180001/storage/browser/blob/bl... storage/browser/blob/blob_url_request_job.cc:337: return true; On 2015/03/06 22:31:38, michaeln wrote: > nice! Yeah, that was easy huh?
https://codereview.chromium.org/942633004/diff/200001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/942633004/diff/200001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:434: // Test creates two blobs (but stores them to the same key), and then Honestly this comment is still confusing? The test puts one Blob and one File into IDB, we expect a file on disk for each. https://codereview.chromium.org/942633004/diff/200001/content/browser/loader/... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/942633004/diff/200001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:326: const std::string kBlobFileData = ""; dont think we need this https://codereview.chromium.org/942633004/diff/200001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:327: const uint64_t kBlobLength = kBlobFileData.size(); since its always zero, kZeroLength would be more clear https://codereview.chromium.org/942633004/diff/200001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:328: if (!kBlobFileData.empty()) { its always empty so we don't need the if https://codereview.chromium.org/942633004/diff/200001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:390: int result = upload->Read(write_buff.get(), kExpectedStreamLength, Ah, I see what tripped the dcheck in the stream reader. kExpectedStreamLength is 0. The production code probably shouldn't change to accomodate this test, this test should alloc a small non-zero-length buffer. https://codereview.chromium.org/942633004/diff/200001/net/base/upload_data_st... File net/base/upload_data_stream.cc (right): https://codereview.chromium.org/942633004/diff/200001/net/base/upload_data_st... net/base/upload_data_stream.cc:47: if (!buf_len || is_eof_) This is a little troublesome because generally reading 0 bytes back indicates eof. How is this hit by the tests? https://codereview.chromium.org/942633004/diff/200001/storage/browser/blob/bl... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/942633004/diff/200001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.cc:18: DCHECK(length > 0); While your here, can you convert this to an early return to accommodate callers of BrowserContext::CreateMemoryBackedBlob() creating empty blobs by passing in zero length.
https://codereview.chromium.org/942633004/diff/200001/net/base/upload_data_st... File net/base/upload_data_stream.cc (right): https://codereview.chromium.org/942633004/diff/200001/net/base/upload_data_st... net/base/upload_data_stream.cc:47: if (!buf_len || is_eof_) On 2015/03/09 21:15:18, michaeln wrote: > This is a little troublesome because generally reading 0 bytes back indicates > eof. How is this hit by the tests? Can we instead set is_eof_ to true in SetSize, if size is 0? And have a net/ test for that case, as we apparently don't have one currently. In fact, this fix and that change should be in a separate CL. I'm happy to review it.
https://codereview.chromium.org/942633004/diff/200001/net/base/upload_data_st... File net/base/upload_data_stream.cc (right): https://codereview.chromium.org/942633004/diff/200001/net/base/upload_data_st... net/base/upload_data_stream.cc:47: if (!buf_len || is_eof_) On 2015/03/09 21:20:09, mmenke wrote: > On 2015/03/09 21:15:18, michaeln wrote: > > This is a little troublesome because generally reading 0 bytes back indicates > > eof. How is this hit by the tests? > > Can we instead set is_eof_ to true in SetSize, if size is 0? And have a net/ > test for that case, as we apparently don't have one currently. > > In fact, this fix and that change should be in a separate CL. I'm happy to > review it. The test should be in elements_upload_data_stream_unittest.cc (We don't currently test this class independently, though we probably should).
https://codereview.chromium.org/942633004/diff/200001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/942633004/diff/200001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:434: // Test creates two blobs (but stores them to the same key), and then On 2015/03/09 21:15:18, michaeln wrote: > Honestly this comment is still confusing? The test puts one Blob and one File > into IDB, we expect a file on disk for each. My bad - intended to fix that in previous patch. https://codereview.chromium.org/942633004/diff/200001/content/browser/loader/... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/942633004/diff/200001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:326: const std::string kBlobFileData = ""; On 2015/03/09 21:15:18, michaeln wrote: > dont think we need this Yeah, it was handy when writing the test to make the file either empty (or not). Removed dead code as suggested. https://codereview.chromium.org/942633004/diff/200001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:327: const uint64_t kBlobLength = kBlobFileData.size(); On 2015/03/09 21:15:18, michaeln wrote: > since its always zero, kZeroLength would be more clear Done. https://codereview.chromium.org/942633004/diff/200001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:328: if (!kBlobFileData.empty()) { On 2015/03/09 21:15:18, michaeln wrote: > its always empty so we don't need the if Done. https://codereview.chromium.org/942633004/diff/200001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:390: int result = upload->Read(write_buff.get(), kExpectedStreamLength, On 2015/03/09 21:15:18, michaeln wrote: > Ah, I see what tripped the dcheck in the stream reader. kExpectedStreamLength is > 0. The production code probably shouldn't change to accomodate this test, this > test should alloc a small non-zero-length buffer. Done. https://codereview.chromium.org/942633004/diff/200001/net/base/upload_data_st... File net/base/upload_data_stream.cc (right): https://codereview.chromium.org/942633004/diff/200001/net/base/upload_data_st... net/base/upload_data_stream.cc:47: if (!buf_len || is_eof_) On 2015/03/09 21:22:41, mmenke wrote: > On 2015/03/09 21:20:09, mmenke wrote: > > On 2015/03/09 21:15:18, michaeln wrote: > > > This is a little troublesome because generally reading 0 bytes back > indicates > > > eof. How is this hit by the tests? > > > > Can we instead set is_eof_ to true in SetSize, if size is 0? And have a net/ > > test for that case, as we apparently don't have one currently. > > > > In fact, this fix and that change should be in a separate CL. I'm happy to > > review it. > > The test should be in elements_upload_data_stream_unittest.cc (We don't > currently test this class independently, though we probably should). Reverted this change as suggested. https://codereview.chromium.org/942633004/diff/200001/storage/browser/blob/bl... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/942633004/diff/200001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.cc:18: DCHECK(length > 0); On 2015/03/09 21:15:18, michaeln wrote: > While your here, can you convert this to an early return to accommodate callers > of BrowserContext::CreateMemoryBackedBlob() creating empty blobs by passing in > zero length. Done.
lgtm https://codereview.chromium.org/942633004/diff/260001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/942633004/diff/260001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:187: int blob_file_count_ = 0; go c++ 11! maybe init disk_usage_ this way too https://codereview.chromium.org/942633004/diff/260001/content/browser/loader/... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/942633004/diff/260001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:335: scoped_ptr<BlobDataHandle> handle1 = for clarity, i'd make the id and handle vars use the same naming scheme so id0 and handle0 would pair up. wait... better yet... i don't see where handle1 is used, so maybe just delete this block and drop the numbering https://codereview.chromium.org/942633004/diff/260001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:372: const size_t kExpectedStreamLength = 3U * kZeroLength; nit: maybe use the zero constant here and below https://codereview.chromium.org/942633004/diff/260001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:377: int kAttemptedReadLength = kExpectedStreamLength + 1; nit: kBufferLength or kBufferSize is more typical https://codereview.chromium.org/942633004/diff/260001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:379: scoped_refptr<net::IOBuffer> write_buff = nit: read_buf or io_buffer?
https://codereview.chromium.org/942633004/diff/260001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/942633004/diff/260001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:187: int blob_file_count_ = 0; On 2015/03/09 22:34:06, michaeln wrote: > go c++ 11! maybe init disk_usage_ this way too I try to avoid making changes unrelated to the reason for the change - just to preserve the history, but I'm I'm happy to do so if you think that won't cause confusion. https://codereview.chromium.org/942633004/diff/260001/content/browser/loader/... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/942633004/diff/260001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:335: scoped_ptr<BlobDataHandle> handle1 = On 2015/03/09 22:34:06, michaeln wrote: > for clarity, i'd make the id and handle vars use the same naming scheme so id0 > and handle0 would pair up. > > wait... better yet... i don't see where handle1 is used, so maybe just delete > this block and drop the numbering Done. I "cargo culted" that from the test above (ResolveBlobAndCreateUploadDataStream). https://codereview.chromium.org/942633004/diff/260001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:379: scoped_refptr<net::IOBuffer> write_buff = On 2015/03/09 22:34:06, michaeln wrote: > nit: read_buf or io_buffer? Well (if I'm not mistaken). Read is reading from the UploadDataStream, and writing to that buffer. I'll go with io_buffer.
The CQ bit was checked by cmumford@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinaba@chromium.org, jsbell@chromium.org, mmenke@chromium.org, michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/942633004/#ps280001 (title: "More test cleanup.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/942633004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/942633004/diff/260001/content/browser/loader/... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/942633004/diff/260001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:335: scoped_ptr<BlobDataHandle> handle1 = On 2015/03/09 23:00:10, cmumford wrote: > On 2015/03/09 22:34:06, michaeln wrote: > > for clarity, i'd make the id and handle vars use the same naming scheme so id0 > > and handle0 would pair up. > > > > wait... better yet... i don't see where handle1 is used, so maybe just delete > > this block and drop the numbering > > Done. I "cargo culted" that from the test above > (ResolveBlobAndCreateUploadDataStream). But the block about "// First an empty blob" is still in the cl and blob_id1 is still paired up with handle2? I think you can delete lines 331 thru 336 because blob_id0 and handle are not used below. https://codereview.chromium.org/942633004/diff/280001/content/browser/loader/... File content/browser/loader/upload_data_stream_builder_unittest.cc (left): https://codereview.chromium.org/942633004/diff/280001/content/browser/loader/... content/browser/loader/upload_data_stream_builder_unittest.cc:112: blob_storage_context.AddFinishedBlob(blob_data_builder.get()); looks like this handle in the existing test actually is needed
The CQ bit was checked by cmumford@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, jsbell@chromium.org, mmenke@chromium.org, kinaba@chromium.org Link to the patchset: https://codereview.chromium.org/942633004/#ps320001 (title: "Re-removed handle1")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/942633004/320001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Josh: I couldn't figure out why Android (and only Android) is failing to set the access/modified times in CallFutimes. futimes (see os_compat_android.cc) is failing with errno set to EPERM (1). We have unit tests which I believe run on Android, so this function should work, but maybe it's a process permission issue with the content browser tests - or a security feature??? Let me know if you think this is too hackey to land.
On 2015/03/13 23:21:56, cmumford wrote: > Josh: I couldn't figure out why Android (and only Android) is failing to set the > access/modified times in CallFutimes. futimes (see os_compat_android.cc) is > failing with errno set to EPERM (1). We have unit tests which I believe run on > Android, so this function should work, but maybe it's a process permission issue > with the content browser tests - or a security feature??? I looked for places that call SetTimes and there aren't many, but this one: https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_un... ... is oddly enough in a test that's only enabled on OS_ANDROID. :P Anyway... I think this is fine - lgtm - to land with the test as you have it. But please file a bug and add it to the TODO. It seems easy enough to write a completely standalone test that calls SetTimes and compare the behavior.
The CQ bit was checked by cmumford@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, mmenke@chromium.org, kinaba@chromium.org, jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/942633004/#ps380001 (title: "Added bug for test re: Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/942633004/380001
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/3a61739491730a1acd96c09a6ae7b79825eb445f Cr-Commit-Position: refs/heads/master@{#320636} |
