drive: Add showroot=true to WAPI feed URLs and ignore "no parent" entries.

drive: Add showroot=true to WAPI feed URLs and ignore "no parent" entries.

Forces DriveResourceMetadata to completely reject "no parent" entries (except the root).
Makes ChangeListLoader to ignore "no parent" entries coming from the server.
showroot=true lets us know the entries belonging to the "My Drive" root, so this CL only impacts the entries
those are not in "My Drive".
e.g. shared-with-me entries and entries the user (or a Drive app) explicitly set "no parent".

BUG=174233, 164119
TEST=unittests. Open Files App and verify Google Drive is available. Note that this CL changes the behavior about "no parent" entries.


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

[Haruki Sato, 2013-03-11 08:49:15]

This is what we talked offline. could you take a look?

[kinaba, 2013-03-11 09:22:37]

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:55: const int32
kProtoVersion = 3;
Do we need to trash the old protos at this point?
(In other words, are we storing no parent entries to the disk?)
If not, I think we can postpone the version-up until we're ready to
shared-with-me tab, so that at least existing user data won't
disappear suddenly.

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_url_generator.cc (right):

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_url_generator.cc:97: result =
net::AppendOrReplaceQueryParameter(result, "showroot", "true");
I think this should be moved to AddStandardUrlParams.
(as far as I tried, it worked.)

Other operations like CreateDirectory, CopyHostedDocument, GetResourceEntry,
or ResumeUploadOperations returns ResourceEntry with parent field,
whose behavior changes with "showroot=true".

