gdata: Remove FindEntryByPathAndRunSync() from GDataWapiFeedLoader.

gdata: Remove FindEntryByPathAndRunSync() from GDataWapiFeedLoader.

GDataWapiFeedLoader sould only focus on loading the feeds, without
doing the extra step to find an entry.

The number of callers of FindEntryByPathAndRunSync() is reduced from 4 to 2.

Along the way, make LoadDocumentFeedCallback parameter mandatory for
LoadFromServer() and friends. Looking at callers, it's safe to make this
mandatory.

BUG=141196, 126634
TEST=file manager works as before (loading from the server and from the cache)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151654

[achuithb, 2012-08-14 23:57:36]

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

http://codereview.chromium.org/10824303/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2851:
directory_service_->FindEntryByPathAndRunSync(search_file_path, callback);
Why not just fix this? It's easy, right?

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

http://codereview.chromium.org/10824303/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:868:
FileOperationCallback callback = params->callback;
This callback code is a bit difficult to follow.

We make a copy of params->callback, then we may Run + Reset it, and then we pass
along the copy (not the original params->callback) to ReloadFromServerIfNeeded.
It would be nice if we didn't have callback and params->callback be different.

WDYT? Don't have to fix this here if you don't want to.

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

http://codereview.chromium.org/10824303/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h:120: void
LoadFromCache(bool should_load_from_server,
Not in this patch, but can this be simplified so we can get rid of
search_file_path?

http://codereview.chromium.org/10824303/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h:141: //
|feed_load_callback| must not be null
nit: period at end of sentence here and elsewhere.

[satorux1, 2012-08-15 00:03:26]

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

http://codereview.chromium.org/10824303/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2851:
directory_service_->FindEntryByPathAndRunSync(search_file_path, callback);
On 2012/08/14 23:57:36, achuith.bhandarkar wrote:
> Why not just fix this? It's easy, right?

I'll fix this in the next patch, which makes things much cleaner. Stay tuned.

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

http://codereview.chromium.org/10824303/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h:120: void
LoadFromCache(bool should_load_from_server,
On 2012/08/14 23:57:36, achuith.bhandarkar wrote:
> Not in this patch, but can this be simplified so we can get rid of
> search_file_path?

This is just what I was doing after sending the initial patch. :) Done

Now the patch is even better!

http://codereview.chromium.org/10824303/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h:141: //
|feed_load_callback| must not be null
On 2012/08/14 23:57:36, achuith.bhandarkar wrote:
> nit: period at end of sentence here and elsewhere.

Done.

[achuithb, 2012-08-15 00:13:15]

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

http://codereview.chromium.org/10824303/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:868:
FileOperationCallback callback = params->callback;
On 2012/08/14 23:57:36, achuith.bhandarkar wrote:
> This callback code is a bit difficult to follow.
> 
> We make a copy of params->callback, then we may Run + Reset it, and then we
pass
> along the copy (not the original params->callback) to
ReloadFromServerIfNeeded.
> It would be nice if we didn't have callback and params->callback be different.
> 
> WDYT? Don't have to fix this here if you don't want to.

ping

http://codereview.chromium.org/10824303/diff/8001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10824303/diff/8001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2004: 0,  // Not used.
Can we say what this param is?

http://codereview.chromium.org/10824303/diff/8001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2007: GURL(), /* feed not
explicitly set */
Can we use // style comment here instead?

[satorux1, 2012-08-15 00:26:29]

I'm already on a bus. If there are no other comments, please lgtm so I can check
this in after fixing the nits. Thanks.

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

http://codereview.chromium.org/10824303/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:868:
FileOperationCallback callback = params->callback;
On 2012/08/15 00:13:15, achuith.bhandarkar wrote:
> On 2012/08/14 23:57:36, achuith.bhandarkar wrote:
> > This callback code is a bit difficult to follow.
> > 
> > We make a copy of params->callback, then we may Run + Reset it, and then we
> pass
> > along the copy (not the original params->callback) to
> ReloadFromServerIfNeeded.
> > It would be nice if we didn't have callback and params->callback be
different.
> > 
> > WDYT? Don't have to fix this here if you don't want to.
> 
> ping

Oops, missed this. Fully agree it's hard to follow. Let me clean this up in a
separate patch

http://codereview.chromium.org/10824303/diff/8001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10824303/diff/8001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2004: 0,  // Not used.
On 2012/08/15 00:13:15, achuith.bhandarkar wrote:
> Can we say what this param is?

Will do

http://codereview.chromium.org/10824303/diff/8001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2007: GURL(), /* feed not
explicitly set */
On 2012/08/15 00:13:15, achuith.bhandarkar wrote:
> Can we use // style comment here instead?
Will do

[achuithb, 2012-08-15 00:41:48]

lgtm

[achuithb, 2012-08-15 00:42:33]

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

http://codereview.chromium.org/10824303/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:868:
FileOperationCallback callback = params->callback;
On 2012/08/15 00:26:29, satorux1 wrote:
> On 2012/08/15 00:13:15, achuith.bhandarkar wrote:
> > On 2012/08/14 23:57:36, achuith.bhandarkar wrote:
> > > This callback code is a bit difficult to follow.
> > > 
> > > We make a copy of params->callback, then we may Run + Reset it, and then
we
> > pass
> > > along the copy (not the original params->callback) to
> > ReloadFromServerIfNeeded.
> > > It would be nice if we didn't have callback and params->callback be
> different.
> > > 
> > > WDYT? Don't have to fix this here if you don't want to.
> > 
> > ping
> 
> Oops, missed this. Fully agree it's hard to follow. Let me clean this up in a
> separate patch

Could you please add a TODO in this CL?

[satorux1, 2012-08-15 01:26:23]

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

http://codereview.chromium.org/10824303/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:868:
FileOperationCallback callback = params->callback;
On 2012/08/15 00:42:33, achuith.bhandarkar wrote:
> On 2012/08/15 00:26:29, satorux1 wrote:
> > On 2012/08/15 00:13:15, achuith.bhandarkar wrote:
> > > On 2012/08/14 23:57:36, achuith.bhandarkar wrote:
> > > > This callback code is a bit difficult to follow.
> > > > 
> > > > We make a copy of params->callback, then we may Run + Reset it, and then
> we
> > > pass
> > > > along the copy (not the original params->callback) to
> > > ReloadFromServerIfNeeded.
> > > > It would be nice if we didn't have callback and params->callback be
> > different.
> > > > 
> > > > WDYT? Don't have to fix this here if you don't want to.
> > > 
> > > ping
> > 
> > Oops, missed this. Fully agree it's hard to follow. Let me clean this up in
a
> > separate patch
> 
> Could you please add a TODO in this CL?

Done.

http://codereview.chromium.org/10824303/diff/8001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10824303/diff/8001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2004: 0,  // Not used.
On 2012/08/15 00:26:29, satorux1 wrote:
> On 2012/08/15 00:13:15, achuith.bhandarkar wrote:
> > Can we say what this param is?
> 
> Will do 

Done.

http://codereview.chromium.org/10824303/diff/8001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2007: GURL(), /* feed not
explicitly set */
On 2012/08/15 00:26:29, satorux1 wrote:
> On 2012/08/15 00:13:15, achuith.bhandarkar wrote:
> > Can we use // style comment here instead?
> Will do
> 

Done.

[commit-bot: I haz the power, 2012-08-15 02:08:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/satorux@chromium.org/10824303/1007

[commit-bot: I haz the power, 2012-08-15 04:27:18]

Change committed as 151654