Chromium Code Reviews
Help | Chromium Project | Sign in
(222)

Issue 11358157: chromeos: Replace ModifyCacheState with a number of small functions to simplify DriveCache (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 5 months ago by hashimoto
Modified:
2 years, 5 months ago
Reviewers:
kinaba
CC:
chromium-reviews, achuith+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 : _ #

Total comments: 6

Patch Set 2 : Address comments #

Total comments: 2

Patch Set 3 : Remove unnecessary 'else' #

Messages

Total messages: 7 (0 generated)
hashimoto
Please review.
2 years, 5 months ago (2012-11-09 07:10:16 UTC) #1
kinaba
Thanks! Now the day has come that I can now understand the code of DriveCache! ...
2 years, 5 months ago (2012-11-09 08:10:22 UTC) #2
hashimoto
http://codereview.chromium.org/11358157/diff/5001/chrome/browser/chromeos/drive/drive_cache.cc File chrome/browser/chromeos/drive/drive_cache.cc (right): http://codereview.chromium.org/11358157/diff/5001/chrome/browser/chromeos/drive/drive_cache.cc#newcode652 chrome/browser/chromeos/drive/drive_cache.cc:652: if (success && !symlink_path.empty()) On 2012/11/09 08:10:22, kinaba wrote: ...
2 years, 5 months ago (2012-11-09 08:39:34 UTC) #3
kinaba
lgtm http://codereview.chromium.org/11358157/diff/4002/chrome/browser/chromeos/drive/drive_cache.cc File chrome/browser/chromeos/drive/drive_cache.cc (right): http://codereview.chromium.org/11358157/diff/4002/chrome/browser/chromeos/drive/drive_cache.cc#newcode720 chrome/browser/chromeos/drive/drive_cache.cc:720: } else { // File exists, move it ...
2 years, 5 months ago (2012-11-09 09:11:09 UTC) #4
hashimoto
http://codereview.chromium.org/11358157/diff/4002/chrome/browser/chromeos/drive/drive_cache.cc File chrome/browser/chromeos/drive/drive_cache.cc (right): http://codereview.chromium.org/11358157/diff/4002/chrome/browser/chromeos/drive/drive_cache.cc#newcode720 chrome/browser/chromeos/drive/drive_cache.cc:720: } else { // File exists, move it to ...
2 years, 5 months ago (2012-11-09 09:16:30 UTC) #5
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hashimoto@chromium.org/11358157/11001
2 years, 5 months ago (2012-11-09 09:16:50 UTC) #6
I haz the power (commit-bot)
2 years, 5 months ago (2012-11-09 10:56:05 UTC) #7
Change committed as 166890
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 700cc9d