Remove root resource id aliasing from DriveResourceMetadata.

chrome/browser/chromeos/drive/drive_file_system_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/drive/drive_file_system.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 124 matching lines @@
     mock_directory_observer_.reset(new StrictMock<MockDirectoryChangeObserver>);
 
     cache_->RequestInitializeForTesting();
     google_apis::test_util::RunBlockingPoolTask();
 
     SetUpResourceMetadataAndFileSystem();
   }
 
   void SetUpResourceMetadataAndFileSystem() {
     resource_metadata_.reset(new DriveResourceMetadata(
-        fake_drive_service_->GetRootResourceId(),
         cache_->GetCacheDirectoryPath(DriveCache::CACHE_TYPE_META),
         blocking_task_runner_));
 
     file_system_.reset(new DriveFileSystem(profile_.get(),
                                            cache_.get(),
                                            fake_drive_service_.get(),
                                            drive_webapps_registry_.get(),
                                            resource_metadata_.get(),
                                            blocking_task_runner_));
     file_system_->AddObserver(mock_directory_observer_.get());
@@ skipping 156 matching lines @@
   // equal to that of "account_metadata.json" test data, indicating the cache is
   // holding the latest file system info.
   bool SaveTestFileSystem(SaveTestFileSystemParam param) {
     // Destroy the existing resource metadata to close DB.
     resource_metadata_.reset();
 
     const std::string root_resource_id =
         fake_drive_service_->GetRootResourceId();
     scoped_ptr<DriveResourceMetadata, test_util::DestroyHelperForTests>
         resource_metadata(new DriveResourceMetadata(
-            root_resource_id,
             cache_->GetCacheDirectoryPath(DriveCache::CACHE_TYPE_META),
             blocking_task_runner_));
 
     DriveFileError error = DRIVE_FILE_ERROR_FAILED;
     resource_metadata->Initialize(
         google_apis::test_util::CreateCopyResultCallback(&error));
     google_apis::test_util::RunBlockingPoolTask();
     if (error != DRIVE_FILE_OK)
       return false;
 
     resource_metadata->SetLargestChangestamp(
         param == USE_SERVER_TIMESTAMP ? 654321 : 1,
         google_apis::test_util::CreateCopyResultCallback(&error));
     google_apis::test_util::RunBlockingPoolTask();
     if (error != DRIVE_FILE_OK)
       return false;
 
     // drive/root is already prepared by DriveResourceMetadata.
     // TODO(haruki): Create drive/root here when we start creating it in

[Haruki Sato, 2013/04/10 03:44:35]

Could you remove this TODO?
Thanks!

[hidehiko, 2013/04/10 06:11:19]

Done.

     // ChangeListLoader.
     base::FilePath file_path;
 
+    // drive/root
+    resource_metadata->AddEntry(
+        util::CreateMyDriveRootEntry(root_resource_id),
+        google_apis::test_util::CreateCopyResultCallback(&error, &file_path));
+    google_apis::test_util::RunBlockingPoolTask();
+    if (error != DRIVE_FILE_OK)
+      return false;
+
     // drive/root/File1
     DriveEntryProto file1;
     file1.set_title("File1");
     file1.set_resource_id("resource_id:File1");
     file1.set_parent_resource_id(root_resource_id);
     file1.mutable_file_specific_info()->set_file_md5("md5");
     file1.mutable_file_info()->set_is_directory(false);
     file1.mutable_file_info()->set_size(1048576);
     resource_metadata->AddEntry(
         file1,
@@ skipping 119 matching lines @@
     scoped_ptr<DriveEntryProtoVector> entries) {
   ASSERT_EQ(DRIVE_FILE_OK, error);
   ASSERT_TRUE(entries.get());
   ASSERT_FALSE(entries->empty());
 
   (*counter)++;
   if (*counter >= expected_counter)
     message_loop->Quit();
 }
 
 TEST_F(DriveFileSystemTest, DuplicatedAsyncInitialization) {

[kinaba, 2013/04/10 04:34:44]

As far as I recall, original intent of this test was to
assure that more than one full feed fetch will never run parallel.

Probably we should use GetEntryInfoByPath("drive/root")
instead of ReadDirectoryByPath("drive") or ReadDirectoryByPath("drive/root"),
so that it is not complicated by the fast-fetch mode.

[hidehiko, 2013/04/10 06:11:19]

Good to know. Done. Also removed EXPECT_CALL below, because full fetch won't
trigger the OnDirectoryChanged event.

   // The root directory will be loaded that triggers the event.
   EXPECT_CALL(*mock_directory_observer_, OnDirectoryChanged(
-      Eq(base::FilePath(FILE_PATH_LITERAL("drive/root"))))).Times(1);
+      Eq(base::FilePath(FILE_PATH_LITERAL("drive"))))).Times(1);
 
   int counter = 0;
   ReadDirectoryWithSettingCallback callback = base::Bind(
       &AsyncInitializationCallback,
       &counter,
       2,
       &message_loop_);
 
   file_system_->ReadDirectoryByPath(
-      base::FilePath(FILE_PATH_LITERAL("drive/root")), callback);
+      base::FilePath(FILE_PATH_LITERAL("drive")), callback);
   file_system_->ReadDirectoryByPath(
-      base::FilePath(FILE_PATH_LITERAL("drive/root")), callback);
+      base::FilePath(FILE_PATH_LITERAL("drive")), callback);
   message_loop_.Run();  // Wait to get our result
   EXPECT_EQ(2, counter);
 
   // ReadDirectoryByPath() was called twice, but the account metadata should
   // only be loaded once. In the past, there was a bug that caused it to be
   // loaded twice.
