Merge 144878 - gdata: Get rid of GDataFileSystem::GetCacheState()

Instead, add GDataCache::GetCacheEntryOnUIThread().

BUG=133552
TEST=the existing testis covered this functionality

Review URL: https://chromiumcodereview.appspot.com/10690028

TBR=satorux@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10698158

git-svn-id: svn://svn.chromium.org/chrome/branches/1180/src@146223 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 115 insertions, 151 deletions

diff --git a/chrome/browser/chromeos/extensions/file_browser_private_api.cc b/chrome/browser/chromeos/extensions/file_browser_private_api.cc
index e4ae2c8..40bc3c7 100644
--- a/chrome/browser/chromeos/extensions/file_browser_private_api.cc
+++ b/chrome/browser/chromeos/extensions/file_browser_private_api.cc
@@ -1667,7 +1667,7 @@ void GetGDataFilePropertiesFunction::OnOperationComplete(
     }
   }
 
-  system_service->file_system()->GetCacheState(
+  system_service->cache()->GetCacheEntryOnUIThread(
       file_proto->gdata_entry().resource_id(),
       file_proto->file_md5(),
       base::Bind(
@@ -1677,8 +1677,10 @@ void GetGDataFilePropertiesFunction::OnOperationComplete(
 
 void GetGDataFilePropertiesFunction::CacheStateReceived(
     base::DictionaryValue* property_dict,
-    base::PlatformFileError error,
-    int cache_state) {
+    bool success,
+    const gdata::GDataCache::CacheEntry& cache_entry) {
+  const int cache_state = (success ? cache_entry.cache_state :
+                           gdata::GDataCache::CACHE_STATE_NONE);
   property_dict->SetBoolean(
       "isPinned",
       gdata::GDataCache::IsCachePinned(cache_state));
diff --git a/chrome/browser/chromeos/extensions/file_browser_private_api.h b/chrome/browser/chromeos/extensions/file_browser_private_api.h
index d11f52f..1a897fd 100644
--- a/chrome/browser/chromeos/extensions/file_browser_private_api.h
+++ b/chrome/browser/chromeos/extensions/file_browser_private_api.h
@@ -14,6 +14,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/platform_file.h"
 #include "chrome/browser/chromeos/extensions/file_browser_event_router.h"
+#include "chrome/browser/chromeos/gdata/gdata_cache.h"
 #include "chrome/browser/extensions/extension_function.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "googleurl/src/url_util.h"
@@ -437,8 +438,8 @@ class GetGDataFilePropertiesFunction : public FileBrowserFunction {
                      scoped_ptr<gdata::GDataFileProto> file_proto);
 
   void CacheStateReceived(base::DictionaryValue* property_dict,
-                          base::PlatformFileError error,
-                          int cache_state);
+                          bool success,
+                          const gdata::GDataCache::CacheEntry& cache_entry);
 
   size_t current_index_;
   base::ListValue* path_list_;
diff --git a/chrome/browser/chromeos/gdata/gdata_cache.cc b/chrome/browser/chromeos/gdata/gdata_cache.cc
index 24f4b9b..2737fe2 100644
--- a/chrome/browser/chromeos/gdata/gdata_cache.cc
+++ b/chrome/browser/chromeos/gdata/gdata_cache.cc
@@ -346,7 +346,7 @@ void RunGetFileFromCacheCallback(const GetFileFromCacheCallback& callback,
 }
 
 // Runs callback with pointers dereferenced.
-// Used to implement GetResourceIdsOfBacklog().
+// Used to implement GetResourceIdsOfBacklogOnUIThread().
 void RunGetResourceIdsCallback(const GetResourceIdsCallback& callback,
                                std::vector<std::string>* to_fetch,
                                std::vector<std::string>* to_upload) {
@@ -358,6 +358,19 @@ void RunGetResourceIdsCallback(const GetResourceIdsCallback& callback,
     callback.Run(*to_fetch, *to_upload);
 }
 
+// Runs callback with pointers dereferenced.
+// Used to implement GetCacheEntryOnUIThread().
+void RunGetCacheEntryCallback(
+    const GDataCache::GetCacheEntryCallback& callback,
+    bool* success,
+    GDataCache::CacheEntry* cache_entry) {
+  DCHECK(success);
+  DCHECK(cache_entry);
+
+  if (!callback.is_null())
+    callback.Run(*success, *cache_entry);
+}
+
 }  // namespace
 
 std::string GDataCache::CacheEntry::ToString() const {
@@ -445,6 +458,28 @@ void GDataCache::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void GDataCache::GetCacheEntryOnUIThread(
+    const std::string& resource_id,
+    const std::string& md5,
+    const GetCacheEntryCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  bool* success = new bool(false);
+  GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry;
+  pool_->GetSequencedTaskRunner(sequence_token_)->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&GDataCache::GetCacheEntryHelper,
+                 base::Unretained(this),
+                 resource_id,
+                 md5,
+                 success,
+                 cache_entry),
+      base::Bind(&RunGetCacheEntryCallback,
+                 callback,
+                 base::Owned(success),
+                 base::Owned(cache_entry)));
+}
+
 void GDataCache::GetResourceIdsOfBacklogOnUIThread(
     const GetResourceIdsCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -1463,6 +1498,20 @@ void GDataCache::OnCommitDirty(base::PlatformFileError* error,
     FOR_EACH_OBSERVER(Observer, observers_, OnCacheCommitted(resource_id));
 }
 
+void GDataCache::GetCacheEntryHelper(const std::string& resource_id,
+                                     const std::string& md5,
+                                     bool* success,
+                                     GDataCache::CacheEntry* cache_entry) {
+  AssertOnSequencedWorkerPool();
+  DCHECK(success);
+  DCHECK(cache_entry);
+
+  scoped_ptr<GDataCache::CacheEntry> value(GetCacheEntry(resource_id, md5));
+  *success = value.get();
+  if (*success)
+    *cache_entry = *value;
+}
+
 // static
 FilePath GDataCache::GetCacheRootPath(Profile* profile) {
   FilePath cache_base_path;
diff --git a/chrome/browser/chromeos/gdata/gdata_cache.h b/chrome/browser/chromeos/gdata/gdata_cache.h
index 4e615df..a8995b0 100644
--- a/chrome/browser/chromeos/gdata/gdata_cache.h
+++ b/chrome/browser/chromeos/gdata/gdata_cache.h
@@ -122,9 +122,9 @@ class GDataCache {
     CacheEntry(const std::string& md5,
                CacheSubDirectoryType sub_dir_type,
                int cache_state)
-    : md5(md5),
-      sub_dir_type(sub_dir_type),
-      cache_state(cache_state) {
+        : md5(md5),
+          sub_dir_type(sub_dir_type),
+          cache_state(cache_state) {
     }
 
     bool IsPresent() const { return IsCachePresent(cache_state); }
@@ -140,6 +140,16 @@ class GDataCache {
     int cache_state;
   };
 
+  // Callback for GetCacheEntryOnUIThread.
+  // |success| indicates if the operation was successful.
+  // |cache_entry| is the obtained cache entry.
+  //
+  // TODO(satorux): Unlike other callback types, this has to be defined
+  // inside GDataCache as CacheEntry is inside GDataCache. We should get them
+  // outside of GDataCache.
+  typedef base::Callback<void(bool success, const CacheEntry& cache_entry)>
+      GetCacheEntryCallback;
+
   static bool IsCachePresent(int cache_state) {
     return cache_state & CACHE_STATE_PRESENT;
   }
@@ -205,6 +215,15 @@ class GDataCache {
   // Must be called on UI thread.
   void RemoveObserver(Observer* observer);
 
+  // Gets the cache entry by the given resource ID and MD5.
+  // See also GetCacheEntry().
+  //
+  // Must be called on UI thread. |callback| is run on UI thread.
+  void GetCacheEntryOnUIThread(
+    const std::string& resource_id,
+    const std::string& md5,
+    const GetCacheEntryCallback& callback);
+
   // Gets the resource IDs of pinned-but-not-fetched files and
   // dirty-but-not-uploaded files.
   //
@@ -418,6 +437,12 @@ class GDataCache {
                      const std::string& md5,
                      const CacheOperationCallback& callback);
 
+  // Helper function to implement GetCacheEntryOnUIThread().
+  void GetCacheEntryHelper(const std::string& resource_id,
+                           const std::string& md5,
+                           bool* success,
+                           GDataCache::CacheEntry* cache_entry);
+
   // The root directory of the cache (i.e. <user_profile_dir>/GCache/v1).
   const FilePath cache_root_path_;
   // Paths for all subdirectories of GCache, one for each
diff --git a/chrome/browser/chromeos/gdata/gdata_file_system.cc b/chrome/browser/chromeos/gdata/gdata_file_system.cc
index ae21b46..6264caf 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.cc
@@ -190,21 +190,6 @@ void OnTransferRegularFileCompleteForCopy(
     relay_proxy->PostTask(FROM_HERE, base::Bind(callback, error));
 }
 
-// Gets a cache entry from a GDataCache, must be called on the blocking pool.
-// The result value is copied to cache_entry on success.
-void GetCacheEntryOnBlockingPool(
-    GDataCache* cache,
-    const std::string& resource_id,
-    const std::string& md5,
-    GDataCache::CacheEntry* cache_entry,
-    bool* success) {
-  scoped_ptr<GDataCache::CacheEntry> value(
-      cache->GetCacheEntry(resource_id, md5));
-  *success = value.get();
-  if (*success)
-    *cache_entry = *value;
-}
-
 // Runs GetFileCallback with pointers dereferenced.
 // Used for PostTaskAndReply().
 void RunGetFileCallbackHelper(const GetFileCallback& callback,
@@ -253,21 +238,6 @@ void OnAddUploadFileCompleted(
     callback.Run(error);
 }
 
-// Used to implement GetCacheState.
-void RunGetCacheStateCallbackHelper(
-    const GetCacheStateCallback& callback,
-    GDataCache::CacheEntry* cache_entry,
-    bool* success) {
-  DCHECK(cache_entry);
-  DCHECK(success);
-  if (callback.is_null())
-    return;
-
-  callback.Run(
-      base::PLATFORM_FILE_OK,
-      *success ? cache_entry->cache_state : GDataCache::CACHE_STATE_NONE);
-}
-
 // The class to wait for the initial load of root feed and runs the callback
 // after the initialization.
 class InitialLoadObserver : public GDataFileSystemInterface::Observer {
@@ -2272,62 +2242,6 @@ void GDataFileSystem::OnUpdatedFileUploaded(
                              base::Passed(&upload_file_info)));
 }
 
-void GDataFileSystem::GetCacheState(const std::string& resource_id,
-                                    const std::string& md5,
-                                    const GetCacheStateCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
-         BrowserThread::CurrentlyOn(BrowserThread::IO));
-
-  // Always post a task to the UI thread to call GetCacheStateOnUIThread even if
-  // GetCacheState is called on the UI thread. This ensures that, regardless of
-  // whether GDataFileSystem is locked or not, GDataFileSystem is unlocked when
-  // GetCacheStateOnUIThread is called.
-  const bool posted = BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&GDataFileSystem::GetCacheStateOnUIThread,
-                 ui_weak_ptr_,
-                 resource_id,
-                 md5,
-                 CreateRelayCallback(callback)));
-  DCHECK(posted);
-}
-
-void GDataFileSystem::GetCacheStateOnUIThread(
-    const std::string& resource_id,
-    const std::string& md5,
-    const GetCacheStateCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  GDataEntry* entry = root_->GetEntryByResourceId(resource_id);
-  if (!entry || !entry->AsGDataFile()) {
-    const bool posted = BrowserThread::PostTask(
-        BrowserThread::UI,
-        FROM_HERE,
-        base::Bind(callback,
-                   base::PLATFORM_FILE_ERROR_NOT_FOUND,
-                   GDataCache::CACHE_STATE_NONE));
-    DCHECK(posted);
-    return;
-  }
-
-  GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry;
-  bool* success = new bool(false);
-  PostBlockingPoolSequencedTaskAndReply(
-      FROM_HERE,
-      sequence_token_,
-      base::Bind(&GetCacheEntryOnBlockingPool,
-                 cache_,
-                 resource_id,
-                 md5,
-                 cache_entry,
-                 success),
-      base::Bind(&RunGetCacheStateCallbackHelper,
-                 callback,
-                 base::Owned(cache_entry),
-                 base::Owned(success)));
-}
-
 void GDataFileSystem::GetAvailableSpace(
     const GetAvailableSpaceCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
@@ -2827,23 +2741,13 @@ void GDataFileSystem::OnFileDownloaded(
   // If user cancels download of a pinned-but-not-fetched file, mark file as
   // unpinned so that we do not sync the file again.
   if (status == GDATA_CANCELLED) {
-    GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry;
-    bool* success = new bool(false);
-    PostBlockingPoolSequencedTaskAndReply(
-        FROM_HERE,
-        sequence_token_,
-        base::Bind(&GetCacheEntryOnBlockingPool,
-                   cache_,
-                   params.resource_id,
-                   params.md5,
-                   cache_entry,
-                   success),
+    cache_->GetCacheEntryOnUIThread(
+        params.resource_id,
+        params.md5,
         base::Bind(&GDataFileSystem::UnpinIfPinned,
                    ui_weak_ptr_,
                    params.resource_id,
-                   params.md5,
-                   base::Owned(cache_entry),
-                   base::Owned(success)));
+                   params.md5));
   }
 
   // At this point, the disk can be full or nearly full for several reasons:
@@ -2871,14 +2775,15 @@ void GDataFileSystem::OnFileDownloaded(
                  base::Owned(has_enough_space)));
 }
 
-void GDataFileSystem::UnpinIfPinned(const std::string& resource_id,
-                                    const std::string& md5,
-                                    GDataCache::CacheEntry* cache_entry,
-                                    bool* cache_entry_is_valid) {
+void GDataFileSystem::UnpinIfPinned(
+    const std::string& resource_id,
+    const std::string& md5,
+    bool success,
+    const GDataCache::CacheEntry& cache_entry) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // TODO(hshi): http://crbug.com/127138 notify when file properties change.
   // This allows file manager to clear the "Available offline" checkbox.
-  if (*cache_entry_is_valid && cache_entry->IsPinned())
+  if (success && cache_entry.IsPinned())
     cache_->UnpinOnUIThread(resource_id, md5, CacheOperationCallback());
 }
 
diff --git a/chrome/browser/chromeos/gdata/gdata_file_system.h b/chrome/browser/chromeos/gdata/gdata_file_system.h
index 21a68cb..27d4f51 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.h
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.h
@@ -103,10 +103,6 @@ typedef base::Callback<void(base::PlatformFileError error,
 typedef base::Callback<void(const std::string& resource_id)>
     GetDocumentResourceIdCallback;
 
-// Callback for GetCacheState operation.
-typedef base::Callback<void(base::PlatformFileError error,
-                            int cache_state)> GetCacheStateCallback;
-
 // GData file system abstraction layer.
 // The interface is defined to make GDataFileSystem mockable.
 class GDataFileSystemInterface {
@@ -297,16 +293,6 @@ class GDataFileSystemInterface {
       const std::string& resource_id,
       const FileOperationCallback& callback) = 0;
 
-  // Gets the cache state of file corresponding to |resource_id| and |md5| if it
-  // exists on disk.
-  // Initializes cache if it has not been initialized.
-  // Upon completion, |callback| is invoked on the thread where this method was
-  // called with the cache state if file exists in cache or CACHE_STATE_NONE
-  // otherwise.
-  virtual void GetCacheState(const std::string& resource_id,
-                             const std::string& md5,
-                             const GetCacheStateCallback& callback) = 0;
-
   // Finds an entry (a file or a directory) by |file_path|. This call will also
   // retrieve and refresh file system content from server and disk cache.
   //
@@ -434,9 +420,6 @@ class GDataFileSystem : public GDataFileSystemInterface,
   virtual void UpdateFileByResourceId(
       const std::string& resource_id,
       const FileOperationCallback& callback) OVERRIDE;
-  virtual void GetCacheState(const std::string& resource_id,
-                             const std::string& md5,
-                             const GetCacheStateCallback& callback) OVERRIDE;
   virtual void GetEntryInfoByPath(
       const FilePath& file_path,
       const GetEntryInfoCallback& callback) OVERRIDE;
@@ -845,8 +828,8 @@ class GDataFileSystem : public GDataFileSystemInterface,
   // Unpins file if cache entry is pinned.
   void UnpinIfPinned(const std::string& resource_id,
                      const std::string& md5,
-                     GDataCache::CacheEntry* cache_entry,
-                     bool* cache_entry_is_valid);
+                     bool success,
+                     const GDataCache::CacheEntry& cache_entry);
 
   // Similar to OnFileDownloaded() but takes |has_enough_space| so we report
   // an error in case we don't have enough disk space.
@@ -1158,9 +1141,6 @@ class GDataFileSystem : public GDataFileSystemInterface,
       const FilePath& file_path);
   void OnRequestDirectoryRefresh(GetDocumentsParams* params,
                                  base::PlatformFileError error);
-  void GetCacheStateOnUIThread(const std::string& resource_id,
-                               const std::string& md5,
-                               const GetCacheStateCallback& callback);
   void GetAvailableSpaceOnUIThread(const GetAvailableSpaceCallback& callback);
   void AddUploadedFileOnUIThread(UploadMode upload_mode,
                                  const FilePath& virtual_dir_path,
diff --git a/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc b/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc
index 3e73d8d..ceff6d8 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc
@@ -211,6 +211,7 @@ class GDataFileSystemTest : public testing::Test {
         expected_error_(base::PLATFORM_FILE_OK),
         expected_cache_state_(0),
         expected_sub_dir_type_(GDataCache::CACHE_TYPE_META),
+        expected_success_(true),
         expect_outgoing_symlink_(false),
         root_feed_changestamp_(0) {
   }
@@ -730,26 +731,29 @@ class GDataFileSystemTest : public testing::Test {
     RunAllPendingForIO();
   }
 
-  void TestGetCacheState(const std::string& resource_id, const std::string& md5,
-                         base::PlatformFileError expected_error,
-                         int expected_cache_state, GDataFile* expected_file) {
-    expected_error_ = expected_error;
+  void TestGetCacheState(const std::string& resource_id,
+                         const std::string& md5,
+                         bool expected_success,
+                         int expected_cache_state,
+                         GDataFile* expected_file) {
+    expected_success_ = expected_success;
     expected_cache_state_ = expected_cache_state;
 
-    file_system_->GetCacheState(resource_id, md5,
+    cache_->GetCacheEntryOnUIThread(resource_id, md5,
         base::Bind(&GDataFileSystemTest::VerifyGetCacheState,
                    base::Unretained(this)));
 
     RunAllPendingForIO();
   }
 
-  void VerifyGetCacheState(base::PlatformFileError error, int cache_state) {
+  void VerifyGetCacheState(bool success,
+                           const GDataCache::CacheEntry& cache_entry) {
     ++num_callback_invocations_;
 
-    EXPECT_EQ(expected_error_, error);
+    EXPECT_EQ(expected_success_, success);
 
-    if (error == base::PLATFORM_FILE_OK) {
-      EXPECT_EQ(expected_cache_state_, cache_state);
+    if (success) {
+      EXPECT_EQ(expected_cache_state_, cache_entry.cache_state);
     }
   }
 
@@ -1321,6 +1325,7 @@ class GDataFileSystemTest : public testing::Test {
   base::PlatformFileError expected_error_;
   int expected_cache_state_;
   GDataCache::CacheSubDirectoryType expected_sub_dir_type_;
+  bool expected_success_;
   bool expect_outgoing_symlink_;
   std::string expected_file_extension_;
   int root_feed_changestamp_;
@@ -2963,7 +2968,7 @@ TEST_F(GDataFileSystemTest, GetCacheState) {
 
     // Get its cache state.
     num_callback_invocations_ = 0;
-    TestGetCacheState(resource_id, md5, base::PLATFORM_FILE_OK,
+    TestGetCacheState(resource_id, md5, true,
                       GDataCache::CACHE_STATE_PRESENT, file);
     EXPECT_EQ(1, num_callback_invocations_);
   }
@@ -2993,14 +2998,14 @@ TEST_F(GDataFileSystemTest, GetCacheState) {
 
     // Get its cache state.
     num_callback_invocations_ = 0;
-    TestGetCacheState(resource_id, md5, base::PLATFORM_FILE_OK,
+    TestGetCacheState(resource_id, md5, true,
                       expected_cache_state, file);
     EXPECT_EQ(1, num_callback_invocations_);
   }
 
   {  // Test cache state of a non-existent file.
     num_callback_invocations_ = 0;
-    TestGetCacheState("pdf:12345", "abcd", base::PLATFORM_FILE_ERROR_NOT_FOUND,
+    TestGetCacheState("pdf:12345", "abcd", false,
                       0, NULL);
     EXPECT_EQ(1, num_callback_invocations_);
   }
diff --git a/chrome/browser/chromeos/gdata/mock_gdata_file_system.h b/chrome/browser/chromeos/gdata/mock_gdata_file_system.h
index a94bb54..ec51570 100644
--- a/chrome/browser/chromeos/gdata/mock_gdata_file_system.h
+++ b/chrome/browser/chromeos/gdata/mock_gdata_file_system.h
@@ -68,9 +68,6 @@ class MockGDataFileSystem : public GDataFileSystemInterface {
   MOCK_METHOD2(UpdateFileByResourceId,
                void(const std::string& resource_id,
                     const FileOperationCallback& callback));
-  MOCK_METHOD3(GetCacheState, void(const std::string& resource_id,
-                                   const std::string& md5,
-                                   const GetCacheStateCallback& callback));
   MOCK_METHOD2(GetFileInfoByPath, void(const FilePath& file_path,
                                        const GetFileInfoCallback& callback));
   MOCK_METHOD2(GetEntryInfoByPath, void(const FilePath& file_path,