drive: Remove FileCacheObserver::OnCacheCommitted

drive: Remove FileCacheObserver::OnCacheCommitted

FileCacheObserver::CommitDirty does nothing but notifying cache observers about OnCacheComitted event whose only user is SyncClient.
We should abandon this complexity by simply calling SyncClient methods from FileSystem.

Remove FileCacheObserver::OnCacheCommitted
Remove FileCache::CommitDirtyOnUIThread and StoreLocallyModifiedOnUIThread
Rename SyncClient methods.
Call SyncClient methods from FileSystem directly.
Add OperationObserver::OnCacheFileUploadNeededByOperation to trigger upload from CopyOperation.

BUG=247274
TEST=unit_tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205568

[kinaba, 2013-06-11 07:08:37]

Sorry I'm late.

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/file_cache.cc (left):

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/file_cache.cc:400: CACHED_FILE_LOCALLY_MODIFIED),
Except this call StoreInternal is called always with CACHED_FILE_FROM_SERVER,
probably you can remove the paramter.

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/file_system.h (right):

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/file_system.h:182: friend class FileSystemTest;
Good catch. Do we need this friend declaration at all,
if it compiles with the wrong name?

Ah, got it. It's for accessing |sync_client_|.
Rather than making it a friend, I think adding a public method
sync_client_for_testing() looks cleaner. (change_list_loader() actually
falls into the same category.) WDYT?

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/file_system_unittest.cc (right):

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/file_system_unittest.cc:797: entry =
GetResourceEntryByPathSync(kFileInRoot);
GetResourceEntryByPathSync ends up with local resource metadata look up.
To test that it is indeed synced, I think we should query fake_drive_service
here.

[hashimoto, 2013-06-11 07:37:56]

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/file_cache.cc (left):

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/file_cache.cc:400: CACHED_FILE_LOCALLY_MODIFIED),
On 2013/06/11 07:08:37, kinaba wrote:
> Except this call StoreInternal is called always with CACHED_FILE_FROM_SERVER,
> probably you can remove the paramter.

Done.
I'll merge StoreInternal to Store in another patch later.

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/file_system.h (right):

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/file_system.h:182: friend class FileSystemTest;
On 2013/06/11 07:08:37, kinaba wrote:
> Good catch. Do we need this friend declaration at all,
> if it compiles with the wrong name?
> 
> Ah, got it. It's for accessing |sync_client_|.
> Rather than making it a friend, I think adding a public method
> sync_client_for_testing() looks cleaner. (change_list_loader() actually
> falls into the same category.) WDYT?

Sounds good, done.
Maybe we should make a patch to rename change_list_loader() to
change_list_loader_for_testing() too.

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/file_system_unittest.cc (right):

https://codereview.chromium.org/16628003/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/file_system_unittest.cc:797: entry =
GetResourceEntryByPathSync(kFileInRoot);
On 2013/06/11 07:08:37, kinaba wrote:
> GetResourceEntryByPathSync ends up with local resource metadata look up.
> To test that it is indeed synced, I think we should query fake_drive_service
> here.

Good catch, I should have checked the change stamp or used
FakeDriveService::GetResourceEntry as you say.
Switched to check with fake_drive_service_.

[kinaba, 2013-06-11 07:52:29]

lgtm. Thanks!

[hashimoto, 2013-06-11 08:02:05]

Oops, FileSystemTest.PinAndUnpin is failing because SyncClient does not delay
the sync.
Added EXPECT_CALL to fix this.

PTAL?

[kinaba, 2013-06-11 08:03:39]

lgtm

[commit-bot: I haz the power, 2013-06-11 08:04:34]

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

[commit-bot: I haz the power, 2013-06-11 09:38:46]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-06-11 13:00:41]

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

[commit-bot: I haz the power, 2013-06-11 17:13:24]

Change committed as 205568