Set root resource ID upon full feed update.

We used to set the resource ID at DriveResourceMetadata's 
constructor as for WAPI the resource ID for root is 
a fixed string, but for Drive API it is not fixed and 
was set when About resource is obtained. 

This caused a trouble for maintaining the code and 
this CL is trying to consolidate setting root resource 
ID at one place. 

Although for Drive API 'root' can be used for a convenient 
alias for queries, Drive API usually returns a reference 
to root directory (e.g. reference to parent folder) 
as its real root directory resource ID rather than 'root' 
alias, therefore we don't use it here. 

This was originally reviewed in https://chromiumcodereview.appspot.com/11227020/
and submitted three times (r164876, r165219, r165303) but reverted due to
unit_test flakiness (DriveFileSystemTest.ContentSearchWithNewEntry failed
once in a while indeterministically).

Removed parts that uses a different root resource ID for unit tests.

BUG=152230, 152288
TEST=compiles, pass all existing tests.


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

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

7 files changed, 64 insertions, 22 deletions

diff --git a/chrome/browser/chromeos/drive/drive_feed_loader.cc b/chrome/browser/chromeos/drive/drive_feed_loader.cc
index 3ea1604..2190de9 100644
--- a/chrome/browser/chromeos/drive/drive_feed_loader.cc
+++ b/chrome/browser/chromeos/drive/drive_feed_loader.cc
@@ -377,7 +377,9 @@ void DriveFeedLoader::OnGetAboutResource(
   }
 
   int64 largest_changestamp = about_resource->largest_change_id();