[hashimoto, 2013-03-11 09:32:17]

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:519: if (old_entry &&
!old_entry->parent_resource_id().empty()) {
Can't this parent_resource_id().empty() check be replaced with old_entry !=
root_.get()?

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:523:
GetParent(entry_proto.parent_resource_id());
Why do you allow entry_proto.parent_resource_id() to be empty here?
How about checking it in the if-statement above?

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:627: DriveDirectory*
DriveResourceMetadata::GetParent(
Could you rename this method to GetDirectory() while you are here?
I think the name "GetParent" is quite confusing.

[Haruki Sato, 2013-03-11 14:15:35]

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:519: if (old_entry &&
!old_entry->parent_resource_id().empty()) {
On 2013/03/11 09:32:17, hashimoto wrote:
> Can't this parent_resource_id().empty() check be replaced with old_entry !=
> root_.get()?
Yes, assuming all non-root entry has parent_resource_id.
Revised.

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:523:
GetParent(entry_proto.parent_resource_id());
On 2013/03/11 09:32:17, hashimoto wrote:
> Why do you allow entry_proto.parent_resource_id() to be empty here?
> How about checking it in the if-statement above?

Done. And we can simplify it. Thanks.

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:627: DriveDirectory*
DriveResourceMetadata::GetParent(
On 2013/03/11 09:32:17, hashimoto wrote:
> Could you rename this method to GetDirectory() while you are here?
> I think the name "GetParent" is quite confusing.

Done.

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:55: const int32
kProtoVersion = 3;
On 2013/03/11 09:22:38, kinaba wrote:
> Do we need to trash the old protos at this point?
> (In other words, are we storing no parent entries to the disk?)
> If not, I think we can postpone the version-up until we're ready to
> shared-with-me tab, so that at least existing user data won't
> disappear suddenly.
I think yes. They are added to the root, aren't they?
My change just ignores ALL the "no parent" entries.
So without version-up, I guess we will have the issues like 
https://code.google.com/p/chromium/issues/detail?id=127087
again.
I feel like just hide those entries would cause less confusion.

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_url_generator.cc (right):

https://codereview.chromium.org/12585003/diff/4005/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_url_generator.cc:97: result =
net::AppendOrReplaceQueryParameter(result, "showroot", "true");
On 2013/03/11 09:22:38, kinaba wrote:
> I think this should be moved to AddStandardUrlParams.
> (as far as I tried, it worked.)
> 
> Other operations like CreateDirectory, CopyHostedDocument, GetResourceEntry,
> or ResumeUploadOperations returns ResourceEntry with parent field,
> whose behavior changes with "showroot=true".

Done.
Thanks.

[kinaba, 2013-03-12 01:40:09]

On 2013/03/11 14:15:35, Haruki Sato wrote:
> chrome/browser/chromeos/drive/drive_resource_metadata.h:55: const int32
kProtoVersion = 3;
> On 2013/03/11 09:22:38, kinaba wrote:
> > Do we need to trash the old protos at this point?
> I think yes. They are added to the root, aren't they?

They are.

> My change just ignores ALL the "no parent" entries.
> So without version-up, I guess we will have the issues like 
> https://code.google.com/p/chromium/issues/detail?id=127087
> again.

Yes, but this is already happening in the existing proto file.
So I don't think it will add more confusion than currently we have.
By this patch, new shared kittens won't come up anyway.

> I feel like just hide those entries would cause less confusion.

Although from our PoV it is unexpected that those entries are shown,
from users PoV the entries are consistently shown from M20 to M26, and
I suspect many users are considering it to be the expected bahvior.
Hiding those file before adding shared-with-me tab will make those
files unavailable, which I afraid to confuse users.

In either way, we have to increment the version (once again) at the point we
start introducing the "drive/other" folder, in order to collect non /-/mine
entries into ResourceMetadata. Hence at that timing we can clean up
unexpected orphan entries from the root directory.

[Haruki Sato, 2013-03-12 06:03:47]

On 2013/03/12 01:40:09, kinaba wrote:
> On 2013/03/11 14:15:35, Haruki Sato wrote:
> > chrome/browser/chromeos/drive/drive_resource_metadata.h:55: const int32
> kProtoVersion = 3;
> > On 2013/03/11 09:22:38, kinaba wrote:
> > > Do we need to trash the old protos at this point?
> > I think yes. They are added to the root, aren't they?
> 
> They are.
> 
> > My change just ignores ALL the "no parent" entries.
> > So without version-up, I guess we will have the issues like 
> > https://code.google.com/p/chromium/issues/detail?id=127087
> > again.
> 
> Yes, but this is already happening in the existing proto file.
> So I don't think it will add more confusion than currently we have.
> By this patch, new shared kittens won't come up anyway.
> 
> > I feel like just hide those entries would cause less confusion.
> 
> Although from our PoV it is unexpected that those entries are shown,
> from users PoV the entries are consistently shown from M20 to M26, and
> I suspect many users are considering it to be the expected bahvior.
> Hiding those file before adding shared-with-me tab will make those
> files unavailable, which I afraid to confuse users.
> 
> In either way, we have to increment the version (once again) at the point we
> start introducing the "drive/other" folder, in order to collect non /-/mine
> entries into ResourceMetadata. Hence at that timing we can clean up
> unexpected orphan entries from the root directory.

I'm OK with either way.
However I just don't like stale metadata stored anywhere, and I think we can use
version-up more casually.
For those stale kittens, I believe current (M26) bugs only have "no
parent"&&"shared-with-me" entries as stale metadata.
With this CL, in addition  to them, "no parent"&&"owned-by-me" files can be
stale as well.

Considering the number of those entries, I guess users have more "no
parent"&&"shared-with-me" than "no parent"&&"owned-by-me", so the CL's impact on
stale metadata might be small. And we will version-up later in other CL anyway.
Reverted the version-up.

[Haruki Sato, 2013-03-12 06:10:13]

Rebased and reverted the version-up change.
Could you take another look? Thanks.

[kinaba, 2013-03-12 06:14:13]

lgtm for patch set 7.
I hope we can more casually version up after satoru's fast-fetch feature lands.
Currently full feed reloading takes order of hours for large account.

[hashimoto, 2013-03-12 07:55:17]

lgtm with nits

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:403: // this assumes
|entry_proto| contains metadata only about the entry itself.
This comment seems to be redundant.
Only DriveDirectoryProto has fields for child entries (it's only used for
serialization, though), and DriveEntryProto has no field for child entries.

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:488: const std::string&
parent_resource_id) {
nit: s/parent_resource_id/resource_id/?

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:350: // Helper function
to get a parent directory given |parent_resource_id|.
nit: Please fix the comment.

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:354: DriveDirectory*
GetDirectory(const std::string& parent_resource_id);
nit: s/parent_resource_id/resource_id/?

[Haruki Sato, 2013-03-12 08:17:23]

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:403: // this assumes
|entry_proto| contains metadata only about the entry itself.
On 2013/03/12 07:55:17, hashimoto wrote:
> This comment seems to be redundant.
> Only DriveDirectoryProto has fields for child entries (it's only used for
> serialization, though), and DriveEntryProto has no field for child entries.

Thank you for the good explanation. Seemsl like I had trouble understanding this
when I was reading it.
Removed.

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:488: const std::string&
parent_resource_id) {
On 2013/03/12 07:55:17, hashimoto wrote:
> nit: s/parent_resource_id/resource_id/?

Done. Thanks.

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:350: // Helper function
to get a parent directory given |parent_resource_id|.
On 2013/03/12 07:55:17, hashimoto wrote:
> nit: Please fix the comment.

Done. thanks.

https://codereview.chromium.org/12585003/diff/7009/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:354: DriveDirectory*
GetDirectory(const std::string& parent_resource_id);
On 2013/03/12 07:55:17, hashimoto wrote:
> nit: s/parent_resource_id/resource_id/?

Done.

[commit-bot: I haz the power, 2013-03-12 08:17:47]

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

[commit-bot: I haz the power, 2013-03-12 12:29:24]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-12 13:30:00]

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

[commit-bot: I haz the power, 2013-03-13 01:23:33]

Change committed as 187734