Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10116044:
gdata: Support mounting archive files in GData cache. (Closed)
|
Created:
8 years, 8 months ago by hshi Modified:
8 years, 8 months ago CC:
chromium-reviews, achuith+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Descriptiongdata: Support mounting archive files in GData cache.
Handle the archive file to be mounted like a dirty file, e.g. by moving it to
cache/persistent/<resource_id>.mounted, which prevents it from being modified,
deleted, or moved.
BUG=chromium-os:28678
TEST=Verify that mounting/unmounting archive files on GData works correctly.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133571
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 7
Patch Set 3 : gdata: Support mounting archive files in GData cache. #Patch Set 4 : gdata: Support mounting archive files in GData cache. #
Total comments: 2
Patch Set 5 : gdata: Support mounting archive files in GData cache. #Patch Set 6 : gdata: Support mounting archive files in GData cache. #Patch Set 7 : gdata: Support mounting archive files in GData cache. #
Total comments: 6
Patch Set 8 : gdata: Support mounting archive files in GData cache. #
Total comments: 13
Patch Set 9 : gdata: Support mounting archive files in GData cache. #
Total comments: 3
Patch Set 10 : gdata: Support mounting archive files in GData cache. #Patch Set 11 : gdata: Support mounting archive files in GData cache. #
Total comments: 9
Patch Set 12 : gdata: Support mounting archive files in GData cache. #
Total comments: 16
Patch Set 13 : gdata: Support mounting archive files in GData cache. #Messages
Total messages: 52 (0 generated)
Hi, this CL is split from (https://chromiumcodereview.appspot.com/10008100) as suggested by Satoru. This CL addresses the changes needed in gdata to support the "mounted" state for archive files in GData cache.
i assume u've run the scenarios to mount and unmount. if yes, lgtm, with nits. http://codereview.chromium.org/10116044/diff/1/chrome/browser/chromeos/gdata/... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/1/chrome/browser/chromeos/gdata/... chrome/browser/chromeos/gdata/gdata_file_system.cc:2079: } else if ((to_mount && entry->IsMounted()) || don't need "else", 'cos previous if returns. http://codereview.chromium.org/10116044/diff/1/chrome/browser/chromeos/gdata/... chrome/browser/chromeos/gdata/gdata_file_system.cc:2083: } else { ditto. http://codereview.chromium.org/10116044/diff/1/chrome/browser/chromeos/gdata/... chrome/browser/chromeos/gdata/gdata_file_system.cc:4134: << (entry ? (entry->IsDirty() ? "is dirty" : "is mounted") : nit: u can append "is" to "Entry", less characters :)
http://codereview.chromium.org/10116044/diff/1/chrome/browser/chromeos/gdata/... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/1/chrome/browser/chromeos/gdata/... chrome/browser/chromeos/gdata/gdata_file_system.cc:4134: << (entry ? (entry->IsDirty() ? "is dirty" : "is mounted") : Fixed with patch set #2, can you review, thanks! On 2012/04/18 22:57:40, kuan wrote: > nit: u can append "is" to "Entry", less characters :)
LGTM.
Can you write some unit tests? We usually write unit tests when adding new features. http://codereview.chromium.org/10116044/diff/5001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/5001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2043: base::Bind(&GDataFileSystem::SetMountedStateOnIOThreadPool, It's sad to see a new non-static member function runs on the thread pool. I'm planning to get rid of all of them. However, there doesn't seem to be an easy way to avoid doing this, so I'm fine with that at this moment... http://codereview.chromium.org/10116044/diff/5001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2084: } nit: maybe add a blank line here? The code here looks a bit too packed. http://codereview.chromium.org/10116044/diff/5001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2097: CACHED_FILE_MOUNTED); add a blank line? http://codereview.chromium.org/10116044/diff/5001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2112: } blank line? http://codereview.chromium.org/10116044/diff/5001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10116044/diff/5001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.h:404: // Mark or unmark a file as locally mounted. nit: Marks or Unmarks
http://codereview.chromium.org/10116044/diff/5001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/5001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2080: if ((to_mount && entry->IsMounted()) || if (to_mount == entry->IsMounted()) http://codereview.chromium.org/10116044/diff/5001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2103: source_path = unmounted_path; I would find source_path = to_mount ? unmounted_path : mounted_path; etc. more readable (but I'm fine with this too)
On 2012/04/18 23:43:14, tbarzic wrote: > I would find > source_path = to_mount ? unmounted_path : mounted_path; > etc. > more readable (but I'm fine with this too) That would also be my preference usually, unfortunately in this case there are 4 statements, the last of which chooses between SetMountedState and ClearMountedState. I tried to change it in the format of "to_mount ? A : B" but the line is too long and the whole thing just looks a bit awkward.
Started tryjobs on behalf. FWIW, here's the command lines: % trychange.py -R codereview.chromium.org/10116044 --email hshi@chromium.org,satorux@chromium.org --name gdata --root src -p1 % trychange.py -R codereview.chromium.org/10116044 --email hshi@chromium.org,satorux@chromium.org --name gdata --root src -p1 --bot=linux_chromeos --bot=linux_chromeos_clang The first command line was to run mac_rel win_rel linux_rel linux_clang, but these were irrelevant, as the code here only compiles on chromeos. The second command line was to run linux_chromeos and linux_chromeos_clang.
It appears that try build has failed with linux_chromeos_clang: need changes in
mock file system declarations. I'll combine the fix for these together with the
unit test changes then update.
chrome/browser/chromeos/gdata/gdata_sync_client_unittest.cc:36:31: error:
allocating an object of abstract class type 'MockGDataFileSystem'
mock_file_system_(new MockGDataFileSystem),
^
./chrome/browser/chromeos/gdata/gdata_file_system.h:367:16: note: unimplemented
pure virtual method 'IsUnderGDataCacheDirectory' in 'MockGDataFileSystem'
virtual bool IsUnderGDataCacheDirectory(const FilePath& path) const = 0;
^
./chrome/browser/chromeos/gdata/gdata_file_system.h:407:16: note: unimplemented
pure virtual method 'SetMountedState' in 'MockGDataFileSystem'
virtual void SetMountedState(const FilePath& file_path, bool to_mount,
^
On 2012/04/19 00:21:57, satorux1 wrote:
> Started tryjobs on behalf. FWIW, here's the command lines:
>
> % trychange.py -R codereview.chromium.org/10116044 --email
> mailto:hshi@chromium.org,satorux@chromium.org --name gdata --root src -p1
>
> % trychange.py -R codereview.chromium.org/10116044 --email
> mailto:hshi@chromium.org,satorux@chromium.org --name gdata --root src -p1
> --bot=linux_chromeos --bot=linux_chromeos_clang
>
> The first command line was to run mac_rel win_rel linux_rel linux_clang, but
> these were irrelevant, as the code here only compiles on chromeos.
>
> The second command line was to run linux_chromeos and linux_chromeos_clang.
i'm gonna have to hold back the lgtm, i apolgoize. i forgot 2 things: 1) you need to modify ScanCacheDirectory to handle scanning of mounted files and set the state in CacheEntry. 2) because of 1, files that are already mounted when u start chrome will not have a md5 extension, so in SetMountedStateOnIOThreadPool, u shld GetCacheEntry with an empty md5 (pls comment why). for unittest reference, u can search for "DirtyCache" in file.
For files already mounted when I start chrome, is there any way to recover the md5 (for example by re-calculating md5 hash of the file)? Or is this value lost forever? On 2012/04/19 11:03:00, kuan wrote: > i'm gonna have to hold back the lgtm, i apolgoize. > > i forgot 2 things: > 1) you need to modify ScanCacheDirectory to handle scanning of mounted files and > set the state in CacheEntry. > 2) because of 1, files that are already mounted when u start chrome will not > have a md5 extension, so in SetMountedStateOnIOThreadPool, u shld GetCacheEntry > with an empty md5 (pls comment why). > > for unittest reference, u can search for "DirtyCache" in file.
On 2012/04/19 17:35:49, hshi wrote: > For files already mounted when I start chrome, is there any way to recover the > md5 (for example by re-calculating md5 hash of the file)? Or is this value lost > forever? we don't calculate the md5; it's provided by GData server. it's lost in cache's CacheEntry, but not in GDataFile. > On 2012/04/19 11:03:00, kuan wrote: > > i'm gonna have to hold back the lgtm, i apolgoize. > > > > i forgot 2 things: > > 1) you need to modify ScanCacheDirectory to handle scanning of mounted files > and > > set the state in CacheEntry. > > 2) because of 1, files that are already mounted when u start chrome will not > > have a md5 extension, so in SetMountedStateOnIOThreadPool, u shld > GetCacheEntry > > with an empty md5 (pls comment why). > > > > for unittest reference, u can search for "DirtyCache" in file.
i guess u're asking because u want to rename it back to .md5 extension on unmounting. that md5 can be passed into ur SetMountedState from GDataFile::file_md5().
Here's an experiment I just did: force shut down a device with an archive already mounted. When I power it back on, previously mounted archives (whether on local Downloads/ or on GData) are no longer mounted -- File Manager no longer recognizes the archive as being mounted because the mount point is lost and DiskMountManager no longer has the mount point info in its records. Thus I think it would be insufficient to set the cache entry with "mounted" state in ScanCacheDirectory. I can't think of a clean solution for this situation -- maybe I can move it back to GCache/v1/tmp/? On 2012/04/19 17:41:21, kuan wrote: > i guess u're asking because u want to rename it back to .md5 extension on > unmounting. that md5 can be passed into ur SetMountedState from > GDataFile::file_md5().
On 2012/04/19 18:05:41, hshi wrote: > Here's an experiment I just did: force shut down a device with an archive > already mounted. When I power it back on, previously mounted archives (whether > on local Downloads/ or on GData) are no longer mounted -- File Manager no longer > recognizes the archive as being mounted because the mount point is lost and > DiskMountManager no longer has the mount point info in its records. > > Thus I think it would be insufficient to set the cache entry with "mounted" > state in ScanCacheDirectory. I can't think of a clean solution for this > situation -- maybe I can move it back to GCache/v1/tmp/? > > On 2012/04/19 17:41:21, kuan wrote: > > i guess u're asking because u want to rename it back to .md5 extension on > > unmounting. that md5 can be passed into ur SetMountedState from > > GDataFile::file_md5(). Yeah, in theory there shouldn't be any mounted devices/archives when chrome starts, they all get unmounted when cros-disks shutdown. We didn't care about this too much on chrome side since mount state would not persist after shutdown. But we probably have to take this into account for gdata archives (since mount state is preserved in cache). I guess resetting mount state on initial cache scan should work, but kuan will know better :)
On 2012/04/19 18:14:22, tbarzic wrote: > On 2012/04/19 18:05:41, hshi wrote: > > Here's an experiment I just did: force shut down a device with an archive > > already mounted. When I power it back on, previously mounted archives (whether > > on local Downloads/ or on GData) are no longer mounted -- File Manager no > longer > > recognizes the archive as being mounted because the mount point is lost and > > DiskMountManager no longer has the mount point info in its records. > > > > Thus I think it would be insufficient to set the cache entry with "mounted" > > state in ScanCacheDirectory. I can't think of a clean solution for this > > situation -- maybe I can move it back to GCache/v1/tmp/? > > > > On 2012/04/19 17:41:21, kuan wrote: > > > i guess u're asking because u want to rename it back to .md5 extension on > > > unmounting. that md5 can be passed into ur SetMountedState from > > > GDataFile::file_md5(). > > Yeah, in theory there shouldn't be any mounted devices/archives when chrome > starts, they all get unmounted when cros-disks shutdown. We didn't care about > this too much on chrome side since mount state would not persist after shutdown. > But we probably have to take this into account for gdata archives (since mount > state is preserved in cache). > > I guess resetting mount state on initial cache scan should work, but kuan will > know better :) i wld think ben or zel shld chime in on this. to cache, once the extension is .mounted, it's mounted. i don't know how ben or zel want to handle the restart case.
On 2012/04/19 18:17:06, kuan wrote: > On 2012/04/19 18:14:22, tbarzic wrote: > > On 2012/04/19 18:05:41, hshi wrote: > > > Here's an experiment I just did: force shut down a device with an archive > > > already mounted. When I power it back on, previously mounted archives > (whether > > > on local Downloads/ or on GData) are no longer mounted -- File Manager no > > longer > > > recognizes the archive as being mounted because the mount point is lost and > > > DiskMountManager no longer has the mount point info in its records. > > > > > > Thus I think it would be insufficient to set the cache entry with "mounted" > > > state in ScanCacheDirectory. I can't think of a clean solution for this > > > situation -- maybe I can move it back to GCache/v1/tmp/? > > > > > > On 2012/04/19 17:41:21, kuan wrote: > > > > i guess u're asking because u want to rename it back to .md5 extension on > > > > unmounting. that md5 can be passed into ur SetMountedState from > > > > GDataFile::file_md5(). > > > > Yeah, in theory there shouldn't be any mounted devices/archives when chrome > > starts, they all get unmounted when cros-disks shutdown. We didn't care about > > this too much on chrome side since mount state would not persist after > shutdown. > > But we probably have to take this into account for gdata archives (since mount > > state is preserved in cache). > > > > I guess resetting mount state on initial cache scan should work, but kuan will > > know better :) > > i wld think ben or zel shld chime in on this. to cache, once the extension is > .mounted, it's mounted. i don't know how ben or zel want to handle the restart > case. I agree with Toni's suggestion on resetting any (unclean) mount state during the initial cache scan. In addition, we also need to make sure the file manager unmounts all archives and resets the mount state upon logout/shutdown.
if we reset all mount points on logout/shutdown, then we probably need to unmount the file in cache on logout/shutdown too. that means the cache will never have .mounted files on chrome start. On Thu, Apr 19, 2012 at 11:28 AM, <benchan@chromium.org> wrote: > On 2012/04/19 18:17:06, kuan wrote: > >> On 2012/04/19 18:14:22, tbarzic wrote: >> > On 2012/04/19 18:05:41, hshi wrote: >> > > Here's an experiment I just did: force shut down a device with an >> archive >> > > already mounted. When I power it back on, previously mounted archives >> (whether >> > > on local Downloads/ or on GData) are no longer mounted -- File >> Manager no >> > longer >> > > recognizes the archive as being mounted because the mount point is >> lost >> > and > >> > > DiskMountManager no longer has the mount point info in its records. >> > > >> > > Thus I think it would be insufficient to set the cache entry with >> > "mounted" > >> > > state in ScanCacheDirectory. I can't think of a clean solution for >> this >> > > situation -- maybe I can move it back to GCache/v1/tmp/? >> > > >> > > On 2012/04/19 17:41:21, kuan wrote: >> > > > i guess u're asking because u want to rename it back to .md5 >> extension >> > on > >> > > > unmounting. that md5 can be passed into ur SetMountedState from >> > > > GDataFile::file_md5(). >> > >> > Yeah, in theory there shouldn't be any mounted devices/archives when >> chrome >> > starts, they all get unmounted when cros-disks shutdown. We didn't care >> > about > >> > this too much on chrome side since mount state would not persist after >> shutdown. >> > But we probably have to take this into account for gdata archives (since >> > mount > >> > state is preserved in cache). >> > >> > I guess resetting mount state on initial cache scan should work, but >> kuan >> > will > >> > know better :) >> > > i wld think ben or zel shld chime in on this. to cache, once the >> extension is >> .mounted, it's mounted. i don't know how ben or zel want to handle the >> > restart > >> case. >> > > I agree with Toni's suggestion on resetting any (unclean) mount state > during the > initial cache scan. In addition, we also need to make sure the file manager > unmounts all archives and resets the mount state upon logout/shutdown. > > http://codereview.chromium.**org/10116044/<http://codereview.chromium.org/101... >
Here's a cheap-and-quick solution suggested by Satoru: (1) When mounting, copy, instead of move, the cached blob to persistent/<resource_id>.mounted, and mount that path (2) When unmounting, move the cached blob back, possibly overwriting the original file in tmp/ if it had not been evicted (3) When freshly booting up the machine, simply discard any persistent/<resource_id>.mounted blobs. If it is present in tmp/, then mark it present as normal, otherwise it does not exist.
Hi all, can you please review Patch Set #4 that implements Satoru's suggestion. I have verified that it cleanly discards the mounted cache blobs at boot-up. (TODO: unit tests are still not in this patch set) The downside of making a copy is that it may be slow for large archives. The sole reason for the copy is to preserve the md5 in the filename, which makes it a bit silly. I wonder if we can modify the naming scheme so that mounted files still have the md5 component (for example, persistent/<resource_id>.<md5>_mounted). It would complicate the parsing of the file extension in ScanCacheDirectory() but it will avoid a copy.
http://codereview.chromium.org/10116044/diff/13003/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/13003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2121: // When unmounting, move the file back to replace the original blob maybe comment that u're moving in case it was evicted.
http://codereview.chromium.org/10116044/diff/13003/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/13003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2073: if (md5.compare(kMountedArchiveFileExtension) == 0) { i'm wondering if u can just do md5 == kMountedArchiveFileExtension?
Sure I'll do that. I wasn't exactly sure std::string's operator == works as strcmp (or just compares reference) On 2012/04/19 21:02:15, kuan wrote: > http://codereview.chromium.org/10116044/diff/13003/chrome/browser/chromeos/gd... > File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): > > http://codereview.chromium.org/10116044/diff/13003/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_file_system.cc:2073: if > (md5.compare(kMountedArchiveFileExtension) == 0) { > i'm wondering if u can just do md5 == kMountedArchiveFileExtension?
Hi, I have uploaded Patch Set #5 with the following changes: (1) Added unit test for SetMountedState; (2) To solve the "missing md5" problem, change the filename for mounted blob to "resource_id.md5.mounted" as suggested by Zel. This eliminates the need to make a copy and simplifies handling of ScanCacheDirectory.
Sorry but there is one more issue. Please see changes in Patch Set #6. Issue is: if a mounted archive is also pinned right before logout/shutdown, then when we login/reboot, ScanCacheDirectory needs to move the blob to persistent/<resource_id>.<md5> instead of tmp/<resource_id>.<md5>. Unfortuatenly we could not tell which directory to move to based on the mounted file path (persistent/<resource_id>.<md5>.mounted). Thus we have to check again and perform the move when handling the symlinks under the PINNED subdirectory.
Uploaded Patch Set #7 due to conflicts with TOT (GDataFileBase got renamed to GDataEntry last evening in another CL). Thanks. On 2012/04/19 23:59:41, hshi wrote: > Sorry but there is one more issue. Please see changes in Patch Set #6. > > Issue is: if a mounted archive is also pinned right before logout/shutdown, then > when we login/reboot, ScanCacheDirectory needs to move the blob to > persistent/<resource_id>.<md5> instead of tmp/<resource_id>.<md5>. Unfortuatenly > we could not tell which directory to move to based on the mounted file path > (persistent/<resource_id>.<md5>.mounted). Thus we have to check again and > perform the move when handling the symlinks under the PINNED subdirectory.
http://codereview.chromium.org/10116044/diff/31003/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/31003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2143: FilePath::StringType md5 = FilePath::StringType(); nit: the following is sufficient FilePath::StringType md5; http://codereview.chromium.org/10116044/diff/31003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2160: } This function is already large. can you factor out this part into a separate function, like: if (ParseCacheFileName(file_path, &resource_id, &md5, &extra_suffix)) { ... } where extra_suffix returns "mounted", so you can do something like DCHECK(!to_mount || extra_suffix == "mounted"); You can put this function gdata_util.cc and write a unit test for it. http://codereview.chromium.org/10116044/diff/31003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:4388: { nit: move { to the previous line. http://codereview.chromium.org/10116044/diff/31003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:4390: // suffix ".mounted" and moved to tmp. Move it again to persistent. I'm confused. when can it happen?
http://codereview.chromium.org/10116044/diff/31003/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/31003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:4390: // suffix ".mounted" and moved to tmp. Move it again to persistent. On 2012/04/20 20:11:25, satorux1 wrote: > I'm confused. when can it happen? This happens when a zip file is pinned and also mounted at the last signout/shutdown. The next time chrome starts, there will be a mounted blob at persistent/resource_id.md5.mounted and a symlink under pinned/, but ScanCacheDirectory is called first on the TMP/PERSISTENT subdirs and then on the OUTGOING/PINNED. For the call on PERSISTENT, the code block I just added below will strip the ".mounted" suffix and move the blob to tmp/ (because there is no way to tell that it is a pinned file until we scan the pinned subdir). Later when we arrive here, we will realize that the file got moved to the wrong directory, hence moving it back to persistent).
Please review Patch Set #8. This refactors the ParseCacheFilePath code, thanks! On 2012/04/20 20:11:25, satorux1 wrote: http://codereview.chromium.org/10116044/diff/31003/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_file_system.cc:2160: } > This function is already large. can you factor out this part into a separate > function, like: > > if (ParseCacheFileName(file_path, &resource_id, &md5, &extra_suffix)) { > ... > } > > > where extra_suffix returns "mounted", so you can do something like > > DCHECK(!to_mount || extra_suffix == "mounted");
http://codereview.chromium.org/10116044/diff/31003/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/31003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:4390: // suffix ".mounted" and moved to tmp. Move it again to persistent. On 2012/04/20 20:20:30, hshi wrote: > On 2012/04/20 20:11:25, satorux1 wrote: > > I'm confused. when can it happen? > > This happens when a zip file is pinned and also mounted at the last > signout/shutdown. The next time chrome starts, there will be a mounted blob at > persistent/resource_id.md5.mounted and a symlink under pinned/, but > ScanCacheDirectory is called first on the TMP/PERSISTENT subdirs and then on the > OUTGOING/PINNED. For the call on PERSISTENT, the code block I just added below > will strip the ".mounted" suffix and move the blob to tmp/ (because there is no > way to tell that it is a pinned file until we scan the pinned subdir). Later > when we arrive here, we will realize that the file got moved to the wrong > directory, hence moving it back to persistent). Oh I see. This is fairly tricky and hard to test... can we make it simpler? I'd suggest to just remove the .mounted file. After all, this situation (shut down before unmounting) rarely happens, and we can recover this by fetch the file again. If the file is pinned, GDataSyncClient will fetch it automatically if not cached locally. http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:4344: util::ParseCacheFilePath(current, &resource_id, &md5, &extra_suffix); oh this turned out to be useful elsewhere. cool. http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util.cc (right): http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:269: // The extra_suffix is only non-empty for mounted archives. We usually don't have a function comment in .cc file, if we have it in .h file. Please move this to .h file. http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:274: { move { to the previous line. http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:281: int num_extensions; please initialize this with 0, to be extra defensive. http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:282: FilePath::StringType extension[2]; extension -> extensions http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:283: for (num_extensions = 0; num_extensions < 2; ++num_extensions) { rather than 2, please do arraysize(extensions) http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:287: extension[num_extensions] = instead of using a fixed array, using vector<> and add it by push_back() would be cleaner. then you can get rid of num_extensions. const int kNumExtensionsToExtract = 2; for (int i = 0; i < kNumExtensionsToExtract; ++i) { http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util.h (right): http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.h:66: // Extracts resource_id, md5, and extra_suffix from cache path. would be nice to add some example of input and output to make it clear what this function does. for instance, it'd be important to tell that |extra_suffix| contains '.' or not. http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.h:70: std::string* extra_suffix); i suggested "suffix", but "extension" would be better to make it consistent with the terminology used elsewhere. http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util_unittest.cc (right): http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util_unittest.cc:62: EXPECT_EQ(resource_id, "pdf:a1b2"); seems to me that "pdf:" shouldn't be part of the resource id.
http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util_unittest.cc (right): http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util_unittest.cc:62: EXPECT_EQ(resource_id, "pdf:a1b2"); I also assumed "pdf:" is not part of resource id initially, but it looks like our TOT cache codebase indeed treats it as part of resource id string. See gdata_file_system_unittest.cc: RemoveFromDirtyCache for example. On 2012/04/20 21:46:10, satorux1 wrote: > seems to me that "pdf:" shouldn't be part of the resource id.
http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util_unittest.cc (right): http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util_unittest.cc:62: EXPECT_EQ(resource_id, "pdf:a1b2"); On 2012/04/20 22:04:51, hshi wrote: > I also assumed "pdf:" is not part of resource id initially, but it looks like > our TOT cache codebase indeed treats it as part of resource id string. See > gdata_file_system_unittest.cc: RemoveFromDirtyCache for example. > > On 2012/04/20 21:46:10, satorux1 wrote: > > seems to me that "pdf:" shouldn't be part of the resource id. > You are right. would be nice to mention it in the function comment in the header file.
Please see Patch Set #9, thanks! On 2012/04/20 22:27:20, satorux1 wrote: > http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... > File chrome/browser/chromeos/gdata/gdata_util_unittest.cc (right): > > http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_util_unittest.cc:62: EXPECT_EQ(resource_id, > "pdf:a1b2"); > On 2012/04/20 22:04:51, hshi wrote: > > I also assumed "pdf:" is not part of resource id initially, but it looks like > > our TOT cache codebase indeed treats it as part of resource id string. See > > gdata_file_system_unittest.cc: RemoveFromDirtyCache for example. > > > > On 2012/04/20 21:46:10, satorux1 wrote: > > > seems to me that "pdf:" shouldn't be part of the resource id. > > > > You are right. would be nice to mention it in the function comment in the header > file.
http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util.h (right): http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.h:70: std::string* extra_suffix); On 2012/04/20 21:46:10, satorux1 wrote: > i suggested "suffix", but "extension" would be better to make it consistent with > the terminology used elsewhere. missed this comment? http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:4381: file_util::Delete(current, false); Then, the symlink in "pinned" directory will point to a non-existent file, right? We should change GDataSyncClient to handle such a dangling symlink properly. As of now, it deletes dangling symlinks: // Remove the symbolic link if it's dangling. Something went wrong. if (!file_util::PathExists(destination)) { LOG(WARNING) << "Removing " << file_path.value() << " (dangling)"; file_util::Delete(file_path, false /* recursive */); continue; } Can you change this code? It'd be nicer to do it in a separate patch, as this patch is reasonably big. http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util.cc (right): http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:298: { move { to the previous line. http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:313: break; add default: NOTREACHED();
Hi Satoru, I uploaded Patch Set #10 to address the issues: rename "extra_suffix" to "extra_extension"; fix the coding style issues; adding "NOTREATCHED()" for the default case. Note that since we added the NOTREATCHED() guard, I think I can safely remove the DCHECK for extensions.size() to be in the [0..2] range. As for the dangling symlink logic, can I do that in a separate CL? I think for files already mounted when chrome starts, most likely the file is not pinned. For pinned file I agree that GDataSyncClient would delete the symlink. On 2012/04/20 23:05:39, satorux1 wrote: > http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... > File chrome/browser/chromeos/gdata/gdata_util.h (right): > > http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_util.h:70: std::string* extra_suffix); > On 2012/04/20 21:46:10, satorux1 wrote: > > i suggested "suffix", but "extension" would be better to make it consistent > with > > the terminology used elsewhere. > > missed this comment? > > http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gd... > File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): > > http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_file_system.cc:4381: > file_util::Delete(current, false); > Then, the symlink in "pinned" directory will point to a non-existent file, > right? We should change GDataSyncClient to handle such a dangling symlink > properly. As of now, it deletes dangling symlinks: > > // Remove the symbolic link if it's dangling. Something went wrong. > if (!file_util::PathExists(destination)) { > LOG(WARNING) << "Removing " << file_path.value() << " (dangling)"; > file_util::Delete(file_path, false /* recursive */); > continue; > } > > Can you change this code? It'd be nicer to do it in a separate patch, as this > patch is reasonably big. > > http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gd... > File chrome/browser/chromeos/gdata/gdata_util.cc (right): > > http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_util.cc:298: { > move { to the previous line. > > http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_util.cc:313: break; > add > > default: > NOTREACHED();
I'm afraid that this last patch set is failing the InitializeCache test.
Problem is that one of the test cases intentionally uses non-alpha-numeric
resource_id's that contain the "." character:
const initial_cache_resources[] = {
...
// Cache resource in tmp dir, i.e. not pinned or dirty, with resource_id
// containing non-alphanumeric characters, to test resource_id is escaped
and
// unescaped correctly.
{ "subdir_feed.json", "tmp:`~!@#$%^&*()-_=+[{|]}\\;',<.>/?",
"md5_tmp_non_alphanumeric",
This causes the ParseCacheFilePath to incorrectly get two extension strings
from the filepath.
Is this a valid case? If so then it would require the
util::ParseCacheFilePath to be aware that the extra_extension can only be
equal to the "mounted" string.
On Fri, Apr 20, 2012 at 4:28 PM, <hshi@google.com> wrote:
> Hi Satoru, I uploaded Patch Set #10 to address the issues: rename
> "extra_suffix"
> to "extra_extension"; fix the coding style issues; adding "NOTREATCHED()"
> for
> the default case. Note that since we added the NOTREATCHED() guard, I
> think I
> can safely remove the DCHECK for extensions.size() to be in the [0..2]
> range.
>
> As for the dangling symlink logic, can I do that in a separate CL? I think
> for
> files already mounted when chrome starts, most likely the file is not
> pinned.
> For pinned file I agree that GDataSyncClient would delete the symlink.
>
>
> On 2012/04/20 23:05:39, satorux1 wrote:
>
> http://codereview.chromium.**org/10116044/diff/52002/**
>
chrome/browser/chromeos/gdata/**gdata_util.h<http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gdata/gdata_util.h>
>
>> File chrome/browser/chromeos/gdata/**gdata_util.h (right):
>>
>
>
> http://codereview.chromium.**org/10116044/diff/52002/**
>
chrome/browser/chromeos/gdata/**gdata_util.h#newcode70<http://codereview.chromium.org/10116044/diff/52002/chrome/browser/chromeos/gdata/gdata_util.h#newcode70>
>
>> chrome/browser/chromeos/gdata/**gdata_util.h:70: std::string*
>> extra_suffix);
>> On 2012/04/20 21:46:10, satorux1 wrote:
>> > i suggested "suffix", but "extension" would be better to make it
>> consistent
>> with
>> > the terminology used elsewhere.
>>
>
> missed this comment?
>>
>
>
> http://codereview.chromium.**org/10116044/diff/46003/**
>
chrome/browser/chromeos/gdata/**gdata_file_system.cc<http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gdata/gdata_file_system.cc>
>
>> File chrome/browser/chromeos/gdata/**gdata_file_system.cc (right):
>>
>
>
> http://codereview.chromium.**org/10116044/diff/46003/**
>
chrome/browser/chromeos/gdata/**gdata_file_system.cc#**newcode4381<http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gdata/gdata_file_system.cc#newcode4381>
>
>> chrome/browser/chromeos/gdata/**gdata_file_system.cc:4381:
>> file_util::Delete(current, false);
>> Then, the symlink in "pinned" directory will point to a non-existent file,
>> right? We should change GDataSyncClient to handle such a dangling symlink
>> properly. As of now, it deletes dangling symlinks:
>>
>
> // Remove the symbolic link if it's dangling. Something went wrong.
>>
>
> if (!file_util::PathExists(**destination)) {
>> LOG(WARNING) << "Removing " << file_path.value() << " (dangling)";
>> file_util::Delete(file_path, false /* recursive */);
>> continue;
>> }
>>
>
> Can you change this code? It'd be nicer to do it in a separate patch, as
>> this
>> patch is reasonably big.
>>
>
>
> http://codereview.chromium.**org/10116044/diff/46003/**
>
chrome/browser/chromeos/gdata/**gdata_util.cc<http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gdata/gdata_util.cc>
>
>> File chrome/browser/chromeos/gdata/**gdata_util.cc (right):
>>
>
>
> http://codereview.chromium.**org/10116044/diff/46003/**
>
chrome/browser/chromeos/gdata/**gdata_util.cc#newcode298<http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gdata/gdata_util.cc#newcode298>
>
>> chrome/browser/chromeos/gdata/**gdata_util.cc:298: {
>> move { to the previous line.
>>
>
>
> http://codereview.chromium.**org/10116044/diff/46003/**
>
chrome/browser/chromeos/gdata/**gdata_util.cc#newcode313<http://codereview.chromium.org/10116044/diff/46003/chrome/browser/chromeos/gdata/gdata_util.cc#newcode313>
>
>> chrome/browser/chromeos/gdata/**gdata_util.cc:313: break;
>> add
>>
>
> default:
>> NOTREACHED();
>>
>
>
>
>
http://codereview.chromium.**org/10116044/<http://codereview.chromium.org/101...
>
On 2012/04/21 00:02:04, hshi wrote: > I'm afraid that this last patch set is failing the InitializeCache test. > > Problem is that one of the test cases intentionally uses non-alpha-numeric > resource_id's that contain the "." character: I don't think this is a valid use case. We rely on that the resource ID does not contain '.' (which is rather fragile).
Kuan, are you ok with the assumption that resource_id will never contain the "."
character? We're kind of relying on this assumption to differentiate between the
two-part filename format ("<resource_id>.<md5>") and the three-part filename
format ("<resource_id>.<md5>.mounted") in gdata::util::ParseCacheFilePath.
Thanks!
Uploaded Patch Set #11 that tentatively removes the "." character from the test
case.
On 2012/04/21 00:12:49, satorux1 wrote:
> On 2012/04/21 00:02:04, hshi wrote:
> > I'm afraid that this last patch set is failing the InitializeCache test.
> >
> > Problem is that one of the test cases intentionally uses non-alpha-numeric
> > resource_id's that contain the "." character:
>
> I don't think this is a valid use case. We rely on that the resource ID does
not
> contain '.' (which is rather fragile).
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Satoru, can I commit this change now? I have got all the try tests passing. Need an LGTM with commiter priviledge. Thanks!
http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2145: // The extra_extension can only be ".mounted" when unmounting. The comment is a bit confusing. Can you put "If we're unmounting the extra extension must be ".mounted"." (Or something similar) Btw. can the extension be .mounted if we are doing mount operation (if not, we may want to check that too)? http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2163: GDataRootDirectory::CACHE_TYPE_TMP; Optional (but really optional:)): I think rearranging this block of code like this would make it easier to follow: unmount_subdir =… unmounted_path =… mount_subdir =… mounted_path =… http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.h:1214: // - moves |source_path| to |dest_path| what are |source_path| and |dest_path|? http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util.cc (right): http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:295: switch (extensions.size()) { What do you think about this: size_t extension_count = extensions.size(); if (extension_count > 0) *md5 = extensions[extension_count -1]; if (extension_count > 1) *extra_extension = extensions[extension_count - 2];
http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2145: // The extra_extension can only be ".mounted" when unmounting. How about the following // The extra_extension shall be ".mounted" if and only if we're unmounting. DCHECK(!to_mount == (extra_extension == kMountedArchiveFileExtension)); On 2012/04/23 18:03:22, tbarzic wrote: > The comment is a bit confusing. Can you put "If we're unmounting the extra > extension must be ".mounted"." (Or something similar) > Btw. can the extension be .mounted if we are doing mount operation (if not, we > may want to check that too)?
http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.h:1214: // - moves |source_path| to |dest_path| On 2012/04/23 18:03:22, tbarzic wrote: > what are |source_path| and |dest_path|? In the case we're mounting, the |source_path| is the original (unmounted) path to the cache blob, and |dest_path| is the mounted path; if we're unmounting it is the opposite. Do you want me to put more explanation here? I'm just following the examples such as the comments in UnpinOnIOThreadPool.
btw, why am I suddenly the only reviewer here (adding back satorux and kuan) http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:2145: // The extra_extension can only be ".mounted" when unmounting. sg (you could probably write iff instead of "if and only if") On 2012/04/23 18:56:22, hshi wrote: > How about the following > > // The extra_extension shall be ".mounted" if and only if we're unmounting. > DCHECK(!to_mount == (extra_extension == kMountedArchiveFileExtension)); > > On 2012/04/23 18:03:22, tbarzic wrote: > > The comment is a bit confusing. Can you put "If we're unmounting the extra > > extension must be ".mounted"." (Or something similar) > > Btw. can the extension be .mounted if we are doing mount operation (if not, we > > may want to check that too)? >
Please review Patch Set #12. This addresses the remaining comments and adds separate escape/unescape routines for gdata cache filenames (instead of replacing slash '/' with unicode, this does the '%XX' (hex)-style escaping). http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.h:1214: // - moves |source_path| to |dest_path| On 2012/04/23 19:01:09, hshi wrote: > On 2012/04/23 18:03:22, tbarzic wrote: > > what are |source_path| and |dest_path|? > > In the case we're mounting, the |source_path| is the original (unmounted) path > to the cache blob, and |dest_path| is the mounted path; if we're unmounting it > is the opposite. > > Do you want me to put more explanation here? I'm just following the examples > such as the comments in UnpinOnIOThreadPool. Done. http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util.cc (right): http://codereview.chromium.org/10116044/diff/48029/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:295: switch (extensions.size()) { On 2012/04/23 18:03:22, tbarzic wrote: > What do you think about this: > size_t extension_count = extensions.size(); > if (extension_count > 0) > *md5 = extensions[extension_count -1]; > if (extension_count > 1) > *extra_extension = extensions[extension_count - 2]; Done.
http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util.cc (right): http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:272: for (unsigned int i = 0; i < filename.length(); ++i) { unsigned int -> size_t length() -> size() - size() is more common. http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:277: escaped.push_back(kHexString[c & 0xf]); base::StringAppendF(&escaped, "%%%02X", c) may be a bit simpler. http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:287: for (unsigned int i = 0; i < filename.length(); ++i) { size_t and size() http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:288: unsigned char c = static_cast<unsigned char>(filename[i]); you probably don't need to cast this to unsigned. http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:291: HexDigitToInt(filename[i + 2]); one more space to align two lines vertically? http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util_unittest.cc (right): http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util_unittest.cc:55: std::string filename_unescaped("tmp:`~!@#$%^&*()-_=+[{|]}\\\\;\',<.>/?"); const std::string kUnescapedFileName http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util_unittest.cc:56: std::string filename_escaped("tmp:`~!@#$%25^&*()-_=+[{|]}\\\\;\',<%2E>%2F?"); ditto
http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util_unittest.cc (right): http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util_unittest.cc:55: std::string filename_unescaped("tmp:`~!@#$%^&*()-_=+[{|]}\\\\;\',<.>/?"); On 2012/04/23 21:02:25, satorux1 wrote: > const std::string kUnescapedFileName Satoru, it seems that we only declare char[] strings this way (for example, const char kSymLinkToDevNull[] = "/dev/null";). The unit tests in gdata_file_system_unittest.cc declare the std::string as local vars. I think it would be better to conform to what the other code is doing?
http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util_unittest.cc (right): http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util_unittest.cc:55: std::string filename_unescaped("tmp:`~!@#$%^&*()-_=+[{|]}\\\\;\',<.>/?"); On 2012/04/23 21:47:37, hshi wrote: > On 2012/04/23 21:02:25, satorux1 wrote: > > const std::string kUnescapedFileName > > Satoru, it seems that we only declare char[] strings this way (for example, > const char kSymLinkToDevNull[] = "/dev/null";). The unit tests in > gdata_file_system_unittest.cc declare the std::string as local vars. I think it > would be better to conform to what the other code is doing? kSomethingLike this is a naming convention used for constants. Here, the two strings seem to be constants.
Please review Patch Set #13. Sorry that there are many bogus diffs because of merge conflicts with TOT causing try failures. http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util.cc (right): http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:272: for (unsigned int i = 0; i < filename.length(); ++i) { On 2012/04/23 21:02:25, satorux1 wrote: > unsigned int -> size_t > > length() -> size() - size() is more common. Done. http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:277: escaped.push_back(kHexString[c & 0xf]); On 2012/04/23 21:02:25, satorux1 wrote: > base::StringAppendF(&escaped, "%%%02X", c) may be a bit simpler. Done. http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:287: for (unsigned int i = 0; i < filename.length(); ++i) { On 2012/04/23 21:02:25, satorux1 wrote: > size_t and size() Done. http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:288: unsigned char c = static_cast<unsigned char>(filename[i]); On 2012/04/23 21:02:25, satorux1 wrote: > you probably don't need to cast this to unsigned. Done. http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util.cc:291: HexDigitToInt(filename[i + 2]); On 2012/04/23 21:02:25, satorux1 wrote: > one more space to align two lines vertically? Done. http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_util_unittest.cc (right): http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util_unittest.cc:55: std::string filename_unescaped("tmp:`~!@#$%^&*()-_=+[{|]}\\\\;\',<.>/?"); On 2012/04/23 21:49:07, satorux1 wrote: > On 2012/04/23 21:47:37, hshi wrote: > > On 2012/04/23 21:02:25, satorux1 wrote: > > > const std::string kUnescapedFileName > > > > Satoru, it seems that we only declare char[] strings this way (for example, > > const char kSymLinkToDevNull[] = "/dev/null";). The unit tests in > > gdata_file_system_unittest.cc declare the std::string as local vars. I think > it > > would be better to conform to what the other code is doing? > > kSomethingLike this is a naming convention used for constants. Here, the two > strings seem to be constants. Done. http://codereview.chromium.org/10116044/diff/64002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_util_unittest.cc:56: std::string filename_escaped("tmp:`~!@#$%25^&*()-_=+[{|]}\\\\;\',<%2E>%2F?"); On 2012/04/23 21:02:25, satorux1 wrote: > ditto Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@google.com/10116044/30016
Change committed as 133571 |
