Simplify implementation of FindEntryByPathSync

Simplify implementation of FindEntryByPathSync

In addition, RemoveEntry and RemoveChild now return void.

BUG=137725
TEST=unit tests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150448

[achuithb, 2012-08-07 10:29:52]

Satoru-san, please take a look.

[satorux1, 2012-08-07 10:43:46]

I'm confused. Why do we need FindChildAsync()?

GDataEntry/GDataFile/GDataDirectory are designed for in-memory representation of
the file system metadata, hence all methods are synchronous.

I thought we wanted to deprecate this layer and use
GDataEntryProto/GDataDirectoryProto for the new metadata storage.

http://codereview.chromium.org/10825218/diff/2002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10825218/diff/2002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:67: void
FindNextComponent(std::vector<FilePath::StringType> components,
function comment is missing.

http://codereview.chromium.org/10825218/diff/2002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:399: callback.Run(entry ?
GDATA_FILE_OK : GDATA_FILE_ERROR_NOT_FOUND, entry);
To make it more asynchronous, can you post the callback to the current message
loop, rather than calling it directly here?

http://codereview.chromium.org/10825218/diff/2002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:653: else
I'm rather confused. Simplifying the FindEntryByPathSync() is a good thing, but
it's logically a separate change. I'd suggest to do this in a separate patch.

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

http://codereview.chromium.org/10825218/diff/2002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:290: void FindChildAsync(const
FilePath::StringType& file_name,
base_name ?

[achuithb, 2012-08-07 22:17:15]

My goal was to make all GDataDirectory methods async. FindChild is the only
synchronous one, so I thought I'd add an async version. But we really want to
eliminate these functions altogether, and have GDataDirectoryService provide the
appropriate replacement functionality, right?

I got rid of FindChildAsync and related changes. I think the changes to
FindEntryByPathSync are still good. PTAL.

[achuithb, 2012-08-07 22:20:01]

On 2012/08/07 22:17:15, achuith.bhandarkar wrote:
> My goal was to make all GDataDirectory methods async. FindChild is the only
> synchronous one, so I thought I'd add an async version. But we really want to
> eliminate these functions altogether, and have GDataDirectoryService provide
the
> appropriate replacement functionality, right?
> 
> I got rid of FindChildAsync and related changes. I think the changes to
> FindEntryByPathSync are still good. PTAL.

I think we'll need something very similar to what I implemented. Which is, look
up each component of the path, find the corresponding directory entry, look up
the next part etc, in an asynchronous manner.

[satorux1, 2012-08-07 22:27:05]

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

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2869:
parent_dir->RemoveEntry(entry);
Is this a good thing to do? I think removing an non-existent entry should result
in GDATA_FILE_ERROR_NOT_FOUND

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:389: const std::string&
file_name(entry->base_name());
file_name -> base_name ?

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:627: }
Much simpler! Nice.

[achuithb, 2012-08-07 22:28:32]

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

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2869:
parent_dir->RemoveEntry(entry);
On 2012/08/07 22:27:05, satorux1 wrote:
> Is this a good thing to do? I think removing an non-existent entry should
result
> in GDATA_FILE_ERROR_NOT_FOUND

We already look up the entry using FindEntryByPathSync at line 2854. I don't
think this redundant check adds any value.

[satorux1, 2012-08-07 22:28:45]

On 2012/08/07 22:17:15, achuith.bhandarkar wrote:
> My goal was to make all GDataDirectory methods async. FindChild is the only
> synchronous one, so I thought I'd add an async version. But we really want to
> eliminate these functions altogether, and have GDataDirectoryService provide
the
> appropriate replacement functionality, right?

I think GDataDirectoryService should provide all the file manipulation/lookup
APIs (which use proto, not GDataEntry* family), and we should use it from
everywhere.

> 
> I got rid of FindChildAsync and related changes. I think the changes to
> FindEntryByPathSync are still good. PTAL.

[achuithb, 2012-08-07 22:29:43]

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:389: const std::string&
file_name(entry->base_name());
On 2012/08/07 22:27:05, satorux1 wrote:
> file_name -> base_name ?

Done.

[achuithb, 2012-08-07 22:30:30]

On 2012/08/07 22:28:45, satorux1 wrote:
> On 2012/08/07 22:17:15, achuith.bhandarkar wrote:
> > My goal was to make all GDataDirectory methods async. FindChild is the only
> > synchronous one, so I thought I'd add an async version. But we really want
to
> > eliminate these functions altogether, and have GDataDirectoryService provide
> the
> > appropriate replacement functionality, right?
> 
> I think GDataDirectoryService should provide all the file manipulation/lookup
> APIs (which use proto, not GDataEntry* family), and we should use it from
> everywhere.

Got it!

> > 
> > I got rid of FindChildAsync and related changes. I think the changes to
> > FindEntryByPathSync are still good. PTAL.

[satorux1, 2012-08-07 22:38:09]

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

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2869:
parent_dir->RemoveEntry(entry);
On 2012/08/07 22:28:32, achuith.bhandarkar wrote:
> On 2012/08/07 22:27:05, satorux1 wrote:
> > Is this a good thing to do? I think removing an non-existent entry should
> result
> > in GDATA_FILE_ERROR_NOT_FOUND
> 
> We already look up the entry using FindEntryByPathSync at line 2854. I don't
> think this redundant check adds any value.

Ah that's a good point.

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:356: RemoveChild(entry);
The new behavior may be rather scary. Previously if |entry| is not found in this
directory, this function did nothing but returned false. With the new behavior,
|entry| is deleted regardless. 

I guess we want to keep the original behavior just in case.

[achuithb, 2012-08-07 22:48:35]

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10825218/diff/12001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:356: RemoveChild(entry);
On 2012/08/07 22:38:09, satorux1 wrote:
> The new behavior may be rather scary. Previously if |entry| is not found in
this
> directory, this function did nothing but returned false. With the new
behavior,
> |entry| is deleted regardless. 
> 
> I guess we want to keep the original behavior just in case.

We're using this function in 2 places in GDataFileSystem and 1 place in
GDataDirectoryService. In all cases, we do a parent test, so we are effectively
doing something like
entry->parent()->RemoveEntry(entry);

I believe the return false case is a dead code path, and I think we should get
rid of it.

This code is not going to get merged into the branch, so even though this is
potentially risky, I think it's better due to its simplicity.

[satorux1, 2012-08-07 22:55:29]

LGTM with a suggestion

http://codereview.chromium.org/10825218/diff/2006/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10825218/diff/2006/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:389: const std::string&
base_name(entry->base_name());
maybe add

// The |entry| must be present in this directory.
DCHECK_EQ(entry, FindChild(base_name));

[achuithb, 2012-08-07 23:00:41]

http://codereview.chromium.org/10825218/diff/2006/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10825218/diff/2006/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:389: const std::string&
base_name(entry->base_name());
On 2012/08/07 22:55:30, satorux1 wrote:
> maybe add
> 
> // The |entry| must be present in this directory.
> DCHECK_EQ(entry, FindChild(base_name));

Good idea. Done.