Parent/child fixes.

Parent/child fixes.

* Split GDataDirectory::children_ into child_files_ and child_directories_. This is necessary because we plan to offload child_files_ to GDataDB (but not child_directories_).
* Add helpers FindChild and AddChild. Rename RemoveEntryFromChildrenList to RemoveChild.
* Do not serialize GDataEntry::file_name, serialize GDataEntry::parent_resource_id
* Use SetParent instead of set_parent, which also sets the parent_resource_id.
* Implement GetFilePath using recursion instead for simplicity.
* Move SetFileNameFromTitle to within AddEntry.
* Move kGDataRootDirectory (gdata) to ctor of GDataRootDirectory.
* Other minor cleanup.

BUG=chromium-os:29232
TEST=compiles, unit tests pass. Everything looks ok in chrome file manager.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=134405

[achuithb, 2012-04-27 21:17:13]

Please review

[achuithb, 2012-04-27 21:19:16]

We need to distinguish between child files and child directories, so I've split
them up. I needed to add helpers FindChild and AddChild to iterate over both
vectors.

I no longer serialize file_name, since this is transient and can change based on
de-duping. Instead, I ensure parent_resource_id is always correct, and serialize
this.

Please take a look. Thanks!

[satorux1, 2012-04-27 21:35:41]

Could you explain "We need to distinguish between child file"? and add it to the
patch description?

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata.proto (right):

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata.proto:28: optional string parent_resource_id
= 4;
please don't renumber. This will break compatibility with the existing
serialized proto.

please add  optional string parent_resource_id = 7;

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:102: parent_resource_id_ = parent ?
parent->resource_id() : "";
IIRC, parent_resource_id_ was used for delta updates. not sure if inheriting
this from here is the right thing to do.

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.h:101: void set_file_name(const
FilePath::StringType& name) { file_name_ = name; }
Could you also emphasize that this does not contain the directory path, and you
should use GetFilePath() for that? That said, it may make sense to rename it to
"base name".

Besides, you might want to make it private or protected.

[achuithb, 2012-04-27 22:15:03]

I've added another comment to GDataFileSystem::RenameFileOnFilesystem (shouldn't
it be RenameFileOnFileSystem?)

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata.proto (right):

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata.proto:28: optional string parent_resource_id
= 4;
On 2012/04/27 21:35:41, satorux1 wrote:
> please don't renumber. This will break compatibility with the existing
> serialized proto.
> 
> please add  optional string parent_resource_id = 7;

Done.

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:102: parent_resource_id_ = parent ?
parent->resource_id() : "";
On 2012/04/27 21:35:41, satorux1 wrote:
> IIRC, parent_resource_id_ was used for delta updates. not sure if inheriting
> this from here is the right thing to do.

I think it is. It should always point to the correct parent. Zel can chime in
here.

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.h:101: void set_file_name(const
FilePath::StringType& name) { file_name_ = name; }
On 2012/04/27 21:35:41, satorux1 wrote:
> Could you also emphasize that this does not contain the directory path, and
you
> should use GetFilePath() for that? That said, it may make sense to rename it
to
> "base name".
> 
> Besides, you might want to make it private or protected.

I have added the comment.

I use it in gdata_db_unittest. I'll add a TODO here to make this private when I
get rid of paths in GDataDB.

base_name is a good idea.

I'll rename in the next CL so the diff stays manageable, unless you want me to
do it here.

[satorux1, 2012-04-27 22:29:41]

LGTM

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10258004/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.h:101: void set_file_name(const
FilePath::StringType& name) { file_name_ = name; }
On 2012/04/27 22:15:03, achuith.bhandarkar wrote:
> On 2012/04/27 21:35:41, satorux1 wrote:
> > Could you also emphasize that this does not contain the directory path, and
> you
> > should use GetFilePath() for that? That said, it may make sense to rename it
> to
> > "base name".
> > 
> > Besides, you might want to make it private or protected.
> 
> I have added the comment.
> 
> I use it in gdata_db_unittest. I'll add a TODO here to make this private when
I
> get rid of paths in GDataDB.
> 
> base_name is a good idea.
> 
> I'll rename in the next CL so the diff stays manageable, unless you want me to
> do it here.

Please do it in a separate patch! :)

http://codereview.chromium.org/10258004/diff/11001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10258004/diff/11001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2557: // to file_name.
file a bug and add a URL?

http://codereview.chromium.org/10258004/diff/11001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10258004/diff/11001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:99: // This not the full path, use
GetFilePath for that.
This not -> This is not

[achuithb, 2012-04-27 22:43:22]

http://codereview.chromium.org/10258004/diff/11001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10258004/diff/11001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2557: // to file_name.
On 2012/04/27 22:29:41, satorux1 wrote:
> file a bug and add a URL?

Done.

http://codereview.chromium.org/10258004/diff/11001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10258004/diff/11001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:99: // This not the full path, use
GetFilePath for that.
On 2012/04/27 22:29:41, satorux1 wrote:
> This not -> This is not

Done.

[zel, 2012-04-27 23:10:30]

lgtm

http://codereview.chromium.org/10258004/diff/8005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10258004/diff/8005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:333: const GDataFileCollection&
child_files() const { return child_files_; }
it would be great to provide an iterator that goes over both

[achuithb, 2012-04-28 00:20:15]

Thanks for the reviews Zel and Satoru-san.

http://codereview.chromium.org/10258004/diff/8005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10258004/diff/8005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:333: const GDataFileCollection&
child_files() const { return child_files_; }
On 2012/04/27 23:10:31, zel wrote:
> it would be great to provide an iterator that goes over both

Yup, we would hide the read from the database (as necessary) behind such an
iterator.