Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10832241:
Drive: Removes unused cache files after the initial feed fetch. (Closed)
|
Created:
8 years, 4 months ago by yoshiki Modified:
8 years, 3 months ago CC:
chromium-reviews, achuith+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDrive: Removes unused cache files after the initial feed fetch.
BUG=128088
TEST=manual
TBR=ben@chromium.org
# for chrome/chrome_browser.gypi and chrome/chrome_tests.gypi
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152328
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154430
Patch Set 1 #Patch Set 2 : Review fix (#2) #
Total comments: 6
Patch Set 3 : Review fix (#4) #Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : Adding scoped_ptr<StaleCacheFilesRemover> to GDataFileSystem #
Total comments: 46
Patch Set 7 : Review (#7) fix & add unit_test. #
Total comments: 15
Patch Set 8 : Review fix (#10) #
Total comments: 2
Patch Set 9 : Review fix (#12) #Patch Set 10 : Review fix (#15) #Patch Set 11 : rebase #Patch Set 12 : rebase #Patch Set 13 : Pretend we have enough space on creating a cache. #Patch Set 14 : rebase #Patch Set 15 : Fix the memory leak. #
Total comments: 2
Patch Set 16 : rebase #Patch Set 17 : Move the ownership of |stale_cache_files_remover_| to GDataSystemService (reverting Patch Set 15) #
Total comments: 14
Patch Set 18 : rebase #Patch Set 19 : Review Fix (#26) #Patch Set 20 : Fix the build error. #
Total comments: 6
Patch Set 21 : Review fix (#28) #Patch Set 22 : rebase #
Total comments: 1
Messages
Total messages: 35 (0 generated)
Thank you for taking this class, but I think this approach is rather overkill. DriveCacheScrubService, owned by GDataSystemService, seems to be unnecessary, as what we do is one shot (just check if there are stale files upon OnInitialLoadFinished)). I think the following would be simpler: 1) create a class like StaleCacheFilesRemover. 2) in RunAndNotifyInitialLoadFinished(), create a StaleCacheFilesRemover, and do the job 3) upon completion, StaleCacheFilesRemover removes itself.
Done. PTAL On 2012/08/10 17:14:46, satorux1 wrote: > Thank you for taking this class, but I think this approach is rather overkill. > DriveCacheScrubService, owned by GDataSystemService, seems to be unnecessary, as > what we do is one shot (just check if there are stale files upon > OnInitialLoadFinished)). > > I think the following would be simpler: > > 1) create a class like StaleCacheFilesRemover. > 2) in RunAndNotifyInitialLoadFinished(), create a StaleCacheFilesRemover, and do > the job > 3) upon completion, StaleCacheFilesRemover removes itself.
http://codereview.chromium.org/10832241/diff/8001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10832241/diff/8001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2823: remover->ScrubCacheAfterDelay(); I think "scrub" is not a popular verb in code. Let's just call it Start(); http://codereview.chromium.org/10832241/diff/8001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/stale_cache_files_remover.cc (right): http://codereview.chromium.org/10832241/diff/8001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:21: const int kDriveScrubDelayAfterFirstFeedFetch = 10; maybe just kDelaySeconds = 10; http://codereview.chromium.org/10832241/diff/8001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:23: void RemoveCacheIfNecessary( function comment is missing. please fix other places too. http://codereview.chromium.org/10832241/diff/8001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:92: void StaleCacheFilesRemover::ScrubCache() { Because it's delayed, it's not guaranteed that cache_ and file_system_ are valid by the time this function is called (i.e. they may be already deleted). I think we should change the strategy here... The easiest way to solve this problem is to implement this feature inside of GDataFileSystem, but we don't want to add more code there. let me think a bit about it. http://codereview.chromium.org/10832241/diff/8001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/stale_cache_files_remover.h (right): http://codereview.chromium.org/10832241/diff/8001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:19: class StaleCacheFilesRemover { class comment is missing. http://codereview.chromium.org/10832241/diff/8001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:25: void ScrubCacheAfterDelay(); function comment is missing. Please fix other places too.
On 2012/08/10 23:47:40, satorux1 wrote: http://codereview.chromium.org/10832241/diff/8001/chrome/browser/chromeos/gda... > chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:92: void > StaleCacheFilesRemover::ScrubCache() { > Because it's delayed, it's not guaranteed that cache_ and file_system_ are valid > by the time this function is called (i.e. they may be already deleted). > > I think we should change the strategy here... The easiest way to solve this > problem is to implement this feature inside of GDataFileSystem, but we don't > want to add more code there. let me think a bit about it. How about adding scoped_ptr<StaleCacheFilesRemover> to GDataFileSystem? It is somewhat overkill but safer.
On 2012/08/11 00:26:34, yoshiki wrote: > On 2012/08/10 23:47:40, satorux1 wrote: > http://codereview.chromium.org/10832241/diff/8001/chrome/browser/chromeos/gda... > > chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:92: void > > StaleCacheFilesRemover::ScrubCache() { > > Because it's delayed, it's not guaranteed that cache_ and file_system_ are > valid > > by the time this function is called (i.e. they may be already deleted). > > > > I think we should change the strategy here... The easiest way to solve this > > problem is to implement this feature inside of GDataFileSystem, but we don't > > want to add more code there. let me think a bit about it. > > How about adding scoped_ptr<StaleCacheFilesRemover> to GDataFileSystem? It is > somewhat overkill but safer. sounds good!
http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2845: stale_cache_file_remover_->Start(); I suggest you to call this here, but looking at the function name (RunAndNotifyInitialLoadFinished), this may not be the right place to call. Maybe we might want to make the remover an observer of GDataFileSystem, then let it owned by GDataSystemService. Which will look like your original patch. :) http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/stale_cache_files_remover.cc (right): http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:25: void RemoveCacheIfNecessary( Please make it a private function of StaleCacheFilesRemover. please see below for why. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:31: scoped_ptr<gdata::GDataEntryProto> file_proto) { entry_proto http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:38: cache_md5 != file_proto->file_specific_info().file_md5()) { The condition looks complicated. Would be nice to separate: // The entry is not found in the file system. if (error != gdata::GDATA_FILE_OK) { cache->RemoveOnUIThread(resource_id, CacheOperationCallback()); return; } // The entry is found but the MD5 does not match. DCHECK(entry_proto.get()); if (cache_md5 != file_proto->file_specific_info().file_md5()) { cache->RemoveOnUIThread(resource_id, CacheOperationCallback()); return; } http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:39: cache->RemoveOnUIThread(resource_id, CacheOperationCallback()); Instead of passing a null-callback, could you pass a simple callback like: void EmitDebugLog(...) { DVLOG(1) << ... } base::Bind(&EmitDebugLog) http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:43: // Otherwise, does nothing with the cache. don't need this. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:48: void GetEntryInfoAndRemoveCacheIfNecessary( Please make it a private function of StaleCacheFilesRemover. Rather than passing file_system_ and cache_ to this function, having this as a private function would be cleaner. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:58: // Removes the chehe if GetCacheEntryOnUIThread() is failed. cache. If you use emacs, please use M-x ispell-comments-and-strings http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:58: // Removes the chehe if GetCacheEntryOnUIThread() is failed. is failed -> failed http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:60: cache->RemoveOnUIThread(resource_id, CacheOperationCallback()); This probably won't work if GetCacheEntryOnUIThread() failed. Instead of this, please add LOG(WARNING) http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:78: weak_ptr_factory_(this) { allow_this_in... http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:84: // Sets the timer to start removing. please remove comment here. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:96: // with the list. please move this to .h file. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:107: // GetEntryInfoAndRemoveCacheIfNecessary() please move this to .h file. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:116: "", // Doesn't check MD5. // Don't check MD5. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/stale_cache_files_remover.h (right): http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:19: // This class removes slace cache files, which are existed as file but are no maybe // This class removes stale cache files, which are present locally, but no longer present on the server. This can happen if files are removed from the server from other devices, or from the web interface. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:28: // Removes slate cache files. The execution is delayed so the actual removal stale http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:29: // starts several (|kDelaySeconds| in slate_cache_file_remover.cc) seconds Let's remove the delay. I think it's simpler not to delay. If needed, we can add it a later time. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:34: void RemoveSlateCacheFiles(); stale http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:35: void OnGetResourceIdsOfAllFiles(const std::vector<std::string>& resource_ids); function comments missing. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:37: GDataFileSystemInterface* file_system_; GDataFileSystemInterface* file_system_; // Not owned. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:38: GDataCache* cache_; ditto. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:40: base::WeakPtrFactory<StaleCacheFilesRemover> weak_ptr_factory_; please add // Note: This should remain the last member so it'll be destroyed and // invalidate its weak pointers before any other members are destroyed.
Forgot to mention but please write unit tests.
http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2845: stale_cache_file_remover_->Start(); On 2012/08/14 17:19:37, satorux1 wrote: > I suggest you to call this here, but looking at the function name > (RunAndNotifyInitialLoadFinished), this may not be the right place to call. > > Maybe we might want to make the remover an observer of GDataFileSystem, then let > it owned by GDataSystemService. > > Which will look like your original patch. :) Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/stale_cache_files_remover.cc (right): http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:25: void RemoveCacheIfNecessary( On 2012/08/14 17:19:37, satorux1 wrote: > Please make it a private function of StaleCacheFilesRemover. please see below > for why. Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:31: scoped_ptr<gdata::GDataEntryProto> file_proto) { On 2012/08/14 17:19:37, satorux1 wrote: > entry_proto Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:38: cache_md5 != file_proto->file_specific_info().file_md5()) { On 2012/08/14 17:19:37, satorux1 wrote: > The condition looks complicated. Would be nice to separate: > > // The entry is not found in the file system. > if (error != gdata::GDATA_FILE_OK) { > cache->RemoveOnUIThread(resource_id, CacheOperationCallback()); > return; > } > > // The entry is found but the MD5 does not match. > DCHECK(entry_proto.get()); > if (cache_md5 != file_proto->file_specific_info().file_md5()) { > cache->RemoveOnUIThread(resource_id, CacheOperationCallback()); > return; > } Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:39: cache->RemoveOnUIThread(resource_id, CacheOperationCallback()); On 2012/08/14 17:19:37, satorux1 wrote: > Instead of passing a null-callback, could you pass a simple callback like: > > void EmitDebugLog(...) { > DVLOG(1) << ... > } > > base::Bind(&EmitDebugLog) Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:43: // Otherwise, does nothing with the cache. On 2012/08/14 17:19:37, satorux1 wrote: > don't need this. Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:48: void GetEntryInfoAndRemoveCacheIfNecessary( On 2012/08/14 17:19:37, satorux1 wrote: > Please make it a private function of StaleCacheFilesRemover. > > Rather than passing file_system_ and cache_ to this function, having this as a > private function would be cleaner. Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:58: // Removes the chehe if GetCacheEntryOnUIThread() is failed. On 2012/08/14 17:19:37, satorux1 wrote: > cache. If you use emacs, please use M-x ispell-comments-and-strings Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:58: // Removes the chehe if GetCacheEntryOnUIThread() is failed. On 2012/08/14 17:19:37, satorux1 wrote: > is failed -> failed Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:60: cache->RemoveOnUIThread(resource_id, CacheOperationCallback()); On 2012/08/14 17:19:37, satorux1 wrote: > This probably won't work if GetCacheEntryOnUIThread() failed. Instead of this, > please add LOG(WARNING) Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:78: weak_ptr_factory_(this) { On 2012/08/14 17:19:37, satorux1 wrote: > allow_this_in... Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:84: // Sets the timer to start removing. On 2012/08/14 17:19:37, satorux1 wrote: > please remove comment here. Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:96: // with the list. On 2012/08/14 17:19:37, satorux1 wrote: > please move this to .h file. This method is removed http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:107: // GetEntryInfoAndRemoveCacheIfNecessary() On 2012/08/14 17:19:37, satorux1 wrote: > please move this to .h file. Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:116: "", // Doesn't check MD5. On 2012/08/14 17:19:37, satorux1 wrote: > // Don't check MD5. Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/stale_cache_files_remover.h (right): http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:19: // This class removes slace cache files, which are existed as file but are no On 2012/08/14 17:19:37, satorux1 wrote: > maybe > > // This class removes stale cache files, which are present locally, but no > longer present on the server. This can happen if files are removed from the > server from other devices, or from the web interface. Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:28: // Removes slate cache files. The execution is delayed so the actual removal On 2012/08/14 17:19:37, satorux1 wrote: > stale Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:29: // starts several (|kDelaySeconds| in slate_cache_file_remover.cc) seconds On 2012/08/14 17:19:37, satorux1 wrote: > Let's remove the delay. I think it's simpler not to delay. If needed, we can add > it a later time. Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:34: void RemoveSlateCacheFiles(); On 2012/08/14 17:19:37, satorux1 wrote: > stale Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:35: void OnGetResourceIdsOfAllFiles(const std::vector<std::string>& resource_ids); On 2012/08/14 17:19:37, satorux1 wrote: > function comments missing. Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:37: GDataFileSystemInterface* file_system_; On 2012/08/14 17:19:37, satorux1 wrote: > GDataFileSystemInterface* file_system_; // Not owned. Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:38: GDataCache* cache_; On 2012/08/14 17:19:37, satorux1 wrote: > ditto. Done. http://codereview.chromium.org/10832241/diff/3002/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/stale_cache_files_remover.h:40: base::WeakPtrFactory<StaleCacheFilesRemover> weak_ptr_factory_; On 2012/08/14 17:19:37, satorux1 wrote: > please add > > > // Note: This should remain the last member so it'll be destroyed and > // invalidate its weak pointers before any other members are destroyed. Done.
Thank you for writing tests! http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:77: void VerifyCacheFileState(GDataFileError error, function comment is missing. http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2663: run_loop.RunUntilIdle(); this is new to me. why is this used? shouldn't we use test_util::RunBlockingPoolTask() instead to wait for the feed to be loaded? http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/stale_cache_files_remover.cc (right): http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:24: if (error != gdata::GDATA_FILE_OK) add {} http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:26: "Failed to remove a stale cache file. resource_id:" << resource_id; formatting looks weird LOG(WARNING) << "Failed to remove a stale cache file. resource_id: " << resource_id;
http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:77: void VerifyCacheFileState(GDataFileError error, On 2012/08/16 11:03:53, satorux1 wrote: > function comment is missing. Done. http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2663: run_loop.RunUntilIdle(); On 2012/08/16 11:03:53, satorux1 wrote: > this is new to me. why is this used? shouldn't we use > test_util::RunBlockingPoolTask() instead to wait for the feed to be loaded? We don't need RunBlockingPoolTask(), because in this unit_tests, loading feeds (including parsing) is done synchronously. This waits the completion of a series of UI-thread tasks to remove stale cache files. This is completely same as message_loop_.QuitWhenIdle(). But the comment of MessageLoop::QuitWhenIdle() suggests that QuitWhenIdle() is deprecated and we should use RunLoop instead. http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/stale_cache_files_remover.cc (right): http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:24: if (error != gdata::GDATA_FILE_OK) On 2012/08/16 11:03:53, satorux1 wrote: > add {} Done. http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:26: "Failed to remove a stale cache file. resource_id:" << resource_id; On 2012/08/16 11:03:53, satorux1 wrote: > formatting looks weird > > LOG(WARNING) << "Failed to remove a stale cache file. resource_id: " > << resource_id; Done.
http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2663: run_loop.RunUntilIdle(); On 2012/08/17 03:49:59, yoshiki wrote: > On 2012/08/16 11:03:53, satorux1 wrote: > > this is new to me. why is this used? shouldn't we use > > test_util::RunBlockingPoolTask() instead to wait for the feed to be loaded? > > We don't need RunBlockingPoolTask(), because in this unit_tests, loading feeds > (including parsing) is done synchronously. > > This waits the completion of a series of UI-thread tasks to remove stale cache > files. This is completely same as message_loop_.QuitWhenIdle(). But the comment > of MessageLoop::QuitWhenIdle() suggests that QuitWhenIdle() is deprecated and we > should use RunLoop instead. I'm more confused. If feed loading is done synchronously, why do we need to quit the message loop? why do we need this? http://codereview.chromium.org/10832241/diff/15002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/10832241/diff/15002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:77: // Callback to GDataCache::StoreOnUIThread used in RemoveStaleCacheFiles test. Callback for
http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2663: run_loop.RunUntilIdle(); On 2012/08/17 10:38:41, satorux1 wrote: > On 2012/08/17 03:49:59, yoshiki wrote: > > On 2012/08/16 11:03:53, satorux1 wrote: > > > this is new to me. why is this used? shouldn't we use > > > test_util::RunBlockingPoolTask() instead to wait for the feed to be loaded? > > > > We don't need RunBlockingPoolTask(), because in this unit_tests, loading feeds > > (including parsing) is done synchronously. > > > > This waits the completion of a series of UI-thread tasks to remove stale cache > > files. This is completely same as message_loop_.QuitWhenIdle(). But the > comment > > of MessageLoop::QuitWhenIdle() suggests that QuitWhenIdle() is deprecated and > we > > should use RunLoop instead. > > I'm more confused. If feed loading is done synchronously, why do we need to > quit the message loop? why do we need this? Sorry for confusion, my comment has a wrong method name. Please s/QuitWhenIdle/RunUntilIdle/g in my comment. MessageLoop::RunUntilIdle() is deprecated, according to the comment. http://code.google.com/searchframe#OAMlx_jo-ck/src/base/message_loop.h&exact_... http://codereview.chromium.org/10832241/diff/15002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/10832241/diff/15002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:77: // Callback to GDataCache::StoreOnUIThread used in RemoveStaleCacheFiles test. On 2012/08/17 10:38:41, satorux1 wrote: > Callback for Done.
http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2663: run_loop.RunUntilIdle(); On 2012/08/18 03:31:51, yoshiki wrote: > On 2012/08/17 10:38:41, satorux1 wrote: > > On 2012/08/17 03:49:59, yoshiki wrote: > > > On 2012/08/16 11:03:53, satorux1 wrote: > > > > this is new to me. why is this used? shouldn't we use > > > > test_util::RunBlockingPoolTask() instead to wait for the feed to be > loaded? > > > > > > We don't need RunBlockingPoolTask(), because in this unit_tests, loading > feeds > > > (including parsing) is done synchronously. > > > > > > This waits the completion of a series of UI-thread tasks to remove stale > cache > > > files. This is completely same as message_loop_.QuitWhenIdle(). But the > > comment > > > of MessageLoop::QuitWhenIdle() suggests that QuitWhenIdle() is deprecated > and > > we > > > should use RunLoop instead. > > > > I'm more confused. If feed loading is done synchronously, why do we need to > > quit the message loop? why do we need this? > > Sorry for confusion, my comment has a wrong method name. Please > s/QuitWhenIdle/RunUntilIdle/g in my comment. > > MessageLoop::RunUntilIdle() is deprecated, according to the comment. > http://code.google.com/searchframe#OAMlx_jo-ck/src/base/message_loop.h&exact_... Sorry, but I'm still confused. Could you explain why this is needed? If LoadRootFeedDocument() is synchronous, we don't have to deal with a message loop, right?
LGTM with requests: http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:15: #include "base/run_loop.h" please remove. see below. http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2663: run_loop.RunUntilIdle(); On 2012/08/18 11:33:16, satorux1 wrote: > On 2012/08/18 03:31:51, yoshiki wrote: > > On 2012/08/17 10:38:41, satorux1 wrote: > > > On 2012/08/17 03:49:59, yoshiki wrote: > > > > On 2012/08/16 11:03:53, satorux1 wrote: > > > > > this is new to me. why is this used? shouldn't we use > > > > > test_util::RunBlockingPoolTask() instead to wait for the feed to be > > loaded? > > > > > > > > We don't need RunBlockingPoolTask(), because in this unit_tests, loading > > feeds > > > > (including parsing) is done synchronously. > > > > > > > > This waits the completion of a series of UI-thread tasks to remove stale > > cache > > > > files. This is completely same as message_loop_.QuitWhenIdle(). But the > > > comment > > > > of MessageLoop::QuitWhenIdle() suggests that QuitWhenIdle() is deprecated > > and > > > we > > > > should use RunLoop instead. > > > > > > I'm more confused. If feed loading is done synchronously, why do we need to > > > quit the message loop? why do we need this? > > > > Sorry for confusion, my comment has a wrong method name. Please > > s/QuitWhenIdle/RunUntilIdle/g in my comment. > > > > MessageLoop::RunUntilIdle() is deprecated, according to the comment. > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/base/message_loop.h&exact_... > > Sorry, but I'm still confused. Could you explain why this is needed? If > LoadRootFeedDocument() is synchronous, we don't have to deal with a message > loop, right? I guess the synchronization is needed to let the StaleCacheFilesRemove do the job. Is this correct? If so, please change it to test_util::RunBlockingPoolTask(). As of now, we are using in-memory cache implementation for tests (crrev.com/151013), but can be changed in the future. Anyway, it's not obvious why we need synchronization here, so please add a comment like: // Wait for StaleCacheFilesRemover to finish cleaning up the stale file. test_util::RunBlockingPoolTask();
Thanks for your review. I'll commit this after rebase. http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:15: #include "base/run_loop.h" On 2012/08/19 05:24:59, satorux1 wrote: > please remove. see below. Done. http://codereview.chromium.org/10832241/diff/10002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:2663: run_loop.RunUntilIdle(); On 2012/08/19 05:24:59, satorux1 wrote: > On 2012/08/18 11:33:16, satorux1 wrote: > > On 2012/08/18 03:31:51, yoshiki wrote: > > > On 2012/08/17 10:38:41, satorux1 wrote: > > > > On 2012/08/17 03:49:59, yoshiki wrote: > > > > > On 2012/08/16 11:03:53, satorux1 wrote: > > > > > > this is new to me. why is this used? shouldn't we use > > > > > > test_util::RunBlockingPoolTask() instead to wait for the feed to be > > > loaded? > > > > > > > > > > We don't need RunBlockingPoolTask(), because in this unit_tests, loading > > > feeds > > > > > (including parsing) is done synchronously. > > > > > > > > > > This waits the completion of a series of UI-thread tasks to remove stale > > > cache > > > > > files. This is completely same as message_loop_.QuitWhenIdle(). But the > > > > comment > > > > > of MessageLoop::QuitWhenIdle() suggests that QuitWhenIdle() is > deprecated > > > and > > > > we > > > > > should use RunLoop instead. > > > > > > > > I'm more confused. If feed loading is done synchronously, why do we need > to > > > > quit the message loop? why do we need this? > > > > > > Sorry for confusion, my comment has a wrong method name. Please > > > s/QuitWhenIdle/RunUntilIdle/g in my comment. > > > > > > MessageLoop::RunUntilIdle() is deprecated, according to the comment. > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/base/message_loop.h&exact_... > > > > Sorry, but I'm still confused. Could you explain why this is needed? If > > LoadRootFeedDocument() is synchronous, we don't have to deal with a message > > loop, right? > > I guess the synchronization is needed to let the StaleCacheFilesRemove do the > job. Is this correct? Right. > If so, please change it to test_util::RunBlockingPoolTask(). As of now, we are > using in-memory cache implementation for tests (crrev.com/151013), but can be > changed in the future. > > Anyway, it's not obvious why we need synchronization here, so please add a > comment like: > > // Wait for StaleCacheFilesRemover to finish cleaning up the stale file. > test_util::RunBlockingPoolTask(); Done. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10832241/3010
Try job failure for 10832241-3010 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10832241/3012
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10832241/16003
Change committed as 152328
Satoru-san, could you take a look at the change of gdata_file_system.h again? This CL has been reverted after once committed due to memorybot failure. I changed the order of the member variables of GDataFileSystem (see the diff between PatchSet #14 and #15) to fix the failure.
http://codereview.chromium.org/10832241/diff/8008/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10832241/diff/8008/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.h:839: // be constructed before them and be destructed after them. Wow, that's subtle. If so, why don't you move this to the very end before WeakPtr stuff? StaleCacheFilesRemover is owned by this class, but placed below. why?
http://codereview.chromium.org/10832241/diff/8008/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10832241/diff/8008/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.h:839: // be constructed before them and be destructed after them. On 2012/08/21 20:57:06, satorux1 wrote: > Wow, that's subtle. If so, why don't you move this to the very end before > WeakPtr stuff? StaleCacheFilesRemover is owned by this class, but placed below. > why? Sorry, I misunderstood. So the observer list has to be created *before* |stale_cache_files_remover_| and destructed *after* |stale_cache_files_remover_|, right? I think the issue we were hit was not a memory leak but a use-after-free. file_system_->RemoveObserver(this) was called after the observer list was destructed, right? What about moving the ownership of |stale_cache_files_remover_| to GDataSystemService?
Sorry for delayed response. On 2012/08/21 21:29:06, satorux1 wrote: > http://codereview.chromium.org/10832241/diff/8008/chrome/browser/chromeos/gda... > File chrome/browser/chromeos/gdata/gdata_file_system.h (right): > > http://codereview.chromium.org/10832241/diff/8008/chrome/browser/chromeos/gda... > chrome/browser/chromeos/gdata/gdata_file_system.h:839: // be constructed before > them and be destructed after them. > On 2012/08/21 20:57:06, satorux1 wrote: > > Wow, that's subtle. If so, why don't you move this to the very end before > > WeakPtr stuff? StaleCacheFilesRemover is owned by this class, but placed > below. > > why? > > Sorry, I misunderstood. So the observer list has to be created *before* > |stale_cache_files_remover_| and destructed *after* > |stale_cache_files_remover_|, right? > > I think the issue we were hit was not a memory leak but a use-after-free. > file_system_->RemoveObserver(this) was called after the observer list was > destructed, right? > > What about moving the ownership of |stale_cache_files_remover_| to > GDataSystemService? Done. And I've separated the test from file_system_unittest.cc. PTAL.
http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc (right): http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:58: void GetEntryInfoByPathCallback( I thought we had this in gdata_test_util.h. If so, please use it. Otherwise please add it. http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:67: void GetEntryInfoByResourceIdCallback( ditto. http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:105: class MockFreeDiskSpaceGetter : public FreeDiskSpaceGetterInterface { do we define the same mock class elsewhere? if so, please put this in a separate file and share. http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:111: class MockGDataUploader : public GDataUploaderInterface { ditto. http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:138: class MockDriveWebAppsRegistry : public DriveWebAppsRegistryInterface { ditto. http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:151: class StaleCacheFileRemover : public testing::Test { StaleCacheFilesRemoverTest http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:235: void LoadChangeFeed(const std::string& filename, if this is used elsewhere, please put this in gdata_test_util and share.
http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc (right): http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:58: void GetEntryInfoByPathCallback( On 2012/08/28 15:38:02, satorux1 wrote: > I thought we had this in gdata_test_util.h. If so, please use it. Otherwise > please add it. Thanks, I didn't know that. Done. http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:67: void GetEntryInfoByResourceIdCallback( On 2012/08/28 15:38:02, satorux1 wrote: > ditto. Done. http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:105: class MockFreeDiskSpaceGetter : public FreeDiskSpaceGetterInterface { On 2012/08/28 15:38:02, satorux1 wrote: > do we define the same mock class elsewhere? if so, please put this in a separate > file and share. Done. Moved to gdata_test_util.h http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:111: class MockGDataUploader : public GDataUploaderInterface { On 2012/08/28 15:38:02, satorux1 wrote: > ditto. Done. Moved to gdata_test_util.h http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:138: class MockDriveWebAppsRegistry : public DriveWebAppsRegistryInterface { On 2012/08/28 15:38:02, satorux1 wrote: > ditto. Done. Moved to gdata_test_util.h http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:151: class StaleCacheFileRemover : public testing::Test { On 2012/08/28 15:38:02, satorux1 wrote: > StaleCacheFilesRemoverTest Done. http://codereview.chromium.org/10832241/diff/28002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:235: void LoadChangeFeed(const std::string& filename, On 2012/08/28 15:38:02, satorux1 wrote: > if this is used elsewhere, please put this in gdata_test_util and share. Moved it to test_util and removed LoadChangeFeed() and UpdateContent().
Sorry for the belated response. Thank you for creating new mock files, and moving test functions to gdata_test_util.h! I think the patch is now very big. Can you split this into two patches? 1) create new mock files, and move test functions to gdata_test_util.h 2) the StaleCacheFilesRemover stuff This way, it's easier to tell what part of code is new and not. http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/drive_file_system_unittest.cc (right): http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/drive_file_system_unittest.cc:255: root_feed_changestamp_++); looks tricky. please increment outside the function call. http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/mock_drive_web_apps_registry.h (right): http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/mock_drive_web_apps_registry.h:29: } } // namespace gdata http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc (right): http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:72: chromeos::CrosLibrary::Initialize(true /* use_stub */); Why is this needed? please add a comment.
PTAL http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/drive_file_system_unittest.cc (right): http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/drive_file_system_unittest.cc:255: root_feed_changestamp_++); On 2012/08/30 04:38:15, satorux1 wrote: > looks tricky. please increment outside the function call. Done. https://chromiumcodereview.appspot.com/10905016/diff2/1:4001/chrome/browser/c... http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/mock_drive_web_apps_registry.h (right): http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/mock_drive_web_apps_registry.h:29: } On 2012/08/30 04:38:15, satorux1 wrote: > } // namespace gdata Done. https://chromiumcodereview.appspot.com/10905016/diff2/1:4001/chrome/browser/c... http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc (right): http://codereview.chromium.org/10832241/diff/43018/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover_unittest.cc:72: chromeos::CrosLibrary::Initialize(true /* use_stub */); On 2012/08/30 04:38:15, satorux1 wrote: > Why is this needed? please add a comment. I realize it is unnecessarily at this test. Deleted.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10832241/56003
Failed to apply patch for chrome/chrome_browser.gypi: While running patch -p1 --forward --force; patching file chrome/chrome_browser.gypi Hunk #1 FAILED at 623. 1 out of 1 hunk FAILED -- saving rejects to file chrome/chrome_browser.gypi.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10832241/58003
Change committed as 154430
I think this CL probably introduced this crasher: crbug.com/148752 http://codereview.chromium.org/10832241/diff/58003/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/stale_cache_files_remover.cc (right): http://codereview.chromium.org/10832241/diff/58003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/stale_cache_files_remover.cc:49: cache_->GetResourceIdsOfAllFilesOnUIThread( I think this line has a problem.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
