Remove root resource id aliasing from DriveResourceMetadata.

Remove root resource id aliasing from DriveResourceMetadata.

This is a part of Drive API v2 migration. On Drive API v2, the root resource
id is not fixed, while it is fixed on GData WAPI, which is "folder:root".
By this CL, the MyDrive root entry will be created when the "drive" (grand
root) is accessed for the first time, instead of creating it in
DriveResourceMetadata::Initialize.
Along with the change, this CL also implements the Fast-Fetch behavior
in ChangeListLoader::DoLoadDirectoryFromServcer.

BUG=174237
TEST=Ran unit_tests and tested manually on chromeos-chrome


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

[hidehiko, 2013-04-09 14:02:18]

Thank you for your review in advance,
- hidehiko

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

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

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:387:
DCHECK(!directory_fetch_info.empty());
Also this method assumes it's request for <drive>?
Is it good to denote that here as well?

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:427:
resource_metadata_->RefreshDirectory(
Any reason to use RefreshDirectory rather than AddEntry?
Can we remove the creation for "other" in DriveResourceMetadata with this?

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/change_list_loader.h (right):

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.h:169: // |callback must not be
null.
|callback| ? missing "|".

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

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:339: //
TODO(haruki): Create drive/root here when we start creating it in
Could you remove this TODO?
Thanks!

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

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

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:357: // <other> directory
should always exists, but mydrive root should be
nit: exists -> exist

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:358: // created by root
resource id retreived from the server.
retrieved

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/change_list_loader.h (right):

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.h:161: void
DoLoadDirectoryFromServerAfterGetEntryInfoByPath(
How about naming these methods in a way that it explicitly tells this is
handling the special case of reading "/drive"?
And regard them as sub-routines of DoLoadDirectoryFromServer
(i.e., normal case uses LoadFromServer and special case uses the new ones).

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.h:167: // Part of
DoLoadDIrectoryFromServer. Called when GetAboutResource for mydrive
nit: DoLoadD<i>rectory

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

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:490:
TEST_F(DriveFileSystemTest, DuplicatedAsyncInitialization) {
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.

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:524:
fake_drive_service_->resource_list_load_count() == 2)
Besides, this ambiguity will go if we switch to GetEntryInfoByPath.

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:547: // Getting the
"My Drive" root entry should cause the resource load to happen.
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.

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:744: }
May want to add a test verifying that "drive/root" exists
after reading "drive".

[kinaba, 2013-04-10 04:44:33]

Could you also please update the comment at:
https://codereview.chromium.org/12978002/?

Originally I introduced the code for showing "xxx files fetched"
message until the (root directory) feed is loaded, but now with this patch
the purpose has changed to make sure reading "/drive" precedes "/drive/root".

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

Thank you for your review. PTAL?
(Sorry, rebase diff is mixed...)

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

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:357: // <other> directory
should always exists, but mydrive root should be
On 2013/04/10 04:34:44, kinaba wrote:
> nit: exists -> exist

Done.

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:358: // created by root
resource id retreived from the server.
On 2013/04/10 04:34:44, kinaba wrote:
> retrieved

Done.

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:387:
DCHECK(!directory_fetch_info.empty());
On 2013/04/10 03:44:35, Haruki Sato wrote:
> Also this method assumes it's request for <drive>?
> Is it good to denote that here as well?

Done.

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:427:
resource_metadata_->RefreshDirectory(
Two reasons:
1) AddEntry returns an error if the entry already exists. It is not what we want
here. Actually, there is a change for such a case; During the fetching
AboutResource from the server, full-fetch may complete successfully.
2) I want to keep the same manner for fast-fetch of other directories, here.
Without an entry for drive/other, it would be removed.

On 2013/04/10 03:44:35, Haruki Sato wrote:
> Any reason to use RefreshDirectory rather than AddEntry?
> Can we remove the creation for "other" in DriveResourceMetadata with this?

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/change_list_loader.h (right):

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.h:167: // Part of
DoLoadDIrectoryFromServer. Called when GetAboutResource for mydrive
On 2013/04/10 04:34:44, kinaba wrote:
> nit: DoLoadD<i>rectory

Done.

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.h:169: // |callback must not be
null.
On 2013/04/10 03:44:35, Haruki Sato wrote:
> |callback| ? missing "|".

Done.

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.h:169: // |callback must not be
null.
On 2013/04/10 03:44:35, Haruki Sato wrote:
> |callback| ? missing "|".

Done.

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

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:339: //
TODO(haruki): Create drive/root here when we start creating it in
On 2013/04/10 03:44:35, Haruki Sato wrote:
> Could you remove this TODO?
> Thanks!

Done.

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:490:
TEST_F(DriveFileSystemTest, DuplicatedAsyncInitialization) {
On 2013/04/10 04:34:44, kinaba wrote:
> 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.

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

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:524:
fake_drive_service_->resource_list_load_count() == 2)
On 2013/04/10 04:34:44, kinaba wrote:
> Besides, this ambiguity will go if we switch to GetEntryInfoByPath.

Done.

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:547: // Getting the
"My Drive" root entry should cause the resource load to happen.
On 2013/04/10 04:34:44, kinaba wrote:
> 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.

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

https://codereview.chromium.org/13866009/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:744: }
On 2013/04/10 04:34:44, kinaba wrote:
> May want to add a test verifying that "drive/root" exists
> after reading "drive".

Done.

[kinaba, 2013-04-10 06:50:20]

lgtm

https://codereview.chromium.org/13866009/diff/14001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/change_list_loader.cc (right):

https://codereview.chromium.org/13866009/diff/14001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/change_list_loader.cc:191:
util::kDriveOtherDirSpecialResourceId) {
I think we can simply remove this check because DoLoadDirectoryFromServer is
handling it correctly.

[Haruki Sato, 2013-04-10 07:02:51]

lgtm

thank you so much!

[hidehiko, 2013-04-10 07:43:05]

Thank you for your review. Sending to CQ.

https://codereview.chromium.org/13866009/diff/14001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/change_list_loader.cc (right):

https://codereview.chromium.org/13866009/diff/14001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/change_list_loader.cc:191:
util::kDriveOtherDirSpecialResourceId) {
On 2013/04/10 06:50:20, kinaba wrote:
> I think we can simply remove this check because DoLoadDirectoryFromServer is
> handling it correctly.

Indeed. Done.

[commit-bot: I haz the power, 2013-04-10 07:43:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/13866009/24002

[commit-bot: I haz the power, 2013-04-10 10:18:46]

Change committed as 193362