Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10008100:
gdata: Support mouting archive file in GData cache (Closed)
|
Created:
8 years, 8 months ago by hshi Modified:
8 years, 8 months ago CC:
chromium-reviews, nkostylev+watch_chromium.org, mihaip+watch_chromium.org, Aaron Boodman, rginda+watch_chromium.org, arv (Not doing code reviews), stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Description gdata: Support mounting archive files in GData cache
Pass the file extension in the 2nd argument to DiskMountManager::MountPath
to indicate the archive format (e.g.: .zip, .rar, etc.)
When mounting and unmounting archive files in GData cache, call
GDataFileSystem::SetMountedState to set or clear the "mounted" state
accordingly.
BUG=chromium-os:28678
TEST=Verify that mounting/unmounting archive files on GData works correctly.
Patch Set 1 #Patch Set 2 : Test CL for Issue 28678 #Patch Set 3 : Test CL for Issue 28678 #Patch Set 4 : Test CL for Issue 28678 #Patch Set 5 : gdata: Support mounting archive files under GData cache #
Total comments: 45
Patch Set 6 : gdata: Support mounting archive files in GData cache #
Total comments: 10
Patch Set 7 : gdata: Support mounting archive files in GData cache #Patch Set 8 : gdata: Support mounting archive files in GData cache #
Total comments: 2
Patch Set 9 : gdata: Support mounting archive files in GData cache #
Total comments: 1
Patch Set 10 : gdata: Support mounting archive files in GData cache. #Messages
Total messages: 22 (0 generated)
https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/disks/disk_mount_manager.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/disks/disk_mount_manager.cc:61: const std::string& source_name, |source_name| -> |source_format| https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/disks/disk_mount_manager.h (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/disks/disk_mount_manager.h:205: const std::string& source_name, calling it |source_name| is misleading here. it should be called |source_format| https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_event_router.cc:419: disk->device_path(), std::string(), chromeos::MOUNT_TYPE_DEVICE); nit: perhaps add a comment here to indicate that it auto-detects the filesystem format if the second argument is empty. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_event_router.cc:486: DiskMountManager::GetInstance()->MountPath(device_path, std::string(), ditto https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_private_api.cc:1005: const std::string& display_name = files[0].display_name; const FilePath::StringType& https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_private_api.cc:1022: void AddMountFunction::OnMountedStateSet(base::PlatformFileError error, seems like this method is only used on Chrome OS, maybe we can guard the entire method by #if defined(OS_CHROMEOS) https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_private_api.cc:1030: disk_mount_manager->MountPath(file_path.AsUTF8Unsafe(), file_name, passing file_name as the second argument to MountPath() seems misleading as it should only be the file extension. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_private_api.cc:1090: system_service->file_system()->ClearMountedState(source_path, base::Bind( nit: line break before base::Bind https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/extensions/file_browser_private_api.h (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_private_api.h:247: // Callback for SetMountedState. nit: to be consistent with other comments, maybe: A callback method to handle the result of SetMountedState. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_private_api.h:250: const std::string& file_name, const FilePath::StringType& instead of const std::string& for |file_name| https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_private_api.h:273: // Callback for ClearMountedState. nit: to be consistent with other comments, maybe: A callback method to handle the result of ClearMountedState. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:1819: gdata_cache_path_.IsParent(path); nit: can this fit in one line? also, please add unit test. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:1976: std::string()); Should we pass md5 to GetCacheEntry here? https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:1984: } nit: line break https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:1985: base::PlatformFileError error; this can be defined in the scope below. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:1999: error = ModifyCacheState(src_path, dst_path, base::PlatformFileError error = ... https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2027: } nit: line break https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2028: base::PlatformFileError error; this can be defined within the scope below https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2033: GDataRootDirectory::CACHE_TYPE_PERSISTENT, nit: thisseems like a weird indentation https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2037: entry->IsPinned() ? ditto https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2042: error = ModifyCacheState(src_path, dst_path, base::PlatformFileError error = ... https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.h:193: CACHED_FILE_MOUNTED_ARCHIVE, nit: prefer CACHED_FILE_IN_USE or CACHED_FILE_MOUNTED as we may reuse this mechanism for marking a cache file being opened as read-only. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.h:406: // Set mounted state of an archive Prefer a more elaborated comment, e.g. Marks a file as locally mounted to prevent it from being modified or evicted from cache. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.h:408: const std::string& file_name, const FilePath::StringType& https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.h:412: // Clear mounted state of an archive ditto https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/reso... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_manager.js:2955: this.resolveSelectResults_(urls, function(urls) { Have you tested if this change also works with archives in non-gdata directory (e.g. in Downloads)? https://chromiumcodereview.appspot.com/10008100/diff/2005/chromeos/dbus/cros_... File chromeos/dbus/cros_disks_client.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chromeos/dbus/cros_... chromeos/dbus/cros_disks_client.cc:116: const std::string& source_name, |source_name| -> |source_format| https://chromiumcodereview.appspot.com/10008100/diff/2005/chromeos/dbus/cros_... chromeos/dbus/cros_disks_client.cc:379: const std::string& source_name, |source_name| -> |source_format| https://chromiumcodereview.appspot.com/10008100/diff/2005/chromeos/dbus/cros_... File chromeos/dbus/cros_disks_client.h (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chromeos/dbus/cros_... chromeos/dbus/cros_disks_client.h:182: const std::string& source_name, |source_name| -> |source_format|
https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/reso... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_manager.js:2955: this.resolveSelectResults_(urls, function(urls) { On 2012/04/17 20:57:53, Ben Chan wrote: > Have you tested if this change also works with archives in non-gdata directory > (e.g. in Downloads)? Yes this is tested with archives in non-gdata directory such as Downloads. It works because resolveSelectResults_ checks for (this.isOnGData()) and will only invoke private.getGDataFiles() for files in gdata; for non-gdata it is a passthru.
https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_private_api.cc:1010: system_service->file_system()->IsUnderGDataCacheDirectory(source_path)) { are you sure this is ever satisfied? afaik |files| will contain list of virtual paths (something like special/gdata/foo.zip), not cache file paths.. or am I missing something? https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_private_api.cc:1022: void AddMountFunction::OnMountedStateSet(base::PlatformFileError error, On 2012/04/17 20:57:53, Ben Chan wrote: > seems like this method is only used on Chrome OS, maybe we can guard the entire > method by #if defined(OS_CHROMEOS) actually, whole file is used only on chromeos.. but it seems that we guard code that would not compile on other platforms (I'm not sure why though.. we should probably clean it up a bit..) https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_private_api.cc:1085: // check if the source path is under GData cache directory Comments have to end with . Also, they have to begin with capital letter, so: // Check if the source path in under GData cache directory. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:1971: const std::string& file_name, I don't see that you really use file_name and mount_type.. Why don't you bind it to the callback before you call the method. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:1974: std::string resource_id = file_path.BaseName().RemoveExtension().value(); I think you have to acquire |lock_| before you access cache. Same in ClearMountedState https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:1987: base::AutoLock lock(lock_); You should move this to the method scope. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2017: const FileOperationCallback& callback) { This looks very similar to SetMountState.. Why don't you try to merge these two in one method SetMountedState(file_path, is_mounted, callback) https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:3416: entry->IsMounted() ? CACHED_FILE_MOUNTED_ARCHIVE : This looks a bit ugly (at least to me:) ) https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.h:193: CACHED_FILE_MOUNTED_ARCHIVE, We should probably add CACHED_FILE_MOUNTED_ARCHIVE to the cache_paths in InsertGDataCachePaths in gdata_util.cc Otherwise registered file browser handlers won't be able to read mounted archives. https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.h:406: // Set mounted state of an archive On 2012/04/17 20:57:53, Ben Chan wrote: > Prefer a more elaborated comment, e.g. > > Marks a file as locally mounted to prevent it from being modified or evicted > from cache. Also, comments must end with . https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.h:499: const std::string& file_name, fix indent
https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/extensions/file_browser_private_api.cc:1010: system_service->file_system()->IsUnderGDataCacheDirectory(source_path)) { On 2012/04/17 21:33:13, tbarzic wrote: > are you sure this is ever satisfied? > afaik |files| will contain list of virtual paths (something like > special/gdata/foo.zip), not cache file paths.. or am I missing something? Ok, looks like this changed recently :) ignore the comment
https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.h:193: CACHED_FILE_MOUNTED_ARCHIVE, On 2012/04/17 21:33:13, tbarzic wrote: > We should probably add CACHED_FILE_MOUNTED_ARCHIVE to the cache_paths in > InsertGDataCachePaths in gdata_util.cc > > Otherwise registered file browser handlers won't be able to read mounted > archives. I am not sure that this applies to the case of mounted archives. It is similar to the handling of locally modified ("dirty") files -- that the file itself is placed under the PERSISTENT directory, but the filename is set to <resource_id>.local (and <resource_id>.mounted for our case). P.S. I am unable to locate the InsertGDataCachePaths function, maybe it is renamed/moved?
https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2017: const FileOperationCallback& callback) { On 2012/04/17 21:33:13, tbarzic wrote: > This looks very similar to SetMountState.. > > Why don't you try to merge these two in one method SetMountedState(file_path, > is_mounted, callback) > Yes I had initially attempted to merge the two, but unfortunately the SetMountedStateCallback requires passing back the new path to the cached blob (persistent/<resource_id>.mounted) whereas ClearMountedState does not. However I can simplify the SetMountedState as Ben suggested and bind the file_name and mount_type before the call.
https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/2005/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2017: const FileOperationCallback& callback) { On 2012/04/17 22:36:45, hshi wrote: > On 2012/04/17 21:33:13, tbarzic wrote: > > This looks very similar to SetMountState.. > > > > Why don't you try to merge these two in one method SetMountedState(file_path, > > is_mounted, callback) > > > > Yes I had initially attempted to merge the two, but unfortunately the > SetMountedStateCallback requires passing back the new path to the cached blob > (persistent/<resource_id>.mounted) whereas ClearMountedState does not. However I > can simplify the SetMountedState as Ben suggested and bind the file_name and > mount_type before the call. when I think about it, it may have sense to pass dst_path to ClearMountedState callback too (it can be ignored in file_browser_pricate_api for now)
Please review Patch Set #6 which addresses Ben's and Toni's comments regarding Patch Set #5. Also adding owners of affected code under chrome/browser/resource/file_manager and chromeos/dbus. Thank you!
i only looked at gdata_file_system.* and gdata_files.h. http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2022: const SetMountedStateCallback& callback) { Any method that accesses cache files (i.e. in cache subdirectories) or cache memory (i.e. root_->GetCacheEntry) needs to happen: 1) on IO Thread Pool with sequenced ID (or FILE thread temporarily for now) 2) after InitializeCacheOnIOThreadPool If u refer to StoreToCache example, it first calls InitializeCacheIfNecessary then post StoreToCacheOnIOThreadPool task to blocking pool, with a reply on the calling thread. u need to adopt the same logic, to ensure that SetMountedState happens after cache initialization and on io thread pool (or file thread for now). http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2046: // Returns path of the file if it were to be unmounted. nit: s/Returns/Gets/ http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2053: // Returns path of the file if it were to be mounted. nit: s/Returns/Gets/ http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2063: entry->cache_state = GDataFile::SetCacheMounted(entry->cache_state); please use a variable "int cache_state" to store the returned state. http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.h:494: const SetMountedStateCallback& callback) OVERRIDE; nit: either align all parameters after (, or 4-space indentation with one per line
http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.h:406: // or evicted from cache. marking it as mounted doesn't automatically prevent it from being modified or moved. u have to add that logic. in StoreToCacheOnIOThreadPool, PinOnIOThreadPool, UnpinOnIOThreadPool and RemoveFromCacheOnIOThreadPool, look for "entry->IsDirty()" to see how dirty files are handled. if u want mounted files handled exactly the same way, maybe just need to add "|| entry->IsMounted()", but make sure.
http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2022: const SetMountedStateCallback& callback) { (1) In terms of the complexity of the change, is there any difference if I were to make this happen on the FILE thread vs on the IO Thread Pool? Currently SetMountedState is called on the UI thread (AddMountFunction::GetLocalPathsResponseOnUIThread) so it seems that I will need to post the task anyway. (2) In the StoreToCache example the same resource_id and md5 parameters are passed to the request task and reply task in PostBlockingPoolSequencedTaskAndReply. Is there any way for the request task to pass parameters to the reply task? In case to_mount==true, it is necessary to pass back the mounted path (GCache/v1/persistent/<resource_id>.mounted) to the callback which will then call disk mount manager to mount the path. On 2012/04/18 00:36:36, kuan wrote: > Any method that accesses cache files (i.e. in cache subdirectories) or cache > memory (i.e. root_->GetCacheEntry) needs to happen: > 1) on IO Thread Pool with sequenced ID (or FILE thread temporarily for now) > 2) after InitializeCacheOnIOThreadPool > If u refer to StoreToCache example, it first calls InitializeCacheIfNecessary > then post StoreToCacheOnIOThreadPool task to blocking pool, with a reply on the > calling thread. u need to adopt the same logic, to ensure that SetMountedState > happens after cache initialization and on io thread pool (or file thread for > now).
http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2022: const SetMountedStateCallback& callback) { On 2012/04/18 01:29:05, hshi1 wrote: > (1) In terms of the complexity of the change, is there any difference if I were > to make this happen on the FILE thread vs on the IO Thread Pool? Currently > SetMountedState is called on the UI thread > (AddMountFunction::GetLocalPathsResponseOnUIThread) so it seems that I will need > to post the task anyway. just call PostBlockingPoolSequencedTaskAndReply to post task and reply. internally, that method now posts task to FILE thread. initially, cache tasks happened on IO thread pool, with a sequenced id for sequencing. they were later changed to FILE thread. i'm positive we'll revert to IO thread pool in the near future. fyi, this task needs to happen and queued on the same thread as all cache tasks. it doesn't matter which thread your method is called, even if it's called on the same thread as that for cache task, u have to post task to _queue_ it in thread pool with the same sequence id as other cache tasks. when we switch back to IO thread pool later, your method will never have the same sequence id as that for cache tasks, so if the task is not posted to io thread pool with sequenced id, it will be out of sync with other cache tasks. PostBlockingPoolSequencedTaskAndReply will shield you from having to change the thread later, and all cache APIs call that. > (2) In the StoreToCache example the same resource_id and md5 parameters are > passed to the request task and reply task in > PostBlockingPoolSequencedTaskAndReply. Is there any way for the request task to > pass parameters to the reply task? In case to_mount==true, it is necessary to > pass back the mounted path (GCache/v1/persistent/<resource_id>.mounted) to the > callback which will then call disk mount manager to mount the path. refer to GetFromCacheInternal and GetFromCacheOnIOThreadPool which also passes a FilePath from task to reply. > On 2012/04/18 00:36:36, kuan wrote: > > Any method that accesses cache files (i.e. in cache subdirectories) or cache > > memory (i.e. root_->GetCacheEntry) needs to happen: > > 1) on IO Thread Pool with sequenced ID (or FILE thread temporarily for now) > > 2) after InitializeCacheOnIOThreadPool > > If u refer to StoreToCache example, it first calls InitializeCacheIfNecessary > > then post StoreToCacheOnIOThreadPool task to blocking pool, with a reply on > the > > calling thread. u need to adopt the same logic, to ensure that > SetMountedState > > happens after cache initialization and on io thread pool (or file thread for > > now). >
http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2022: const SetMountedStateCallback& callback) { Thanks for the explanation, I think I understood most of it, but can you clarify one more thing? For binding the task and reply, how do I decide whether I need to pass in either base::Unretained(this) or GetWeakPtrForCurrentThread() as the 2nd argument? On 2012/04/18 11:32:54, kuan wrote: > On 2012/04/18 01:29:05, hshi1 wrote: > > (1) In terms of the complexity of the change, is there any difference if I > were > > to make this happen on the FILE thread vs on the IO Thread Pool? Currently > > SetMountedState is called on the UI thread > > (AddMountFunction::GetLocalPathsResponseOnUIThread) so it seems that I will > need > > to post the task anyway. > > just call PostBlockingPoolSequencedTaskAndReply to post task and reply. > internally, that method now posts task to FILE thread. initially, cache tasks > happened on IO thread pool, with a sequenced id for sequencing. they were later > changed to FILE thread. i'm positive we'll revert to IO thread pool in the near > future. > fyi, this task needs to happen and queued on the same thread as all cache tasks. > it doesn't matter which thread your method is called, even if it's called on > the same thread as that for cache task, u have to post task to _queue_ it in > thread pool with the same sequence id as other cache tasks. when we switch back > to IO thread pool later, your method will never have the same sequence id as > that for cache tasks, so if the task is not posted to io thread pool with > sequenced id, it will be out of sync with other cache tasks. > PostBlockingPoolSequencedTaskAndReply will shield you from having to change the > thread later, and all cache APIs call that. > > > (2) In the StoreToCache example the same resource_id and md5 parameters are > > passed to the request task and reply task in > > PostBlockingPoolSequencedTaskAndReply. Is there any way for the request task > to > > pass parameters to the reply task? In case to_mount==true, it is necessary to > > pass back the mounted path (GCache/v1/persistent/<resource_id>.mounted) to the > > callback which will then call disk mount manager to mount the path. > > refer to GetFromCacheInternal and GetFromCacheOnIOThreadPool which also passes a > FilePath from task to reply. > > > On 2012/04/18 00:36:36, kuan wrote: > > > Any method that accesses cache files (i.e. in cache subdirectories) or cache > > > memory (i.e. root_->GetCacheEntry) needs to happen: > > > 1) on IO Thread Pool with sequenced ID (or FILE thread temporarily for now) > > > 2) after InitializeCacheOnIOThreadPool > > > If u refer to StoreToCache example, it first calls > InitializeCacheIfNecessary > > > then post StoreToCacheOnIOThreadPool task to blocking pool, with a reply on > > the > > > calling thread. u need to adopt the same logic, to ensure that > > SetMountedState > > > happens after cache initialization and on io thread pool (or file thread for > > > now). > > >
http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10008100/diff/16002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2022: const SetMountedStateCallback& callback) { On 2012/04/18 17:28:15, hshi1 wrote: > Thanks for the explanation, I think I understood most of it, but can you clarify > one more thing? For binding the task and reply, how do I decide whether I need > to pass in either base::Unretained(this) or GetWeakPtrForCurrentThread() as the > 2nd argument? u use base::Unretained(this) for the task to run on IO thread that needs to access GDataFileSystem's class members (like root_->GetCacheEntry). u use GetWeakPtrForCurrentThread() for the reply to run on the thread where u call PostBlockingPoolSequencedTaskAndReply. > On 2012/04/18 11:32:54, kuan wrote: > > On 2012/04/18 01:29:05, hshi1 wrote: > > > (1) In terms of the complexity of the change, is there any difference if I > > were > > > to make this happen on the FILE thread vs on the IO Thread Pool? Currently > > > SetMountedState is called on the UI thread > > > (AddMountFunction::GetLocalPathsResponseOnUIThread) so it seems that I will > > need > > > to post the task anyway. > > > > just call PostBlockingPoolSequencedTaskAndReply to post task and reply. > > internally, that method now posts task to FILE thread. initially, cache tasks > > happened on IO thread pool, with a sequenced id for sequencing. they were > later > > changed to FILE thread. i'm positive we'll revert to IO thread pool in the > near > > future. > > fyi, this task needs to happen and queued on the same thread as all cache > tasks. > > it doesn't matter which thread your method is called, even if it's called on > > the same thread as that for cache task, u have to post task to _queue_ it in > > thread pool with the same sequence id as other cache tasks. when we switch > back > > to IO thread pool later, your method will never have the same sequence id as > > that for cache tasks, so if the task is not posted to io thread pool with > > sequenced id, it will be out of sync with other cache tasks. > > PostBlockingPoolSequencedTaskAndReply will shield you from having to change > the > > thread later, and all cache APIs call that. > > > > > (2) In the StoreToCache example the same resource_id and md5 parameters are > > > passed to the request task and reply task in > > > PostBlockingPoolSequencedTaskAndReply. Is there any way for the request task > > to > > > pass parameters to the reply task? In case to_mount==true, it is necessary > to > > > pass back the mounted path (GCache/v1/persistent/<resource_id>.mounted) to > the > > > callback which will then call disk mount manager to mount the path. > > > > refer to GetFromCacheInternal and GetFromCacheOnIOThreadPool which also passes > a > > FilePath from task to reply. > > > > > On 2012/04/18 00:36:36, kuan wrote: > > > > Any method that accesses cache files (i.e. in cache subdirectories) or > cache > > > > memory (i.e. root_->GetCacheEntry) needs to happen: > > > > 1) on IO Thread Pool with sequenced ID (or FILE thread temporarily for > now) > > > > 2) after InitializeCacheOnIOThreadPool > > > > If u refer to StoreToCache example, it first calls > > InitializeCacheIfNecessary > > > > then post StoreToCacheOnIOThreadPool task to blocking pool, with a reply > on > > > the > > > > calling thread. u need to adopt the same logic, to ensure that > > > SetMountedState > > > > happens after cache initialization and on io thread pool (or file thread > for > > > > now). > > > > > >
Hi could you please review this patch set #7. It addresses Kuan's comments for gdata_file_system.cc. Still need comments regarding the changes in cros_disk_client.cc and in file_manager.js. Thanks!
https://chromiumcodereview.appspot.com/10008100/diff/14014/chrome/browser/chr... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/14014/chrome/browser/chr... chrome/browser/chromeos/gdata/gdata_file_system.cc:2059: FilePath* cache_file_path) { DCHECK(error); DCHECK(cache_file_path);
https://chromiumcodereview.appspot.com/10008100/diff/14014/chrome/browser/chr... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): https://chromiumcodereview.appspot.com/10008100/diff/14014/chrome/browser/chr... chrome/browser/chromeos/gdata/gdata_file_system.cc:2059: FilePath* cache_file_path) { Oops sorry forgot to check this. Patch set #9 uploaded, thanks! On 2012/04/18 19:38:14, Ben Chan wrote: > DCHECK(error); > DCHECK(cache_file_path);
hshi, can you split this patch to smaller patches? smaller patches are generally preferred for many reasons, but most importantly it's easier to review. There seems to be four logical changes in this patch: chrome/browser/chromeos/disks chrome/browser/chromeos/extensions (and .js) chrome/browser/chromeos/gdata chromeos/dbus
https://chromiumcodereview.appspot.com/10008100/diff/2022/chromeos/dbus/cros_... File chromeos/dbus/cros_disks_client.h (right): https://chromiumcodereview.appspot.com/10008100/diff/2022/chromeos/dbus/cros_... chromeos/dbus/cros_disks_client.h:182: const std::string& source_format, you probably need to modify mock_cros_disks_client.h too.
Hi, I have split this CL into two separate CLs as suggested by Santoru. Unfortunately I'm unable to easily split it into four because the changes in the disks and dbus headers require extensions to be updated also. The two separate CLs are: (1) https://chromiumcodereview.appspot.com/10116044: gdata: add support for the "mounted" state; (2) https://chromiumcodereview.appspot.com/10008100: (this CL): extensions/disks/dbus: passing the archive file format argument, set mounted state accordingly when mounting and unmounting archive files in GData cache;
Sorry, Part#2 of the CL is published at a separate link now:(http://codereview.chromium.org/10083067/). Due to the dependency, I had to rebase part#2 whenever part#1 needs to change. |
