gdata: Remove GDataDirectoryService::FindEntryByPathAndRunSync().

gdata: Remove GDataDirectoryService::FindEntryByPathAndRunSync().

The number of callers of FindEntryByPathAndRunSync() in GDataFileSystem
is reduced from 2 to 0, hence the function is finally removed from
GDataDirectoryService.

Along the way, GDataFileSystem::FindEntryByPathAsyncOnUIThread() is
simplified and renamed to LoadIfNeeded().

Remove GDataFileSystem::FindEntryByPathSyncOnUIThread() as well.

BUG=141196, 142420
TEST=file manager works as before

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

[satorux1, 2012-08-17 17:28:38]

the metadata lookup business is cleaned up. hope you like it.

[achuithb, 2012-08-17 19:19:31]

I think the CL description needs to be updated.

[achuithb, 2012-08-17 19:36:49]

I really like the CL!

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

http://codereview.chromium.org/10831375/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:714: // Post a task to the
same thread, rather than calling it here, as
Could you please also add a comment here that the feed has already been loaded,
so we're just calling the callback asynchronously without doing anything.

We could also consider putting this in an else block so the early returns are
not necessary. I think it would make this more readable, but up to you. I try to
avoid early returns, but they are ok for error conditions.

http://codereview.chromium.org/10831375/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1996: DCHECK(entries.get());
Is this DCHECK valid for empty directories?

[satorux1, 2012-08-17 20:35:29]

I thought the patch description covered changed I made. Anything missing?

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

http://codereview.chromium.org/10831375/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:714: // Post a task to the
same thread, rather than calling it here, as
On 2012/08/17 19:36:49, achuith.bhandarkar wrote:
> Could you please also add a comment here that the feed has already been
loaded,
> so we're just calling the callback asynchronously without doing anything.

Added a comment.


> We could also consider putting this in an else block so the early returns are
> not necessary. I think it would make this more readable, but up to you. I try
to
> avoid early returns, but they are ok for error conditions.

Let's keep this as is for now.

http://codereview.chromium.org/10831375/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1996: DCHECK(entries.get());
On 2012/08/17 19:36:49, achuith.bhandarkar wrote:
> Is this DCHECK valid for empty directories?

Yes. added a comment.

[achuithb, 2012-08-17 20:51:18]

On 2012/08/17 20:35:29, satorux1 wrote:
> I thought the patch description covered changed I made. Anything missing?
> 

Also remove GDataFileSystem::FindEntryByPathSyncOnUIThread.

GetEntryInfo and ReadDirectoryByPath callbacks are broken up so LoadIfNeeded is
called first, and in the callback, GDataDirectoryService::GetEntryInfo and
ReadDirectoryByPath are called.

LoadIfNeeded now takes a FileOperationCallback instead of a search path and
FileEntryCallback.

[achuithb, 2012-08-17 20:52:05]

lgtm

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

http://codereview.chromium.org/10831375/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1996: DCHECK(entries.get());
On 2012/08/17 20:35:30, satorux1 wrote:
> On 2012/08/17 19:36:49, achuith.bhandarkar wrote:
> > Is this DCHECK valid for empty directories?
> 
> Yes. added a comment.

nit: s/emptry/empty

[achuithb, 2012-08-17 20:52:54]

This is a good CL. The names are much clearer and the cleanup around
LoadIfNeeded was much needed.

[satorux1, 2012-08-17 20:57:34]

On 2012/08/17 20:51:18, achuith.bhandarkar wrote:
> On 2012/08/17 20:35:29, satorux1 wrote:
> > I thought the patch description covered changed I made. Anything missing?
> > 
> 
> Also remove GDataFileSystem::FindEntryByPathSyncOnUIThread.

Good point. Added.

> GetEntryInfo and ReadDirectoryByPath callbacks are broken up so LoadIfNeeded
is
> called first, and in the callback, GDataDirectoryService::GetEntryInfo and
> ReadDirectoryByPath are called.
> 
> LoadIfNeeded now takes a FileOperationCallback instead of a search path and
> FileEntryCallback.

I'd rather not to go into details about semantic changes in the patch
description, but thank you for the suggestions.