drive: Use "/drive/root" namespace and fix Files app and tests.

drive: Use "/drive/root" namespace and fix Files app and tests.

Create "/drive/root" at DriveResourceMetadata initialization.
"/drive/other" is also created but is empty for now.

BUG=174233
TEST=unittests. Open Files App and verify Google Drive is available.


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

[Haruki Sato, 2013-03-29 08:02:37]

Thanks to your help, changes for test is factored out.
Could you take a look?
This could be checked in after the non-refactoring period.

Thanks.

[hidehiko, 2013-04-01 04:21:51]

lgtm with a nit.

https://codereview.chromium.org/13149003/diff/5001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

https://codereview.chromium.org/13149003/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:1103: 
Don't need to edit?

[kinaba, 2013-04-01 04:30:35]

lgtm

[Haruki Sato, 2013-04-02 07:54:18]

Thank you for the review.
Some browser_tests are fixed.
I also just uploaded the following CL
https://codereview.chromium.org/13456002/
.
Could you take a look?

Thanks.

https://codereview.chromium.org/13149003/diff/5001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

https://codereview.chromium.org/13149003/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:1103: 
On 2013/04/01 04:21:51, hidehiko wrote:
> Don't need to edit?

Done. Thanks.

[hidehiko, 2013-04-02 08:09:29]

Two nitpicks.

https://codereview.chromium.org/13149003/diff/21001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

https://codereview.chromium.org/13149003/diff/21001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:435: base::FilePath
file_path;
Unnecessary variable?

https://codereview.chromium.org/13149003/diff/21001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/file_browser/file_watcher_test/test.js
(right):

