chromeos: Replace ModifyCacheState with a number of small functions to simplify DriveCache
ModifyCacheState was doing a number of things at the same time:
- Move or copy a file. (and copy was only used by Store())
- If the symlink_path is not empty, create or delete symlink
That says, ModifyCacheState is unnecessarily complicated so that it can be dividend into four small functions, MoveFile, CopyFile, CreateSymlink and DeleteSymlink.
BUG=None
TEST=unit_tests --gtest_filter="Drive*"
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166890

Thanks! Now the day has come that I can now understand the code of DriveCache!
I have a few comments. Could you clarify them?
http://codereview.chromium.org/11358157/diff/5001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_cache.cc (right):
http://codereview.chromium.org/11358157/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_cache.cc:652: if (success &&
!symlink_path.empty())
If I understand correctly, cache_entry.is_pinned() is false by default, so we
can safely move |symlink_path| definition and replace this block as:
if (success && cache_entry.is_pinned()) {
FilePath symlink_path = ...
CreateSymlink(...);
}
which looks clearer.
http://codereview.chromium.org/11358157/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_cache.cc:726: dest_path = source_path;
In this case, we don't create symlink, so there's no need to set |dest_path|.
How about deleting this line and bringing the declaration of |source_path| to
the else branch.
http://codereview.chromium.org/11358157/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_cache.cc:727: create_symlink = false;
Well, besides, I have some doubt in the logic here (not the guilt of your
refactoring).
Is !cache_entry.is_present() implies cache_entry.is_pinned()?
If so, we can, and should, just do
DCHECK(cache_entry.is_pinned())
return DRIVE_FILE_OK;
here instead of manipulating |create_simlink|. Otherwise, we set
cache_entry.set_is_persistent(true) below that doesn't fit our comment above.
If not, we should create simlink (to /dev/null), I guess...
Could you clarify the point?