Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2416713002:
Write out CacheStorageCache size to index file. (Closed)
DescriptionWrite out CacheStorageCache size to index file.
Persisting the size allows CacheStorage and CacheStorageManager to avoid
using simple cache to unnecessarily calculate the cache size if it is
already known.
BUG=617963
Committed: https://crrev.com/ffb8a161dcd0ff73a7fff6111a2d6f383c39dec8
Cr-Commit-Position: refs/heads/master@{#440425}
Patch Set 1 #Patch Set 2 : Added out-of-date index tests. #
Total comments: 23
Patch Set 3 : Consolidated observer methods, renames, cleanup, etc. #
Total comments: 28
Patch Set 4 : Moved CacheStorageIndex + many misc fixes. #
Total comments: 46
Patch Set 5 : Scheduling index writes + lots of other fixes. #
Total comments: 8
Patch Set 6 : Only updating cache sizes for non-doomed caches #
Total comments: 40
Patch Set 7 : New tests, misc changes. #
Total comments: 8
Patch Set 8 : Renamed functions & improved comments. #Patch Set 9 : Added parens to DCHECK #Patch Set 10 : Posting index write to the IO thread. #
Total comments: 2
Patch Set 11 : Use callback to wait for index write, fixed mem leak. #Patch Set 12 : Rebased on tip-of-tree to resolve conflict. #Patch Set 13 : Fixed leaks in GetAllOriginsUsageWithOldIndex #
Total comments: 3
Patch Set 14 : BrowserThread::PostDelayedTask(IO, ...) --> base::ThreadTaskRunnerHandle::Get()->Post* #
Total comments: 1
Messages
Total messages: 56 (27 generated)
cmumford@chromium.org changed reviewers: + jkarlin@chromium.org
jkarlin: I need to pull a test or two from crrev.com/2277453004. You're welcome to start the review now, or wait till it's final tomorrow. Your email requested that I create a new index for CacheStorageManager. Initially I did that, and was only writing out the total size (of all stores), but there's not a GetManagerSize type of function. The only one I found was GetAllOriginsUsageGetSizes which returns a vector of CacheStorageUsageInfo's. I don't think you were asking me to persist out all of that data in the index. So let me know if this is along the lines of what you were thinking for this initial CL.
jkarlin: I'm going to implement a timer for CacheStorage::ScheduleWriteIndex(), but I believe it's ready for the first look now.
Great to see this. Here are some initial thoughts. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:949: cache->SetObserver(nullptr); Let's keep the cache in doomed_caches_ instead of carrying it in the callback so that we can delete it when the CacheStorage is deleted. Not the end of the world if the Cache doesn't get to finish updating its size. If you do that, then you don't have to SetObserver(nullptr) since you can guarantee that the cache dies first. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.h (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage.h:49: struct CacheInfo { Suggest CacheMetadata? Seems ever so slightly more specific. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.h (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.h:161: void SetObserver(CacheStorageCacheObserver* observer); This needs documentation, especially to specify that SetObserver(nullptr) must be called if the observer is deleted before the cache. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_observer.h (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_observer.h:16: // An cache was modified, possibly changing it's size. s/An/A/ https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:281: 0), can you add /* cache size */ after the 0 to make it clear what it's for? https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_manager.cc (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager.cc:17: #include "base/files/important_file_writer.h" This is unused. I'm not sure we'd want an ImportantFileWriter here. Overall it's probably better to risk a crash and determine size on next start (after doing the modified time check) than to wait for a flush after every put/delete.
https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:73: class CacheStorage::CacheStorageIndex { Let's put this in its own file with its own unit tests. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:226: virtual void WriteIndex(const CacheStorageIndex* index, Prefer const CacheStorageIndex& index https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1150: void CacheStorage::GetSizeThenCloseAllCachesImpl(const SizeCallback& callback) { I don't think this function should change. It should continue to call the cache's GetSizeThenClose. The reason is that closing the cache in the same operation that it gets the size guarantees that the cache won't grow or shrink after returning the size, allowing you to delete the caches with a more accurate final size. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_observer.h (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_observer.h:17: virtual void CacheModified(const CacheStorageCache* cache, These two functions are subtly different and it's not clear when each would be called and why. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_observer.h:21: virtual void SetCacheSize(const CacheStorageCache* cache, Prefer CacheSizeSet as you're not actually setting the size here.
Sorry for the long delay (on other bugs). PTAL. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:73: class CacheStorage::CacheStorageIndex { On 2016/10/21 19:24:39, jkarlin wrote: > Let's put this in its own file with its own unit tests. Do you still want this to be an inner class of CacheStorage? Protobuf currently generates content::CacheStorageIndex, so we can't collide with that. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:226: virtual void WriteIndex(const CacheStorageIndex* index, On 2016/10/21 19:24:39, jkarlin wrote: > Prefer const CacheStorageIndex& index Done. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:949: cache->SetObserver(nullptr); On 2016/10/21 18:04:19, jkarlin wrote: > Let's keep the cache in doomed_caches_ instead of carrying it in the callback so > that we can delete it when the CacheStorage is deleted. Not the end of the world > if the Cache doesn't get to finish updating its size. > > If you do that, then you don't have to SetObserver(nullptr) since you can > guarantee that the cache dies first. Done. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1150: void CacheStorage::GetSizeThenCloseAllCachesImpl(const SizeCallback& callback) { On 2016/10/21 19:24:39, jkarlin wrote: > I don't think this function should change. It should continue to call the > cache's GetSizeThenClose. The reason is that closing the cache in the same > operation that it gets the size guarantees that the cache won't grow or shrink > after returning the size, allowing you to delete the caches with a more accurate > final size. Done. Not sure why I modified it that way, but all tests pass. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.h (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage.h:49: struct CacheInfo { On 2016/10/21 18:04:19, jkarlin wrote: > Suggest CacheMetadata? Seems ever so slightly more specific. Done. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.h (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.h:161: void SetObserver(CacheStorageCacheObserver* observer); On 2016/10/21 18:04:19, jkarlin wrote: > This needs documentation, especially to specify that SetObserver(nullptr) must > be called if the observer is deleted before the cache. Function deleted. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_observer.h (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_observer.h:16: // An cache was modified, possibly changing it's size. On 2016/10/21 18:04:19, jkarlin wrote: > s/An/A/ Done. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_observer.h:17: virtual void CacheModified(const CacheStorageCache* cache, On 2016/10/21 19:24:39, jkarlin wrote: > These two functions are subtly different and it's not clear when each would be > called and why. Agreed. I decided to merge the two into CacheSizeSet with a SET/DELTA enum - similar to fseek. Hopefully this is more clear. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_observer.h:21: virtual void SetCacheSize(const CacheStorageCache* cache, On 2016/10/21 19:24:39, jkarlin wrote: > Prefer CacheSizeSet as you're not actually setting the size here. Acknowledged. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:281: 0), On 2016/10/21 18:04:19, jkarlin wrote: > can you add /* cache size */ after the 0 to make it clear what it's for? Done. https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_manager.cc (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager.cc:17: #include "base/files/important_file_writer.h" On 2016/10/21 18:04:19, jkarlin wrote: > This is unused. I'm not sure we'd want an ImportantFileWriter here. Overall it's > probably better to risk a crash and determine size on next start (after doing > the modified time check) than to wait for a flush after every put/delete. Acknowledged.
https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:73: class CacheStorage::CacheStorageIndex { On 2016/11/10 17:28:16, cmumford wrote: > On 2016/10/21 19:24:39, jkarlin wrote: > > Let's put this in its own file with its own unit tests. > > Do you still want this to be an inner class of CacheStorage? Protobuf currently > generates content::CacheStorageIndex, so we can't collide with that. The protobuf should be in package (read namespace) content.proto. It's a mistake that it isn't already. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:62: const int64_t CacheStorage::kSizeUnknown; Needs to be assigned something https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:420: content::CacheStorageIndex pb_index; better to write it out, protobuf_index or proto_index https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:549: // with a random name and move them there. Then, rewrite the index file. Need to update this comment. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:556: index_last_modified <= file_info.last_modified) { So we need to guarantee that the backend (SimpleCache) is done making changes before we write the CacheStorage index. Can you write that in a comment somewhere? I do worry about races, but suspect that this approach is sufficient for quota manager. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:560: index_modified = true; Why do we need to write this back to disk? https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:823: DCHECK(cache_map_.empty()); Move this up a line with the other DCHECK https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1018: size_t idx = 0, max = index_->num_entries(); What's this about? index_->num_entries() == index_->ordered_cache_metadata().size() right? DCHECK_EQ perhaps? https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1120: DCHECK(initialized_); This is a duplicate of a DCHECK a few lines up https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1142: void CacheStorage::CloseAllCaches() { Who calls this? https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1144: auto map_iter = cache_map_.find(cache_metadata.name); Why iterate over ordered_cache_metadata and then look it up in cache_map_? Why not just iterate over cache_map_ to begin with? https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1196: barrier_closure.Run(); This can cause trouble. If all of the indexes have known size then running the closure could invoke SizeRetrievedFromAllCaches, which invokes the callback (which could delete this for all we know). https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:269: std::move(quota_manager_proxy), blob_context, 0); nit: can you add /* cache size */ after the 0? https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1396: DCHECK_EQ(cache_size_, cache_size); Hmm, how about DCHECK(cache_size == CacheStorage::kSizeUnknown || cache_size_ == cache_size) so that you don't need to run the outer conditional in release code? https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.h (right): https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.h:162: // or calling SetObserver(nullptr) to stop receiving notification of changes. s/calling/call/
What are your thoughts on CacheStorage::ScheduleWriteIndex()? I was thinking of scheduling a write with a five minute delay to minimize disk writes. Sound reasonable? https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:62: const int64_t CacheStorage::kSizeUnknown; On 2016/11/11 18:24:57, jkarlin wrote: > Needs to be assigned something It's initialized in the header, but because kSizeUnknown is ODR-use it needs to be defined here to avoid a link error. See http://en.cppreference.com/w/cpp/language/definition#ODR-use. Apparently DCHECK_NE takes a reference to the parameter forcing this definition. Let me know if you still want it initialized in the *.cc and I'll move it. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:420: content::CacheStorageIndex pb_index; On 2016/11/11 18:24:56, jkarlin wrote: > better to write it out, protobuf_index or proto_index Done. I went with proto_index. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:549: // with a random name and move them there. Then, rewrite the index file. On 2016/11/11 18:24:56, jkarlin wrote: > Need to update this comment. Done. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:556: index_last_modified <= file_info.last_modified) { On 2016/11/11 18:24:56, jkarlin wrote: > So we need to guarantee that the backend (SimpleCache) is done making changes > before we write the CacheStorage index. Can you write that in a comment > somewhere? > > I do worry about races, but suspect that this approach is sufficient for quota > manager. Comment added to ScheduleWriteIndex. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:560: index_modified = true; On 2016/11/11 18:24:56, jkarlin wrote: > Why do we need to write this back to disk? Good point - clearing the size is sufficient. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:823: DCHECK(cache_map_.empty()); On 2016/11/11 18:24:56, jkarlin wrote: > Move this up a line with the other DCHECK Done. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1018: size_t idx = 0, max = index_->num_entries(); On 2016/11/11 18:24:56, jkarlin wrote: > What's this about? index_->num_entries() == > index_->ordered_cache_metadata().size() right? DCHECK_EQ perhaps? I'm assuming you were asking about using |max| and the early break if idx > max? I was just preserving the initial logic. I wasn't sure of the current implementation is using |max| to avoid calling size(), or if ordered_cache_names_ could grow during this for-loop - which I wouldn't expect, but I wanted to be cautious and stick with the initial logic. I'll remove the idx>max check, and let me know if you want it back (or if I've misunderstood your question). https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1120: DCHECK(initialized_); On 2016/11/11 18:24:56, jkarlin wrote: > This is a duplicate of a DCHECK a few lines up Done. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1142: void CacheStorage::CloseAllCaches() { On 2016/11/11 18:24:56, jkarlin wrote: > Who calls this? Woops - I called this in CacheStorage::GetSizeThenCloseAllCachesImpl, but returned that to it's initial implementation, and forgot to delete this uncalled function. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1196: barrier_closure.Run(); On 2016/11/11 18:24:56, jkarlin wrote: > This can cause trouble. If all of the indexes have known size then running the > closure could invoke SizeRetrievedFromAllCaches, which invokes the callback > (which could delete this for all we know). Agreed, but SizeRetrievedFromAllCaches uses PostTask instead of Run. That should be safe right? Or am I mistaken? https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:269: std::move(quota_manager_proxy), blob_context, 0); On 2016/11/11 18:24:57, jkarlin wrote: > nit: can you add /* cache size */ after the 0? Done. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1396: DCHECK_EQ(cache_size_, cache_size); On 2016/11/11 18:24:57, jkarlin wrote: > Hmm, how about DCHECK(cache_size == CacheStorage::kSizeUnknown || cache_size_ == > cache_size) so that you don't need to run the outer conditional in release code? Done. https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.h (right): https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.h:162: // or calling SetObserver(nullptr) to stop receiving notification of changes. On 2016/11/11 18:24:57, jkarlin wrote: > s/calling/call/ Done.
Close! https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1196: barrier_closure.Run(); On 2016/11/22 17:45:03, cmumford wrote: > On 2016/11/11 18:24:56, jkarlin wrote: > > This can cause trouble. If all of the indexes have known size then running the > > closure could invoke SizeRetrievedFromAllCaches, which invokes the callback > > (which could delete this for all we know). > > Agreed, but SizeRetrievedFromAllCaches uses PostTask instead of Run. That should > be safe right? Or am I mistaken? Ah, I was looking at SizeRetrievedFromCache which doesn't PostTask, but you're right this does. Okay, all good. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:11: #include <unordered_map> Unused https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:477: if (index_modified) { Since only one place marks index_modified as true, we can get rid of the index_modified variable and move this logic to the relevant spot. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:654: // modification-times are used for out-of-date checks. I'd say post a cancelable (with 5 second delay) task to write the index. If another change occurs before the task runs, cancel the current task and start a new one. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:661: switch (whence) { This complexity (deltas vs total size) seems unnecessary. Why not just always send the cache size? It looks like CacheStorageIndex could just loop through its map of caches on demand to get the total size. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:721: index_.swap(index); index_ = std::move(index); https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:819: index_.swap(index_before_delete); index_ = std::move(index_before_delete); https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.h (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.h:10: #include <list> Nothing uses this that I can see. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.h:54: // TODO(cmumford): Rename this type (or use existing). Can we resolve this TODO in this CL? https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.h:133: // Note: Not to be confused with class by same name generated by protobuf. This looks out of place, there is no MemoryLoader protobuf https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.h:246: std::unique_ptr<CacheStorageIndex> index_; nit: prefer cache_index_ for clarity https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:503: void CacheStorageCache::SetObserver(CacheStorageCacheObserver* observer) { DCHECK that cache_observer_ is nullptr? https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1399: cache_size_ == cache_size); Can you also add a LOG() that this happened? https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_observer.h (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_observer.h:21: virtual void CacheSizeSet(const CacheStorageCache* cache, See my earlier comment, but I think this could turn into: virtual void CacheSizeUpdated(const CacheStorageCache* cache, int64_t cache_size); Sorry to suggest another name change but I like CacheSizeUpdated much more, but feel free to ignore the name change if you'd rather keep it. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_index.h (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index.h:16: Class needs a comment. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_index_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index_unittest.cc:6: #include <list> https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index_unittest.cc:57: EXPECT_EQ(1000u, it->size); Also EXPECT_EQ(3u, index2.num_entries()); ASSERT_EQ(3u, index2.ordered_cache_metadata().size()); EXPECT_EQ(1031, index2.GetStorageSize()); https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index_unittest.cc:157: EXPECT_EQ(1000u, it->size); Also: EXPECT_EQ(3u, index2.num_entries()); ASSERT_EQ(3u, index2.ordered_cache_metadata().size()); EXPECT_EQ(1031, index2.GetStorageSize()); https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_manager.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager.cc:301: continue; Woot! https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager_unittest.cc:463: CacheStorageIndex cache_index_; s/cache_index_/callback_cache_index_/ to be consistent with the other callback members. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager_unittest.cc:932: if (MemoryOnly()) Rather than do this check, use TEST_F(CacheStorageManagerTest which only does disk. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager_unittest.cc:1035: int64_t cache_size_v2 = GetSizeThenCloseAllCaches(origin1_); Did you mean to use Size(origin1_) for cache_size_v2 here? I could see either way.
https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:11: #include <unordered_map> On 2016/11/23 16:02:17, jkarlin wrote: > Unused Done. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:477: if (index_modified) { On 2016/11/23 16:02:18, jkarlin wrote: > Since only one place marks index_modified as true, we can get rid of the > index_modified variable and move this logic to the relevant spot. index_modified_ is being set when looping all cache entries - more than one of which may be modified. We only want to write this out once. Or am I missing something? https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:654: // modification-times are used for out-of-date checks. On 2016/11/23 16:02:17, jkarlin wrote: > I'd say post a cancelable (with 5 second delay) task to write the index. If > another change occurs before the task runs, cancel the current task and start a > new one. Done. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:661: switch (whence) { On 2016/11/23 16:02:18, jkarlin wrote: > This complexity (deltas vs total size) seems unnecessary. Why not just always > send the cache size? It looks like CacheStorageIndex could just loop through its > map of caches on demand to get the total size. You're totally right - fixed. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:721: index_.swap(index); On 2016/11/23 16:02:17, jkarlin wrote: > index_ = std::move(index); Done. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:819: index_.swap(index_before_delete); On 2016/11/23 16:02:18, jkarlin wrote: > index_ = std::move(index_before_delete); Done. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.h (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.h:10: #include <list> On 2016/11/23 16:02:18, jkarlin wrote: > Nothing uses this that I can see. Acknowledged. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.h:54: // TODO(cmumford): Rename this type (or use existing). On 2016/11/23 16:02:18, jkarlin wrote: > Can we resolve this TODO in this CL? Sure. I renamed CacheStorageIndexCallback to CacheStorageIndexLoadCallback (used only in *.cc file), and renamed CacheStorage::CacheMetadataCallback to CacheStorage::IndexCallback. Let me know if you prefer a different name. PS: Should EnumerateCaches() be renamed? It doesn't really enumerate the caches (aside from loading them lazily). https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.h:133: // Note: Not to be confused with class by same name generated by protobuf. On 2016/11/23 16:02:18, jkarlin wrote: > This looks out of place, there is no MemoryLoader protobuf Oops - was added when CacheStorageIndex was being forward declared (and it's gone now). https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.h:246: std::unique_ptr<CacheStorageIndex> index_; On 2016/11/23 16:02:18, jkarlin wrote: > nit: prefer cache_index_ for clarity Done. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:503: void CacheStorageCache::SetObserver(CacheStorageCacheObserver* observer) { On 2016/11/23 16:02:18, jkarlin wrote: > DCHECK that cache_observer_ is nullptr? This won't work because SetObserver() is still used to both set and clear the observer. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1399: cache_size_ == cache_size); On 2016/11/23 16:02:18, jkarlin wrote: > Can you also add a LOG() that this happened? Done. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_observer.h (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_observer.h:21: virtual void CacheSizeSet(const CacheStorageCache* cache, On 2016/11/23 16:02:18, jkarlin wrote: > See my earlier comment, but I think this could turn into: > > virtual void CacheSizeUpdated(const CacheStorageCache* cache, int64_t > cache_size); > > Sorry to suggest another name change but I like CacheSizeUpdated much more, but > feel free to ignore the name change if you'd rather keep it. I went with CacheSizeUpdated. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_index.h (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index.h:16: On 2016/11/23 16:02:18, jkarlin wrote: > Class needs a comment. Done. I also added some documentation to README.md https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_index_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index_unittest.cc:6: On 2016/11/23 16:02:18, jkarlin wrote: > #include <list> <list> is included in cache_storage_index.h so git-cl-lint didn't complain. I added it, but think it's redundant. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index_unittest.cc:57: EXPECT_EQ(1000u, it->size); On 2016/11/23 16:02:18, jkarlin wrote: > Also > > EXPECT_EQ(3u, index2.num_entries()); > ASSERT_EQ(3u, index2.ordered_cache_metadata().size()); > EXPECT_EQ(1031, index2.GetStorageSize()); Done - thx. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index_unittest.cc:157: EXPECT_EQ(1000u, it->size); On 2016/11/23 16:02:18, jkarlin wrote: > Also: > EXPECT_EQ(3u, index2.num_entries()); > ASSERT_EQ(3u, index2.ordered_cache_metadata().size()); > EXPECT_EQ(1031, index2.GetStorageSize()); Done. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_manager.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager.cc:301: continue; On 2016/11/23 16:02:18, jkarlin wrote: > Woot! Acknowledged. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager_unittest.cc:463: CacheStorageIndex cache_index_; On 2016/11/23 16:02:18, jkarlin wrote: > s/cache_index_/callback_cache_index_/ to be consistent with the other callback > members. Done. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager_unittest.cc:932: if (MemoryOnly()) On 2016/11/23 16:02:18, jkarlin wrote: > Rather than do this check, use TEST_F(CacheStorageManagerTest which only does > disk. Done. I left these two tests at the current location. Do you want all TEST_F's above the TEST_P's? https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager_unittest.cc:1035: int64_t cache_size_v2 = GetSizeThenCloseAllCaches(origin1_); On 2016/11/23 16:02:18, jkarlin wrote: > Did you mean to use Size(origin1_) for cache_size_v2 here? I could see either > way. Switched to Size() as suggested as closing is irrelevant to this test (and done in two lines).
https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:477: if (index_modified) { On 2016/11/29 18:10:20, cmumford wrote: > On 2016/11/23 16:02:18, jkarlin wrote: > > Since only one place marks index_modified as true, we can get rid of the > > index_modified variable and move this logic to the relevant spot. > > index_modified_ is being set when looping all cache entries - more than one of > which may be modified. We only want to write this out once. > > Or am I missing something? Quite right. Never mind me :) https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.h (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage.h:54: // TODO(cmumford): Rename this type (or use existing). On 2016/11/29 18:10:20, cmumford wrote: > On 2016/11/23 16:02:18, jkarlin wrote: > > Can we resolve this TODO in this CL? > > Sure. I renamed CacheStorageIndexCallback to CacheStorageIndexLoadCallback (used > only in *.cc file), and renamed CacheStorage::CacheMetadataCallback to > CacheStorage::IndexCallback. Let me know if you prefer a different name. > > PS: Should EnumerateCaches() be renamed? It doesn't really enumerate the caches > (aside from loading them lazily). Perhaps in the future, but let's leave it for now. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:503: void CacheStorageCache::SetObserver(CacheStorageCacheObserver* observer) { On 2016/11/29 18:10:20, cmumford wrote: > On 2016/11/23 16:02:18, jkarlin wrote: > > DCHECK that cache_observer_ is nullptr? > > This won't work because SetObserver() is still used to both set and clear the > observer. Acknowledged. https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager_unittest.cc:932: if (MemoryOnly()) On 2016/11/29 18:10:21, cmumford wrote: > On 2016/11/23 16:02:18, jkarlin wrote: > > Rather than do this check, use TEST_F(CacheStorageManagerTest which only does > > disk. > > Done. I left these two tests at the current location. Do you want all TEST_F's > above the TEST_P's? They're already mixed up, so I wouldn't worry about that in this CL. https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:662: // TODO(cmumford): Write the index in between other cache writes as This TODO should be done now, otherwise we could write out the index in the midst of a CacheStorage operation. That may or may not cause problems, but it's much safer to schedule it. The easy way to solve this is to make WriteIndex schedule an operation to do the actual work in WriteIndexImpl() https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_index.cc (right): https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index.cc:59: // Deleted (but still referenced) caches can still run. Hmm, this means that we might be getting a size update for a cache that's been deleted. But there is nothing stopping a new cache with the same name from being created. Which means that the doomed cache can update the size of the new cache! That's a bug. https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index.cc:70: void CacheStorageIndex::SetCacheSizeModified(const std::string& cache_name, Is this still called? https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager_unittest.cc:208: // Write is done on I/O thread, so wait for completion. The write is done on the task runner passed to CacheStorage. Perhaps just rewrite as "Wait for write to complete."
https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:662: // TODO(cmumford): Write the index in between other cache writes as On 2016/12/01 17:43:31, jkarlin wrote: > This TODO should be done now, otherwise we could write out the index in the > midst of a CacheStorage operation. That may or may not cause problems, but it's > much safer to schedule it. > > The easy way to solve this is to make WriteIndex schedule an operation to do the > actual work in WriteIndexImpl() Done. https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_index.cc (right): https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index.cc:59: // Deleted (but still referenced) caches can still run. On 2016/12/01 17:43:31, jkarlin wrote: > Hmm, this means that we might be getting a size update for a cache that's been > deleted. But there is nothing stopping a new cache with the same name from being > created. Which means that the doomed cache can update the size of the new cache! > That's a bug. Agreed. I added a check in CacheStorage::CacheSizeUpdated(), and only call this if the cache is not doomed. https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index.cc:70: void CacheStorageIndex::SetCacheSizeModified(const std::string& cache_name, On 2016/12/01 17:43:31, jkarlin wrote: > Is this still called? Only used by a lingering test - deleted it (and the test code). https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager_unittest.cc:208: // Write is done on I/O thread, so wait for completion. On 2016/12/01 17:43:31, jkarlin wrote: > The write is done on the task runner passed to CacheStorage. Perhaps just > rewrite as "Wait for write to complete." Done.
Partial review.. will get to more on Monday. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:438: // Additionally invalidate any index entries where the cache was modified We're just invalidating the size of the index entry right? Not the whole index entry. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:439: // after the index (making it out-of-date) Needs a period. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:507: cache_index_(base::MakeUnique<CacheStorageIndex>()), Why create this in the constructor? It's a bug if something tries to use cache_index_ before its been loaded in init. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:652: static const int64_t kWriteIndexDelay = 5; kWriteIndexDelaySecs https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:668: cache_loader_->WriteIndex(*cache_index_, base::Bind(&DoNothingWithBool)); The callback needs to call RunNext on the scheduler else the scheduler won't advance until the next Schedule operation is called. You could use: scheduler_->WrapCallbackToRunNext(base::Bind(&DoNothingWithBool)) https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:861: cache->SetObserver(nullptr); You could call this as soon as a cache is doomed, then you won't get those problematic observer updates. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage.h (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.h:265: bool index_write_pending_; This bool shouldn't be necessary, can't you infer this from index_write_task_->IsCancelled()?
Here is the rest, sorry for the delay. I was on triage duty. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:267: CacheStorageCache* cache = new CacheStorageCache( As part of the transition to MakeUnique, could you change this to auto CacheStorageCache = base::MakeUnique<CacheStorageCache>(...)? https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:285: CacheStorageCache* cache = new CacheStorageCache( Same. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:503: void CacheStorageCache::SetObserver(CacheStorageCacheObserver* observer) { Perhaps: DCHECK(observer ^ cache_observer_)? https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1398: LOG(ERROR) << "Cache size/index mismatch"; Can you add a TODO to add UMA for this as well? https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_index.h (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:11: #include "content/browser/cache_storage/cache_storage.h" new line above this line https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:45: int64_t GetCacheSize(const std::string& cache_name) const; Add comment of what it will return if the size is unknown. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:53: int64_t GetStorageSize(); This needs a comment. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:65: int64_t storage_size_ = CacheStorage::kSizeUnknown; Can you add a TODO to make this class DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_index_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index_unittest.cc:28: void TearDown() override {} DISALLOW_COPY_AND_ASSIGN(CacheStorageIndexTest) https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:98: const bool is_incognito = MemoryOnly(); why this variable? https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:174: ->GetURLRequestContext(); Huh, it seems strange that 'git cl format' would do this. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:179: const bool is_incognito = MemoryOnly(); Ah, that's why. Please remove "is_incognito" here as well. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:938: TEST_F(CacheStorageManagerTest, GetAllOriginsUsageWithOldIndex) { Add a simple test to show the expected case, e.g., add a resource, get its size, close the manager, reopen the manager, and the size is correct.
https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:438: // Additionally invalidate any index entries where the cache was modified On 2016/12/09 19:49:19, jkarlin wrote: > We're just invalidating the size of the index entry right? Not the whole index > entry. Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:439: // after the index (making it out-of-date) On 2016/12/09 19:49:19, jkarlin wrote: > Needs a period. Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:507: cache_index_(base::MakeUnique<CacheStorageIndex>()), On 2016/12/09 19:49:19, jkarlin wrote: > Why create this in the constructor? It's a bug if something tries to use > cache_index_ before its been loaded in init. Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:652: static const int64_t kWriteIndexDelay = 5; On 2016/12/09 19:49:19, jkarlin wrote: > kWriteIndexDelaySecs Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:668: cache_loader_->WriteIndex(*cache_index_, base::Bind(&DoNothingWithBool)); On 2016/12/09 19:49:19, jkarlin wrote: > The callback needs to call RunNext on the scheduler else the scheduler won't > advance until the next Schedule operation is called. You could use: > > scheduler_->WrapCallbackToRunNext(base::Bind(&DoNothingWithBool)) Done. I also tweaked the method names slightly since WriteIndex didn't actually do the write - it only scheduled one. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:861: cache->SetObserver(nullptr); On 2016/12/09 19:49:19, jkarlin wrote: > You could call this as soon as a cache is doomed, then you won't get those > problematic observer updates. Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage.h (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.h:265: bool index_write_pending_; On 2016/12/09 19:49:19, jkarlin wrote: > This bool shouldn't be necessary, can't you infer this from > index_write_task_->IsCancelled()? Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:267: CacheStorageCache* cache = new CacheStorageCache( On 2016/12/14 14:19:55, jkarlin wrote: > As part of the transition to MakeUnique, could you change this to auto > CacheStorageCache = base::MakeUnique<CacheStorageCache>(...)? The CacheStorageCache constructor is provide, so this won't work. If you still prefer MakeUnique<>() let me know. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:285: CacheStorageCache* cache = new CacheStorageCache( On 2016/12/14 14:19:55, jkarlin wrote: > Same. see above. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:503: void CacheStorageCache::SetObserver(CacheStorageCacheObserver* observer) { On 2016/12/14 14:19:56, jkarlin wrote: > Perhaps: > DCHECK(observer ^ cache_observer_)? Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1398: LOG(ERROR) << "Cache size/index mismatch"; On 2016/12/14 14:19:55, jkarlin wrote: > Can you add a TODO to add UMA for this as well? Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_index.h (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:11: #include "content/browser/cache_storage/cache_storage.h" On 2016/12/14 14:19:56, jkarlin wrote: > new line above this line Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:45: int64_t GetCacheSize(const std::string& cache_name) const; On 2016/12/14 14:19:56, jkarlin wrote: > Add comment of what it will return if the size is unknown. Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:53: int64_t GetStorageSize(); On 2016/12/14 14:19:56, jkarlin wrote: > This needs a comment. Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:65: int64_t storage_size_ = CacheStorage::kSizeUnknown; On 2016/12/14 14:19:56, jkarlin wrote: > Can you add a TODO to make this class DISALLOW_COPY_AND_ASSIGN? Implemented, See DoomedCache, FinalizeDoomedCache, and ResetDoomedCache. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_index_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index_unittest.cc:28: void TearDown() override {} On 2016/12/14 14:19:56, jkarlin wrote: > DISALLOW_COPY_AND_ASSIGN(CacheStorageIndexTest) Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:98: const bool is_incognito = MemoryOnly(); On 2016/12/14 14:19:56, jkarlin wrote: > why this variable? It was in origin/master (and refactored). Removed it. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:174: ->GetURLRequestContext(); On 2016/12/14 14:19:56, jkarlin wrote: > Huh, it seems strange that 'git cl format' would do this. Agreed - I manually reverted prior version. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:179: const bool is_incognito = MemoryOnly(); On 2016/12/14 14:19:56, jkarlin wrote: > Ah, that's why. Please remove "is_incognito" here as well. Done. https://codereview.chromium.org/2416713002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:938: TEST_F(CacheStorageManagerTest, GetAllOriginsUsageWithOldIndex) { On 2016/12/14 14:19:56, jkarlin wrote: > Add a simple test to show the expected case, e.g., add a resource, get its size, > close the manager, reopen the manager, and the size is correct. Done - added CacheSizeCorrectAfterReopen.
lgtm with a few comments https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... File content/browser/cache_storage/cache_storage.h (right): https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... content/browser/cache_storage/cache_storage.h:219: void WriteIndex(const base::Callback<void(bool)>& callback); The naming convention for this directory has been for the operation to have one name, and the method that actually runs it to be Impl. So in this case: ScheduleWriteIndex -- posts the 5 second callback WriteIndex -- schedules the operation WriteIndexImpl -- does the actual work https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... File content/browser/cache_storage/cache_storage_index.h (right): https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:58: // Mark the cache as doomed. This will remove the cache metadata from the This needs a bit more documentation. For instance, once this is called you need to either finalize or restore it before making any other modifications. https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:59: // index. To completely delete the metadata via FinalizeDoomedCache, and use s/via/call/ https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:64: void ResetDoomedCache(); RestoreDoomedCache? RevertDoomedCache?
https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... File content/browser/cache_storage/cache_storage.h (right): https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... content/browser/cache_storage/cache_storage.h:219: void WriteIndex(const base::Callback<void(bool)>& callback); On 2016/12/19 17:48:15, jkarlin wrote: > The naming convention for this directory has been for the operation to have one > name, and the method that actually runs it to be Impl. > > So in this case: > > ScheduleWriteIndex -- posts the 5 second callback > WriteIndex -- schedules the operation > WriteIndexImpl -- does the actual work Done. https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... File content/browser/cache_storage/cache_storage_index.h (right): https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:58: // Mark the cache as doomed. This will remove the cache metadata from the On 2016/12/19 17:48:15, jkarlin wrote: > This needs a bit more documentation. For instance, once this is called you need > to either finalize or restore it before making any other modifications. Done. https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:59: // index. To completely delete the metadata via FinalizeDoomedCache, and use On 2016/12/19 17:48:15, jkarlin wrote: > s/via/call/ Done. https://codereview.chromium.org/2416713002/diff/120001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:64: void ResetDoomedCache(); On 2016/12/19 17:48:15, jkarlin wrote: > RestoreDoomedCache? RevertDoomedCache? I went with RestoreDoomedCache.
The CQ bit was checked by cmumford@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2416713002/#ps140001 (title: "Renamed functions & improved comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
cmumford@chromium.org changed reviewers: + clamy@chromium.org
clamy@chromium.org: Please review changes in content/browser/BUILD.gn
The CQ bit was checked by cmumford@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
BUIL.gn changes lgtm.
The CQ bit was checked by cmumford@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
jkarlin@ I made one fix where ScheduleWriteIndex was running WriteIndex on the task runner instead of the IO thread. Still trying to figure out the memory leak.
The CQ bit was checked by cmumford@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by cmumford@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jkarlin@ This CL is now fixed. I made three changes you may want to give one more look before I land it. 1) I modified CacheStorage::InitiateScheduledIndexWriteForTest to receive a callback. This is for the CacheStorageManager unit tests to avoid relying on the assumption that when RunUntilIdle() returns that the write is finished. 2) Added NULL check in CacheStorageManagerTest::DestroyStorageManager. 3) deleting CacheStorageCacheHandle in CacheStorageManagerTest::GetAllOriginsUsageWithOldIndex and GetOriginSizeWithOldIndex.
https://codereview.chromium.org/2416713002/diff/180001/content/browser/cache_... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/180001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:654: BrowserThread::PostDelayedTask( nit: How about base::ThreadTaskRunnerHandle::Get()->PostDelayedTask? https://codereview.chromium.org/2416713002/diff/240001/content/browser/cache_... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/240001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1020: original_handle = nullptr; Are you saying that this change fixed something? Aren't release() and = nullptr equivalent?
https://codereview.chromium.org/2416713002/diff/180001/content/browser/cache_... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2416713002/diff/180001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:654: BrowserThread::PostDelayedTask( On 2016/12/22 13:17:59, jkarlin wrote: > nit: How about base::ThreadTaskRunnerHandle::Get()->PostDelayedTask? > Done. https://codereview.chromium.org/2416713002/diff/240001/content/browser/cache_... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/240001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1020: original_handle = nullptr; On 2016/12/22 13:17:59, jkarlin wrote: > Are you saying that this change fixed something? Aren't release() and = nullptr > equivalent? No, reset() is equivalent, but release just makes unique_ptr "forget" about the owned pointer. So you could do: void* p = ptr.release(); // release ownership of the object and transfer to p. delete p;
slgtm! https://codereview.chromium.org/2416713002/diff/240001/content/browser/cache_... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2416713002/diff/240001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1020: original_handle = nullptr; On 2016/12/22 14:22:11, cmumford wrote: > On 2016/12/22 13:17:59, jkarlin wrote: > > Are you saying that this change fixed something? Aren't release() and = > nullptr > > equivalent? > > No, reset() is equivalent, but release just makes unique_ptr "forget" about the > owned pointer. So you could do: > > void* p = ptr.release(); // release ownership of the object and transfer to p. > delete p; Ah, right. I was thinking about reset().
The CQ bit was checked by cmumford@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org, clamy@chromium.org Link to the patchset: https://codereview.chromium.org/2416713002/#ps260001 (title: "BrowserThread::PostDelayedTask(IO, ...) --> base::ThreadTaskRunnerHandle::Get()->Post*")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1482417030328780,
"parent_rev": "d70106f23466d34711fc5d3c3d2c902eede0fb62", "commit_rev":
"9982464e533a4670dd74b8e5a792f74096127b58"}
Message was sent while issue was closed.
Description was changed from ========== Write out CacheStorageCache size to index file. Persisting the size allows CacheStorage and CacheStorageManager to avoid using simple cache to unnecessarily calculate the cache size if it is already known. BUG=617963 ========== to ========== Write out CacheStorageCache size to index file. Persisting the size allows CacheStorage and CacheStorageManager to avoid using simple cache to unnecessarily calculate the cache size if it is already known. BUG=617963 Review-Url: https://codereview.chromium.org/2416713002 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Write out CacheStorageCache size to index file. Persisting the size allows CacheStorage and CacheStorageManager to avoid using simple cache to unnecessarily calculate the cache size if it is already known. BUG=617963 Review-Url: https://codereview.chromium.org/2416713002 ========== to ========== Write out CacheStorageCache size to index file. Persisting the size allows CacheStorage and CacheStorageManager to avoid using simple cache to unnecessarily calculate the cache size if it is already known. BUG=617963 Committed: https://crrev.com/ffb8a161dcd0ff73a7fff6111a2d6f383c39dec8 Cr-Commit-Position: refs/heads/master@{#440425} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/ffb8a161dcd0ff73a7fff6111a2d6f383c39dec8 Cr-Commit-Position: refs/heads/master@{#440425}
Message was sent while issue was closed.
bcwhite@chromium.org changed reviewers: + bcwhite@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2416713002/diff/260001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2416713002/diff/260001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1395: DCHECK_EQ(cache_size_, cache_size); Would you remove this, please? It causes crash-looping on debug builds after an unclean shutdown. |