https://codereview.chromium.org/13149003/diff/21001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_browser/file_watcher_test/test.js:236:
function getPath(relativePath, isOnDrive) {
You may want to add a new shared utility?

[Haruki Sato, 2013-04-03 04:33:41]

Thank you for the review.

Abe-san, Kinaba-san, 
Do you know how to add a JS lib shared among ExtensionBrowserTests?

https://codereview.chromium.org/13149003/diff/21001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

https://codereview.chromium.org/13149003/diff/21001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:435: base::FilePath
file_path;
On 2013/04/02 08:09:30, hidehiko wrote:
> Unnecessary variable?
It actually is necessary one, just moved from L441.
Its pointer is passed to CreateCopyResultCallback and the result is ignored.
Do you think it should be in L441?

https://codereview.chromium.org/13149003/diff/21001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/file_browser/file_watcher_test/test.js
(right):

https://codereview.chromium.org/13149003/diff/21001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_browser/file_watcher_test/test.js:236:
function getPath(relativePath, isOnDrive) {
On 2013/04/02 08:09:30, hidehiko wrote:
> You may want to add a new shared utility?

Thank you for the comment. Seems like I have this function twice in this CL.

Do you know how? or someone who might know it?

[kinaba, 2013-04-03 04:54:44]

On 2013/04/03 04:33:41, Haruki Sato wrote:
> Thank you for the review.
> 
> Abe-san, Kinaba-san, 
> Do you know how to add a JS lib shared among ExtensionBrowserTests?

No idea. In existing tests I see test_util.js is duplicated below
chrome/test/data/extensions/api_test/file_system/,
so there may be fundamentally no way to do that.

But I'm not sure because I'm totally unfamiliar here.

[commit-bot: I haz the power, 2013-04-04 02:52:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/13149003/21001

[commit-bot: I haz the power, 2013-04-04 02:52:48]

Failed to apply patch for
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/drive/drive_file_system_unittest.cc
  Hunk #1 FAILED at 127.
  Hunk #2 FAILED at 139.
  Hunk #3 succeeded at 308 (offset -89 lines).
  Hunk #4 succeeded at 340 (offset -89 lines).
  Hunk #5 succeeded at 354 (offset -89 lines).
  Hunk #6 succeeded at 361 (offset -89 lines).
  Hunk #7 succeeded at 375 (offset -89 lines).
  Hunk #8 succeeded at 391 (offset -89 lines).
  Hunk #9 succeeded at 405 (offset -89 lines).
  Hunk #10 succeeded at 493 (offset -89 lines).
  Hunk #11 succeeded at 503 (offset -89 lines).
  Hunk #12 succeeded at 525 (offset -89 lines).
  Hunk #13 succeeded at 566 (offset -89 lines).
  Hunk #14 succeeded at 581 (offset -89 lines).
  Hunk #15 succeeded at 591 (offset -89 lines).
  Hunk #16 succeeded at 601 (offset -89 lines).
  Hunk #17 succeeded at 610 (offset -89 lines).
  Hunk #18 succeeded at 631 (offset -89 lines).
  Hunk #19 succeeded at 652 (offset -89 lines).
  Hunk #20 succeeded at 677 (offset -89 lines).
  Hunk #21 succeeded at 691 (offset -89 lines).
  Hunk #22 succeeded at 702 (offset -89 lines).
  Hunk #23 succeeded at 710 (offset -89 lines).
  Hunk #24 succeeded at 725 (offset -89 lines).
  Hunk #25 succeeded at 737 (offset -89 lines).
  Hunk #26 succeeded at 764 (offset -89 lines).
  Hunk #27 succeeded at 823 (offset -89 lines).
  Hunk #28 succeeded at 869 (offset -89 lines).
  Hunk #29 succeeded at 928 (offset -89 lines).
  Hunk #30 succeeded at 994 (offset -89 lines).
  Hunk #31 succeeded at 1006 (offset -89 lines).
  Hunk #32 succeeded at 1042 (offset -89 lines).
  Hunk #33 succeeded at 1081 (offset -89 lines).
  Hunk #34 succeeded at 1135 (offset -89 lines).
  Hunk #35 succeeded at 1152 (offset -89 lines).
  Hunk #36 succeeded at 1172 (offset -89 lines).
  Hunk #37 succeeded at 1204 (offset -89 lines).
  Hunk #38 succeeded at 1242 (offset -89 lines).
  Hunk #39 succeeded at 1258 (offset -89 lines).
  Hunk #40 succeeded at 1275 (offset -89 lines).
  Hunk #41 succeeded at 1299 (offset -89 lines).
  Hunk #42 succeeded at 1318 (offset -89 lines).
  Hunk #43 succeeded at 1342 (offset -89 lines).
  Hunk #44 succeeded at 1361 (offset -89 lines).
  Hunk #45 succeeded at 1403 (offset -89 lines).
  Hunk #46 succeeded at 1426 (offset -89 lines).
  Hunk #47 succeeded at 1446 (offset -89 lines).
  Hunk #48 succeeded at 1477 (offset -89 lines).
  Hunk #49 succeeded at 1514 (offset -89 lines).
  Hunk #50 succeeded at 1539 (offset -89 lines).
  Hunk #51 succeeded at 1560 (offset -89 lines).
  Hunk #52 succeeded at 1570 (offset -89 lines).
  Hunk #53 succeeded at 1595 (offset -89 lines).
  Hunk #54 succeeded at 1614 (offset -89 lines).
  Hunk #55 succeeded at 1640 (offset -89 lines).
  Hunk #56 succeeded at 1664 (offset -89 lines).
  Hunk #57 succeeded at 1716 (offset -89 lines).
  Hunk #58 succeeded at 1746 (offset -89 lines).
  Hunk #59 succeeded at 1775 (offset -89 lines).
  Hunk #60 succeeded at 1802 (offset -89 lines).
  Hunk #61 succeeded at 1826 (offset -89 lines).
  Hunk #62 succeeded at 1836 (offset -89 lines).
  Hunk #63 succeeded at 1875 (offset -89 lines).
  Hunk #64 succeeded at 1919 (offset -89 lines).
  Hunk #65 succeeded at 1942 (offset -89 lines).
  Hunk #66 succeeded at 1962 (offset -89 lines).
  Hunk #67 succeeded at 1997 (offset -89 lines).
  Hunk #68 succeeded at 2062 (offset -89 lines).
  Hunk #69 succeeded at 2079 (offset -89 lines).
  Hunk #70 succeeded at 2122 (offset -89 lines).
  2 out of 70 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/drive/drive_file_system_unittest.cc.rej

Patch:       chrome/browser/chromeos/drive/drive_file_system_unittest.cc
Index: chrome/browser/chromeos/drive/drive_file_system_unittest.cc
diff --git a/chrome/browser/chromeos/drive/drive_file_system_unittest.cc
b/chrome/browser/chromeos/drive/drive_file_system_unittest.cc
index
96340a1c1b8cea881ada4d759e58450ed11a6594..52ff6376b8f29720c386ade8836894be857fd6ab
100644
--- a/chrome/browser/chromeos/drive/drive_file_system_unittest.cc
+++ b/chrome/browser/chromeos/drive/drive_file_system_unittest.cc
@@ -127,7 +127,7 @@ class FakeDriveUploader : public
google_apis::DriveUploaderInterface {
                    base::Passed(&resource_entry)));
   }
 
-  // Pretends that an existing file ("drive/File 1.txt") was uploaded
+  // Pretends that an existing file ("drive/root/File 1.txt") was uploaded
   // successfully, and returns an entry for the file in
   // "chromeos/gdata/root_feed.json" to the caller.
   virtual void UploadExistingFile(
@@ -139,9 +139,9 @@ class FakeDriveUploader : public
google_apis::DriveUploaderInterface {
       const google_apis::UploadCompletionCallback& callback) OVERRIDE {
     DCHECK(!callback.is_null());
 
-    // This function can only handle "drive/File 1.txt" whose resource ID is
-    // "file:2_file_resource_id".
-    DCHECK_EQ("drive/File 1.txt", drive_file_path.value());
+    // This function can only handle "drive/root/File 1.txt" whose resource ID
+    // is "file:2_file_resource_id".
+    DCHECK_EQ("drive/root/File 1.txt", drive_file_path.value());
     const std::string kResourceId = "file:2_file_resource_id";
     EXPECT_EQ(kResourceId, resource_id);
 
@@ -397,9 +397,9 @@ class DriveFileSystemTest : public testing::Test {
   };
 
   // Saves a file representing a filesystem with directories:
-  // drive, drive/Dir1, drive/Dir1/SubDir2
+  // drive/root, drive/root/Dir1, drive/root/Dir1/SubDir2
   // and files
-  // drive/File1, drive/Dir1/File2, drive/Dir1/SubDir2/File3.
+  // drive/root/File1, drive/root/Dir1/File2, drive/root/Dir1/SubDir2/File3.
   // If |use_up_to_date_timestamp| is true, sets the changestamp to 654321,
   // equal to that of "account_metadata.json" test data, indicating the cache
is
   // holding the latest file system info.
@@ -429,7 +429,12 @@ class DriveFileSystemTest : public testing::Test {
     if (error != DRIVE_FILE_OK)
       return false;
 
-    // drive/File1
+    // drive/root is already prepared by DriveResourceMetadata.
+    // TODO(haruki): Create drive/root here when we start creating it in
+    // ChangeListLoader.
+    base::FilePath file_path;
+
+    // drive/root/File1
     DriveEntryProto file1;
     file1.set_title("File1");
     file1.set_resource_id("resource_id:File1");
@@ -438,7 +443,6 @@ class DriveFileSystemTest : public testing::Test {
     file1.mutable_file_specific_info()->set_file_md5("md5");
     file1.mutable_file_info()->set_is_directory(false);
     file1.mutable_file_info()->set_size(1048576);
-    base::FilePath file_path;
     resource_metadata->AddEntry(
         file1,
         google_apis::test_util::CreateCopyResultCallback(&error, &file_path));
@@ -446,7 +450,7 @@ class DriveFileSystemTest : public testing::Test {
     if (error != DRIVE_FILE_OK)
       return false;
 
-    // drive/Dir1
+    // drive/root/Dir1
     DriveEntryProto dir1;
     dir1.set_title("Dir1");
     dir1.set_resource_id("resource_id:Dir1");
@@ -460,7 +464,7 @@ class DriveFileSystemTest : public testing::Test {
     if (error != DRIVE_FILE_OK)
       return false;
 
-    // drive/Dir1/File2
+    // drive/root/Dir1/File2
     DriveEntryProto file2;
     file2.set_title("File2");
     file2.set_resource_id("resource_id:File2");
@@ -476,7 +480,7 @@ class DriveFileSystemTest : public testing::Test {
     if (error != DRIVE_FILE_OK)
       return false;
 
-    // drive/Dir1/SubDir2
+    // drive/root/Dir1/SubDir2
     DriveEntryProto dir2;
     dir2.set_title("SubDir2");
     dir2.set_resource_id("resource_id:SubDir2");
@@ -490,7 +494,7 @@ class DriveFileSystemTest : public testing::Test {
     if (error != DRIVE_FILE_OK)
       return false;
 
-    // drive/Dir1/SubDir2/File3
+    // drive/root/Dir1/SubDir2/File3
     DriveEntryProto file3;
     file3.set_title("File3");
     file3.set_resource_id("resource_id:File3");
@@ -578,7 +582,7 @@ void AsyncInitializationCallback(
 TEST_F(DriveFileSystemTest, DuplicatedAsyncInitialization) {
   // The root directory will be loaded that triggers the event.
   EXPECT_CALL(*mock_directory_observer_, OnDirectoryChanged(
-      Eq(base::FilePath(FILE_PATH_LITERAL("drive"))))).Times(1);
+      Eq(base::FilePath(FILE_PATH_LITERAL("drive/root"))))).Times(1);
 
   int counter = 0;
   ReadDirectoryWithSettingCallback callback = base::Bind(
@@ -588,9 +592,9 @@ TEST_F(DriveFileSystemTest, DuplicatedAsyncInitialization) {
       &message_loop_);
 
   file_system_->ReadDirectoryByPath(
-      base::FilePath(FILE_PATH_LITERAL("drive")), callback);
+      base::FilePath(FILE_PATH_LITERAL("drive/root")), callback);
   file_system_->ReadDirectoryByPath(
-      base::FilePath(FILE_PATH_LITERAL("drive")), callback);
+      base::FilePath(FILE_PATH_LITERAL("drive/root")), callback);
   message_loop_.Run();  // Wait to get our result
   EXPECT_EQ(2, counter);
 
@@ -610,20 +614,34 @@ TEST_F(DriveFileSystemTest, DuplicatedAsyncInitialization)
{
       << ": " << fake_drive_service_->resource_list_load_count();
 }
 
-TEST_F(DriveFileSystemTest, GetRootEntry) {
-  const base::FilePath kFilePath = base::FilePath(FILE_PATH_LITERAL("drive"));
+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);
   A…
(message too large)

[commit-bot: I haz the power, 2013-04-04 03:08:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/13149003/35001

[commit-bot: I haz the power, 2013-04-04 03:08:36]

Presubmit check for 13149003-35001 failed and returned exit status 1.

INFO:root:Found 19 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/ui/webui/chromeos/drive_internals_ui.cc

Presubmit checks took 3.1s to calculate.

[Haruki Sato, 2013-04-04 03:11:48]

Satoru-san, could you OWNER-review for chrome/browser/ui/webui/chromeos/OWNERS ?

[satorux1, 2013-04-05 01:55:30]

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:540:
TEST_F(DriveFileSystemTest, GetMyDriveRootEntry) {
Shouldn't we have a test for drive/other too?

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_prefetcher_unittest.cc (right):

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_prefetcher_unittest.cc:102: const TestEntry
kComplexDrive[] = {
The table became a bit harder to read. Can you do:

#define FPL FILE_PATH_LITERAL
...
#undef FPL

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc (right):

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:1236: // There
is only "other", which is empty.
Why was "drive/root" removed here?

https://codereview.chromium.org/13149003/diff/35001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/file_browser/handler_test_runner/test.js
(right):

https://codereview.chromium.org/13149003/diff/35001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_browser/handler_test_runner/test.js:12:
*   on the test mount point.
no need to change the line?

[Haruki Sato, 2013-04-05 04:35:25]

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:540:
TEST_F(DriveFileSystemTest, GetMyDriveRootEntry) {
On 2013/04/05 01:55:30, satorux1 wrote:
> Shouldn't we have a test for drive/other too?

Done.

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_prefetcher_unittest.cc (right):

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_prefetcher_unittest.cc:102: const TestEntry
kComplexDrive[] = {
On 2013/04/05 01:55:30, satorux1 wrote:
> The table became a bit harder to read. Can you do:
> 
> #define FPL FILE_PATH_LITERAL
> ...
> #undef FPL

Done.

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc (right):

https://codereview.chromium.org/13149003/diff/35001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:1236: // There
is only "other", which is empty.
On 2013/04/05 01:55:30, satorux1 wrote:
> Why was "drive/root" removed here?

Thank you for the comment.

This was from the original CL we split from, where we added "/drive/root" in
ChangeListLoader.
In this intermediate CL, we are creating it in DriveResourceMetadata, but seems
like I forgot to update this unittest.
I revisited this unittest and changed it reflecting the current (in this CL)
usage.

https://codereview.chromium.org/13149003/diff/35001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/file_browser/handler_test_runner/test.js
(right):

https://codereview.chromium.org/13149003/diff/35001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_browser/handler_test_runner/test.js:12:
*   on the test mount point.
On 2013/04/05 01:55:30, satorux1 wrote:
> no need to change the line?

Good catch. Thank you!

[satorux1, 2013-04-05 04:45:01]

hmm, Patch Set 7 contains a lot of unnecessary changes...

Could you remove Patch Set 7 and upload two patch sets for 1) just rebase only
2) your change.

[satorux1, 2013-04-05 04:45:16]

I meant unrelated changes.

[Haruki Sato, 2013-04-05 05:48:28]

On 2013/04/05 04:45:16, satorux1 wrote:
> I meant unrelated changes.

How is this now? Does it work for you?

[satorux1, 2013-04-05 05:50:24]

LGTM

[commit-bot: I haz the power, 2013-04-05 05:51:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/13149003/60004

[commit-bot: I haz the power, 2013-04-05 06:55:39]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-05 07:17:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/13149003/21002

[commit-bot: I haz the power, 2013-04-05 13:16:56]

Change committed as 192554