chromeos: Replace ModifyCacheState with a number of small functions to simplify DriveCache

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

[hashimoto, 2012-11-09 07:10:16]

Please review.

[kinaba, 2012-11-09 08:10:22]

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?

[hashimoto, 2012-11-09 08:39:34]

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())
On 2012/11/09 08:10:22, kinaba wrote:
> 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.

Sounds great.
Done.

http://codereview.chromium.org/11358157/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_cache.cc:726: dest_path = source_path;
On 2012/11/09 08:10:22, kinaba wrote:
> 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.

Sounds sweat.
Done.

http://codereview.chromium.org/11358157/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_cache.cc:727: create_symlink = false;
On 2012/11/09 08:10:22, kinaba wrote:
> 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?

Good catch.
Done.

[kinaba, 2012-11-09 09:11:09]

lgtm

http://codereview.chromium.org/11358157/diff/4002/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_cache.cc (right):

http://codereview.chromium.org/11358157/diff/4002/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_cache.cc:720: } else {  // File exists, move
it to persistent dir.
nit: you might want to drop else { ... }, since we're returning from the if
branch.

[hashimoto, 2012-11-09 09:16:30]

http://codereview.chromium.org/11358157/diff/4002/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_cache.cc (right):

http://codereview.chromium.org/11358157/diff/4002/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_cache.cc:720: } else {  // File exists, move
it to persistent dir.
On 2012/11/09 09:11:10, kinaba wrote:
> nit: you might want to drop else { ... }, since we're returning from the if
> branch.

Good catch.
Done.

[commit-bot: I haz the power, 2012-11-09 09:16:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/11358157/11001

[commit-bot: I haz the power, 2012-11-09 10:56:05]

Change committed as 166890