-  resource_metadata_->InitializeRootEntry(about_resource->root_folder_id());
+
+  // Copy the root resource ID for use in UpdateFromFeed().
+  params->root_resource_id = about_resource->root_folder_id();
 
   if (local_changestamp >= largest_changestamp) {
     if (local_changestamp > largest_changestamp) {
@@ -482,6 +484,7 @@ void DriveFeedLoader::OnFeedFromServerLoaded(scoped_ptr<LoadFeedParams> params,
   UpdateFromFeed(params->feed_list,
                  params->start_changestamp,
                  params->root_feed_changestamp,
+                 params->root_resource_id,
                  base::Bind(&DriveFeedLoader::OnUpdateFromFeed,
                             weak_ptr_factory_.GetWeakPtr(),
                             params->load_finished_callback));
@@ -840,15 +843,28 @@ void DriveFeedLoader::UpdateFromFeed(
     const ScopedVector<google_apis::DocumentFeed>& feed_list,
     int64 start_changestamp,
     int64 root_feed_changestamp,
+    const std::string& root_resource_id,
     const base::Closure& update_finished_callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!update_finished_callback.is_null());
   DVLOG(1) << "Updating directory with a feed";
 
+  if (start_changestamp == 0) {
+    // This is a full fetch and on full fetch the root has to be initialized
+    // before children are added by DriveFeedProcessor.
+    if (google_apis::util::IsDriveV2ApiEnabled()) {
+      resource_metadata_->InitializeRootEntry(root_resource_id);
+    } else {
+      // Use fixed root resource ID for WAPI.
+      resource_metadata_->InitializeRootEntry(kWAPIRootDirectoryResourceId);
+    }
+  }
+
   feed_processor_.reset(new DriveFeedProcessor(resource_metadata_));
   // Don't send directory content change notification while performing
   // the initial content retrieval.
   const bool should_notify_changed_directories = (start_changestamp != 0);
+
   feed_processor_->ApplyFeeds(
       feed_list,
       start_changestamp,
diff --git a/chrome/browser/chromeos/drive/drive_feed_loader.h b/chrome/browser/chromeos/drive/drive_feed_loader.h
index 3b355b0..74c3fb7 100644
--- a/chrome/browser/chromeos/drive/drive_feed_loader.h
+++ b/chrome/browser/chromeos/drive/drive_feed_loader.h
@@ -77,6 +77,9 @@ struct LoadFeedParams {
   FileOperationCallback load_finished_callback;
   ScopedVector<google_apis::DocumentFeed> feed_list;
   scoped_ptr<GetDocumentsUiState> ui_state;
+  // On initial feed load for Drive API, remember root ID for
+  // DriveResourceData initialization later in UpdateFromFeed().
+  std::string root_resource_id;
 };
 
 // Defines set of parameters sent to callback OnProtoLoaded().
@@ -139,11 +142,13 @@ class DriveFeedLoader {
   //
   // See comments at DriveFeedProcessor::ApplyFeeds() for
   // |start_changestamp| and |root_feed_changestamp|.
+  // |root_resource_id| is used for Drive API.
   // |update_finished_callback| must not be null.
   void UpdateFromFeed(
     const ScopedVector<google_apis::DocumentFeed>& feed_list,
     int64 start_changestamp,
     int64 root_feed_changestamp,
+    const std::string& root_resource_id,
     const base::Closure& update_finished_callback);
 
   // Indicates whether there is a feed refreshing server request is in flight.
diff --git a/chrome/browser/chromeos/drive/drive_file_system_unittest.cc b/chrome/browser/chromeos/drive/drive_file_system_unittest.cc
index 90c06b7..d8708d1 100644
--- a/chrome/browser/chromeos/drive/drive_file_system_unittest.cc
+++ b/chrome/browser/chromeos/drive/drive_file_system_unittest.cc
@@ -628,7 +628,7 @@ class DriveFileSystemTest : public testing::Test {
     DriveEntryProto* dir_base = root_dir->mutable_drive_entry();
     PlatformFileInfoProto* platform_info = dir_base->mutable_file_info();
     dir_base->set_title("drive");
-    dir_base->set_resource_id(kDriveRootDirectoryResourceId);
+    dir_base->set_resource_id(kWAPIRootDirectoryResourceId);
     dir_base->set_upload_url("http://resumable-create-media/1");
     platform_info->set_is_directory(true);
 
@@ -876,7 +876,7 @@ TEST_F(DriveFileSystemTest, SearchRootDirectory) {
   scoped_ptr<DriveEntryProto> entry = GetEntryInfoByPathSync(
       FilePath(FILE_PATH_LITERAL(kFilePath)));
   ASSERT_TRUE(entry.get());
-  EXPECT_EQ(kDriveRootDirectoryResourceId, entry->resource_id());
+  EXPECT_EQ(kWAPIRootDirectoryResourceId, entry->resource_id());
 }
 
 TEST_F(DriveFileSystemTest, SearchExistingFile) {
@@ -2597,7 +2597,7 @@ TEST_F(DriveFileSystemTest, RequestDirectoryRefresh) {
 
   // We'll fetch documents in the root directory with its resource ID.
   EXPECT_CALL(*mock_drive_service_,
-              GetDocuments(Eq(GURL()), _, _, kDriveRootDirectoryResourceId, _))
+              GetDocuments(Eq(GURL()), _, _, kWAPIRootDirectoryResourceId, _))
       .Times(1);
   // We'll notify the directory change to the observer.
   EXPECT_CALL(*mock_directory_observer_,
diff --git a/chrome/browser/chromeos/drive/drive_resource_metadata.cc b/chrome/browser/chromeos/drive/drive_resource_metadata.cc
index 0260eb4..545cd7c 100644
--- a/chrome/browser/chromeos/drive/drive_resource_metadata.cc
+++ b/chrome/browser/chromeos/drive/drive_resource_metadata.cc
@@ -187,8 +187,8 @@ DriveResourceMetadata::DriveResourceMetadata()
       loaded_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   root_ = CreateDriveDirectory().Pass();
-  if (!google_apis::util::IsDriveV2ApiEnabled())
-    InitializeRootEntry(kDriveRootDirectoryResourceId);
+  root_->set_title(kDriveRootDirectory);
+  root_->SetBaseNameFromTitle();
 }
 
 DriveResourceMetadata::~DriveResourceMetadata() {
@@ -208,11 +208,10 @@ scoped_ptr<DriveDirectory> DriveResourceMetadata::CreateDriveDirectory() {
   return scoped_ptr<DriveDirectory>(new DriveDirectory(this));
 }
 
-void DriveResourceMetadata::InitializeRootEntry(const std::string& root_id) {
-  root_ = CreateDriveDirectory().Pass();
-  root_->set_title(kDriveRootDirectory);
-  root_->SetBaseNameFromTitle();
-  root_->set_resource_id(root_id);
+void DriveResourceMetadata::InitializeRootEntry(const std::string& id) {
+  DCHECK(!id.empty());
+  DCHECK(root_->resource_id().empty());
+  root_->set_resource_id(id);
   AddEntryToResourceMap(root_.get());
 }
 
@@ -220,11 +219,17 @@ void DriveResourceMetadata::ClearRoot() {
   if (!root_.get())
     return;
 
+  // The root is not yet initialized.
+  if (root_->resource_id().empty())
+    return;
+
   // Note that children have a reference to root_,
   // so we need to delete them here.
   root_->RemoveChildren();
   RemoveEntryFromResourceMap(root_->resource_id());
   DCHECK(resource_map_.empty());
+  // The resource_map_ should be empty here, but to make sure for non-Debug
+  // build.
   resource_map_.clear();
   root_.reset();
 }
@@ -342,7 +347,7 @@ void DriveResourceMetadata::RemoveEntryFromParent(
   DCHECK(!callback.is_null());
 
   // Disallow deletion of root.
-  if (resource_id == kDriveRootDirectoryResourceId) {
+  if (resource_id == root_->resource_id()) {
     PostFileMoveCallbackError(callback, DRIVE_FILE_ERROR_ACCESS_DENIED);
     return;
   }
@@ -660,7 +665,6 @@ void DriveResourceMetadata::InitResourceMap(
   DCHECK(!resource_metadata_db_.get());
   DCHECK(!callback.is_null());
 
-
   SerializedMap* serialized_resources = &create_params->serialized_resources;
   resource_metadata_db_ = create_params->db.Pass();
   if (serialized_resources->empty()) {
@@ -668,6 +672,8 @@ void DriveResourceMetadata::InitResourceMap(
     return;
   }
 
+  // Save root directory resource ID as ClearRoot() resets |root_|.
+  std::string saved_root_resource_id = root_->resource_id();
   ClearRoot();
 
   // Version check.
@@ -729,7 +735,7 @@ void DriveResourceMetadata::InitResourceMap(
       } else {
         NOTREACHED() << "Parent is not a directory " << parent->resource_id();
       }
-    } else if (entry->resource_id() == kDriveRootDirectoryResourceId) {
+    } else if (entry->resource_id() == saved_root_resource_id) {
       root_.reset(entry->AsDriveDirectory());
       DCHECK(root_.get());
       AddEntryToResourceMap(root_.get());
@@ -815,6 +821,9 @@ bool DriveResourceMetadata::ParseFromString(
   }
 
   root_->FromProto(proto.drive_directory());
+  // Call AddEntryToResourceMap() instead of InitializeRootEntry() as the root
+  // resource ID is already set from proto.
+  AddEntryToResourceMap(root_.get());
 
   loaded_ = true;
   largest_changestamp_ = proto.largest_changestamp();
diff --git a/chrome/browser/chromeos/drive/drive_resource_metadata.h b/chrome/browser/chromeos/drive/drive_resource_metadata.h
index 950b05d..d6e7a72 100644
--- a/chrome/browser/chromeos/drive/drive_resource_metadata.h
+++ b/chrome/browser/chromeos/drive/drive_resource_metadata.h
@@ -48,9 +48,11 @@ enum DriveFileType {
 // name is used in URLs for the file manager, hence user-visible.
 const FilePath::CharType kDriveRootDirectory[] = FILE_PATH_LITERAL("drive");
 
-// The resource ID for the root directory is defined in the spec:
+// The resource ID for the root directory for WAPI is defined in the spec:
 // https://developers.google.com/google-apps/documents-list/
-const char kDriveRootDirectoryResourceId[] = "folder:root";
+// Note that this special ID only applies to WAPI. Drive uses a non-constant
+// unique ID given in About resource.
+const char kWAPIRootDirectoryResourceId[] = "folder:root";
 
 // This should be incremented when incompatibility change is made in
 // drive.proto.
@@ -146,8 +148,8 @@ class DriveResourceMetadata {
   // Creates a DriveDirectory instance.
   scoped_ptr<DriveDirectory> CreateDriveDirectory();
 
-  // Sets root directory resource id and initialize the root entry.
-  void InitializeRootEntry(const std::string& root_id);
+  // Sets root directory resource ID and put root to ResourceMap.
+  void InitializeRootEntry(const std::string& id);
 
   // Add |doc entry| to directory with path |directory_path| and invoke the
   // callback asynchronously.
diff --git a/chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc b/chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc
index 2591e06..af89f22 100644
--- a/chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc
+++ b/chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc
@@ -75,6 +75,8 @@ DriveResourceMetadataTest::DriveResourceMetadataTest()
 }
 
 void DriveResourceMetadataTest::Init() {
+  resource_metadata_.InitializeRootEntry(kWAPIRootDirectoryResourceId);
+
   int sequence_id = 1;
   DriveDirectory* dir1 = AddDirectory(resource_metadata_.root(), sequence_id++);
   DriveDirectory* dir2 = AddDirectory(resource_metadata_.root(), sequence_id++);
@@ -124,7 +126,7 @@ TEST_F(DriveResourceMetadataTest, VersionCheck) {
   DriveEntryProto* mutable_entry =
       proto.mutable_drive_directory()->mutable_drive_entry();
   mutable_entry->mutable_file_info()->set_is_directory(true);
-  mutable_entry->set_resource_id(kDriveRootDirectoryResourceId);
+  mutable_entry->set_resource_id(kWAPIRootDirectoryResourceId);
   mutable_entry->set_upload_url(kResumableCreateMediaUrl);
   mutable_entry->set_title("drive");
 
@@ -158,9 +160,15 @@ TEST_F(DriveResourceMetadataTest, GetEntryByResourceId_RootDirectory) {
   DriveResourceMetadata resource_metadata;
   // Look up the root directory by its resource ID.
   DriveEntry* entry = resource_metadata.GetEntryByResourceId(
-      kDriveRootDirectoryResourceId);
+      kWAPIRootDirectoryResourceId);
+  ASSERT_FALSE(entry);
+  // Initialize root and look it up again.
+  resource_metadata.InitializeRootEntry(kWAPIRootDirectoryResourceId);
+  entry = resource_metadata.GetEntryByResourceId(
+      kWAPIRootDirectoryResourceId);
   ASSERT_TRUE(entry);
-  EXPECT_EQ(kDriveRootDirectoryResourceId, entry->resource_id());
+
+  EXPECT_EQ(kWAPIRootDirectoryResourceId, entry->resource_id());
 }
 
 TEST_F(DriveResourceMetadataTest, GetEntryInfoByResourceId) {
@@ -351,6 +359,7 @@ TEST_F(DriveResourceMetadataTest, DBTest) {
 
   // InitFromDB should succeed.
   DriveResourceMetadata test_resource_metadata;
+  test_resource_metadata.InitializeRootEntry(kWAPIRootDirectoryResourceId);
   test_resource_metadata.InitFromDB(db_path, blocking_task_runner,
       base::Bind(&InitFromDBCallback, DRIVE_FILE_OK));
   google_apis::test_util::RunBlockingPoolTask();
@@ -442,7 +451,7 @@ TEST_F(DriveResourceMetadataTest, RemoveEntryFromParent) {
 
   // Try removing root. This should fail.
   resource_metadata_.RemoveEntryFromParent(
-      kDriveRootDirectoryResourceId,
+      resource_metadata_.root()->resource_id(),
       base::Bind(&test_util::CopyResultsFromFileMoveCallback,
           &error, &drive_file_path));
   google_apis::test_util::RunBlockingPoolTask();
diff --git a/chrome/browser/chromeos/drive/drive_test_util.cc b/chrome/browser/chromeos/drive/drive_test_util.cc
index 7aaeb96..27a788e 100644
--- a/chrome/browser/chromeos/drive/drive_test_util.cc
+++ b/chrome/browser/chromeos/drive/drive_test_util.cc
@@ -131,6 +131,7 @@ bool LoadChangeFeed(const std::string& relative_path,
       feed_list,
       start_changestamp,
       root_feed_changestamp,
+      kWAPIRootDirectoryResourceId,
       base::Bind(&base::DoNothing));
   // DriveFeedLoader::UpdateFromFeed is asynchronous, so wait for it to finish.
   google_apis::test_util::RunBlockingPoolTask();