-  EXPECT_EQ(1, fake_drive_service_->about_resource_load_count());
+  // The actual number of GetAboutResource invocation should be two;
+  // one for checking if it is necessary to load the data from server or not,
+  // and the other for creating mydrive root entry.
+  EXPECT_EQ(2, fake_drive_service_->about_resource_load_count());
   // On the other hand, the resource list could be loaded twice. One for
   // just the directory contents, and one for the entire resource list.
   //
   // The |callback| function gets called back soon after the directory content
   // is loaded, and the full resource load is done in background asynchronously.
   // So it depends on timing whether we receive the full resource load request
   // at this point.
   EXPECT_TRUE(fake_drive_service_->resource_list_load_count() == 1 ||
               fake_drive_service_->resource_list_load_count() == 2)

[kinaba, 2013/04/10 04:34:44]

Besides, this ambiguity will go if we switch to GetEntryInfoByPath.

[hidehiko, 2013/04/10 06:11:19]

Done.

       << ": " << fake_drive_service_->resource_list_load_count();
 }
 
 TEST_F(DriveFileSystemTest, GetGrandRootEntry) {
   const base::FilePath kFilePath =
       base::FilePath(FILE_PATH_LITERAL("drive"));
   scoped_ptr<DriveEntryProto> entry = GetEntryInfoByPathSync(kFilePath);
   ASSERT_TRUE(entry.get());
   EXPECT_EQ(util::kDriveGrandRootSpecialResourceId, entry->resource_id());
 
   // Getting the grand root entry should not cause the resource load to happen.
   EXPECT_EQ(0, fake_drive_service_->about_resource_load_count());
   EXPECT_EQ(0, fake_drive_service_->resource_list_load_count());
 }
 
 TEST_F(DriveFileSystemTest, GetMyDriveRootEntry) {
   const base::FilePath kFilePath =
       base::FilePath(FILE_PATH_LITERAL("drive/root"));
   scoped_ptr<DriveEntryProto> entry = GetEntryInfoByPathSync(kFilePath);
   ASSERT_TRUE(entry.get());
   EXPECT_EQ(fake_drive_service_->GetRootResourceId(), entry->resource_id());
 
-  // Getting the "My Drive" root entry should not cause the resource load to
-  // happen.
-  EXPECT_EQ(0, fake_drive_service_->about_resource_load_count());
-  EXPECT_EQ(0, fake_drive_service_->resource_list_load_count());
+  // Getting the "My Drive" root entry should cause the resource load to happen.

[kinaba, 2013/04/10 04:34:44]

As far as I recall, original intent of this test was to
assure that GetEntryInfoByPath to an existing entry does not
cause remote query.

Probably it is better to change kFilePath="drive" and adjust
expectations accordingly.

[hidehiko, 2013/04/10 06:11:19]

Good to know. Just removed this case. Note that the /drive entry is tested
GetGrandRootEntry above.

+  EXPECT_EQ(1, fake_drive_service_->about_resource_load_count());
+  EXPECT_EQ(1, fake_drive_service_->resource_list_load_count());
 }
 
 TEST_F(DriveFileSystemTest, GetOtherDirEntry) {
   const base::FilePath kFilePath =
       base::FilePath(FILE_PATH_LITERAL("drive/other"));
   scoped_ptr<DriveEntryProto> entry = GetEntryInfoByPathSync(kFilePath);
   ASSERT_TRUE(entry.get());
   EXPECT_EQ(util::kDriveOtherDirSpecialResourceId, entry->resource_id());
 
   // Getting the "other" directory entry should not cause the resource load to
@@ skipping 166 matching lines @@
 
   const base::FilePath kFilePath = base::FilePath(
       FILE_PATH_LITERAL("drive/other/Orphan File 1.txt"));
   scoped_ptr<DriveEntryProto> entry = GetEntryInfoByPathSync(kFilePath);
   ASSERT_TRUE(entry.get());
   EXPECT_EQ("file:1_orphanfile_resource_id", entry->resource_id());
 }
 
 TEST_F(DriveFileSystemTest, ReadDirectoryByPath_Root) {
   EXPECT_CALL(*mock_directory_observer_, OnDirectoryChanged(
-      Eq(base::FilePath(FILE_PATH_LITERAL("drive/root"))))).Times(1);
+      Eq(base::FilePath(FILE_PATH_LITERAL("drive"))))).Times(1);
 
   // ReadDirectoryByPath() should kick off the resource list loading.
   scoped_ptr<DriveEntryProtoVector> entries(
-      ReadDirectoryByPathSync(base::FilePath::FromUTF8Unsafe("drive/root")));
+      ReadDirectoryByPathSync(base::FilePath::FromUTF8Unsafe("drive")));
   // The root directory should be read correctly.
   ASSERT_TRUE(entries.get());
-  EXPECT_EQ(8U, entries->size());
+  EXPECT_EQ(2U, entries->size());
 }

[kinaba, 2013/04/10 04:34:44]

May want to add a test verifying that "drive/root" exists
after reading "drive".

[hidehiko, 2013/04/10 06:11:19]

Done.

 
 TEST_F(DriveFileSystemTest, ReadDirectoryByPath_NonRootDirectory) {
   // ReadDirectoryByPath() should kick off the resource list loading.
   scoped_ptr<DriveEntryProtoVector> entries(
       ReadDirectoryByPathSync(
           base::FilePath::FromUTF8Unsafe("drive/root/Directory 1")));
   // The non root directory should also be read correctly.
   // There was a bug (crbug.com/181487), which broke this behavior.
   // Make sure this is fixed.
   ASSERT_TRUE(entries.get());
@@ skipping 1435 matching lines @@
 
   // An app for foo.exe should now be found, as the registry was loaded.
   drive_webapps_registry_->GetWebAppsForFile(
       base::FilePath(FILE_PATH_LITERAL("foo.exe")),
       "" /* mime_type */,
       &apps);
   EXPECT_EQ(1U, apps.size());
 }
 
 }   // namespace drive