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

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:
1 year, 5 months ago by hashimoto
Modified:
1 year, 5 months ago
Reviewers:
kinaba
CC:
chromium-reviews_chromium.org, 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' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+173 lines, -263 lines) Lint Patch
M chrome/browser/chromeos/drive/drive_cache.cc View 1 2 13 chunks +173 lines, -263 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 7
hashimoto
Please review.
1 year, 5 months ago #1
kinaba
Thanks! Now the day has come that I can now understand the code of DriveCache! ...
1 year, 5 months ago #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: ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #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
1 year, 5 months ago #6
I haz the power (commit-bot)
1 year, 5 months ago #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 1280:2d3e6564b7b6