drive: Fills no-parent entiries in "/drive/other".

drive: Fills no-parent entiries in "/drive/other".

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


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

[Haruki Sato, 2013-04-02 07:53:29]

This is the CL following https://codereview.chromium.org/13149003/
and now we have effective entries in /drive/other.

[kinaba, 2013-04-03 05:39:33]

https://codereview.chromium.org/13456002/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/resource_entry_conversion.cc (right):

https://codereview.chromium.org/13456002/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/resource_entry_conversion.cc:64:
entry_proto.set_parent_resource_id(util::kDriveOtherDirSpecialResourceId);
I'm not sure if this is the right place for this mapping,
because to me it seems to be a detail of ResourceMetadata that
it organizes orphan entries under drive/other.

Can we move the mapping to, say, ChangeListProcessor?

[kinaba, 2013-04-03 05:59:55]

lgtm

https://codereview.chromium.org/13456002/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/resource_entry_conversion.cc (right):

https://codereview.chromium.org/13456002/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/resource_entry_conversion.cc:64:
entry_proto.set_parent_resource_id(util::kDriveOtherDirSpecialResourceId);
On 2013/04/03 05:39:33, kinaba wrote:
> I'm not sure if this is the right place for this mapping,
> because to me it seems to be a detail of ResourceMetadata that
> it organizes orphan entries under drive/other.
> 
> Can we move the mapping to, say, ChangeListProcessor?

Chatted offline, and inclined to feel ok to put it here.
(Many part of the Drive file system is touching directly resource metadata,
and it may not be good to add logic to resource metadata).

Instead, could you add a comment here (together with the conversion of
parent_link above)
that we are forcing a tree-shaped structure (= no multi-parent or zero-parent
entries),
maybe linking to http://crbug.com/158904?

[Haruki Sato, 2013-04-03 06:49:43]

https://codereview.chromium.org/13456002/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/resource_entry_conversion.cc (right):

https://codereview.chromium.org/13456002/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/resource_entry_conversion.cc:64:
entry_proto.set_parent_resource_id(util::kDriveOtherDirSpecialResourceId);
On 2013/04/03 05:59:55, kinaba wrote:
> On 2013/04/03 05:39:33, kinaba wrote:
> > I'm not sure if this is the right place for this mapping,
> > because to me it seems to be a detail of ResourceMetadata that
> > it organizes orphan entries under drive/other.
> > 
> > Can we move the mapping to, say, ChangeListProcessor?
> 
> Chatted offline, and inclined to feel ok to put it here.
> (Many part of the Drive file system is touching directly resource metadata,
> and it may not be good to add logic to resource metadata).
> 
> Instead, could you add a comment here (together with the conversion of
> parent_link above)
> that we are forcing a tree-shaped structure (= no multi-parent or zero-parent
> entries),
> maybe linking to http://crbug.com/158904?
> 

Done.

[hidehiko, 2013-04-03 07:55:40]

lgtm with a nitpick.

https://codereview.chromium.org/13456002/diff/6001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/resource_entry_conversion_unittest.cc
(right):

https://codereview.chromium.org/13456002/diff/6001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/resource_entry_conversion_unittest.cc:35:
EXPECT_EQ("<other>",  entry_proto.parent_resource_id());
nit: using util::kDriveOtherDirSpecialResourceId may be better?

[Haruki Sato, 2013-04-04 06:16:06]

https://codereview.chromium.org/13456002/diff/6001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/resource_entry_conversion_unittest.cc
(right):

https://codereview.chromium.org/13456002/diff/6001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/resource_entry_conversion_unittest.cc:35:
EXPECT_EQ("<other>",  entry_proto.parent_resource_id());
On 2013/04/03 07:55:40, hidehiko wrote:
> nit: using util::kDriveOtherDirSpecialResourceId may be better?

Done. Thanks.

[commit-bot: I haz the power, 2013-04-05 13:47:23]

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

[commit-bot: I haz the power, 2013-04-05 13:47:25]

Failed to apply patch for chrome/browser/chromeos/drive/drive_file_system.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/drive/drive_file_system.cc
  Hunk #1 succeeded at 1090 (offset 76 lines).
  Hunk #2 FAILED at 1115.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/drive/drive_file_system.cc.rej

Patch:       chrome/browser/chromeos/drive/drive_file_system.cc
Index: chrome/browser/chromeos/drive/drive_file_system.cc
diff --git a/chrome/browser/chromeos/drive/drive_file_system.cc
b/chrome/browser/chromeos/drive/drive_file_system.cc
index
5ef2702467fd6ff01d5e21bf2698a90469932ac9..224349097ff586a7c8aca645ea03004d9f31532b
100644
--- a/chrome/browser/chromeos/drive/drive_file_system.cc
+++ b/chrome/browser/chromeos/drive/drive_file_system.cc
@@ -1014,6 +1014,11 @@ void DriveFileSystem::RefreshDirectoryAfterGetEntryInfo(
     callback.Run(DRIVE_FILE_ERROR_NOT_A_DIRECTORY);
     return;
   }
+  if (util::IsSpecialResourceId(entry_proto->resource_id())) {
+    // Do not load special directories. Just return.
+    callback.Run(DRIVE_FILE_OK);
+    return;
+  }
 
   change_list_loader_->LoadDirectoryFromServer(
       entry_proto->resource_id(),
@@ -1110,17 +1115,7 @@ void DriveFileSystem::OnSearch(bool shared_with_me,
                    should_run_callback,
                    callback);
 
-    // Some entries (e.g. shared_with_me files) are not tracked in
-    // DriveResourceMetadata, so that we skip RefreshEntry() and call the
-    // callback directly.
-    if (!shared_with_me) {
-      resource_metadata_->RefreshEntry(entry_proto, entry_info_callback);
-    } else {
-      entry_info_callback.Run(
-          DRIVE_FILE_OK,
-          base::FilePath::FromUTF8Unsafe(entry_proto.base_name()),
-          scoped_ptr<DriveEntryProto>(new DriveEntryProto(entry_proto)));
-    }
+    resource_metadata_->RefreshEntry(entry_proto, entry_info_callback);
   }
 }

[commit-bot: I haz the power, 2013-04-05 14:23:52]

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

[commit-bot: I haz the power, 2013-04-05 16:59:11]

Change committed as 192583