gdata: Add GDataDirectoryService::AddEntryToDirectory()

Replace uses of GDataDirectory::AddEntry() with the new function in tests.
Along the way, make GDataEntry::AddEntry() private.

BUG=137725
TEST=out/Release/unit_tests --gtest_filter=GData*  && out/Release/browser_tests --gtest_filter=GData*

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@149220 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/gdata/gdata_db_unittest.cc

@@ -4,10 +4,12 @@
 
 #include "chrome/browser/chromeos/gdata/gdata_db.h"
 
+#include "base/message_loop.h"
 #include "base/string_number_conversions.h"
 #include "chrome/browser/chromeos/gdata/gdata.pb.h"
 #include "chrome/browser/chromeos/gdata/gdata_db_factory.h"
 #include "chrome/browser/chromeos/gdata/gdata_files.h"
+#include "chrome/browser/chromeos/gdata/gdata_test_util.h"
 #include "chrome/test/base/testing_profile.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -66,6 +68,7 @@ class GDataDBTest : public testing::Test {
 
   scoped_ptr<TestingProfile> profile_;
   scoped_ptr<GDataDB> gdata_db_;
+  MessageLoopForUI message_loop;
 };
 
 void GDataDBTest::SetUp() {
@@ -143,8 +146,15 @@ GDataDirectory* GDataDBTest::AddDirectory(
   const std::string resource_id = std::string("dir_resource_id:") +
                                   dir_name;
   dir->set_title(dir_name);
+  dir->SetBaseNameFromTitle();
   dir->set_resource_id(resource_id);
-  parent->AddEntry(dir);
+  GDataFileError error = GDATA_FILE_ERROR_FAILED;
+  directory_service->AddEntryToDirectory(
+      parent->GetFilePath(),
+      dir,
+      base::Bind(&test_util::CopyErrorCodeFromFileOperationCallback, &error));
+  test_util::RunBlockingPoolTask();
+  EXPECT_EQ(GDATA_FILE_OK, error);
 
   GDataDB::Status status = gdata_db_->Put(*dir);
   EXPECT_EQ(GDataDB::DB_OK, status);
@@ -161,8 +171,15 @@ GDataFile* GDataDBTest::AddFile(GDataDirectory* parent,
   const std::string resource_id = std::string("file_resource_id:") +
                                   title;
   file->set_title(title);
+  file->SetBaseNameFromTitle();
   file->set_resource_id(resource_id);
-  parent->AddEntry(file);
+  GDataFileError error = GDATA_FILE_ERROR_FAILED;
+  directory_service->AddEntryToDirectory(
+      parent->GetFilePath(),
+      file,
+      base::Bind(&test_util::CopyErrorCodeFromFileOperationCallback, &error));
+  test_util::RunBlockingPoolTask();
+  EXPECT_EQ(GDATA_FILE_OK, error);
 
   GDataDB::Status status = gdata_db_->Put(*file);
   EXPECT_EQ(GDataDB::DB_OK, status);

chrome/browser/chromeos/gdata/gdata_file_system.cc

@@ -154,34 +154,6 @@ void GetChildDirectoryPaths(GDataEntry* entry,
 }
 
 
-// Helper function for removing |entry| from |directory|. If |entry| is a
-// directory too, it will collect all its children file paths into
-// |changed_dirs| as well.
-void RemoveEntryFromDirectoryAndCollectChangedDirectories(
-    GDataDirectory* directory,
-    GDataEntry* entry,
-    std::set<FilePath>* changed_dirs) {
-  // Get the list of all sub-directory paths, so we can notify their listeners
-  // that they are smoked.
-  GetChildDirectoryPaths(entry, changed_dirs);
-  directory->RemoveEntry(entry);
-}
-
-// Helper function for adding new |file| from the feed into |directory|. It
-// checks the type of file and updates |changed_dirs| if this file adding
-// operation needs to raise directory notification update. If file is being
-// added to |orphaned_dir_service| such notifications are not raised since
-// we ignore such files and don't add them to the file system now.
-void AddEntryToDirectoryAndCollectChangedDirectories(
-    GDataEntry* entry,
-    GDataDirectory* directory,
-    GDataDirectoryService* orphaned_dir_service,
-    std::set<FilePath>* changed_dirs) {
-  directory->AddEntry(entry);
-  if (entry->AsGDataDirectory() && directory != orphaned_dir_service->root())
-    changed_dirs->insert(entry->GetFilePath());
-}
-
 // Invoked upon completion of TransferRegularFile initiated by Copy.
 //
 // |callback| is run on the thread represented by |relay_proxy|.
@@ -3479,6 +3451,36 @@ void GDataFileSystem::ApplyFeedFromFileUrlMap(
   }
 }
 
+// Helper function for adding new |file| from the feed into |directory|. It
+// checks the type of file and updates |changed_dirs| if this file adding
+// operation needs to raise directory notification update. If file is being
+// added to |orphaned_dir_service| such notifications are not raised since
+// we ignore such files and don't add them to the file system now.
+// static
+void GDataFileSystem::AddEntryToDirectoryAndCollectChangedDirectories(
+    GDataEntry* entry,
+    GDataDirectory* directory,
+    GDataDirectoryService* orphaned_dir_service,
+    std::set<FilePath>* changed_dirs) {
+  directory->AddEntry(entry);
+  if (entry->AsGDataDirectory() && directory != orphaned_dir_service->root())
+    changed_dirs->insert(entry->GetFilePath());
+}
+
+// Helper function for removing |entry| from |directory|. If |entry| is a
+// directory too, it will collect all its children file paths into
+// |changed_dirs| as well.
+// static
+void GDataFileSystem::RemoveEntryFromDirectoryAndCollectChangedDirectories(
+    GDataDirectory* directory,
+    GDataEntry* entry,
+    std::set<FilePath>* changed_dirs) {
+  // Get the list of all sub-directory paths, so we can notify their listeners
+  // that they are smoked.
+  GetChildDirectoryPaths(entry, changed_dirs);
+  directory->RemoveEntry(entry);
+}
+
 GDataDirectory* GDataFileSystem::FindDirectoryForNewEntry(
     GDataEntry* new_entry,
     const FileResourceIdMap& file_map,

chrome/browser/chromeos/gdata/gdata_file_system.h

@@ -536,6 +536,27 @@ class GDataFileSystem : public GDataFileSystemInterface,
                                int feed_changestamp,
                                FileResourceIdMap* file_map);
 
+  // Helper function for adding new |file| from the feed into |directory|. It
+  // checks the type of file and updates |changed_dirs| if this file adding
+  // operation needs to raise directory notification update. If file is being
+  // added to |orphaned_dir_service| such notifications are not raised since
+  // we ignore such files and don't add them to the file system now.
+  // static
+  static void AddEntryToDirectoryAndCollectChangedDirectories(
+      GDataEntry* entry,
+      GDataDirectory* directory,
+      GDataDirectoryService* orphaned_dir_service,
+      std::set<FilePath>* changed_dirs);
+
+  // Helper function for removing |entry| from |directory|. If |entry| is a
+  // directory too, it will collect all its children file paths into
+  // |changed_dirs| as well.
+  // static
+  static void RemoveEntryFromDirectoryAndCollectChangedDirectories(
+      GDataDirectory* directory,
+      GDataEntry* entry,
+      std::set<FilePath>* changed_dirs);
+
   // Finds directory where new |file| should be added to during feed processing.
   // |orphaned_entries_dir| collects files/dirs that don't have a parent in
   // either locally cached file system or in this new feed.

chrome/browser/chromeos/gdata/gdata_file_system_interface.h

@@ -34,10 +34,6 @@ struct SearchResultInfo {
   bool is_directory;
 };
 
-// Used for file operations like removing files.
-typedef base::Callback<void(GDataFileError error)>
-    FileOperationCallback;
-
 // Used to get files from the file system.
 typedef base::Callback<void(GDataFileError error,
                             const FilePath& file_path,

chrome/browser/chromeos/gdata/gdata_files.cc

@@ -6,9 +6,11 @@
 
 #include <vector>
 
+#include "base/message_loop_proxy.h"
 #include "base/platform_file.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
+#include "base/tracked_objects.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/chromeos/gdata/gdata.pb.h"
 #include "chrome/browser/chromeos/gdata/gdata_util.h"
@@ -27,19 +29,6 @@ std::string ExtractResourceId(const GURL& url) {
                                    net::UnescapeRule::URL_SPECIAL_CHARS);
 }
 
-// Replaces file entry |old_entry| with its fresh value |fresh_file|.
-void RefreshFileInternal(scoped_ptr<GDataFile> fresh_file,
-                         GDataEntry* old_entry) {
-  GDataDirectory* entry_parent = old_entry ? old_entry->parent() : NULL;
-  if (entry_parent) {
-    DCHECK_EQ(fresh_file->resource_id(), old_entry->resource_id());
-    DCHECK(old_entry->AsGDataFile());
-
-    entry_parent->RemoveEntry(old_entry);
-    entry_parent->AddEntry(fresh_file.release());
-  }
-}
-
 // Returns true if |proto| is a valid proto as the root directory.
 // Used to reject incompatible proto.
 bool IsValidRootDirectoryProto(const GDataDirectoryProto& proto) {
@@ -456,6 +445,26 @@ GDataDirectoryService::~GDataDirectoryService() {
   resource_map_.clear();
 }
 
+void GDataDirectoryService::AddEntryToDirectory(
+    const FilePath& directory_path,
+    GDataEntry* entry,
+    const FileOperationCallback& callback) {
+  GDataEntry* destination = FindEntryByPathSync(directory_path);
+  GDataFileError error = GDATA_FILE_ERROR_FAILED;
+  if (!destination) {
+    error = GDATA_FILE_ERROR_NOT_FOUND;
+  } else if (!destination->AsGDataDirectory()) {
+    error = GDATA_FILE_ERROR_NOT_A_DIRECTORY;
+  } else {
+    destination->AsGDataDirectory()->AddEntry(entry);
+    error = GDATA_FILE_OK;
+  }
+  if (!callback.is_null()) {
+    base::MessageLoopProxy::current()->PostTask(
+        FROM_HERE, base::Bind(callback, error));
+  }
+}
+
 void GDataDirectoryService::AddEntryToResourceMap(GDataEntry* entry) {
   // GDataFileSystem has already locked.
   DVLOG(1) << "AddEntryToResourceMap " << entry->resource_id();
@@ -525,8 +534,24 @@ void GDataDirectoryService::RefreshFile(scoped_ptr<GDataFile> fresh_file) {
 
   // Need to get a reference here because Passed() could get evaluated first.
   const std::string& resource_id = fresh_file->resource_id();
-  GetEntryByResourceIdAsync(resource_id,
-      base::Bind(&RefreshFileInternal, base::Passed(&fresh_file)));
+  GetEntryByResourceIdAsync(
+      resource_id,
+      base::Bind(&GDataDirectoryService::RefreshFileInternal,
+                 base::Passed(&fresh_file)));
+}
+
+// static
+void GDataDirectoryService::RefreshFileInternal(
+    scoped_ptr<GDataFile> fresh_file,
+    GDataEntry* old_entry) {
+  GDataDirectory* entry_parent = old_entry ? old_entry->parent() : NULL;
+  if (entry_parent) {
+    DCHECK_EQ(fresh_file->resource_id(), old_entry->resource_id());
+    DCHECK(old_entry->AsGDataFile());
+
+    entry_parent->RemoveEntry(old_entry);
+    entry_parent->AddEntry(fresh_file.release());
+  }
 }
 
 // Convert to/from proto.

chrome/browser/chromeos/gdata/gdata_files.h

@@ -64,6 +64,10 @@ const char kGDataRootDirectoryResourceId[] = "folder:root";
 // gdata.proto.
 const int32 kProtoVersion = 1;
 
+// Used for file operations like removing files.
+typedef base::Callback<void(GDataFileError error)>
+    FileOperationCallback;
+
 // Base class for representing files and directories in gdata virtual file
 // system.
 class GDataEntry {
@@ -275,12 +279,6 @@ class GDataDirectory : public GDataEntry {
   bool FromProto(const GDataDirectoryProto& proto) WARN_UNUSED_RESULT;
   void ToProto(GDataDirectoryProto* proto) const;
 
-  // Adds child file to the directory and takes over the ownership of |file|
-  // object. The method will also do name de-duplication to ensure that the
-  // exposed presentation path does not have naming conflicts. Two files with
-  // the same name "Foo" will be renames to "Foo (1)" and "Foo (2)".
-  void AddEntry(GDataEntry* entry);
-
   // Takes the ownership of |entry| from its current parent. If this directory
   // is already the current parent of |file|, this method effectively goes
   // through the name de-duplication for |file| based on the current state of
@@ -305,7 +303,16 @@ class GDataDirectory : public GDataEntry {
   }
 
  private:
+  // TODO(satorux): Remove the friend statements. crbug.com/139649
   friend class GDataDirectoryService;
+  friend class GDataFileSystem;
+
+  // Adds child file to the directory and takes over the ownership of |file|
+  // object. The method will also do name de-duplication to ensure that the
+  // exposed presentation path does not have naming conflicts. Two files with
+  // the same name "Foo" will be renames to "Foo (1)" and "Foo (2)".
+  // TODO(satorux): Remove this. crbug.com/139649
+  void AddEntry(GDataEntry* entry);
 
   // Find a child by its name.
   // TODO(satorux): Remove this. crbug.com/139649
@@ -352,6 +359,12 @@ class GDataDirectoryService {
   const ContentOrigin origin() const { return origin_; }
   void set_origin(ContentOrigin value) { origin_ = value; }
 
+  // Adds |entry| to |directory_path| asynchronously.
+  // Must be called on UI thread. |callback| is called on the UI thread.
+  void AddEntryToDirectory(const FilePath& directory_path,
+                           GDataEntry* entry,
+                           const FileOperationCallback& callback);
+
   // Adds the entry to resource map.
   void AddEntryToResourceMap(GDataEntry* entry);
 
@@ -375,6 +388,10 @@ class GDataDirectoryService {
   // fresh value |fresh_file|.
   void RefreshFile(scoped_ptr<GDataFile> fresh_file);
 
+  // Replaces file entry |old_entry| with its fresh value |fresh_file|.
+  static void RefreshFileInternal(scoped_ptr<GDataFile> fresh_file,
+                                  GDataEntry* old_entry);
+
   // Serializes/Parses to/from string via proto classes.
   void SerializeToString(std::string* serialized_proto) const;
   bool ParseFromString(const std::string& serialized_proto);

chrome/browser/chromeos/gdata/gdata_files_unittest.cc

@@ -7,7 +7,9 @@
 #include <string>
 #include <utility>
 #include <vector>
+#include "base/message_loop.h"
 #include "chrome/browser/chromeos/gdata/gdata.pb.h"
+#include "chrome/browser/chromeos/gdata/gdata_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace gdata {
@@ -210,18 +212,35 @@ TEST(GDataRootDirectoryTest, ParseFromString_DetectNoUploadUrl) {
 }
 
 TEST(GDataRootDirectoryTest, RefreshFile) {
+  MessageLoopForUI message_loop;
   GDataDirectoryService directory_service;
   GDataDirectory* root(directory_service.root());
   // Add a directory to the file system.
   GDataDirectory* directory_entry = new GDataDirectory(root,
                                                        &directory_service);
   directory_entry->set_resource_id("folder:directory_resource_id");
-  root->AddEntry(directory_entry);
+  directory_entry->set_title("directory");
+  directory_entry->SetBaseNameFromTitle();
+  GDataFileError error = GDATA_FILE_ERROR_FAILED;
+  directory_service.AddEntryToDirectory(
+      FilePath(kGDataRootDirectory),
+      directory_entry,
+      base::Bind(&test_util::CopyErrorCodeFromFileOperationCallback, &error));
+  test_util::RunBlockingPoolTask();
+  ASSERT_EQ(GDATA_FILE_OK, error);
 
   // Add a new file to the directory.
   GDataFile* initial_file_entry = new GDataFile(NULL, &directory_service);
   initial_file_entry->set_resource_id("file:file_resource_id");
-  directory_entry->AddEntry(initial_file_entry);
+  initial_file_entry->set_title("file");
+  initial_file_entry->SetBaseNameFromTitle();
+  directory_service.AddEntryToDirectory(
+      directory_entry->GetFilePath(),
+      initial_file_entry,
+      base::Bind(&test_util::CopyErrorCodeFromFileOperationCallback, &error));
+  test_util::RunBlockingPoolTask();
+  ASSERT_EQ(GDATA_FILE_OK, error);
+
   ASSERT_EQ(directory_entry, initial_file_entry->parent());
 
   // Initial file system state set, let's try refreshing entries.

chrome/browser/chromeos/gdata/gdata_test_util.cc

@@ -63,5 +63,11 @@ bool CacheStatesEqual(const GDataCacheEntry& a, const GDataCacheEntry& b) {
           a.is_persistent() == b.is_persistent());
 }
 
+void CopyErrorCodeFromFileOperationCallback(GDataFileError* output,
+                                            GDataFileError error) {
+  DCHECK(output);
+  *output = error;
+}
+
 }  // namespace test_util
 }  // namespace gdata

chrome/browser/chromeos/gdata/gdata_test_util.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_CHROMEOS_GDATA_GDATA_TEST_UTIL_H_
 #define CHROME_BROWSER_CHROMEOS_GDATA_GDATA_TEST_UTIL_H_
 
+#include "chrome/browser/chromeos/gdata/gdata_errorcode.h"
+
 namespace gdata {
 
 class GDataCacheEntry;
@@ -37,6 +39,12 @@ GDataCacheEntry ToCacheEntry(int cache_state);
 // Returns true if the cache state of the given two cache entries are equal.
 bool CacheStatesEqual(const GDataCacheEntry& a, const GDataCacheEntry& b);
 
+// Copies |error| to |output|. Used to run asynchronous functions that take
+// FileOperationCallback from tests.
+void CopyErrorCodeFromFileOperationCallback(
+    GDataFileError* output, GDataFileError error);
+
+
 }  // namespace test_util
 }  // namespace gdata