Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1108083002:
Create blobs from Disk Cache entries. (Closed)
DescriptionCreate blobs from Disk Cache entries.
This CL allows blobs to be backed by disk cache entries, and also
migrates the Cache Storage Cache implementation to use this
functionality. This is very useful in the ServiceWorker case, as the
existing implementation was copying entire disk cache entries to RAM
before serving them to consumers.
BUG=None
Committed: https://crrev.com/9668ba5c2096eb4e6f601d9f1eab8e28f84812b7
Cr-Commit-Position: refs/heads/master@{#335185}
Patch Set 1 #
Total comments: 12
Patch Set 2 : fix fat fingers #Patch Set 3 : rebase #Patch Set 4 : more tests pass #Patch Set 5 : rebase, tighter #Patch Set 6 : fix windows build #Patch Set 7 : rebase? #Patch Set 8 : rebase2 #Patch Set 9 : all your rebases #Patch Set 10 : omg i see rebases #Patch Set 11 : fix build? #Patch Set 12 : cleaner... #Patch Set 13 : narrowergit #Patch Set 14 : first review version, based on trunk for try jobs #Patch Set 15 : rebased to upstream CL, review this upload #
Total comments: 27
Patch Set 16 : partial remediation #Patch Set 17 : fix unit tests build... #
Total comments: 4
Patch Set 18 : rebase #Patch Set 19 : introducing DataHandle... #Patch Set 20 : adding offsets and lengths, first pass... #Patch Set 21 : narrower, preparing to revert upstream... #Patch Set 22 : rebased, removing now moot upstream CL... #Patch Set 23 : build fix #Patch Set 24 : narrower and include fixes #Patch Set 25 : fix tests #Patch Set 26 : narrower still... #Patch Set 27 : rebase #
Total comments: 4
Patch Set 28 : rebase again #Patch Set 29 : closer... #Patch Set 30 : initialize variables... #Patch Set 31 : all your rebases #Patch Set 32 : rebase better #
Total comments: 21
Patch Set 33 : and unit tests... #Patch Set 34 : remediate + windows build #
Total comments: 20
Patch Set 35 : first pass michaeln remediation... #
Total comments: 4
Patch Set 36 : make disk cache entries uploadable #Patch Set 37 : remediate to tsepez review #
Total comments: 13
Patch Set 38 : more cleanup + unit test for reader #Patch Set 39 : narrower #Patch Set 40 : even narrower #
Total comments: 70
Patch Set 41 : rebase #
Total comments: 1
Patch Set 42 : self review #Patch Set 43 : fix unit test memory leak #
Total comments: 4
Patch Set 44 : remediate to reviews, finally (?) fix blob_storage_context_unittest... #Patch Set 45 : ok, less crazy #
Total comments: 2
Patch Set 46 : lose two more const #
Total comments: 5
Patch Set 47 : omg, fix the correct base::Unretained... #
Total comments: 56
Patch Set 48 : partial remediation #Patch Set 49 : and unit tests #Patch Set 50 : unconfuzinate #
Total comments: 2
Patch Set 51 : NULL --> nullptr #
Total comments: 2
Messages
Total messages: 92 (9 generated)
jkarlin@chromium.org changed reviewers: + jkarlin@chromium.org
I realize that this is WIP but I wanted to make sure you were thinking about the lifetime of the CacheStorageCache object. While disk_cache entries exist, we need to keep the backend alive, which means we need to keep the CacheStorageCache alive. This can be done by hanging onto a refptr to CacheStorageCache, perhaps in the blob?
On 2015/04/28 11:51:30, jkarlin wrote: > I realize that this is WIP but I wanted to make sure you were thinking about the > lifetime of the CacheStorageCache object. While disk_cache entries exist, we > need to keep the backend alive, which means we need to keep the > CacheStorageCache alive. This can be done by hanging onto a refptr to > CacheStorageCache, perhaps in the blob? Why do we need to keep the backend alive wile disk cache entries exist? What's the problem scenario here?
On 2015/04/28 16:08:06, gavinp wrote: > On 2015/04/28 11:51:30, jkarlin wrote: > > I realize that this is WIP but I wanted to make sure you were thinking about > the > > lifetime of the CacheStorageCache object. While disk_cache entries exist, we > > need to keep the backend alive, which means we need to keep the > > CacheStorageCache alive. This can be done by hanging onto a refptr to > > CacheStorageCache, perhaps in the blob? > > Why do we need to keep the backend alive wile disk cache entries exist? What's > the problem scenario here? If the CacheStorageCache is closed and later reopened, it will have a new backend. Reads and writes to the same entry will conflict with the old entry right? For instance, what if the entry is deleted in the new backend? The blob is now dangling.
On 2015/04/28 16:22:22, jkarlin wrote: > On 2015/04/28 16:08:06, gavinp wrote: > > On 2015/04/28 11:51:30, jkarlin wrote: > > > I realize that this is WIP but I wanted to make sure you were thinking about > > the > > > lifetime of the CacheStorageCache object. While disk_cache entries exist, we > > > need to keep the backend alive, which means we need to keep the > > > CacheStorageCache alive. This can be done by hanging onto a refptr to > > > CacheStorageCache, perhaps in the blob? > > > > Why do we need to keep the backend alive wile disk cache entries exist? What's > > the problem scenario here? > > If the CacheStorageCache is closed and later reopened, it will have a new > backend. Reads and writes to the same entry will conflict with the old entry > right? For instance, what if the entry is deleted in the new backend? The blob > is now dangling. Ah, I see; there could potentially be problems if a second backend is created in the same directory while IO is ongoing. In practice, this won't be an issue if the cache storage cache object does IO carefully; dooming the entry on put instead of rewriting it (which it looks like it does). In that case, things will just work like they should; the existing blob will be the old deleted entry, and not a mixture of the two. That is sailing a bit close to the wind though; but a test should protect us.
On 2015/04/28 16:33:36, gavinp wrote: > On 2015/04/28 16:22:22, jkarlin wrote: > > On 2015/04/28 16:08:06, gavinp wrote: > > > On 2015/04/28 11:51:30, jkarlin wrote: > > > > I realize that this is WIP but I wanted to make sure you were thinking > about > > > the > > > > lifetime of the CacheStorageCache object. While disk_cache entries exist, > we > > > > need to keep the backend alive, which means we need to keep the > > > > CacheStorageCache alive. This can be done by hanging onto a refptr to > > > > CacheStorageCache, perhaps in the blob? > > > > > > Why do we need to keep the backend alive wile disk cache entries exist? > What's > > > the problem scenario here? > > > > If the CacheStorageCache is closed and later reopened, it will have a new > > backend. Reads and writes to the same entry will conflict with the old entry > > right? For instance, what if the entry is deleted in the new backend? The blob > > is now dangling. > > Ah, I see; there could potentially be problems if a second backend is created in > the same directory while IO is ongoing. > > In practice, this won't be an issue if the cache storage cache object does IO > carefully; dooming the entry on put instead of rewriting it (which it looks like > it does). In that case, things will just work like they should; the existing > blob will be the old deleted entry, and not a mixture of the two. That is > sailing a bit close to the wind though; but a test should protect us. Does the new backend know about the first backend's entry when it dooms? Won't it just delete the file immediately?
On 2015/04/28 16:36:40, jkarlin wrote: > On 2015/04/28 16:33:36, gavinp wrote: > > On 2015/04/28 16:22:22, jkarlin wrote: > > > On 2015/04/28 16:08:06, gavinp wrote: > > > > On 2015/04/28 11:51:30, jkarlin wrote: > > > > > I realize that this is WIP but I wanted to make sure you were thinking > > about > > > > the > > > > > lifetime of the CacheStorageCache object. While disk_cache entries > exist, > > we > > > > > need to keep the backend alive, which means we need to keep the > > > > > CacheStorageCache alive. This can be done by hanging onto a refptr to > > > > > CacheStorageCache, perhaps in the blob? > > > > > > > > Why do we need to keep the backend alive wile disk cache entries exist? > > What's > > > > the problem scenario here? > > > > > > If the CacheStorageCache is closed and later reopened, it will have a new > > > backend. Reads and writes to the same entry will conflict with the old entry > > > right? For instance, what if the entry is deleted in the new backend? The > blob > > > is now dangling. > > > > Ah, I see; there could potentially be problems if a second backend is created > in > > the same directory while IO is ongoing. > > > > In practice, this won't be an issue if the cache storage cache object does IO > > carefully; dooming the entry on put instead of rewriting it (which it looks > like > > it does). In that case, things will just work like they should; the existing > > blob will be the old deleted entry, and not a mixture of the two. That is > > sailing a bit close to the wind though; but a test should protect us. > > Does the new backend know about the first backend's entry when it dooms? Won't > it just delete the file immediately? Yes, it will delete the file immediately. That's what doom always does though; the simple cache is designed so that Doom is just rm. The index should also be updated, but if it's being done through the new backend, that will happen as it should. The existing open entry will work fine, and continue serving the blob. Then, when the blob is done, the old entry is closed, and the file will be removed from disk. On windows it's a bit different, but basically the same thing. The only difference is that the entry is renamed first, because a deleted but open file in windows can block creating a file with the same name in the same directory.
On 2015/04/28 16:47:00, gavinp wrote: > On 2015/04/28 16:36:40, jkarlin wrote: > > On 2015/04/28 16:33:36, gavinp wrote: > > > On 2015/04/28 16:22:22, jkarlin wrote: > > > > On 2015/04/28 16:08:06, gavinp wrote: > > > > > On 2015/04/28 11:51:30, jkarlin wrote: > > > > > > I realize that this is WIP but I wanted to make sure you were thinking > > > about > > > > > the > > > > > > lifetime of the CacheStorageCache object. While disk_cache entries > > exist, > > > we > > > > > > need to keep the backend alive, which means we need to keep the > > > > > > CacheStorageCache alive. This can be done by hanging onto a refptr to > > > > > > CacheStorageCache, perhaps in the blob? > > > > > > > > > > Why do we need to keep the backend alive wile disk cache entries exist? > > > What's > > > > > the problem scenario here? > > > > > > > > If the CacheStorageCache is closed and later reopened, it will have a new > > > > backend. Reads and writes to the same entry will conflict with the old > entry > > > > right? For instance, what if the entry is deleted in the new backend? The > > blob > > > > is now dangling. > > > > > > Ah, I see; there could potentially be problems if a second backend is > created > > in > > > the same directory while IO is ongoing. > > > > > > In practice, this won't be an issue if the cache storage cache object does > IO > > > carefully; dooming the entry on put instead of rewriting it (which it looks > > like > > > it does). In that case, things will just work like they should; the existing > > > blob will be the old deleted entry, and not a mixture of the two. That is > > > sailing a bit close to the wind though; but a test should protect us. > > > > Does the new backend know about the first backend's entry when it dooms? Won't > > it just delete the file immediately? > > Yes, it will delete the file immediately. That's what doom always does though; > the simple cache is designed so that Doom is just rm. The index should also be > updated, but if it's being done through the new backend, that will happen as it > should. The existing open entry will work fine, and continue serving the blob. > Then, when the blob is done, the old entry is closed, and the file will be > removed from disk. > > On windows it's a bit different, but basically the same thing. The only > difference is that the entry is renamed first, because a deleted but open file > in windows can block creating a file with the same name in the same directory. Hah, this might all be moot in the end. The memory backend has a DCHECK that stops the backend from being deleted while entries are open, which is contrary to the documentation in disk_cache.h, but such is the joy of having multiple implementations.
fix build?
gavinp@chromium.org changed reviewers: + dmurph@chromium.org
dmurph, jkarlin: PTAL. jkarlin: I've incorporated an ExtraData class per your earlier suggestions. Bummer that the Memory Cache implementation doesn't meet the specification, I'll look in to updating the specification on that ASAP. dmurph: This follows the design we discussed. I'll need some other reviewers for LGTM clearance in each of the areas, but I wanted to go through it with you two first since I think you're likely to have the largest suggestions; it hopefully saves noise for the next group of reviewers. There's an upstream review (see the description), so git cl try won't work on patch set 15 (the one for review). I uploaded a patch set based on master, so I could run "git cl try", see the results on patch set 14. Thanks!
jkarlin: Also, I'm spinning up a CL downstream from this that removes the last of the disk_cache::Entry* from the structs in the cache storage implementation. I'll share that with you as soon as it's ready for primetime.
Super exciting CL. Just a few comments. https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:129: typedef base::Callback<void(scoped_ptr<CacheMetadata>)> MetadataCallback; Why move this? I prefer to have typdefs and enums at the top so that they're easy to find. https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:131: enum EntryIndex { INDEX_HEADERS = 0, INDEX_RESPONSE_BODY }; Ditto. https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:756: put_context->cache_entry.reset(*entry_ptr.get()); reset(*entry_ptr) https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:388: EXPECT_EQ(storage::DataElement::TYPE_DISK_CACHE_ENTRY, item->type()); This seems redundant with the case. https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:390: const int BODY_STREAM_INDEX = 1; You can get the index from item->disk_cache_entry() https://codereview.chromium.org/1108083002/diff/270001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/270001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:137: // Disk cache entries should not be uploaded, instead only requested Prefer that you put back the "NOTREACHED()" in each case so that if you error out you know which type it failed on. https://codereview.chromium.org/1108083002/diff/270001/content/common/resourc... File content/common/resource_messages.cc (right): https://codereview.chromium.org/1108083002/diff/270001/content/common/resourc... content/common/resource_messages.cc:63: case storage::DataElement::TYPE_DISK_CACHE_ENTRY: // Can't be sent via IPC. Add NOTREACHED() so you can distinguish between disk cache entry and unknown. https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:68: scoped_refptr<ShareableFileReference> file_handle_; file_handle_ should be converted to an ExtraData as well. https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:71: disk_cache::ScopedEntryPtr disk_cache_entry_; // For TYPE_DISK_CACHE_ENTRY. Why store this here? Why not put it in a DataElement like the other types? T https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:72: int disk_cache_stream_index_; // For TYPE_DISK_CACHE_ENTRY. ditto https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (left): https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:307: // and the expected modification time. Add your new type's description to this comment as well. https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:98: DCHECK_EQ(1U, blob_map_.count(external_builder->uuid_)); Why this change? https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:286: case DataElement::TYPE_DISK_CACHE_ENTRY: // This type can't be sent by IPC. Add NOTREACHED(). https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/v... File storage/browser/blob/view_blob_internals_job.cc (right): https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/v... storage/browser/blob/view_blob_internals_job.cc:221: break; Please add NOTREACHED(); to this and TYPE_BLOB
Josh, I've remediated to your first round of comments. I'm waiting for dmurph to weigh in on the shareable file handle extradata usage. The shareable file handle is such a thin abstraction as it is, I keep hoping there's something better than adding another layer around it. https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:129: typedef base::Callback<void(scoped_ptr<CacheMetadata>)> MetadataCallback; On 2015/05/29 14:59:41, jkarlin wrote: > Why move this? I prefer to have typdefs and enums at the top so that they're > easy to find. I moved it back. The reason I moved it down was for readability; I like declarations as close as possible to usage. Our coding standard ordering rule allows this; the ordering requirement in it applies only within classes. However, we all really use grep to find these things, and besides, a weak argument like this doesn't justify moving it away from where it was working perfectly well. https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:131: enum EntryIndex { INDEX_HEADERS = 0, INDEX_RESPONSE_BODY }; On 2015/05/29 14:59:41, jkarlin wrote: > Ditto. Done. https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:756: put_context->cache_entry.reset(*entry_ptr.get()); On 2015/05/29 14:59:41, jkarlin wrote: > reset(*entry_ptr) Done. https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:388: EXPECT_EQ(storage::DataElement::TYPE_DISK_CACHE_ENTRY, item->type()); On 2015/05/29 14:59:41, jkarlin wrote: > This seems redundant with the case. Done. https://codereview.chromium.org/1108083002/diff/270001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:390: const int BODY_STREAM_INDEX = 1; On 2015/05/29 14:59:41, jkarlin wrote: > You can get the index from item->disk_cache_entry() Done. https://codereview.chromium.org/1108083002/diff/270001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/270001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:137: // Disk cache entries should not be uploaded, instead only requested On 2015/05/29 14:59:41, jkarlin wrote: > Prefer that you put back the "NOTREACHED()" in each case so that if you error > out you know which type it failed on. Done. Now I understand why it was written the other way! https://codereview.chromium.org/1108083002/diff/270001/content/common/resourc... File content/common/resource_messages.cc (right): https://codereview.chromium.org/1108083002/diff/270001/content/common/resourc... content/common/resource_messages.cc:63: case storage::DataElement::TYPE_DISK_CACHE_ENTRY: // Can't be sent via IPC. On 2015/05/29 14:59:41, jkarlin wrote: > Add NOTREACHED() so you can distinguish between disk cache entry and unknown. Done. https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:68: scoped_refptr<ShareableFileReference> file_handle_; On 2015/05/29 14:59:41, jkarlin wrote: > file_handle_ should be converted to an ExtraData as well. That's a good idea. dmurph, wdyt? https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:71: disk_cache::ScopedEntryPtr disk_cache_entry_; // For TYPE_DISK_CACHE_ENTRY. On 2015/05/29 14:59:41, jkarlin wrote: > Why store this here? Why not put it in a DataElement like the other types? T DataElement is a IPCable serializable entity; the BlobDataItem is the local abstraction that includes browser process only data for using it. I talked to dmurph about this exact problem and he suggested this. dmurph: can you amplify any or remind me any on why you suggested we do it this way again? https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (left): https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:307: // and the expected modification time. On 2015/05/29 14:59:42, jkarlin wrote: > Add your new type's description to this comment as well. Done. This is why I hate comments like this. https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:98: DCHECK_EQ(1U, blob_map_.count(external_builder->uuid_)); On 2015/05/29 14:59:41, jkarlin wrote: > Why this change? I don't like doing DCHECK with == or != in them; it's best to use DCHECK_EQ, for better printouts on failure. That's not relevant in this case though; as both will have equally useful debug dumps. The other advantage to this formulation is safety; the DCHECK is before any possible dereference, but that's pretty minor. I've moved it back. https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:286: case DataElement::TYPE_DISK_CACHE_ENTRY: // This type can't be sent by IPC. On 2015/05/29 14:59:42, jkarlin wrote: > Add NOTREACHED(). Done. https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/v... File storage/browser/blob/view_blob_internals_job.cc (right): https://codereview.chromium.org/1108083002/diff/270001/storage/browser/blob/v... storage/browser/blob/view_blob_internals_job.cc:221: break; On 2015/05/29 14:59:42, jkarlin wrote: > Please add NOTREACHED(); to this and TYPE_BLOB Done.
Mostly looks good, let me know if you have an questions, we can VC again as well. Biggest issue is the splice thing. https://codereview.chromium.org/1108083002/diff/310001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/310001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:52: }; Hm, what about // The DataHandle class is used to persist resources that are needed for // reading this blob data item. This object will stay around while any // reads are pending event if // * All blobs with this item are deleted, or // * The item is swapped for a different backed version (mem-to-disk or reverse) // In these cases the item will be destructed after all pending reads are // complete. class DataHandle { and we can also remove the ShareableFileReference class variable and replace it with a private class that extends DataHandle as well. https://codereview.chromium.org/1108083002/diff/310001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/1108083002/diff/310001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:417: // Always reuse disk cache entries, as they never have an offset or length. Ah, this might be a problem. If I try to splice a blob that contains the disk cache entry, this logic determines how that works in the 'new' blob. You're going to have to either: 1. Support having an offset and length for reading, and then do what the file case does here 2. Manually copy the memory out into a data item when we're splicing inside of a disk cache entry.
This latest upload addresses dmurph's comments, and as a result, the one thing that was left hanging from jkarlin's earlier review; the ShareableFileReference is now unified in the BlobDataItem::DataHandle data type suggested by dmurph. I've narrowed this a bit as well, you might see some ancillary CLs come across your desk. The splice fix now means that the BlobDataItem::offset() and BlobDataItem::length() stay with us. So the upstream CL is now closed, and this is based on trunk. dmurph, jkarlin: PTAL. I'll add owners tomorrow unless I see architectural objections coming down the pike. https://codereview.chromium.org/1108083002/diff/310001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/310001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:52: }; On 2015/05/29 19:13:43, dmurph wrote: > Hm, what about > > // The DataHandle class is used to persist resources that are needed for > // reading this blob data item. This object will stay around while any > // reads are pending event if > // * All blobs with this item are deleted, or > // * The item is swapped for a different backed version (mem-to-disk or reverse) > // In these cases the item will be destructed after all pending reads are > // complete. > class DataHandle { > > > and we can also remove the ShareableFileReference class variable and replace it > with a private class that extends DataHandle as well. Done. I avoided an extra layer of indirection in the case of the ShareableFileHandle by just making it a descendent of DataHandle, as well. https://codereview.chromium.org/1108083002/diff/310001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/1108083002/diff/310001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:417: // Always reuse disk cache entries, as they never have an offset or length. On 2015/05/29 19:13:43, dmurph wrote: > Ah, this might be a problem. If I try to splice a blob that contains the disk > cache entry, this logic determines how that works in the 'new' blob. > > You're going to have to either: > 1. Support having an offset and length for reading, and then do what the file > case does here > 2. Manually copy the memory out into a data item when we're splicing inside of a > disk cache entry. OK, I went through this, and I think I get it. I went with (1), the most consistent option. That caused a chain of changes down through everything; in particular we can't score a disk_cache::ScopedEntryPtr in the BlobDataItemk anymore, since we might create a second BlobDataItem that should refer to the same cache entry. So the disk_cache_:ScopedEntryPtr has moved in to the DataHandle, which adds an interesting method on it.
I really like this, few more questions here, hopefully we can make this work while keeping our crazy abstraction layers. I'm available tomorrow morning to VC, which might make this faster, as I may be missing something. https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:35: virtual disk_cache::Entry* disk_cache_entry() const; Sorry, why does this need to be here? To get the size? Can we just put the size on creation in the DataElement? Having this here doesn't make as much sense now, kind of defeats the purpose of this generic class. Perhaps I've made everything worse o.O https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/s... File storage/browser/blob/shareable_file_reference.h (right): https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/s... storage/browser/blob/shareable_file_reference.h:20: class STORAGE_EXPORT ShareableFileReference : public BlobDataItem::DataHandle { Can you actually keep this the way it was? And then have: * DataHandle no longer be refcounted * Create these static methods in BlobDataHandle: template <typename T> DataHandle* CreateDataHandleForResource(scoped_refptr<T> resource) template <typename T> DataHandle* CreateDataHandleForResource(scoped_ptr<T> resource) Both of which use private implementations of DataHandle to wrap those resources. This makes the DataHandle a little cleaner as a simple container, and makes it easy to wrap generic resources that are either refcounted or should be owned by that element. Does this make sense? I'm also available to VC early tomorrow, which might be easier, as I might be making this more complex than we need.
I really like this, few more questions here, hopefully we can make this work while keeping our crazy abstraction layers. I'm available tomorrow morning to VC, which might make this faster, as I may be missing something. https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:35: virtual disk_cache::Entry* disk_cache_entry() const; Sorry, why does this need to be here? To get the size? Can we just put the size on creation in the DataElement? Having this here doesn't make as much sense now, kind of defeats the purpose of this generic class. Perhaps I've made everything worse o.O https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/s... File storage/browser/blob/shareable_file_reference.h (right): https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/s... storage/browser/blob/shareable_file_reference.h:20: class STORAGE_EXPORT ShareableFileReference : public BlobDataItem::DataHandle { Can you actually keep this the way it was? And then have: * DataHandle no longer be refcounted * Create these static methods in BlobDataHandle: template <typename T> DataHandle* CreateDataHandleForResource(scoped_refptr<T> resource) template <typename T> DataHandle* CreateDataHandleForResource(scoped_ptr<T> resource) Both of which use private implementations of DataHandle to wrap those resources. This makes the DataHandle a little cleaner as a simple container, and makes it easy to wrap generic resources that are either refcounted or should be owned by that element. Does this make sense? I'm also available to VC early tomorrow, which might be easier, as I might be making this more complex than we need.
I really like this, few more questions here, hopefully we can make this work while keeping our crazy abstraction layers. I'm available tomorrow morning to VC, which might make this faster, as I may be missing something. https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:35: virtual disk_cache::Entry* disk_cache_entry() const; Sorry, why does this need to be here? To get the size? Can we just put the size on creation in the DataElement? Having this here doesn't make as much sense now, kind of defeats the purpose of this generic class. Perhaps I've made everything worse o.O https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/s... File storage/browser/blob/shareable_file_reference.h (right): https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/s... storage/browser/blob/shareable_file_reference.h:20: class STORAGE_EXPORT ShareableFileReference : public BlobDataItem::DataHandle { Can you actually keep this the way it was? And then have: * DataHandle no longer be refcounted * Create these static methods in BlobDataHandle: template <typename T> DataHandle* CreateDataHandleForResource(scoped_refptr<T> resource) template <typename T> DataHandle* CreateDataHandleForResource(scoped_ptr<T> resource) Both of which use private implementations of DataHandle to wrap those resources. This makes the DataHandle a little cleaner as a simple container, and makes it easy to wrap generic resources that are either refcounted or should be owned by that element. Does this make sense? I'm also available to VC early tomorrow, which might be easier, as I might be making this more complex than we need.
https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:35: virtual disk_cache::Entry* disk_cache_entry() const; On 2015/06/05 01:17:22, dmurph wrote: > Sorry, why does this need to be here? To get the size? > > Can we just put the size on creation in the DataElement? > > Having this here doesn't make as much sense now, kind of defeats the purpose of > this generic class. Perhaps I've made everything worse o.O BlobDataItem needs an implementation of disk_cache_entry() for the same reason it needs an implementation of path(). In the URL request, we have to access the disk cache entry. So this is used for much more than the length, see the calls to ReadData and GetDataSize in the url request code in this same CL. If we're going to have a working splice, we need something refcounted that's holding on to the disk cache entry. The disk cache entries themselves can be accessed through a disk_cache::ScopedEntryPtr, which has behaviour as a scoped_ptr<>, or alternative as a disk_cache::Entry*. In earlier revisions of this patch in which splicing didn't work, it was fine to have a ScopedEntryPtr just hanging off of the BlobDataItem. But that's no good anymore, since those can't be copied; we need an interstitial refcounted object. That's why I put this method on the DataHandle. https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/s... File storage/browser/blob/shareable_file_reference.h (right): https://codereview.chromium.org/1108083002/diff/500001/storage/browser/blob/s... storage/browser/blob/shareable_file_reference.h:20: class STORAGE_EXPORT ShareableFileReference : public BlobDataItem::DataHandle { On 2015/06/05 01:17:23, dmurph wrote: > Can you actually keep this the way it was? And then have: > * DataHandle no longer be refcounted > * Create these static methods in BlobDataHandle: > template <typename T> > DataHandle* CreateDataHandleForResource(scoped_refptr<T> resource) > template <typename T> > DataHandle* CreateDataHandleForResource(scoped_ptr<T> resource) > Both of which use private implementations of DataHandle to wrap those > resources. > > This makes the DataHandle a little cleaner as a simple container, and makes it > easy to wrap generic resources that are either refcounted or should be owned by > that element. > > Does this make sense? I'm also available to VC early tomorrow, which might be > easier, as I might be making this more complex than we need. I'm totally confused. If we are going to allow splicing, how can any kind of scoped resource be held on to by a DataHandle unless the data handle is itself ref counted (which is what's in the CL that's uploaded)?? So all DataHandles will be refcounted. So why not make it refcounted if it is exactly a ref counted object? Irrelevant but complicating detail: a disk_cache::ScopedEntryPtr is not scoped_ptr<disk_cache::Entry>, for awkward and terrible historic reasons. Instead it's a scoped_ptr<disk_cache::Entry, MagicClassThatDestructsIt>. I wonder if you're objecting to the layer-removing optimization of making the ShareableFileReference just be a DataHandle. I considered implementing it as a container that just contained a scoped_refptr<ShareableFileReference>, but it seemed silly to have a class which was just a single scoped_refptr<> when that class exists only to be the target of a scoped_refptr... I agree on the VC. I'll put one on your calendar for early Friday so we can ref this hopefully quickly.
Thanks a ton for looking at this so quickly Daniel! I think we're both confused right now, so a VC makes a lot of sense. I'll put one on your calendar.
After talking to Daniel, I've updated this CL per our conversation. There's two updates in the latest upload: 1. I've removed the BlobDataItem::DataHandle::disk_cache_entry() method; instead the BlobDataItem just holds a naked disk_cache::Entry*. This is safe, because its scope is protected by the DataHandle created by the CacheStorageCache. 2. I've added a comment explaining why the inheritance from BlobDataItem::DataHandle in storage::ShareableFileReference. jkarlin, dmurph: PTAL?
Nice: I can now reproduce the test failures. It seems you need to use a release build... Great.
On 2015/06/08 at 21:18:54, gavinp wrote: > Nice: I can now reproduce the test failures. It seems you need to use a release build... Great. Can you put some tests in here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fi... Having the following cases covered would be great: * Using blob builder to create a blob w/ a disk cache entry & verify contents * Splitting a blob with a disk cache entry (adding a previous blob w/ an offset & size) & verify contents. * Possibly testing that the cache entry is destructed correctly if that's easily testable. Sorry I forgot to bring up the unittest earlier, I though you had them but I was mistaking the cache_storage ones. Otherwise it looks great.
https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (left): https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:309: struct CacheStorageCache::MatchContext { Might as well remove MatchContext at this point. https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:47: disk_cache::ScopedEntryPtr entry_; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.h (right): https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.h:103: friend class CacheStorageCacheTest; Is this necessary? Not seeing any uses of it in the unittest https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:384: case storage::DataElement::TYPE_BYTES: { I don't think you need to support TYPE_BYTES at this point. https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:386: } break; I realize the break after the } is semantically correct but it doesn't follow the rest of this directory, and it doesn't follow examples in the style guide: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Sw... https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:481: base::Unretained(this))); Is base::Unretained safe here? Aren't jobs cleared when the renderer dies? Same question applies to ::ReadFileItem. https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:482: DCHECK_LT(result, 0); The header doesn't say that ReadData is async only. Should we really be dchecking for an error here? https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:491: if (result <= 0) { What if the item was 0 bytes? https://codereview.chromium.org/1108083002/diff/590001/storage/common/data_el... File storage/common/data_element.h (right): https://codereview.chromium.org/1108083002/diff/590001/storage/common/data_el... storage/common/data_element.h:141: // each time. How about NOTREACHED()?
Patchset #34 (id:630001) has been deleted
dmurph, jkarlin: I believe this answers all your current comments. Please take a look now? I'll add OWNERS reviewers as well so they can join in the fun! https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (left): https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:309: struct CacheStorageCache::MatchContext { On 2015/06/12 15:53:37, jkarlin wrote: > Might as well remove MatchContext at this point. Done. https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:47: disk_cache::ScopedEntryPtr entry_; On 2015/06/12 15:53:37, jkarlin wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.h (right): https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.h:103: friend class CacheStorageCacheTest; On 2015/06/12 15:53:37, jkarlin wrote: > Is this necessary? Not seeing any uses of it in the unittest Done. https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:384: case storage::DataElement::TYPE_BYTES: { On 2015/06/12 15:53:37, jkarlin wrote: > I don't think you need to support TYPE_BYTES at this point. Lots of the tests use AppendData(); and so a lot of tests would need rewriting if we didn't support it here. Since we're trying to read from Blobs, I think it's maybe better to keep the more conveniently written tests using AppendData(), and have this special case here... https://codereview.chromium.org/1108083002/diff/590001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:386: } break; On 2015/06/12 15:53:37, jkarlin wrote: > I realize the break after the } is semantically correct but it doesn't follow > the rest of this directory, and it doesn't follow examples in the style guide: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Sw... Done. I was probably just used to editing some of the IPC parsers at this point which use that style pervasively. https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:481: base::Unretained(this))); On 2015/06/12 15:53:38, jkarlin wrote: > Is base::Unretained safe here? Aren't jobs cleared when the renderer dies? Same > question applies to ::ReadFileItem. But we're mid-read, so isn't the URLRequest::Delegate guaranteed to hold a reference the entire time? https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:482: DCHECK_LT(result, 0); On 2015/06/12 15:53:38, jkarlin wrote: > The header doesn't say that ReadData is async only. Should we really be > dchecking for an error here? True enough; and the memory cache is actually sync on this operation. Done. https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:491: if (result <= 0) { On 2015/06/12 15:53:38, jkarlin wrote: > What if the item was 0 bytes? Then we never called ReadDiskCacheEntryItem and there's no code path to here, so something really bad has happened. Let's call that a failure. See https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... https://codereview.chromium.org/1108083002/diff/590001/storage/common/data_el... File storage/common/data_element.h (right): https://codereview.chromium.org/1108083002/diff/590001/storage/common/data_el... storage/common/data_element.h:141: // each time. On 2015/06/12 15:53:38, jkarlin wrote: > How about NOTREACHED()? It introduces crashes. The comment was deceptive, I've updated it. The entry is compared in the BlobDataItem object so we're good.
gavinp@chromium.org changed reviewers: + inferno@chromium.org, michaeln@chromium.org, mmenke@chromium.org
Welcome, additional reviewers! michaeln: Please take a look. inferno: See changes to messages. mmenke: See UploadDataStreamBuilder.
lgtm
https://codereview.chromium.org/1108083002/diff/650001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/650001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:140: // from blink. Are you sure this comment about them being requested from blink is right? If we can't send them via IPCs, to or from blink, and they're created in the renderer process...I'm not following how these interact at all with blink. Admittedly, I know next to nothing about blobs (loader/ is such a catch all of random cruft that it has a lot of code I really don't know, most of it poorly documented, to boot)
https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:481: base::Unretained(this))); On 2015/06/12 18:10:30, gavinp wrote: > On 2015/06/12 15:53:38, jkarlin wrote: > > Is base::Unretained safe here? Aren't jobs cleared when the renderer dies? > Same > > question applies to ::ReadFileItem. > > But we're mid-read, so isn't the URLRequest::Delegate guaranteed to hold a > reference the entire time? I don't know how blob URLRequests are routed. Do they go through the ResourceDispatchHost? If so, when the renderer dies all of its URLRequests (and delegates) die too and there won't be a |this| to run on. mmenke will know
https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:481: base::Unretained(this))); On 2015/06/12 19:17:52, jkarlin wrote: > On 2015/06/12 18:10:30, gavinp wrote: > > On 2015/06/12 15:53:38, jkarlin wrote: > > > Is base::Unretained safe here? Aren't jobs cleared when the renderer dies? > > Same > > > question applies to ::ReadFileItem. > > > > But we're mid-read, so isn't the URLRequest::Delegate guaranteed to hold a > > reference the entire time? > > I don't know how blob URLRequests are routed. Do they go through the > ResourceDispatchHost? If so, when the renderer dies all of its URLRequests (and > delegates) die too and there won't be a |this| to run on. > > mmenke will know Blob URL requests do indeed go through the RDH (Or at least some of them do - there may be other paths for some, not sure). An object actually owning the underlying blob data is stapled to the corresponding URLRequestJob as UserData. So I don't believe Unretained is safe here. More generally, URLRequests may be cancelled at any time, and URLRequestJobs are expected to support that.
https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/590001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:481: base::Unretained(this))); On 2015/06/12 19:23:36, mmenke wrote: > On 2015/06/12 19:17:52, jkarlin wrote: > > On 2015/06/12 18:10:30, gavinp wrote: > > > On 2015/06/12 15:53:38, jkarlin wrote: > > > > Is base::Unretained safe here? Aren't jobs cleared when the renderer dies? > > > > Same > > > > question applies to ::ReadFileItem. > > > > > > But we're mid-read, so isn't the URLRequest::Delegate guaranteed to hold a > > > reference the entire time? > > > > I don't know how blob URLRequests are routed. Do they go through the > > ResourceDispatchHost? If so, when the renderer dies all of its URLRequests > (and > > delegates) die too and there won't be a |this| to run on. > > > > mmenke will know > > Blob URL requests do indeed go through the RDH (Or at least some of them do - > there may be other paths for some, not sure). An object actually owning the > underlying blob data is stapled to the corresponding URLRequestJob as UserData. > > So I don't believe Unretained is safe here. More generally, URLRequests may be > cancelled at any time, and URLRequestJobs are expected to support that. Sorry..scratch that. The UserData stapling is for blob uploads, not sure about blob downloads (Though both do go through the RDH)
inferno@chromium.org changed reviewers: + tsepez@chromium.org - inferno@chromium.org
Great to see this CL! Here's some quick comments, I have to look more closely still at the code and the review history too. https://codereview.chromium.org/1108083002/diff/1/content/browser/cache_stora... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1108083002/diff/1/content/browser/cache_stora... content/browser/cache_storage/cache_storage_cache.cc:642: scoped_ptr<storage::BlobDataBuilder> blob_data( looks like BlobDataBuilder could be stack allocated instead of heap allocated now? https://codereview.chromium.org/1108083002/diff/1/content/browser/loader/uplo... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/1/content/browser/loader/uplo... content/browser/loader/upload_data_stream_builder.cc:137: // Disk cache entries should not be uploaded, instead only requested Once something in blink has a Blob backed by a disk cache entry, what's to prevent it from uploading it? https://codereview.chromium.org/1108083002/diff/1/storage/browser/blob/blob_d... File storage/browser/blob/blob_data_item.cc (right): https://codereview.chromium.org/1108083002/diff/1/storage/browser/blob/blob_d... storage/browser/blob/blob_data_item.cc:16: if (type() == DataElement::TYPE_DISK_CACHE_ENTRY) does this break blob.slice()? https://codereview.chromium.org/1108083002/diff/1/storage/browser/blob/blob_s... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/1108083002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_context.cc:465: CHECK(false) << "Can't append one of these yo"; what's to prevent this? cache.match(x).then(function(response) { var newBlob = new Blob(["hello", response.body.slice(5)]); ... }); https://codereview.chromium.org/1108083002/diff/1/storage/browser/blob/blob_u... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/1/storage/browser/blob/blob_u... storage/browser/blob/blob_url_request_job.cc:465: base::Unretained(this))); 2 questions: 1) How is base::Unretained here safe? I don't think it is. It's safe in ReadFileItem() because when this class is destructed the FileStreamReader is deleted so that completion callback won't be called. In this case, |item| is refcounted and outlives this job. And Entry's are supposed to invoke the completion callback even after being closed. WeakPtr? 2) Is it ok to have two ReadData calls pending against the same Entry instance. Two different jobs could be running at the same time. Pretty sure its ok, but just checking. https://codereview.chromium.org/1108083002/diff/1/storage/common/data_element.h File storage/common/data_element.h (right): https://codereview.chromium.org/1108083002/diff/1/storage/common/data_element... storage/common/data_element.h:116: std::string blob_uuid_; // For TYPE_BLOB and TYPE_DISK_CACHE_ENTRY. where/how is this used for TYPE_DISK_CACHE_ENTRY?
sorry for the confusing comments, i was looking at the wrong snapshot??
I got the right snapshot this time. https://codereview.chromium.org/1108083002/diff/650001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1108083002/diff/650001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:658: scoped_ptr<storage::BlobDataBuilder> blob_data( looks like BlobDataBuilder could be stack allocated instead of heap allocated now? https://codereview.chromium.org/1108083002/diff/650001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/650001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:140: // from blink. Once something in blink has a Blob backed by a disk cache entry, what's to prevent it from uploading? cache.match(x).then(function(response) { xhr.send(response.body) ... }); https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:468: element->SetToDiskCacheEntryRange(item.length() + offset, should this be item.offset() + offset ? https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:481: base::Unretained(this))); 2 questions: 1) How is base::Unretained here safe? I don't think it is. It's safe in ReadFileItem() because when this class is destructed the FileStreamReader is deleted so that completion callback won't be called. In this case, |item| is refcounted and outlives this job. And Entry's are supposed to invoke the completion callback even after being closed. WeakPtr? 2) Is it ok to have two ReadData calls pending against the same Entry instance. Two different jobs could be running at the same time. Pretty sure its ok, but just checking. https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/s... File storage/browser/blob/shareable_file_reference.h (right): https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/s... storage/browser/blob/shareable_file_reference.h:20: // only works because there are no other fields in the DataHandle. I don't think the editorial comments about dbl-derefs and no other fields are helpful? Why say anything, the derivation says it all.
https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/s... File storage/browser/blob/shareable_file_reference.h (right): https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/s... storage/browser/blob/shareable_file_reference.h:20: // only works because there are no other fields in the DataHandle. On 2015/06/12 at 23:43:06, michaeln wrote: > I don't think the editorial comments about dbl-derefs and no other fields are helpful? Why say anything, the derivation says it all. I asked him to add this in case it wasn't clear for new readers. But it can definitely be removed if you think it's obvious.
michaeln, thanks for the first pass review. I've remediated to it mostly; the only exception is the upload handler. I'm not sure right now if there's a code path that will exercise it, I'm looking for that. https://codereview.chromium.org/1108083002/diff/1/content/browser/cache_stora... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1108083002/diff/1/content/browser/cache_stora... content/browser/cache_storage/cache_storage_cache.cc:642: scoped_ptr<storage::BlobDataBuilder> blob_data( On 2015/06/12 22:35:09, michaeln wrote: > looks like BlobDataBuilder could be stack allocated instead of heap allocated > now? Done. https://codereview.chromium.org/1108083002/diff/1/content/browser/loader/uplo... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/1/content/browser/loader/uplo... content/browser/loader/upload_data_stream_builder.cc:137: // Disk cache entries should not be uploaded, instead only requested On 2015/06/12 22:35:09, michaeln wrote: > Once something in blink has a Blob backed by a disk cache entry, what's to > prevent it from uploading it? Unless I'm missing something, there's no code path for it. The only place these blobs are created is in response to requests for cache storage. How can those get to the upload handler? I've updated the comment to just say not implemented. https://codereview.chromium.org/1108083002/diff/1/storage/browser/blob/blob_u... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/1/storage/browser/blob/blob_u... storage/browser/blob/blob_url_request_job.cc:465: base::Unretained(this))); On 2015/06/12 22:35:09, michaeln wrote: > 2 questions: > > 1) How is base::Unretained here safe? I don't think it is. > > It's safe in ReadFileItem() because when this class is destructed the > FileStreamReader is deleted so that completion callback won't be called. > > In this case, |item| is refcounted and outlives this job. And Entry's are > supposed to invoke the completion callback even after being closed. > > WeakPtr? I think you're right. jkarlin first raised this in his review, but I dismissed it too easily, not seeing the real reason it was safe. WeakPtr. Done. > 2) Is it ok to have two ReadData calls pending against the same Entry instance. > Two different jobs could be running at the same time. Pretty sure its ok, but > just checking. Yes, this is OK. https://codereview.chromium.org/1108083002/diff/650001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1108083002/diff/650001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:658: scoped_ptr<storage::BlobDataBuilder> blob_data( On 2015/06/12 23:43:06, michaeln wrote: > looks like BlobDataBuilder could be stack allocated instead of heap allocated > now? Done. https://codereview.chromium.org/1108083002/diff/650001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/650001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:140: // from blink. On 2015/06/12 18:52:42, mmenke wrote: > Are you sure this comment about them being requested from blink is right? If we > can't send them via IPCs, to or from blink, and they're created in the renderer > process...I'm not following how these interact at all with blink. > > Admittedly, I know next to nothing about blobs (loader/ is such a catch all of > random cruft that it has a lot of code I really don't know, most of it poorly > documented, to boot) Blobs are a mess as well; in particular, blobs have an IPC mechanism, but basically never use it for transfers from the browser process to the renderer process, as near as I can tell (dmurph and michaeln can explain this more). Instead, they are sent via their blob uuid from the browser to the renderer process, and a blob_url_request_job is consed up to request the blob's contents when needed. https://codereview.chromium.org/1108083002/diff/650001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:140: // from blink. On 2015/06/12 23:43:06, michaeln wrote: > Once something in blink has a Blob backed by a disk cache entry, what's to > prevent it from uploading? > > cache.match(x).then(function(response) { > xhr.send(response.body) > ... > }); Ahhh, OK. So right now I think this is safe, because all of the mechanisms to move blobs from the renderer process to the browser process do it by copying the whole thing in to RAM, but yes, once we have enabled that functionality we'll need it. Should that be in this CL, or the follow-up CL that adds the blink-to-browser process movement of response bodies? https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:437: element->SetToBytes(item.bytes() + offset, michaeln: Your comment below on line 468 got me wondering if this is OK? https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:468: element->SetToDiskCacheEntryRange(item.length() + offset, On 2015/06/12 23:43:06, michaeln wrote: > should this be item.offset() + offset ? Done. https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/s... File storage/browser/blob/shareable_file_reference.h (right): https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/s... storage/browser/blob/shareable_file_reference.h:20: // only works because there are no other fields in the DataHandle. On 2015/06/13 00:09:43, dmurph wrote: > On 2015/06/12 at 23:43:06, michaeln wrote: > > I don't think the editorial comments about dbl-derefs and no other fields are > helpful? Why say anything, the derivation says it all. > > I asked him to add this in case it wasn't clear for new readers. But it can > definitely be removed if you think it's obvious. Removed, just because I'm slightly with michaeln here. Let me know if you guys decide it should be back! :)
https://codereview.chromium.org/1108083002/diff/1/content/browser/loader/uplo... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/1/content/browser/loader/uplo... content/browser/loader/upload_data_stream_builder.cc:137: // Disk cache entries should not be uploaded, instead only requested On 2015/06/15 14:01:19, gavinp wrote: > On 2015/06/12 22:35:09, michaeln wrote: > > Once something in blink has a Blob backed by a disk cache entry, what's to > > prevent it from uploading it? > > Unless I'm missing something, there's no code path for it. The only place these > blobs are created is in response to requests for cache storage. How can those > get to the upload handler? > > I've updated the comment to just say not implemented. jkarlin and I just talked this over. It's true that in many cases, blobs coming from CacheStorage are unblobified and moved as bytes when they go back to the browser process (see the CacheStorageCache::PutImpl for an example in blink). However, let's not make all this asymmetry worse; these should be uploadable.
Naive question: how do we know that the renderer/SW has rights to see the contents of the cache entry? https://codereview.chromium.org/1108083002/diff/650001/content/common/resourc... File content/common/resource_messages.cc (right): https://codereview.chromium.org/1108083002/diff/650001/content/common/resourc... content/common/resource_messages.cc:65: case storage::DataElement::TYPE_DISK_CACHE_ENTRY: // Can't be sent via IPC. nit: if one of the cases has { }, all of them should. https://codereview.chromium.org/1108083002/diff/650001/content/common/resourc... content/common/resource_messages.cc:68: case storage::DataElement::TYPE_UNKNOWN: nit: Shouldn't this just be a default: case? https://codereview.chromium.org/1108083002/diff/590002/content/common/resourc... File content/common/resource_messages.cc (right): https://codereview.chromium.org/1108083002/diff/590002/content/common/resourc... content/common/resource_messages.cc:122: DCHECK(type == storage::DataElement::TYPE_BLOB); You should handle the other types in this switch, and return false if you see them (same with default: case). https://codereview.chromium.org/1108083002/diff/590002/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.h (right): https://codereview.chromium.org/1108083002/diff/590002/storage/browser/blob/b... storage/browser/blob/blob_data_builder.h:25: typedef BlobDataItem::DataHandle DataHandle; nit: using.
https://codereview.chromium.org/1108083002/diff/650001/content/common/resourc... File content/common/resource_messages.cc (right): https://codereview.chromium.org/1108083002/diff/650001/content/common/resourc... content/common/resource_messages.cc:68: case storage::DataElement::TYPE_UNKNOWN: On 2015/06/15 15:53:06, Tom Sepez wrote: > nit: Shouldn't this just be a default: case? Without a default, adding a new case results in a compile error. With a default, we get a runtime error instead, if we ever happen to reach this code with the new value. The compile error is much more robust.
On 2015/06/15 15:53:06, Tom Sepez wrote: > Naive question: how do we know that the renderer/SW has rights to see the > contents of the cache entry? > That's handled in the code in cache_storage_cache; it only is able to access the cache that contains the entry if messages are coming from the right context. Notably, this CL isn't changing any behaviour here. All it's doing is sending the data as a blob referencing a cache entry instead of a blob referencing a string of bytes. > https://codereview.chromium.org/1108083002/diff/650001/content/common/resourc... > File content/common/resource_messages.cc (right): > > https://codereview.chromium.org/1108083002/diff/650001/content/common/resourc... > content/common/resource_messages.cc:65: case > storage::DataElement::TYPE_DISK_CACHE_ENTRY: // Can't be sent via IPC. > nit: if one of the cases has { }, all of them should. > > https://codereview.chromium.org/1108083002/diff/650001/content/common/resourc... > content/common/resource_messages.cc:68: case storage::DataElement::TYPE_UNKNOWN: > nit: Shouldn't this just be a default: case? > > https://codereview.chromium.org/1108083002/diff/590002/content/common/resourc... > File content/common/resource_messages.cc (right): > > https://codereview.chromium.org/1108083002/diff/590002/content/common/resourc... > content/common/resource_messages.cc:122: DCHECK(type == > storage::DataElement::TYPE_BLOB); > You should handle the other types in this switch, and return false if you see > them (same with default: case). > > https://codereview.chromium.org/1108083002/diff/590002/storage/browser/blob/b... > File storage/browser/blob/blob_data_builder.h (right): > > https://codereview.chromium.org/1108083002/diff/590002/storage/browser/blob/b... > storage/browser/blob/blob_data_builder.h:25: typedef BlobDataItem::DataHandle > DataHandle; > nit: using.
tsepez: I've tried to answer your questions and follow your review feedback. Let me know what you think. https://codereview.chromium.org/1108083002/diff/650001/content/common/resourc... File content/common/resource_messages.cc (right): https://codereview.chromium.org/1108083002/diff/650001/content/common/resourc... content/common/resource_messages.cc:65: case storage::DataElement::TYPE_DISK_CACHE_ENTRY: // Can't be sent via IPC. On 2015/06/15 15:53:06, Tom Sepez wrote: > nit: if one of the cases has { }, all of them should. Done. https://codereview.chromium.org/1108083002/diff/650001/content/common/resourc... content/common/resource_messages.cc:68: case storage::DataElement::TYPE_UNKNOWN: On 2015/06/15 15:57:26, mmenke wrote: > On 2015/06/15 15:53:06, Tom Sepez wrote: > > nit: Shouldn't this just be a default: case? > > Without a default, adding a new case results in a compile error. With a > default, we get a runtime error instead, if we ever happen to reach this code > with the new value. The compile error is much more robust. That's the reasoning I used here too. https://codereview.chromium.org/1108083002/diff/590002/content/common/resourc... File content/common/resource_messages.cc (right): https://codereview.chromium.org/1108083002/diff/590002/content/common/resourc... content/common/resource_messages.cc:122: DCHECK(type == storage::DataElement::TYPE_BLOB); On 2015/06/15 15:53:06, Tom Sepez wrote: > You should handle the other types in this switch, and return false if you see > them (same with default: case). See mmenke and me's comment in your review of another version of this file. I'll update it, but remove the default. Let me know what you think. https://codereview.chromium.org/1108083002/diff/590002/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.h (right): https://codereview.chromium.org/1108083002/diff/590002/storage/browser/blob/b... storage/browser/blob/blob_data_builder.h:25: typedef BlobDataItem::DataHandle DataHandle; On 2015/06/15 15:53:06, Tom Sepez wrote: > nit: using. Done.
And oh yes, this is missing a unit test for UploadDiskCacheEntryElementReader. Coming soon. I want to see what folks think about the upload implementation first.
The new UploadElementReader is looking pretty good to me too. Some tests would definitely be good. https://codereview.chromium.org/1108083002/diff/1/content/browser/loader/uplo... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/1/content/browser/loader/uplo... content/browser/loader/upload_data_stream_builder.cc:137: // Disk cache entries should not be uploaded, instead only requested On 2015/06/15 14:25:55, gavinp wrote: > On 2015/06/15 14:01:19, gavinp wrote: > > On 2015/06/12 22:35:09, michaeln wrote: > > > Once something in blink has a Blob backed by a disk cache entry, what's to > > > prevent it from uploading it? > > > > Unless I'm missing something, there's no code path for it. The only place > these > > blobs are created is in response to requests for cache storage. How can those > > get to the upload handler? > > > > I've updated the comment to just say not implemented. > > jkarlin and I just talked this over. It's true that in many cases, blobs coming > from CacheStorage are unblobified and moved as bytes when they go back to the > browser process (see the CacheStorageCache::PutImpl for an example in blink). > However, let's not make all this asymmetry worse; these should be uploadable. The browser process is a Blob server of sorts and renderers are clients. Once a blob is created, the browser stores all the data for it. The uuid is passed to/from renderers as a reference to that data. A blob in a renderer can be passed by value in many ways: postMessage(blob), postMessage(objContainingBlobs), xhr.send(blob) xhr.send(formDataReferringToBlobs). CacheStorageCache::Put takes as input a reference to a blob that was created elsewhere and copies its bytes into a diskCache entry. CacheStorageCache::Match later cons's up a new blob around the diskCache entry and returns the value to the renderer by reference. With the Cache and CacheStorage API, is it possible for javascript to get a Blob that is returned by Match()? I've been assuming that it is. If so, then the upload code is reacachable (afaict). https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:435: DCHECK(!item.offset()); gavinp: looks like it is only because of this https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:98: const storage::BlobDataItem*>>* resolved_elements) { needs some indentation https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:174: // TODO(gavinp): What can we do if there's no blob_context? Does that matter? We're making upload readers for blob items? https://codereview.chromium.org/1108083002/diff/700001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.cc (right): https://codereview.chromium.org/1108083002/diff/700001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:47: return range_length_ + range_offset_ - offset_; nit: the math in here might be more readable in terms of 'amount_read'? https://codereview.chromium.org/1108083002/diff/700001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:65: const int result = disk_cache_entry_->ReadData( Calling ReadData() with zero bytes_to_read is ok, just checking.
https://codereview.chromium.org/1108083002/diff/1/content/browser/loader/uplo... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/1/content/browser/loader/uplo... content/browser/loader/upload_data_stream_builder.cc:137: // Disk cache entries should not be uploaded, instead only requested On 2015/06/15 22:17:23, michaeln wrote: > On 2015/06/15 14:25:55, gavinp wrote: > > On 2015/06/15 14:01:19, gavinp wrote: > > > On 2015/06/12 22:35:09, michaeln wrote: > > > > Once something in blink has a Blob backed by a disk cache entry, what's to > > > > prevent it from uploading it? > > > > > > Unless I'm missing something, there's no code path for it. The only place > > these > > > blobs are created is in response to requests for cache storage. How can > those > > > get to the upload handler? > > > > > > I've updated the comment to just say not implemented. > > > > jkarlin and I just talked this over. It's true that in many cases, blobs > coming > > from CacheStorage are unblobified and moved as bytes when they go back to the > > browser process (see the CacheStorageCache::PutImpl for an example in blink). > > However, let's not make all this asymmetry worse; these should be uploadable. > > The browser process is a Blob server of sorts and renderers are clients. Once a > blob is created, the browser stores all the data for it. The uuid is passed > to/from renderers as a reference to that data. A blob in a renderer can be > passed by value in many ways: postMessage(blob), > postMessage(objContainingBlobs), xhr.send(blob) > xhr.send(formDataReferringToBlobs). errr, i meant to say "passed by reference in many ways" > CacheStorageCache::Put takes as input a > reference to a blob that was created elsewhere and copies its bytes into a > diskCache entry. CacheStorageCache::Match later cons's up a new blob around the > diskCache entry and returns the value to the renderer by reference. > > With the Cache and CacheStorage API, is it possible for javascript to get a Blob > that is returned by Match()? I've been assuming that it is. If so, then the > upload code is reacachable (afaict).
Thanks. LGTM.
I've now added the unit test for the UploadDiskCacheEntryElementReader; it covers most of the use cases based on the other two unit tests. mmenke: Can you now take a look over net/ on this, the upload code is now substantially more complex. I have one question. Right now, we've left the DataElement if it's TYPE_DISK_CACHE_ENTRY unserializable. michaeln, I asked you a question about how OK it is that you can't build disk cache entries of this type. We could, potentially, make the BlobDataItem / DataElement confusion a lot easier by actually storing the blob_uuid in the DataElement for TYPE_DISK_CACHE_ENTRY. Then we could serialize them for IPC (as TYPE_BLOB, which is what the target would want), and you'd be able to call UploadDataStreamMumble::Build() with a DataElement of TYPE_DISK_CACHE_ENTRY. Is this desirable? I think it's moot because they are passed around as blob_uuids everywhere that matters. It is a conceptual cleanup I wanted to ask about though... https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/b... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/1108083002/diff/650001/storage/browser/blob/b... storage/browser/blob/blob_storage_context.cc:435: DCHECK(!item.offset()); On 2015/06/15 22:17:23, michaeln wrote: > gavinp: looks like it is only because of this Aha, thanks. https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:98: const storage::BlobDataItem*>>* resolved_elements) { On 2015/06/15 22:17:23, michaeln wrote: > needs some indentation Done. https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:174: // TODO(gavinp): What can we do if there's no blob_context? On 2015/06/15 22:17:23, michaeln wrote: > Does that matter? We're making upload readers for blob items? This comment is a bit wrong; the real problem is that this occurs when we call Build() with an element that is TYPE_DISK_CACHE_ENTRY. We actually NEED to be called with an element of TYPE_BLOB here, so that we call the resolve function, which is the only way to get the BlobDataItem pointer, which is required to find the disk_cache::Entry*... I don't know enough about the situations in which this code is called; how often will it be with a DataElement of TYPE_DISK_CACHE_ENTRY vs TYPE_BLOB? I'm guessing it's always TYPE_BLOB. I think your comment above confirms this? I'm clearing up my comment in my next upload, but I'm hoping we can just remove this worry. Should we be testing for |item| being null? There's a DCHECK on this stuff in ResolveBlobReference() so I figured it's OK to crash... https://codereview.chromium.org/1108083002/diff/700001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.cc (right): https://codereview.chromium.org/1108083002/diff/700001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:47: return range_length_ + range_offset_ - offset_; On 2015/06/15 22:17:24, michaeln wrote: > nit: the math in here might be more readable in terms of 'amount_read'? Done. https://codereview.chromium.org/1108083002/diff/700001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:65: const int result = disk_cache_entry_->ReadData( On 2015/06/15 22:17:24, michaeln wrote: > Calling ReadData() with zero bytes_to_read is ok, just checking. I thought so, and I just double checked all three backends, it's fine. There's even unit tests on it.
Quick question about the description: This allows disk cache entries to back blobs, not the other way around, right?
On 2015/06/16 16:16:11, mmenke wrote: > Quick question about the description: This allows disk cache entries to back > blobs, not the other way around, right? Correct; I've updated the description now.
Only looked at the element reader. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.cc (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:24: DCHECK_LE(range_offset_ + range_length, Should we also DCHECK both of these are >= 0? https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:25: disk_cache_entry_->GetDataSize(disk_cache_stream_index_)); nit: include base/logging.h https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:37: const CompletionCallback& callback) { If there's a pending read from entry, it's fine if we just start a new one, right? I assume so, but want to double check. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:48: const int amount_read = offset_ - range_offset_; nit: Basically no code in chrome or net consts stack variables (Except compile-time constants). Goes for this whole file. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:59: const CompletionCallback& callback) { DCHECK(!callback.is_null())? https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:61: std::min(buf_length, static_cast<int>(BytesRemaining())); need to include the header for min (algorithm?) https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:65: weak_factory_.GetWeakPtr(), callback); Suggest making this callback a class member variable instead, for two reasons: 1) It lets us DCHECK it's NULL in Read. 2) It means if it points to anything, we'll be deleting it on the same thread it's passed to us. Know the other ElementReader subclasses don't work that way. If you prefer it as-is, not something I feel that strongly about. You will need to clear the callback in Init. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:65: weak_factory_.GetWeakPtr(), callback); include base/bind.h https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.h (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:21: // ownership of the data. Think the second sentence should be more assertive, and should be above the constructor, as it's a constraint placed on the caller for the method. Also, there's nothing named |entry| here. Just something like "The caller keeps ownership of |disk_cache_entry|, and is responsible for it outlives the UploadDiskCacheEntryElementReader." https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:22: class NET_EXPORT UploadDiskCacheEntryElementReader include net_export https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:28: int range_length); These are fine as ints, rather than int64s? Not sufficiently familiar with the cache to know if there are any potential problems here. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:32: int range_length() const { return range_length_; } +_for_testing / _for_tests https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:35: const UploadDiskCacheEntryElementReader* AsDiskCacheEntryReader() These methods make me sad. :( The other UploadElementReader subclasses have them so extensions can poke at them. Should we let extensions poke at these uploads, too? If not, this should be rename to *ForTests or *ForTesting https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:37: int Init(const CompletionCallback& callback) override; I generally feel like you should forward declare and include stuff (CompletionCallback, uint64_t, IOBuffer), even if they're used in overridden methods, though, admittedly, the other element reader classes don't do this. For CompletionCallback, that's particularly the case, as you're depending on the fact that it isn't forward declared in upload_element_reader.h, so it can be invoked in the cc file without declaring completion_callback.h. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:54: int offset_; Should document these. It's not clear, in particular, what "offset_" and "range_offset_" are relative to. Or could come up with more self explanatory names. I think something like "range_start_, range_end_, read_position_", all being relative to the start of the entry would be simpler, though they don't match the format of the input arguments. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:33: // read operation to allow testing some races without flake. We're using an in-memory cache, which always returns synchronously, too - think it's worth a comment here. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:44: callback.Run(); This seems scary. Can we swap out the pending_read_callbacks_ before calling them? https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:64: TestCompletionCallback cb; Google style guide discourages abbreviations. In my experience, callback is generally written out. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:66: rv = cb.GetResult(rv); The memory cache always returns synchronously, doesn't it? Think it's clearer if we have a comment on that (Could DCHECK on it, too, optionally) https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:104: std::vector<base::Callback<void(void)>> pending_read_callbacks_; include <vector> https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:131: protected: protected not needed in test fixtures https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:149: scoped_refptr<IOBuffer> iobuffer = new WrappedIOBuffer(read_buffer); nit: io_buffer https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:149: scoped_refptr<IOBuffer> iobuffer = new WrappedIOBuffer(read_buffer); include ref_counted https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:153: EXPECT_EQ(0U, reader.BytesRemaining()); We're relying on the underlying cache to handle reads of |kDataSize| in one go. Think that's worth commenting on. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:154: EXPECT_TRUE(std::equal(kData, kData + kDataSize, read_buffer)); Suggest comparing them as strings using EXPECT_EQ, to get better error output on failure. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:160: EXPECT_EQ(static_cast<uint64>(kDataSize), reader.BytesRemaining()); uint64_t (Not going to mark the rest of these) https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:165: const size_t iobuffer1_size = kDataSize / 3; Think these should kConstantNaming https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:168: const size_t iobuffer2_size = kDataSize - iobuffer1_size; Out of paranoia, suggest separate buffers, with no extra space. Otherwise could imagine a lot of weird behaviors that would pass this test, even in debug builds. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:175: EXPECT_EQ(static_cast<uint64>(iobuffer2_size), reader.BytesRemaining()); uint64_t? And include <stdint> https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:220: } Should have a generic async test (Per earlier comment, these all complete synchronously) https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:242: EXPECT_EQ(static_cast<uint64>(kDataSize), reader.BytesRemaining()); Should do a second read here (To a different buffer), and let it complete.
https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:141: resolved_elements.push_back(std::make_pair(&element, nullptr)); I see, you're saying "what if" element here is of TYPE_DISK_CACHE_ENTRY? I think I'd vote to... else (type() != TYPE_DISK_CACHE_ENTRY) resolved_elements/push_back(...) else NOTREACHED(); https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:174: // TODO(gavinp): What can we do if there's no blob_context? On 2015/06/16 16:07:48, gavinp wrote: > On 2015/06/15 22:17:23, michaeln wrote: > > Does that matter? We're making upload readers for blob items? > > This comment is a bit wrong; the real problem is that this occurs when we call > Build() with an element that is TYPE_DISK_CACHE_ENTRY. We actually NEED to be > called with an element of TYPE_BLOB here, so that we call the resolve function, > which is the only way to get the BlobDataItem pointer, which is required to find > the disk_cache::Entry*... > > I don't know enough about the situations in which this code is called; how often > will it be with a DataElement of TYPE_DISK_CACHE_ENTRY vs TYPE_BLOB? > > I'm guessing it's always TYPE_BLOB. I think your comment above confirms this? > I'm clearing up my comment in my next upload, but I'm hoping we can just remove > this worry. Should we be testing for |item| being null? There's a DCHECK on this > stuff in ResolveBlobReference() so I figured it's OK to crash... The real problem is that DataElement (in storage/common/data_element.h) is being used for two different purposes. (a bit of a hack). Primarily its a data structure used for an IPC represention of data provided by blink to be uploaded or assembled into a blob. This is why it's in the /common directory and resource_messages.h defines IPC::ParamTraits<> for it. I don't believe this representation is used to send data from the browser process to a renderer. Secondarily, its used in the internal representation of a blob's data as stored by the browser process, within BlobDataItem in storage/browser/blob. TYPE_DISK_CACHE_ENTRY only makes sense in the context of the second use case. This kind of data element is more or less a wrapper around a disk_cache::Entry* which has no business being in a renderer process. Maybe the best way to 'fix' this would be to use a different struct for the blob systems internal representation... class StoredBlobDataElement : public storage::DataElement { // add members and methods to support TYPE_DISK_CACHE_ENTRY including a // place to put the disk_cache::Entry*. }; Where BlobDataItem keeps a scoped_ptr to one of those. ??? https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.cc (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:48: const int amount_read = offset_ - range_offset_; On 2015/06/16 18:51:18, mmenke wrote: > nit: Basically no code in chrome or net consts stack variables (Except > compile-time constants). Goes for this whole file. i don't think that's true and i think its helpful delcare them as const, maybe name them kSomething to make it even more clear that the value won't be changing in the method body. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.h (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:54: int offset_; On 2015/06/16 18:51:18, mmenke wrote: > Should document these. It's not clear, in particular, what "offset_" and > "range_offset_" are relative to. > > Or could come up with more self explanatory names. I think something like > "range_start_, range_end_, read_position_", all being relative to the start of > the entry would be simpler, though they don't match the format of the input > arguments. +1 comments &| massaging the names, wasn't obvsious on a first readthru https://codereview.chromium.org/1108083002/diff/650002/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/650002/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:76: disk_cache::Entry* disk_cache_entry_; This special case is icky. Its here only because there's no place to put it in the storage::DataElement struct.
https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.cc (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:48: const int amount_read = offset_ - range_offset_; On 2015/06/16 21:52:54, michaeln wrote: > On 2015/06/16 18:51:18, mmenke wrote: > > nit: Basically no code in chrome or net consts stack variables (Except > > compile-time constants). Goes for this whole file. > > i don't think that's true and i think its helpful delcare them as const, maybe > name them kSomething to make it even more clear that the value won't be changing > in the method body. Per style guide, kSomething is only for compile-time constants, which this is not. And also, while I can't speak for chrome in general, I do anywhere from 5 to 20 reviews most weeks, and no one does this. It's not present in pre-existing code, and no one adds it (And not due to me commenting on it). It's just not the prevailing style, which the style guide makes clear one should always follow.
Thanks mmenke and michaeln for taking another pass through this. https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:141: resolved_elements.push_back(std::make_pair(&element, nullptr)); On 2015/06/16 21:52:53, michaeln wrote: > I see, you're saying "what if" element here is of TYPE_DISK_CACHE_ENTRY? I think > I'd vote to... > > else (type() != TYPE_DISK_CACHE_ENTRY) > resolved_elements/push_back(...) > else > NOTREACHED(); Done. https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:174: // TODO(gavinp): What can we do if there's no blob_context? On 2015/06/16 21:52:53, michaeln wrote: > On 2015/06/16 16:07:48, gavinp wrote: > > On 2015/06/15 22:17:23, michaeln wrote: > > > Does that matter? We're making upload readers for blob items? > > > > This comment is a bit wrong; the real problem is that this occurs when we call > > Build() with an element that is TYPE_DISK_CACHE_ENTRY. We actually NEED to be > > called with an element of TYPE_BLOB here, so that we call the resolve > function, > > which is the only way to get the BlobDataItem pointer, which is required to > find > > the disk_cache::Entry*... > > > > I don't know enough about the situations in which this code is called; how > often > > will it be with a DataElement of TYPE_DISK_CACHE_ENTRY vs TYPE_BLOB? > > > > I'm guessing it's always TYPE_BLOB. I think your comment above confirms this? > > I'm clearing up my comment in my next upload, but I'm hoping we can just > remove > > this worry. Should we be testing for |item| being null? There's a DCHECK on > this > > stuff in ResolveBlobReference() so I figured it's OK to crash... > > The real problem is that DataElement (in storage/common/data_element.h) is being > used for two different purposes. (a bit of a hack). > > Primarily its a data structure used for an IPC represention of data provided by > blink to be uploaded or assembled into a blob. This is why it's in the /common > directory and resource_messages.h defines IPC::ParamTraits<> for it. I don't > believe this representation is used to send data from the browser process to a > renderer. This last sentence is what has been keeping me up at night. Thanks for saying the thing that calms me down. Thanks. > > Secondarily, its used in the internal representation of a blob's data as stored > by the browser process, within BlobDataItem in storage/browser/blob. > > TYPE_DISK_CACHE_ENTRY only makes sense in the context of the second use case. > This kind of data element is more or less a wrapper around a disk_cache::Entry* > which has no business being in a renderer process. Maybe the best way to 'fix' > this would be to use a different struct for the blob systems internal > representation... > > class StoredBlobDataElement : public storage::DataElement { > // add members and methods to support TYPE_DISK_CACHE_ENTRY including a > // place to put the disk_cache::Entry*. > }; > > Where BlobDataItem keeps a scoped_ptr to one of those. > > ??? I think we're safe if it's never used to move data from a browser process to a renderer, because we can only ever have an element of TYPE_DISK_CACHE_ENTRY in a browser process, so we'll never hit our IPC guards. Given that, let's avoid adding another layer of indirection here. Does that make sense? https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.cc (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:24: DCHECK_LE(range_offset_ + range_length, On 2015/06/16 18:51:18, mmenke wrote: > Should we also DCHECK both of these are >= 0? Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:25: disk_cache_entry_->GetDataSize(disk_cache_stream_index_)); On 2015/06/16 18:51:18, mmenke wrote: > nit: include base/logging.h Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:37: const CompletionCallback& callback) { On 2015/06/16 18:51:18, mmenke wrote: > If there's a pending read from entry, it's fine if we just start a new one, > right? I assume so, but want to double check. Yes. The behaviour that we will not call the callback on the Read() if the reader has Init() called while the Read is pending is copied from UploadFileElementReader, which behaves this way without documentation. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:48: const int amount_read = offset_ - range_offset_; On 2015/06/16 18:51:18, mmenke wrote: > nit: Basically no code in chrome or net consts stack variables (Except > compile-time constants). Goes for this whole file. Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:48: const int amount_read = offset_ - range_offset_; On 2015/06/16 21:52:54, michaeln wrote: > On 2015/06/16 18:51:18, mmenke wrote: > > nit: Basically no code in chrome or net consts stack variables (Except > > compile-time constants). Goes for this whole file. > > i don't think that's true and i think its helpful delcare them as const, maybe > name them kSomething to make it even more clear that the value won't be changing > in the method body. In this case, it's moot, because this line doesn't exist anymore. I think mmenke's point is more correct with things like "const int bytes_to_read..." on line 60 below? https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:59: const CompletionCallback& callback) { On 2015/06/16 18:51:18, mmenke wrote: > DCHECK(!callback.is_null())? Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:61: std::min(buf_length, static_cast<int>(BytesRemaining())); On 2015/06/16 18:51:18, mmenke wrote: > need to include the header for min (algorithm?) Done. It's algorithm. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:65: weak_factory_.GetWeakPtr(), callback); On 2015/06/16 18:51:18, mmenke wrote: > include base/bind.h Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:65: weak_factory_.GetWeakPtr(), callback); On 2015/06/16 18:51:18, mmenke wrote: > Suggest making this callback a class member variable instead, for two reasons: > > 1) It lets us DCHECK it's NULL in Read. > 2) It means if it points to anything, we'll be deleting it on the same thread > it's passed to us. > > Know the other ElementReader subclasses don't work that way. If you prefer it > as-is, not something I feel that strongly about. You will need to clear the > callback in Init. I'm leaving this as is, absent further reviewer input. (Other reviewers: chime in if you agree with mmenke!) https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.h (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:21: // ownership of the data. On 2015/06/16 18:51:18, mmenke wrote: > Think the second sentence should be more assertive, and should be above the > constructor, as it's a constraint placed on the caller for the method. Also, > there's nothing named |entry| here. > > Just something like "The caller keeps ownership of |disk_cache_entry|, and is > responsible for it outlives the UploadDiskCacheEntryElementReader." Done. This text was copied directly from upload_bytes_element_reader.h, so I updated the text there as well. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:22: class NET_EXPORT UploadDiskCacheEntryElementReader On 2015/06/16 18:51:18, mmenke wrote: > include net_export Done. Also in the file and bytes upload element headers. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:28: int range_length); On 2015/06/16 18:51:18, mmenke wrote: > These are fine as ints, rather than int64s? Not sufficiently familiar with the > cache to know if there are any potential problems here. The disk_cache interface uses ints, and so I chose to expose that here rather than perform type conversions. Why it doesn't use int64, or uint64 as many of the file interfaces do is a historic question. It would be good to change, because all cache implementations end up having a terribly frustrating number of static casts between integer types; carefully choosing our interface to match the underlying interfaces being used would remove these increasing cache code readability and the readability of users, as your comment shows. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:32: int range_length() const { return range_length_; } On 2015/06/16 18:51:19, mmenke wrote: > +_for_testing / _for_tests Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:35: const UploadDiskCacheEntryElementReader* AsDiskCacheEntryReader() On 2015/06/16 18:51:18, mmenke wrote: > These methods make me sad. :( > > The other UploadElementReader subclasses have them so extensions can poke at > them. Should we let extensions poke at these uploads, too? If not, this should > be rename to *ForTests or *ForTesting Done. I don't think we want to let extensions poke at them. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:37: int Init(const CompletionCallback& callback) override; On 2015/06/16 18:51:18, mmenke wrote: > I generally feel like you should forward declare and include stuff > (CompletionCallback, uint64_t, IOBuffer), even if they're used in overridden > methods, though, admittedly, the other element reader classes don't do this. I'm happy to do this; I was of the impression that for overrides this was frowned upon practice. Done. > > For CompletionCallback, that's particularly the case, as you're depending on the > fact that it isn't forward declared in upload_element_reader.h, so it can be > invoked in the cc file without declaring completion_callback.h. I've added the include. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:54: int offset_; On 2015/06/16 18:51:18, mmenke wrote: > Should document these. It's not clear, in particular, what "offset_" and > "range_offset_" are relative to. > > Or could come up with more self explanatory names. I think something like > "range_start_, range_end_, read_position_", all being relative to the start of > the entry would be simpler, though they don't match the format of the input > arguments. I've done this. I left the inline methods used in tests (with their new _for_tests) names following the format of the input arguments. I left the input arguments as they were, for consistency with the other readers. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:33: // read operation to allow testing some races without flake. On 2015/06/16 18:51:19, mmenke wrote: > We're using an in-memory cache, which always returns synchronously, too - think > it's worth a comment here. Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:44: callback.Run(); On 2015/06/16 18:51:19, mmenke wrote: > This seems scary. Can we swap out the pending_read_callbacks_ before calling > them? Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:64: TestCompletionCallback cb; On 2015/06/16 18:51:19, mmenke wrote: > Google style guide discourages abbreviations. In my experience, callback is > generally written out. Done. Except in disk_cache code, which is where I learned the bad convention. Thanks. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:66: rv = cb.GetResult(rv); On 2015/06/16 18:51:19, mmenke wrote: > The memory cache always returns synchronously, doesn't it? Think it's clearer > if we have a comment on that (Could DCHECK on it, too, optionally) Done. Left the TestCompletionCallback in to avoid having a non-NULL callback (per your request in the UploadDiskCacheEntryElementReader). https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:104: std::vector<base::Callback<void(void)>> pending_read_callbacks_; On 2015/06/16 18:51:19, mmenke wrote: > include <vector> Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:131: protected: On 2015/06/16 18:51:19, mmenke wrote: > protected not needed in test fixtures Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:149: scoped_refptr<IOBuffer> iobuffer = new WrappedIOBuffer(read_buffer); On 2015/06/16 18:51:19, mmenke wrote: > include ref_counted Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:149: scoped_refptr<IOBuffer> iobuffer = new WrappedIOBuffer(read_buffer); On 2015/06/16 18:51:19, mmenke wrote: > nit: io_buffer Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:153: EXPECT_EQ(0U, reader.BytesRemaining()); On 2015/06/16 18:51:19, mmenke wrote: > We're relying on the underlying cache to handle reads of |kDataSize| in one go. > Think that's worth commenting on. Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:154: EXPECT_TRUE(std::equal(kData, kData + kDataSize, read_buffer)); On 2015/06/16 18:51:19, mmenke wrote: > Suggest comparing them as strings using EXPECT_EQ, to get better error output on > failure. Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:160: EXPECT_EQ(static_cast<uint64>(kDataSize), reader.BytesRemaining()); On 2015/06/16 18:51:19, mmenke wrote: > uint64_t (Not going to mark the rest of these) Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:165: const size_t iobuffer1_size = kDataSize / 3; On 2015/06/16 18:51:19, mmenke wrote: > Think these should kConstantNaming Done. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:168: const size_t iobuffer2_size = kDataSize - iobuffer1_size; On 2015/06/16 18:51:19, mmenke wrote: > Out of paranoia, suggest separate buffers, with no extra space. Otherwise could > imagine a lot of weird behaviors that would pass this test, even in debug > builds. Done. But I don't know what you meant by "no extra space", I ended up just using full kDataSize buffers, because why not? Was that contrary to your intention? https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:175: EXPECT_EQ(static_cast<uint64>(iobuffer2_size), reader.BytesRemaining()); On 2015/06/16 18:51:19, mmenke wrote: > uint64_t? And include <stdint> Done. Went with <cstdint> and so std::size_t everywhere. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:220: } On 2015/06/16 18:51:19, mmenke wrote: > Should have a generic async test (Per earlier comment, these all complete > synchronously) Done. More problematically, UploadFileElementReader needs a sync test; it has a serious bug that it calls the callback _and_ returns a value when a read on a file completes synchronously. Amazingly, there's a comment in that file documenting the possibility of a file returning synchronously from a read just a few lines above the code that ultimately bugs. https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:242: EXPECT_EQ(static_cast<uint64>(kDataSize), reader.BytesRemaining()); On 2015/06/16 18:51:19, mmenke wrote: > Should do a second read here (To a different buffer), and let it complete. Done. https://codereview.chromium.org/1108083002/diff/780001/storage/browser/blob/v... File storage/browser/blob/view_blob_internals_job.cc (right): https://codereview.chromium.org/1108083002/diff/780001/storage/browser/blob/v... storage/browser/blob/view_blob_internals_job.cc:225: NOTREACHED(); Whoopsies! https://codereview.chromium.org/1108083002/diff/650002/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/650002/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:76: disk_cache::Entry* disk_cache_entry_; On 2015/06/16 21:52:54, michaeln wrote: > This special case is icky. Its here only because there's no place to put it in > the storage::DataElement struct. Yes. In an earlier revision of this CL, the DataHandle had a method disk_cache_entry() which returned NULL by default, but was overridden in the instance for the CacheStorageCache to return non-NULL. This avoided storing two pointers and the scary naked pointer, but it did mean the clean abstraction of DataHandle() was broken with this specific information about one use case. dmurph and I had a VC to talk about that design, and eventually he suggested I proceed as you see with the naked pointer. If you look up in the patch sets you can still see the earlier version of this.
https://codereview.chromium.org/1108083002/diff/650002/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/650002/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:76: disk_cache::Entry* disk_cache_entry_; On 2015/06/16 at 22:28:02, gavinp wrote: > On 2015/06/16 21:52:54, michaeln wrote: > > This special case is icky. Its here only because there's no place to put it in > > the storage::DataElement struct. > > Yes. In an earlier revision of this CL, the DataHandle had a method disk_cache_entry() which returned NULL by default, but was overridden in the instance for the CacheStorageCache to return non-NULL. This avoided storing two pointers and the scary naked pointer, but it did mean the clean abstraction of DataHandle() was broken with this specific information about one use case. > > dmurph and I had a VC to talk about that design, and eventually he suggested I proceed as you see with the naked pointer. If you look up in the patch sets you can still see the earlier version of this. Michael: ideally, we don't really *need* the DataElement struct, it's just handy because that's how the data is transported. Putting the resources in BlobDataItem makes perfect sense in our refcounting resource model now. Any new blob storage backing should be thrown in here, and then lifetime for that resources abstracted away by using a custom DataHandle.
https://codereview.chromium.org/1108083002/diff/650002/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/1108083002/diff/650002/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:76: disk_cache::Entry* disk_cache_entry_; On 2015/06/16 22:38:41, dmurph wrote: > On 2015/06/16 at 22:28:02, gavinp wrote: > > On 2015/06/16 21:52:54, michaeln wrote: > > > This special case is icky. Its here only because there's no place to put it > in > > > the storage::DataElement struct. > > > > Yes. In an earlier revision of this CL, the DataHandle had a method > disk_cache_entry() which returned NULL by default, but was overridden in the > instance for the CacheStorageCache to return non-NULL. This avoided storing two > pointers and the scary naked pointer, but it did mean the clean abstraction of > DataHandle() was broken with this specific information about one use case. > > > > dmurph and I had a VC to talk about that design, and eventually he suggested I > proceed as you see with the naked pointer. If you look up in the patch sets you > can still see the earlier version of this. > > Michael: ideally, we don't really *need* the DataElement struct, it's just handy > because that's how the data is transported. Putting the resources in > BlobDataItem makes perfect sense in our refcounting resource model now. Any new > blob storage backing should be thrown in here, and then lifetime for that > resources abstracted away by using a custom DataHandle. Also worth mentioning: We can't put a member of disk_cache::Entry* type in DataElement at all, because it's defined in a common/ directory. If it doesn't break a presubmit include rule, it should.
(because the nits don't pick themselves) https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:175: EXPECT_EQ(static_cast<uint64>(iobuffer2_size), reader.BytesRemaining()); On 2015/06/16 22:28:02, gavinp wrote: > On 2015/06/16 18:51:19, mmenke wrote: > > uint64_t? And include <stdint> > > Done. Went with <cstdint> and so std::size_t everywhere. Switched back to <stdint.h>, see the discussion in https://chromium-cpp.appspot.com/ . C++11 library features are not allowed, so that includes <cstdint>. As well, <stdint.h> is explicitly permitted. Not to mention some builders don't have <cstdint>.
https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.cc (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:48: const int amount_read = offset_ - range_offset_; On 2015/06/16 22:22:47, mmenke wrote: > On 2015/06/16 21:52:54, michaeln wrote: > > On 2015/06/16 18:51:18, mmenke wrote: > > > nit: Basically no code in chrome or net consts stack variables (Except > > > compile-time constants). Goes for this whole file. > > > > i don't think that's true and i think its helpful delcare them as const, maybe > > name them kSomething to make it even more clear that the value won't be > changing > > in the method body. > > Per style guide, kSomething is only for compile-time constants, which this is > not. > > And also, while I can't speak for chrome in general, I do anywhere from 5 to 20 > reviews most weeks, and no one does this. It's not present in pre-existing > code, and no one adds it (And not due to me commenting on it). It's just not > the prevailing style, which the style guide makes clear one should always > follow. I have gone ahead and removed most const from automatic variables, except those that were compile time constants which have kConstantNamesLikeThis. I am happy to leave it like this. However, I can't resist a heated debate about our coding standard. So, I'm putting my asbestos underwear on and weighing in. I think that as far as the style guide goes, the consistency rule is covered in https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Existing_Non... . I think it's stretching that rule to apply it, as written, to something as big as "chrome" or even something smaller like "net." I interpret "local" here to mean something narrower than all of net; so I was comfortable with the variation from consistency that this unit test had. I interpret mmenke's statement to mean that he disagrees about the scope of the consistency argument. There's also an argument against sprinkling const around stack variables to be found in our rule about using const (note constexpr is banned in chromium, awkwardly since we have the kConstTypeName rule which could be enforced with constexpr, oh well; but on the other hand, it seems we could add a clang rule for this...). See https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Use_of_const , which says to use const "wherever it makes sense", less than helpful in a guide we're referring to when we potentially disagree. Looking deeper, the only thing listed among "Cons" for the use of const is that it is viral and can make it hard to use library functions in particular. This is not a concern with stack variables. Many of the benefits listed in "Pros" do apply; compiler type checking, helps users reading the code by telling them about things that won't happen to particular variables. But then, awkwardly, the "Decision" text appears to describe what "makes sense" means, and lists three cases. Naturally, it fails to discuss stack variables. One other potential point for guidance is the discussion of naming const variables, https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names . The last sentence contains the restriction on kConstNames that mmenke referred to; but perhaps most hopefully for the argument in favour of const stack variables is that it contemplates local scope specifically, and specifically mentions that local variables that are const may have "usual variable naming rules." Read most strictly, that rule says that variables which are compile-time constants may be named with the usual variable naming rules. However, since it contemplates variables that are "const" with the usual variable naming rules, I'd say this text goes a bit further and says that it is consistent with Google coding rules to name "const" variables with the usual variable naming rules, whether they are compile time const or not. So as I read the standard, the consistency rule doesn't win here. However, if you believe in a broader consistency rule than I do, then you might believe otherwise.
lgtm, thanx for making this cl!! https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/700001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:174: // TODO(gavinp): What can we do if there's no blob_context? On 2015/06/16 22:28:01, gavinp wrote: > On 2015/06/16 21:52:53, michaeln wrote: > > On 2015/06/16 16:07:48, gavinp wrote: > > > On 2015/06/15 22:17:23, michaeln wrote: > > > > Does that matter? We're making upload readers for blob items? > > > > > > This comment is a bit wrong; the real problem is that this occurs when we > call > > > Build() with an element that is TYPE_DISK_CACHE_ENTRY. We actually NEED to > be > > > called with an element of TYPE_BLOB here, so that we call the resolve > > function, > > > which is the only way to get the BlobDataItem pointer, which is required to > > find > > > the disk_cache::Entry*... > > > > > > I don't know enough about the situations in which this code is called; how > > often > > > will it be with a DataElement of TYPE_DISK_CACHE_ENTRY vs TYPE_BLOB? > > > > > > I'm guessing it's always TYPE_BLOB. I think your comment above confirms > this? > > > I'm clearing up my comment in my next upload, but I'm hoping we can just > > remove > > > this worry. Should we be testing for |item| being null? There's a DCHECK on > > this > > > stuff in ResolveBlobReference() so I figured it's OK to crash... > > > > The real problem is that DataElement (in storage/common/data_element.h) is > being > > used for two different purposes. (a bit of a hack). > > > > Primarily its a data structure used for an IPC represention of data provided > by > > blink to be uploaded or assembled into a blob. This is why it's in the /common > > directory and resource_messages.h defines IPC::ParamTraits<> for it. I don't > > believe this representation is used to send data from the browser process to a > > renderer. > > This last sentence is what has been keeping me up at night. Thanks for saying > the thing that calms me down. Thanks. > > > > > Secondarily, its used in the internal representation of a blob's data as > stored > > by the browser process, within BlobDataItem in storage/browser/blob. > > > > TYPE_DISK_CACHE_ENTRY only makes sense in the context of the second use case. > > This kind of data element is more or less a wrapper around a > disk_cache::Entry* > > which has no business being in a renderer process. Maybe the best way to 'fix' > > this would be to use a different struct for the blob systems internal > > representation... > > > > class StoredBlobDataElement : public storage::DataElement { > > // add members and methods to support TYPE_DISK_CACHE_ENTRY including a > > // place to put the disk_cache::Entry*. > > }; > > > > Where BlobDataItem keeps a scoped_ptr to one of those. > > > > ??? > > I think we're safe if it's never used to move data from a browser process to a > renderer, because we can only ever have an element of TYPE_DISK_CACHE_ENTRY in a > browser process, so we'll never hit our IPC guards. > > Given that, let's avoid adding another layer of indirection here. Does that make > sense? sgtm, and if/when we ever decide to refactor, thats for another cl https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.cc (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:48: const int amount_read = offset_ - range_offset_; On 2015/06/16 23:08:04, gavinp wrote: > On 2015/06/16 22:22:47, mmenke wrote: > > On 2015/06/16 21:52:54, michaeln wrote: > > > On 2015/06/16 18:51:18, mmenke wrote: > > > > nit: Basically no code in chrome or net consts stack variables (Except > > > > compile-time constants). Goes for this whole file. > > > > > > i don't think that's true and i think its helpful delcare them as const, > maybe > > > name them kSomething to make it even more clear that the value won't be > > changing > > > in the method body. > > > > Per style guide, kSomething is only for compile-time constants, which this is > > not. > > > > And also, while I can't speak for chrome in general, I do anywhere from 5 to > 20 > > reviews most weeks, and no one does this. It's not present in pre-existing > > code, and no one adds it (And not due to me commenting on it). It's just not > > the prevailing style, which the style guide makes clear one should always > > follow. > > I have gone ahead and removed most const from automatic variables, except those > that were compile time constants which have kConstantNamesLikeThis. I am happy > to leave it like this. > > However, I can't resist a heated debate about our coding standard. So, I'm > putting my asbestos underwear on and weighing in. > > I think that as far as the style guide goes, the consistency rule is covered in > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Existing_Non... > . I think it's stretching that rule to apply it, as written, to something as big > as "chrome" or even something smaller like "net." I interpret "local" here to > mean something narrower than all of net; so I was comfortable with the variation > from consistency that this unit test had. I interpret mmenke's statement to mean > that he disagrees about the scope of the consistency argument. > > There's also an argument against sprinkling const around stack variables to be > found in our rule about using const (note constexpr is banned in chromium, > awkwardly since we have the kConstTypeName rule which could be enforced with > constexpr, oh well; but on the other hand, it seems we could add a clang rule > for this...). > > See > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Use_of_const , > which says to use const "wherever it makes sense", less than helpful in a guide > we're referring to when we potentially disagree. Looking deeper, the only thing > listed among "Cons" for the use of const is that it is viral and can make it > hard to use library functions in particular. This is not a concern with stack > variables. Many of the benefits listed in "Pros" do apply; compiler type > checking, helps users reading the code by telling them about things that won't > happen to particular variables. But then, awkwardly, the "Decision" text appears > to describe what "makes sense" means, and lists three cases. Naturally, it fails > to discuss stack variables. > > One other potential point for guidance is the discussion of naming const > variables, > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names > . The last sentence contains the restriction on kConstNames that mmenke > referred to; but perhaps most hopefully for the argument in favour of const > stack variables is that it contemplates local scope specifically, and > specifically mentions that local variables that are const may have "usual > variable naming rules." > > Read most strictly, that rule says that variables which are compile-time > constants may be named with the usual variable naming rules. However, since it > contemplates variables that are "const" with the usual variable naming rules, > I'd say this text goes a bit further and says that it is consistent with Google > coding rules to name "const" variables with the usual variable naming rules, > whether they are compile time const or not. > > So as I read the standard, the consistency rule doesn't win here. However, if > you believe in a broader consistency rule than I do, then you might believe > otherwise. ok, if you say so :) https://codereview.chromium.org/1108083002/diff/850001/content/browser/loader... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/850001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:181: const std::string blob_id2("id-2"); nit: should two adjacent lines be consistent with one another :) kBlobId2 = "id"; disk_cache_data("x");
https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.cc (right): https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.cc:48: const int amount_read = offset_ - range_offset_; On 2015/06/16 23:08:04, gavinp wrote: > On 2015/06/16 22:22:47, mmenke wrote: > > On 2015/06/16 21:52:54, michaeln wrote: > > > On 2015/06/16 18:51:18, mmenke wrote: > > > > nit: Basically no code in chrome or net consts stack variables (Except > > > > compile-time constants). Goes for this whole file. > > > > > > i don't think that's true and i think its helpful delcare them as const, > maybe > > > name them kSomething to make it even more clear that the value won't be > > changing > > > in the method body. > > > > Per style guide, kSomething is only for compile-time constants, which this is > > not. > > > > And also, while I can't speak for chrome in general, I do anywhere from 5 to > 20 > > reviews most weeks, and no one does this. It's not present in pre-existing > > code, and no one adds it (And not due to me commenting on it). It's just not > > the prevailing style, which the style guide makes clear one should always > > follow. > > I have gone ahead and removed most const from automatic variables, except those > that were compile time constants which have kConstantNamesLikeThis. I am happy > to leave it like this. > > However, I can't resist a heated debate about our coding standard. So, I'm > putting my asbestos underwear on and weighing in. > > I think that as far as the style guide goes, the consistency rule is covered in > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Existing_Non... > . I think it's stretching that rule to apply it, as written, to something as big > as "chrome" or even something smaller like "net." I interpret "local" here to > mean something narrower than all of net; so I was comfortable with the variation > from consistency that this unit test had. I interpret mmenke's statement to mean > that he disagrees about the scope of the consistency argument. > > There's also an argument against sprinkling const around stack variables to be > found in our rule about using const (note constexpr is banned in chromium, > awkwardly since we have the kConstTypeName rule which could be enforced with > constexpr, oh well; but on the other hand, it seems we could add a clang rule > for this...). > > See > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Use_of_const , > which says to use const "wherever it makes sense", less than helpful in a guide > we're referring to when we potentially disagree. Looking deeper, the only thing > listed among "Cons" for the use of const is that it is viral and can make it > hard to use library functions in particular. This is not a concern with stack > variables. Many of the benefits listed in "Pros" do apply; compiler type > checking, helps users reading the code by telling them about things that won't > happen to particular variables. But then, awkwardly, the "Decision" text appears > to describe what "makes sense" means, and lists three cases. Naturally, it fails > to discuss stack variables. > > One other potential point for guidance is the discussion of naming const > variables, > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names > . The last sentence contains the restriction on kConstNames that mmenke > referred to; but perhaps most hopefully for the argument in favour of const > stack variables is that it contemplates local scope specifically, and > specifically mentions that local variables that are const may have "usual > variable naming rules." > > Read most strictly, that rule says that variables which are compile-time > constants may be named with the usual variable naming rules. However, since it > contemplates variables that are "const" with the usual variable naming rules, > I'd say this text goes a bit further and says that it is consistent with Google > coding rules to name "const" variables with the usual variable naming rules, > whether they are compile time const or not. > > So as I read the standard, the consistency rule doesn't win here. However, if > you believe in a broader consistency rule than I do, then you might believe > otherwise. Note that if net/ globally follows a convention, then any subset of net locally does as well. I could scope my comment to any of (upload_element_readers, upload code, net/base, net/, chrome/), and it would still be the case that const is not generally used for variables on the stack, even if they aren't modified. My opinion is that peppering just one subset of the code with consts (Namely, the subset that you have written) harms readability, due to adding more visual noise. If we used const uniformly for stack allocated variables that don't change, that wouldn't be a problem, as people would be used to the extra noise. If you want to try and convince the chrome tear or the network team to start using const everywhere, be my guest - it's not something I'd argue against. One person going on a crusade to const all stack variables in just the code he happens to touch, however, I strongly oppose.
On 2015/06/17 01:24:36, mmenke wrote: > https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... > File net/base/upload_disk_cache_entry_element_reader.cc (right): > > https://codereview.chromium.org/1108083002/diff/760001/net/base/upload_disk_c... > net/base/upload_disk_cache_entry_element_reader.cc:48: const int amount_read = > offset_ - range_offset_; > On 2015/06/16 23:08:04, gavinp wrote: > > On 2015/06/16 22:22:47, mmenke wrote: > > > On 2015/06/16 21:52:54, michaeln wrote: > > > > On 2015/06/16 18:51:18, mmenke wrote: > > > > > nit: Basically no code in chrome or net consts stack variables (Except > > > > > compile-time constants). Goes for this whole file. > > > > > > > > i don't think that's true and i think its helpful delcare them as const, > > maybe > > > > name them kSomething to make it even more clear that the value won't be > > > changing > > > > in the method body. > > > > > > Per style guide, kSomething is only for compile-time constants, which this > is > > > not. > > > > > > And also, while I can't speak for chrome in general, I do anywhere from 5 to > > 20 > > > reviews most weeks, and no one does this. It's not present in pre-existing > > > code, and no one adds it (And not due to me commenting on it). It's just > not > > > the prevailing style, which the style guide makes clear one should always > > > follow. > > > > I have gone ahead and removed most const from automatic variables, except > those > > that were compile time constants which have kConstantNamesLikeThis. I am happy > > to leave it like this. > > > > However, I can't resist a heated debate about our coding standard. So, I'm > > putting my asbestos underwear on and weighing in. > > > > I think that as far as the style guide goes, the consistency rule is covered > in > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Existing_Non... > > . I think it's stretching that rule to apply it, as written, to something as > big > > as "chrome" or even something smaller like "net." I interpret "local" here to > > mean something narrower than all of net; so I was comfortable with the > variation > > from consistency that this unit test had. I interpret mmenke's statement to > mean > > that he disagrees about the scope of the consistency argument. > > > > There's also an argument against sprinkling const around stack variables to be > > found in our rule about using const (note constexpr is banned in chromium, > > awkwardly since we have the kConstTypeName rule which could be enforced with > > constexpr, oh well; but on the other hand, it seems we could add a clang rule > > for this...). > > > > See > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Use_of_const > , > > which says to use const "wherever it makes sense", less than helpful in a > guide > > we're referring to when we potentially disagree. Looking deeper, the only > thing > > listed among "Cons" for the use of const is that it is viral and can make it > > hard to use library functions in particular. This is not a concern with stack > > variables. Many of the benefits listed in "Pros" do apply; compiler type > > checking, helps users reading the code by telling them about things that won't > > happen to particular variables. But then, awkwardly, the "Decision" text > appears > > to describe what "makes sense" means, and lists three cases. Naturally, it > fails > > to discuss stack variables. > > > > One other potential point for guidance is the discussion of naming const > > variables, > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names > > . The last sentence contains the restriction on kConstNames that mmenke > > referred to; but perhaps most hopefully for the argument in favour of const > > stack variables is that it contemplates local scope specifically, and > > specifically mentions that local variables that are const may have "usual > > variable naming rules." > > > > Read most strictly, that rule says that variables which are compile-time > > constants may be named with the usual variable naming rules. However, since it > > contemplates variables that are "const" with the usual variable naming rules, > > I'd say this text goes a bit further and says that it is consistent with > Google > > coding rules to name "const" variables with the usual variable naming rules, > > whether they are compile time const or not. > > > > So as I read the standard, the consistency rule doesn't win here. However, if > > you believe in a broader consistency rule than I do, then you might believe > > otherwise. > > Note that if net/ globally follows a convention, then any subset of net locally > does as well. I could scope my comment to any of (upload_element_readers, > upload code, net/base, net/, chrome/), and it would still be the case that const > is not generally used for variables on the stack, even if they aren't modified. > > My opinion is that peppering just one subset of the code with consts (Namely, > the subset that you have written) harms readability, due to adding more visual > noise. If we used const uniformly for stack allocated variables that don't > change, that wouldn't be a problem, as people would be used to the extra noise. > If you want to try and convince the chrome tear or the network team to start > using const everywhere, be my guest - it's not something I'd argue against. One > person going on a crusade to const all stack variables in just the code he > happens to touch, however, I strongly oppose. I very much appreciate this point of view; I am confused by your discussion of "the subset I have written." Have I failed to remove const anywhere in response to your comments? If therre are any, that is an oversight on my part. I'll go over the whole CL for const later tonight and remove any that are extraneous; I had hoped my latest upload did not pepper it in this way, but I am taking your comment as a suggestion I left them in inappropriately. That's a mistake. I'm sorry. If net/ globally follows a standard, then yes, of course any subset of net follows that standard. The question is if the "locally..." component of the coding standard consistency rule applies to something as large as net, which is the question about if it applies to subsets. It's not clear to me what's appropriate here, but you herein advocate, and I think most of our team likewise advocates, that "locally" is to be interpreted very broadly. I hope my code meets the standard you're advocating. The response I wrote was written because I saw two very respected Chrome developers, you and michaeln, disputing something and discussing both the goals of good coding and coding standards. I shared, parenthetically, my interpretation of the rules as I read them here. I am sad if that wasn't constructive; I felt when writing it that it would help us understand the rules we are respecting and how they interact. If I put too much weight on my non-mainstream interpretation, that's inappropriate.
On 2015/06/17 03:49:48, gavinp wrote: > I very much appreciate this point of view; I am confused by your discussion of > "the subset I have written." Have I failed to remove const anywhere in response > to your comments? If therre are any, that is an oversight on my part. I'll go > over the whole CL for const later tonight and remove any that are extraneous; I > had hoped my latest upload did not pepper it in this way, but I am taking your > comment as a suggestion I left them in inappropriately. That's a mistake. I'm > sorry. I'm sorry, I didn't mean to imply you failed to respond to my comments. I was just elaborating on why I think, as things stand, it's a bad idea. And that I think if you feel using const for local variables better follows the style guide, and offers other benefits as well, the way to go about it is to bring up the idea in front of a wider audience. > If net/ globally follows a standard, then yes, of course any subset of net > follows that standard. The question is if the "locally..." component of the > coding standard consistency rule applies to something as large as net, which is > the question about if it applies to subsets. It's not clear to me what's > appropriate here, but you herein advocate, and I think most of our team likewise > advocates, that "locally" is to be interpreted very broadly. I hope my code > meets the standard you're advocating. > > The response I wrote was written because I saw two very respected Chrome > developers, you and michaeln, disputing something and discussing both the goals > of good coding and coding standards. I shared, parenthetically, my > interpretation of the rules as I read them here. I am sad if that wasn't > constructive; I felt when writing it that it would help us understand the rules > we are respecting and how they interact. If I put too much weight on my > non-mainstream interpretation, that's inappropriate. Not at all. You wrote up your take on the matter, I wanted to respond in kind. I'm sorry I came off as accusatory. That was not at all my intention.
I did go through, and found one const to remove, another to switch to kConstantNaming, and another to leave. Most significantly though, I'm really glad I went through the CL, because I had fixed the use of base::Unretained() in the wrong place in BlobUrlRequestJob, the latest upload addresses this. Please take a look everyone, mmenke especially, I think this deals with all comments you've made. https://codereview.chromium.org/1108083002/diff/850001/content/browser/loader... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/850001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:181: const std::string blob_id2("id-2"); On 2015/06/17 00:57:37, michaeln wrote: > nit: should two adjacent lines be consistent with one another :) > > kBlobId2 = "id"; > disk_cache_data("x"); Awkwardly, this usage is cut and paste from line 172 above, so it's already consistent in its inconsistent way.... Awkward. https://codereview.chromium.org/1108083002/diff/870001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/870001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:426: weak_factory_.GetWeakPtr(), chunk_number)); This was a mistake, but I say we leave it in: it certainly looks a lot safer and it's quite low cost on a class that already has weakptrs. It's a belt _and_ suspenders. https://codereview.chromium.org/1108083002/diff/870001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:478: const int result = item.disk_cache_entry()->ReadData( This const stayed in for consistency with line 423, above. https://codereview.chromium.org/1108083002/diff/870001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:481: base::Unretained(this))); OMG, I fixed the wrong base::Unretained()!
I didn't look at the new uploader code, but the rest lgtm
https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:70: class DiskCacheElementReader : public net::UploadDiskCacheEntryElementReader { We have a short comment on the others. Worth commenting on this one, too? https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:88: scoped_refptr<ResourceRequestBody> resource_request_body_; Need to include base/ref_counted.h (Pre-existing problem) https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:132: optional nit: I've seen senior people discourage blank lines at the start of methods. https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:135: const storage::BlobDataItem*>> resolved_elements; Old issue, but should include <vector> https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:177: case ResourceRequestBody::Element::TYPE_DISK_CACHE_ENTRY: { There's a notreached for this case in the first loop, so if we ever have this type of element in the list, we won't even add it to resolved_elements, will we? The test is passing, so I guess I must be missing something? https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:181: const storage::BlobDataItem* item = element_and_blob_item_pair.second; element_and_blob_item_pair.second is never populated, is it? https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:187: } nit: -2 indent for the close brace. https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:47: net::TestCompletionCallback cb; nit: callback https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:61: disk_cache::Entry* tmp_entry = nullptr; optional: Suggest writing out temp https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:62: net::TestCompletionCallback cb; nit: callback https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_bytes_... File net/base/upload_bytes_element_reader.h (right): https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_bytes_... net/base/upload_bytes_element_reader.h:19: // of |data|, and is responsible for ensuring it outlives the While you're here, |data| -> |bytes|? https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.h (right): https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:29: // The new upload reader object will only read those bytes from |range_offset| Should be "... will only read those bytes *starting* from |range_offset| ..." Alternatively, could shorten it to "...will only read |range_length| bytes starting from |range_offset|." https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:31: // |range_offset| and provide |range_length| as the length the entry's stream. nit: "...and prove the length of the entry's stream as |range_length|." https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:65: } optioanl: Suggest most of these be separated by blank lines. https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:73: DCHECK_NE(rv, ERR_IO_PENDING) nit: include base/logging.h https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:187: EXPECT_EQ(0U, reader.BytesRemaining()); Should we have some check that we don't write beyond the number of bytes we request from either buffer? I'm paranoid. Alternatively, could make the buffers the exact size we end up writing to them, and rely on memory bots catching problems (Suggested this last time, but I wasn't very clear about it). https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:296: TEST_F(UploadDiskCacheEntryElementReaderTest, Range) { Should we also test a 0-byte range? Or could DCHECK that the class isn't created with one - I'm fine either way. https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_elemen... File net/base/upload_element_reader.cc (right): https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_elemen... net/base/upload_element_reader.cc:15: return NULL; optional: Suggest updating these to nullptrs, while you're here, for consistency. https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_data_item.cc:26: BlobDataItem::BlobDataItem(scoped_ptr<DataElement> item, What's the conceptual distinction between a BlobDataItem and DataElement? Just wondering on the motivation for putting the disk_cache_stream_index and entry here, rather than in there. Having only one class know that this is really a disk cache entry seems a bit cleaner. https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:474: bool BlobURLRequestJob::ReadDiskCacheEntryItem(const BlobDataItem& item, Are there tests that exercise this code? https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) I'm going to refrain from commenting on this. Mostly. Already done elsewhere in this file...must...not...comment.... https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:498: if (result <= 0) { This is weird: If ReadData returns an error synchronously, we fail with that error. But if it returns an error asynchronously, we always return ERR_FAILED, rather than the actual error. https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:506: if (!read_buf_->BytesRemaining()) { Is this block needed? The ReadLoop call below already does exactly this.
https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) On 2015/06/17 19:11:16, mmenke wrote: > I'm going to refrain from commenting on this. Mostly. Already done elsewhere > in this file...must...not...comment.... This looks like a bug, actually. In the async case, io will be pending, but we'll also be calling this from the ReadLoop call on line 513, so they'll *both* call NotifyReadComplete, if the first read is asynchronous but the next read completes synchronously, right? This is presumably also a bug in the other place that does it.
Adding info about the blob system for mmenke. refactor plans here: https://bit.ly/BlobStorageRefactor https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_data_item.cc:26: BlobDataItem::BlobDataItem(scoped_ptr<DataElement> item, On 2015/06/17 at 19:11:16, mmenke wrote: > What's the conceptual distinction between a BlobDataItem and DataElement? > > Just wondering on the motivation for putting the disk_cache_stream_index and entry here, rather than in there. Having only one class know that this is really a disk cache entry seems a bit cleaner. DataElement is legacy and is used in other parts of the system. Specifically, for IPC serialization, and needs to be copy-able. This is probably going to eventually be removed in favor of storing member variables in BlobDataItem. This system is in the middle of a refactor, this is one of the current carry-overs that hasn't been removed. We can add a "TODO(dmurph): Migrate DataElement fields into BlobDataItem" https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) This class is super old, crufty, and the current state is....not friendly for the reader. It will get love at the last state of the refactor. We can add TODO(dmurph)s at some places. Any insights on the bad parts of this file is appreciated, I've had trouble figuring out what parts are normal for network request jobs and what parts are bad. Other than that, I'm not sure if there is a bug or not, but we haven't noticed anything. We have layout tests that exercise this system as well.
https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) On 2015/06/17 20:18:07, dmurph wrote: > This class is super old, crufty, and the current state is....not friendly for > the reader. It will get love at the last state of the refactor. We can add > TODO(dmurph)s at some places. > > Any insights on the bad parts of this file is appreciated, I've had trouble > figuring out what parts are normal for network request jobs and what parts are > bad. > > Other than that, I'm not sure if there is a bug or not, but we haven't noticed > anything. We have layout tests that exercise this system as well. My guess is that's because reads generally always happen asynchronously, or greedily (Or both?). However, I'm pretty sure it's a bug that we could easily repro in a unit test, with a mocked out disk_cache::Entry. Something else I'm wondering: UploadElementsReaders and this code need similar logic to handle multiple concatenated blobs of different types. Could we make them share some sort of blob reader internally instead, and get rid of both this class and simplify UploadElementsReaders (To each reader only having one type of data - we'd still want to support in-memory and on-disk uploads, of course) Oh, and I'm happy to consult on the refactoring.
On 2015/06/17 at 20:29:25, mmenke wrote: > https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... > File storage/browser/blob/blob_url_request_job.cc (right): > > https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... > storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) > On 2015/06/17 20:18:07, dmurph wrote: > > This class is super old, crufty, and the current state is....not friendly for > > the reader. It will get love at the last state of the refactor. We can add > > TODO(dmurph)s at some places. > > > > Any insights on the bad parts of this file is appreciated, I've had trouble > > figuring out what parts are normal for network request jobs and what parts are > > bad. > > > > Other than that, I'm not sure if there is a bug or not, but we haven't noticed > > anything. We have layout tests that exercise this system as well. > > My guess is that's because reads generally always happen asynchronously, or greedily (Or both?). However, I'm pretty sure it's a bug that we could easily repro in a unit test, with a mocked out disk_cache::Entry. > > Something else I'm wondering: UploadElementsReaders and this code need similar logic to handle multiple concatenated blobs of different types. Could we make them share some sort of blob reader internally instead, and get rid of both this class and simplify UploadElementsReaders (To each reader only having one type of data - we'd still want to support in-memory and on-disk uploads, of course) > > Oh, and I'm happy to consult on the refactoring. Yes, this is the plan. We wanted to create a BlobReader class (that would only work in the browser) that would encompass all blob reading. This could then be used for * the request job * the upload form * tests (verify blob contents) I can re-prioritize and work on this after my current patch set.
On 2015/06/17 20:33:21, dmurph wrote: > On 2015/06/17 at 20:29:25, mmenke wrote: > > > https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... > > File storage/browser/blob/blob_url_request_job.cc (right): > > > > > https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... > > storage/browser/blob/blob_url_request_job.cc:484: if > (GetStatus().is_io_pending()) > > On 2015/06/17 20:18:07, dmurph wrote: > > > This class is super old, crufty, and the current state is....not friendly > for > > > the reader. It will get love at the last state of the refactor. We can add > > > TODO(dmurph)s at some places. > > > > > > Any insights on the bad parts of this file is appreciated, I've had trouble > > > figuring out what parts are normal for network request jobs and what parts > are > > > bad. > > > > > > Other than that, I'm not sure if there is a bug or not, but we haven't > noticed > > > anything. We have layout tests that exercise this system as well. > > > > My guess is that's because reads generally always happen asynchronously, or > greedily (Or both?). However, I'm pretty sure it's a bug that we could easily > repro in a unit test, with a mocked out disk_cache::Entry. > > > > Something else I'm wondering: UploadElementsReaders and this code need > similar logic to handle multiple concatenated blobs of different types. Could > we make them share some sort of blob reader internally instead, and get rid of > both this class and simplify UploadElementsReaders (To each reader only having > one type of data - we'd still want to support in-memory and on-disk uploads, of > course) > > > > Oh, and I'm happy to consult on the refactoring. > > Yes, this is the plan. We wanted to create a BlobReader class (that would only > work in the browser) that would encompass all blob reading. This could then be > used for > * the request job > * the upload form > * tests (verify blob contents) > > I can re-prioritize and work on this after my current patch set. No need to elevate priority on my account, but I would love to see it get done.
https://codereview.chromium.org/1108083002/diff/870001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/870001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:426: weak_factory_.GetWeakPtr(), chunk_number)); On 2015/06/17 15:14:21, gavinp wrote: > This was a mistake, but I say we leave it in: it certainly looks a lot safer and > it's quite low cost on a class that already has weakptrs. It's a belt _and_ > suspenders. yes please, use a weakptr here too https://codereview.chromium.org/1108083002/diff/870001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:481: base::Unretained(this))); On 2015/06/17 15:14:21, gavinp wrote: > OMG, I fixed the wrong base::Unretained()! yikes, thnx for catching this! https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:474: bool BlobURLRequestJob::ReadDiskCacheEntryItem(const BlobDataItem& item, On 2015/06/17 19:11:16, mmenke wrote: > Are there tests that exercise this code? good question, there should be tests for this in blob_url_request_job_unittest.cc https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) On 2015/06/17 19:20:05, mmenke wrote: > On 2015/06/17 19:11:16, mmenke wrote: > > I'm going to refrain from commenting on this. Mostly. Already done elsewhere > > in this file...must...not...comment.... > > This looks like a bug, actually. In the async case, io will be pending, but > we'll also be calling this from the ReadLoop call on line 513, so they'll *both* > call NotifyReadComplete, if the first read is asynchronous but the next read > completes synchronously, right? > > This is presumably also a bug in the other place that does it. Hmmm, in practice this might not be a bug with the filestream reading case because that class always operates asyncly.
https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) On 2015/06/17 20:49:36, michaeln wrote: > On 2015/06/17 19:20:05, mmenke wrote: > > On 2015/06/17 19:11:16, mmenke wrote: > > > I'm going to refrain from commenting on this. Mostly. Already done > elsewhere > > > in this file...must...not...comment.... > > > > This looks like a bug, actually. In the async case, io will be pending, but > > we'll also be calling this from the ReadLoop call on line 513, so they'll > *both* > > call NotifyReadComplete, if the first read is asynchronous but the next read > > completes synchronously, right? > > > > This is presumably also a bug in the other place that does it. > > Hmmm, in practice this might not be a bug with the filestream reading case > because that class always operates asyncly. Unless, of course, you can concatenate blobs of different types, some of which are async, and some sync...
https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) > Something else I'm wondering: UploadElementsReaders and this code need similar > logic to handle multiple concatenated blobs of different types. Could we make > them share some sort of blob reader internally instead, and get rid of both this > class and simplify UploadElementsReaders (To each reader only having one type of > data - we'd still want to support in-memory and on-disk uploads, of course) > > Oh, and I'm happy to consult on the refactoring. That would be ideal and might help eliminate a bug or two that causes urlreqeusts to outlive their contexts, provided internal consumers of bloburlrequests were retrofitted to use the internal blobreader class instead.
Patchset #48 (id:640018) has been deleted
mmenke, I believe this is everything you mentioned, except for unit tests on BlobURLlRequestJob, which I have in progress on another CL. I just wanted to release my comments early to advance the cause! If you only have time for one thing, please take a close look at the BlobURLRequestJob bug discussion; I am worried I didn't understand you since I didn't see the problem you mentioned. While I'm working on the unit test is a great time to create a test case for this bug and fix it (in a separate CL for the file case as appropriate). https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:70: class DiskCacheElementReader : public net::UploadDiskCacheEntryElementReader { On 2015/06/17 19:11:15, mmenke wrote: > We have a short comment on the others. Worth commenting on this one, too? Done. Comment recapitulates the next line of code less than the other ones at the cost of being less verbose. https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:88: scoped_refptr<ResourceRequestBody> resource_request_body_; On 2015/06/17 19:11:15, mmenke wrote: > Need to include base/ref_counted.h (Pre-existing problem) Done. https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:132: On 2015/06/17 19:11:15, mmenke wrote: > optional nit: I've seen senior people discourage blank lines at the start of > methods. Done. I agree with those people discouraging this. https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:135: const storage::BlobDataItem*>> resolved_elements; On 2015/06/17 19:11:15, mmenke wrote: > Old issue, but should include <vector> Done. https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:177: case ResourceRequestBody::Element::TYPE_DISK_CACHE_ENTRY: { On 2015/06/17 19:11:15, mmenke wrote: > There's a notreached for this case in the first loop, so if we ever have this > type of element in the list, we won't even add it to resolved_elements, will we? > > The test is passing, so I guess I must be missing something? Yes. There was a long chat between me and michaeln about this issue in a few earlier revisions of this patch that you can find. The path that I think you might be missing is that if a DataElement passed to UploadDataStreamBuilder::Build() is of TYPE_BLOB, it not directly inserted into |resolved_elements|, instead it's resolved by ResolveBlobReference(). That path will find the DataElement as well as the BlobDataItem. https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:187: } On 2015/06/17 19:11:15, mmenke wrote: > nit: -2 indent for the close brace. Done. https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:47: net::TestCompletionCallback cb; On 2015/06/17 19:11:16, mmenke wrote: > nit: callback Done. https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:61: disk_cache::Entry* tmp_entry = nullptr; On 2015/06/17 19:11:16, mmenke wrote: > optional: Suggest writing out temp Done. I also changed instances in CacheStorageCache. https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:62: net::TestCompletionCallback cb; On 2015/06/17 19:11:16, mmenke wrote: > nit: callback Done. I grepped the diff to make sure this wasn't anywhere else. Sorry for leaving these around. I'd like to upload a CL to fix the disk cache code for this. I'll ask you to review it. https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_bytes_... File net/base/upload_bytes_element_reader.h (right): https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_bytes_... net/base/upload_bytes_element_reader.h:19: // of |data|, and is responsible for ensuring it outlives the On 2015/06/17 19:11:16, mmenke wrote: > While you're here, |data| -> |bytes|? Done. Also edited slightly for prettier justification. (Yes, that is a thing that I do.) https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader.h (right): https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:29: // The new upload reader object will only read those bytes from |range_offset| On 2015/06/17 19:11:16, mmenke wrote: > Should be "... will only read those bytes *starting* from |range_offset| ..." > > Alternatively, could shorten it to "...will only read |range_length| bytes > starting from |range_offset|." Done. Much better phrasing. https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader.h:31: // |range_offset| and provide |range_length| as the length the entry's stream. On 2015/06/17 19:11:16, mmenke wrote: > nit: "...and prove the length of the entry's stream as |range_length|." Done, using "provide" instead of "prove." https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:65: } On 2015/06/17 19:11:16, mmenke wrote: > optioanl: Suggest most of these be separated by blank lines. Done. https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:73: DCHECK_NE(rv, ERR_IO_PENDING) On 2015/06/17 19:11:16, mmenke wrote: > nit: include base/logging.h Done. https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:187: EXPECT_EQ(0U, reader.BytesRemaining()); On 2015/06/17 19:11:16, mmenke wrote: > Should we have some check that we don't write beyond the number of bytes we > request from either buffer? I'm paranoid. > > Alternatively, could make the buffers the exact size we end up writing to them, > and rely on memory bots catching problems (Suggested this last time, but I > wasn't very clear about it). Aha, I now understand that earlier comment. Done, in the memory bot way. https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:296: TEST_F(UploadDiskCacheEntryElementReaderTest, Range) { On 2015/06/17 19:11:16, mmenke wrote: > Should we also test a 0-byte range? Or could DCHECK that the class isn't > created with one - I'm fine either way. I used the DCHECK option. https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_elemen... File net/base/upload_element_reader.cc (right): https://codereview.chromium.org/1108083002/diff/890001/net/base/upload_elemen... net/base/upload_element_reader.cc:15: return NULL; On 2015/06/17 19:11:16, mmenke wrote: > optional: Suggest updating these to nullptrs, while you're here, for > consistency. Done. https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) On 2015/06/17 19:20:05, mmenke wrote: > On 2015/06/17 19:11:16, mmenke wrote: > > I'm going to refrain from commenting on this. Mostly. Already done elsewhere > > in this file...must...not...comment.... > > This looks like a bug, actually. In the async case, io will be pending, but > we'll also be calling this from the ReadLoop call on line 513, so they'll *both* > call NotifyReadComplete, if the first read is asynchronous but the next read > completes synchronously, right? > > This is presumably also a bug in the other place that does it. Can you be more explicit about which function is double called? I just read through the code and I'm not convinced there's a bug here. In this case, we've performed a read on the underlying disk cache, and it's come back synchronously. If we've already performed an async read earlier in servicing the read of the BlobUrlRequestJob and thus already decided to response asynchronously, we toss this in to that pile and concatenate reads as needed by calling DidReadDiskCacheEntry() on this result, which indeed chains another read until we're done. If the underlying read on the disk cache returns asynchronously, we chain through this callback anyway, so these code paths are identical, right? In the case that we got back a synchronous result on the underlying disk cache item, AND we have not already decided we're stuck going synchronous, we continue the synchronous story, advancing our read pointer in the hopes that we won't have a false return from the ReadLoop(). Isn't this good? (if crufty and very likely to be improved by the refactorings you and others have suggested) https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:498: if (result <= 0) { On 2015/06/17 19:11:16, mmenke wrote: > This is weird: If ReadData returns an error synchronously, we fail with that > error. But if it returns an error asynchronously, we always return ERR_FAILED, > rather than the actual error. Fixed here, and in the case for files. https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:506: if (!read_buf_->BytesRemaining()) { On 2015/06/17 19:11:16, mmenke wrote: > Is this block needed? The ReadLoop call below already does exactly this. Done.
https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) On 2015/06/18 18:50:55, gavinp wrote: > On 2015/06/17 19:20:05, mmenke wrote: > > On 2015/06/17 19:11:16, mmenke wrote: > > > I'm going to refrain from commenting on this. Mostly. Already done > elsewhere > > > in this file...must...not...comment.... > > > > This looks like a bug, actually. In the async case, io will be pending, but > > we'll also be calling this from the ReadLoop call on line 513, so they'll > *both* > > call NotifyReadComplete, if the first read is asynchronous but the next read > > completes synchronously, right? > > > > This is presumably also a bug in the other place that does it. > > Can you be more explicit about which function is double called? I just read > through the code and I'm not convinced there's a bug here. > > In this case, we've performed a read on the underlying disk cache, and it's come > back synchronously. If we've already performed an async read earlier in > servicing the read of the BlobUrlRequestJob and thus already decided to response > asynchronously, we toss this in to that pile and concatenate reads as needed by > calling DidReadDiskCacheEntry() on this result, which indeed chains another read > until we're done. > > If the underlying read on the disk cache returns asynchronously, we chain > through this callback anyway, so these code paths are identical, right? > > In the case that we got back a synchronous result on the underlying disk cache > item, AND we have not already decided we're stuck going synchronous, we continue > the synchronous story, advancing our read pointer in the hopes that we won't > have a false return from the ReadLoop(). > > Isn't this good? (if crufty and very likely to be improved by the refactorings > you and others have suggested) Sorry, I missed the SetStatus call in BlobURLRequestJob. So...erm...Is it even possible for the status to be is_io_pending() when this method is invoked? It looks like in both the places this class sets it, it's cleared in the next callback, except in the case of failure, and this shouldn't be called again until after the previous read completes.
I've uploaded BlobURLRequestJob tests; but your question about async/sync operations has helped me dig a bit deeper into this mechanism, I'm going to make sure we're exercising each of the two paths in the File and DiskCacheEntry cases, so expect another upload to come shortly. https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) On 2015/06/18 19:01:55, mmenke wrote: > On 2015/06/18 18:50:55, gavinp wrote: > > On 2015/06/17 19:20:05, mmenke wrote: > > > On 2015/06/17 19:11:16, mmenke wrote: > > > > I'm going to refrain from commenting on this. Mostly. Already done > > elsewhere > > > > in this file...must...not...comment.... > > > > > > This looks like a bug, actually. In the async case, io will be pending, but > > > we'll also be calling this from the ReadLoop call on line 513, so they'll > > *both* > > > call NotifyReadComplete, if the first read is asynchronous but the next read > > > completes synchronously, right? > > > > > > This is presumably also a bug in the other place that does it. > > > > Can you be more explicit about which function is double called? I just read > > through the code and I'm not convinced there's a bug here. > > > > In this case, we've performed a read on the underlying disk cache, and it's > come > > back synchronously. If we've already performed an async read earlier in > > servicing the read of the BlobUrlRequestJob and thus already decided to > response > > asynchronously, we toss this in to that pile and concatenate reads as needed > by > > calling DidReadDiskCacheEntry() on this result, which indeed chains another > read > > until we're done. > > > > If the underlying read on the disk cache returns asynchronously, we chain > > through this callback anyway, so these code paths are identical, right? > > > > In the case that we got back a synchronous result on the underlying disk cache > > item, AND we have not already decided we're stuck going synchronous, we > continue > > the synchronous story, advancing our read pointer in the hopes that we won't > > have a false return from the ReadLoop(). > > > > Isn't this good? (if crufty and very likely to be improved by the refactorings > > you and others have suggested) > > Sorry, I missed the SetStatus call in BlobURLRequestJob. > > So...erm...Is it even possible for the status to be is_io_pending() when this > method is invoked? It looks like in both the places this class sets it, it's > cleared in the next callback, except in the case of failure, and this shouldn't > be called again until after the previous read completes. I think this is possible. Imagine a read made of a BlobURLRequestJob (the "high level read") which spans two underlying disk cache or file entries that are expected to return asynchronously. I think we'd expect: 1. A read is made of the first disk cache resource (the "first read"). This first read to the underlying disk cache entry returns ERR_IO_PENDING; SetStatus means we now are is_io_pending(), and BlobURLRequestJob::ReadRawData() returns false. 2. The first read completes, and BlobURLRequestJob::DidReadFooEntry() is called. The read was successful, and so we tail-call ReadLoop() from our DidRead...Entry(). This immediately issues a read of the second disk cache resource (the "second read"). ***BINGOBINGOBINGO GetStatus().is_io_pending() is expected to be true here BINGOBINGOBINGO** 3. This second read returns ERR_IO_PENDING. Not much happens until... 4. The second read completes, and BloblURLRequestJob::DidReadFooEntry() is called. When ReadLoop() is called, there's no more bytes to read, and so NotifyReadComplete() gets called and the high level read completes per the usual URLRequestJob logic.
https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) On 2015/06/18 19:49:42, gavinp wrote: > On 2015/06/18 19:01:55, mmenke wrote: > > On 2015/06/18 18:50:55, gavinp wrote: > > > On 2015/06/17 19:20:05, mmenke wrote: > > > > On 2015/06/17 19:11:16, mmenke wrote: > > > > > I'm going to refrain from commenting on this. Mostly. Already done > > > elsewhere > > > > > in this file...must...not...comment.... > > > > > > > > This looks like a bug, actually. In the async case, io will be pending, > but > > > > we'll also be calling this from the ReadLoop call on line 513, so they'll > > > *both* > > > > call NotifyReadComplete, if the first read is asynchronous but the next > read > > > > completes synchronously, right? > > > > > > > > This is presumably also a bug in the other place that does it. > > > > > > Can you be more explicit about which function is double called? I just read > > > through the code and I'm not convinced there's a bug here. > > > > > > In this case, we've performed a read on the underlying disk cache, and it's > > come > > > back synchronously. If we've already performed an async read earlier in > > > servicing the read of the BlobUrlRequestJob and thus already decided to > > response > > > asynchronously, we toss this in to that pile and concatenate reads as needed > > by > > > calling DidReadDiskCacheEntry() on this result, which indeed chains another > > read > > > until we're done. > > > > > > If the underlying read on the disk cache returns asynchronously, we chain > > > through this callback anyway, so these code paths are identical, right? > > > > > > In the case that we got back a synchronous result on the underlying disk > cache > > > item, AND we have not already decided we're stuck going synchronous, we > > continue > > > the synchronous story, advancing our read pointer in the hopes that we won't > > > have a false return from the ReadLoop(). > > > > > > Isn't this good? (if crufty and very likely to be improved by the > refactorings > > > you and others have suggested) > > > > Sorry, I missed the SetStatus call in BlobURLRequestJob. > > > > So...erm...Is it even possible for the status to be is_io_pending() when this > > method is invoked? It looks like in both the places this class sets it, it's > > cleared in the next callback, except in the case of failure, and this > shouldn't > > be called again until after the previous read completes. > > I think this is possible. > > Imagine a read made of a BlobURLRequestJob (the "high level read") which spans > two underlying disk cache or file entries that are expected to return > asynchronously. I think we'd expect: > > 1. A read is made of the first disk cache resource (the "first read"). This > first read to the underlying disk cache entry returns ERR_IO_PENDING; SetStatus > means we now are is_io_pending(), and BlobURLRequestJob::ReadRawData() returns > false. > > 2. The first read completes, and BlobURLRequestJob::DidReadFooEntry() is called. > The read was successful, and so we tail-call ReadLoop() from our > DidRead...Entry(). This immediately issues a read of the second disk cache > resource (the "second read"). ***BINGOBINGOBINGO GetStatus().is_io_pending() is > expected to be true here BINGOBINGOBINGO** Why is is_io_pending true there? We set it to ok again in BlobURLRequestJob::DidReadFooEntry. If it were true, we'd actually be in trouble on sync success, which is what I was worried about. IF it were true, we'd call DidReadDiskCacheEntry(result); here, which would call into NotifyReadComplete(), if we had nothing more to do. And then we'd return true. That true would be passed back up to ReadLoop(), which (Again if we had nothing left to read), would return true. And then we'd be back up to the outer call to DidReadDiskCacheEntry, which would *also* call NotifyReadComplete(), which would be bad. Right? > 3. This second read returns ERR_IO_PENDING. Not much happens until... > > 4. The second read completes, and BloblURLRequestJob::DidReadFooEntry() is > called. When ReadLoop() is called, there's no more bytes to read, and so > NotifyReadComplete() gets called and the high level read completes per the usual > URLRequestJob logic.
On 2015/06/18 19:58:23, mmenke wrote: > https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... > File storage/browser/blob/blob_url_request_job.cc (right): > > https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... > storage/browser/blob/blob_url_request_job.cc:484: if > (GetStatus().is_io_pending()) > On 2015/06/18 19:49:42, gavinp wrote: > > On 2015/06/18 19:01:55, mmenke wrote: > > > On 2015/06/18 18:50:55, gavinp wrote: > > > > On 2015/06/17 19:20:05, mmenke wrote: > > > > > On 2015/06/17 19:11:16, mmenke wrote: > > > > > > I'm going to refrain from commenting on this. Mostly. Already done > > > > elsewhere > > > > > > in this file...must...not...comment.... > > > > > > > > > > This looks like a bug, actually. In the async case, io will be pending, > > but > > > > > we'll also be calling this from the ReadLoop call on line 513, so > they'll > > > > *both* > > > > > call NotifyReadComplete, if the first read is asynchronous but the next > > read > > > > > completes synchronously, right? > > > > > > > > > > This is presumably also a bug in the other place that does it. > > > > > > > > Can you be more explicit about which function is double called? I just > read > > > > through the code and I'm not convinced there's a bug here. > > > > > > > > In this case, we've performed a read on the underlying disk cache, and > it's > > > come > > > > back synchronously. If we've already performed an async read earlier in > > > > servicing the read of the BlobUrlRequestJob and thus already decided to > > > response > > > > asynchronously, we toss this in to that pile and concatenate reads as > needed > > > by > > > > calling DidReadDiskCacheEntry() on this result, which indeed chains > another > > > read > > > > until we're done. > > > > > > > > If the underlying read on the disk cache returns asynchronously, we chain > > > > through this callback anyway, so these code paths are identical, right? > > > > > > > > In the case that we got back a synchronous result on the underlying disk > > cache > > > > item, AND we have not already decided we're stuck going synchronous, we > > > continue > > > > the synchronous story, advancing our read pointer in the hopes that we > won't > > > > have a false return from the ReadLoop(). > > > > > > > > Isn't this good? (if crufty and very likely to be improved by the > > refactorings > > > > you and others have suggested) > > > > > > Sorry, I missed the SetStatus call in BlobURLRequestJob. > > > > > > So...erm...Is it even possible for the status to be is_io_pending() when > this > > > method is invoked? It looks like in both the places this class sets it, > it's > > > cleared in the next callback, except in the case of failure, and this > > shouldn't > > > be called again until after the previous read completes. > > > > I think this is possible. > > > > Imagine a read made of a BlobURLRequestJob (the "high level read") which spans > > two underlying disk cache or file entries that are expected to return > > asynchronously. I think we'd expect: > > > > 1. A read is made of the first disk cache resource (the "first read"). This > > first read to the underlying disk cache entry returns ERR_IO_PENDING; > SetStatus > > means we now are is_io_pending(), and BlobURLRequestJob::ReadRawData() returns > > false. > > > > 2. The first read completes, and BlobURLRequestJob::DidReadFooEntry() is > called. > > The read was successful, and so we tail-call ReadLoop() from our > > DidRead...Entry(). This immediately issues a read of the second disk cache > > resource (the "second read"). ***BINGOBINGOBINGO GetStatus().is_io_pending() > is > > expected to be true here BINGOBINGOBINGO** > > Why is is_io_pending true there? We set it to ok again in > BlobURLRequestJob::DidReadFooEntry. > > If it were true, we'd actually be in trouble on sync success, which is what I > was worried about. IF it were true, we'd call DidReadDiskCacheEntry(result); > here, which would call into NotifyReadComplete(), if we had nothing more to do. > And then we'd return true. That true would be passed back up to ReadLoop(), > which (Again if we had nothing left to read), would return true. And then we'd > be back up to the outer call to DidReadDiskCacheEntry, which would *also* call > NotifyReadComplete(), which would be bad. Right? Right. You beat me to pointing out I was wrong; it's absolutely never true on entry, and now I agree with you, I think it can't be. What should we do? > > > 3. This second read returns ERR_IO_PENDING. Not much happens until... > > > > 4. The second read completes, and BloblURLRequestJob::DidReadFooEntry() is > > called. When ReadLoop() is called, there's no more bytes to read, and so > > NotifyReadComplete() gets called and the high level read completes per the > usual > > URLRequestJob logic.
mmenke: my latest upload is my suggestion about what to do. Remove dead code and add DCHECKs for documentation and/or safety. https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... File storage/browser/blob/blob_url_request_job.cc (right): https://codereview.chromium.org/1108083002/diff/890001/storage/browser/blob/b... storage/browser/blob/blob_url_request_job.cc:484: if (GetStatus().is_io_pending()) On 2015/06/18 19:58:22, mmenke wrote: > On 2015/06/18 19:49:42, gavinp wrote: > > On 2015/06/18 19:01:55, mmenke wrote: > > > On 2015/06/18 18:50:55, gavinp wrote: > > > > On 2015/06/17 19:20:05, mmenke wrote: > > > > > On 2015/06/17 19:11:16, mmenke wrote: > > > > > > I'm going to refrain from commenting on this. Mostly. Already done > > > > elsewhere > > > > > > in this file...must...not...comment.... > > > > > > > > > > This looks like a bug, actually. In the async case, io will be pending, > > but > > > > > we'll also be calling this from the ReadLoop call on line 513, so > they'll > > > > *both* > > > > > call NotifyReadComplete, if the first read is asynchronous but the next > > read > > > > > completes synchronously, right? > > > > > > > > > > This is presumably also a bug in the other place that does it. > > > > > > > > Can you be more explicit about which function is double called? I just > read > > > > through the code and I'm not convinced there's a bug here. > > > > > > > > In this case, we've performed a read on the underlying disk cache, and > it's > > > come > > > > back synchronously. If we've already performed an async read earlier in > > > > servicing the read of the BlobUrlRequestJob and thus already decided to > > > response > > > > asynchronously, we toss this in to that pile and concatenate reads as > needed > > > by > > > > calling DidReadDiskCacheEntry() on this result, which indeed chains > another > > > read > > > > until we're done. > > > > > > > > If the underlying read on the disk cache returns asynchronously, we chain > > > > through this callback anyway, so these code paths are identical, right? > > > > > > > > In the case that we got back a synchronous result on the underlying disk > > cache > > > > item, AND we have not already decided we're stuck going synchronous, we > > > continue > > > > the synchronous story, advancing our read pointer in the hopes that we > won't > > > > have a false return from the ReadLoop(). > > > > > > > > Isn't this good? (if crufty and very likely to be improved by the > > refactorings > > > > you and others have suggested) > > > > > > Sorry, I missed the SetStatus call in BlobURLRequestJob. > > > > > > So...erm...Is it even possible for the status to be is_io_pending() when > this > > > method is invoked? It looks like in both the places this class sets it, > it's > > > cleared in the next callback, except in the case of failure, and this > > shouldn't > > > be called again until after the previous read completes. > > > > I think this is possible. > > > > Imagine a read made of a BlobURLRequestJob (the "high level read") which spans > > two underlying disk cache or file entries that are expected to return > > asynchronously. I think we'd expect: > > > > 1. A read is made of the first disk cache resource (the "first read"). This > > first read to the underlying disk cache entry returns ERR_IO_PENDING; > SetStatus > > means we now are is_io_pending(), and BlobURLRequestJob::ReadRawData() returns > > false. > > > > 2. The first read completes, and BlobURLRequestJob::DidReadFooEntry() is > called. > > The read was successful, and so we tail-call ReadLoop() from our > > DidRead...Entry(). This immediately issues a read of the second disk cache > > resource (the "second read"). ***BINGOBINGOBINGO GetStatus().is_io_pending() > is > > expected to be true here BINGOBINGOBINGO** > > Why is is_io_pending true there? We set it to ok again in > BlobURLRequestJob::DidReadFooEntry. > > If it were true, we'd actually be in trouble on sync success, which is what I > was worried about. IF it were true, we'd call DidReadDiskCacheEntry(result); > here, which would call into NotifyReadComplete(), if we had nothing more to do. > And then we'd return true. That true would be passed back up to ReadLoop(), > which (Again if we had nothing left to read), would return true. And then we'd > be back up to the outer call to DidReadDiskCacheEntry, which would *also* call > NotifyReadComplete(), which would be bad. Right? Right. I've removed the dead code, and added a few fairly explicitly documented DCHECKs that guard against this. While I was in DidReadFileEntry, I removed the dead code you identified in my cut and paste. > > > 3. This second read returns ERR_IO_PENDING. Not much happens until... > > > > 4. The second read completes, and BloblURLRequestJob::DidReadFooEntry() is > > called. When ReadLoop() is called, there's no more bytes to read, and so > > NotifyReadComplete() gets called and the high level read completes per the > usual > > URLRequestJob logic. >
LGTM https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... File content/browser/loader/upload_data_stream_builder.cc (right): https://codereview.chromium.org/1108083002/diff/890001/content/browser/loader... content/browser/loader/upload_data_stream_builder.cc:177: case ResourceRequestBody::Element::TYPE_DISK_CACHE_ENTRY: { On 2015/06/18 18:50:54, gavinp wrote: > On 2015/06/17 19:11:15, mmenke wrote: > > There's a notreached for this case in the first loop, so if we ever have this > > type of element in the list, we won't even add it to resolved_elements, will > we? > > > > The test is passing, so I guess I must be missing something? > > Yes. There was a long chat between me and michaeln about this issue in a few > earlier revisions of this patch that you can find. The path that I think you > might be missing is that if a DataElement passed to > UploadDataStreamBuilder::Build() is of TYPE_BLOB, it not directly inserted into > |resolved_elements|, instead it's resolved by ResolveBlobReference(). That path > will find the DataElement as well as the BlobDataItem. Ah...yea. Just unexpected that a blob is both an element type, and a container of sub element types. https://codereview.chromium.org/1108083002/diff/960001/content/browser/fileap... File content/browser/fileapi/blob_url_request_job_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/960001/content/browser/fileap... content/browser/fileapi/blob_url_request_job_unittest.cc:79: false, NULL, NULL, &cache, nit: nullptr (x2)
Thanks everybody! I'm very excited about this CLs potential, and I'm also looking forward to the discussed blob refactors. Please let me know how I can help with the latter. https://codereview.chromium.org/1108083002/diff/960001/content/browser/fileap... File content/browser/fileapi/blob_url_request_job_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/960001/content/browser/fileap... content/browser/fileapi/blob_url_request_job_unittest.cc:79: false, NULL, NULL, &cache, On 2015/06/18 20:49:05, mmenke wrote: > nit: nullptr (x2) I found quite a few more of these. Thanks for the reminder. I for the one wish to be among the first to welcome our new nullptr overlords.
The CQ bit was checked by gavinp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmurph@chromium.org, tsepez@chromium.org, michaeln@chromium.org, jkarlin@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1108083002/#ps980001 (title: "NULL --> nullptr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108083002/980001
Message was sent while issue was closed.
Committed patchset #51 (id:980001)
Message was sent while issue was closed.
Patchset 51 (id:??) landed as https://crrev.com/9668ba5c2096eb4e6f601d9f1eab8e28f84812b7 Cr-Commit-Position: refs/heads/master@{#335185}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1108083002/diff/980001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/980001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:121: return ReadyForSparseIO(callback); This ain't right, as a new warning points out: ..\..\net\base\upload_disk_cache_entry_element_reader_unittest.cc(120,69) : error: all paths through this function will call itself [-Werror,-Winfinite-recursion] int ReadyForSparseIO(const CompletionCallback& callback) override { ^ Can you fix this, please?
Message was sent while issue was closed.
https://codereview.chromium.org/1108083002/diff/980001/net/base/upload_disk_c... File net/base/upload_disk_cache_entry_element_reader_unittest.cc (right): https://codereview.chromium.org/1108083002/diff/980001/net/base/upload_disk_c... net/base/upload_disk_cache_entry_element_reader_unittest.cc:121: return ReadyForSparseIO(callback); On 2015/06/19 18:07:53, Nico wrote: > This ain't right, as a new warning points out: > > ..\..\net\base\upload_disk_cache_entry_element_reader_unittest.cc(120,69) : > error: all paths through this function will call itself > [-Werror,-Winfinite-recursion] > int ReadyForSparseIO(const CompletionCallback& callback) override { > ^ > > Can you fix this, please? Yes, I'll fix it. It should be: return entry_->ReadyForSparseIO(callback); This code is, I'm happy to tell you, test only and in addition, dead. |
