gdata: Remove use of FindEntryByPathSync() from CopyOnUIThread()

gdata: Remove use of FindEntryByPathSync() from CopyOnUIThread()

The number of calles of FindEntryByPathSync() is reduced from 17 to 15.

BUG=10831212, 126634
TEST=Copying a file (and a hosted document) on Drive works before from file manager

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

[achuithb, 2012-08-09 00:29:27]

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

http://codereview.chromium.org/10828219/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:955: bool
src_file_is_hosted_document = false;
These auto vars seem unnecessary to me. I think it would be cleaner to inline
these in the places where they are needed. If you want to keep these around, can
we move them to line 985 and make them const?

http://codereview.chromium.org/10828219/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:970: // TODO(benchan):
Implement copy for directories. In the interim,
Can we update this TODO with an engineer current on webdrive and a bug id? If we
don't want to ever implement this, we should just get rid of this TODO.

http://codereview.chromium.org/10828219/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:996: // TODO(benchan):
Reimplement this once the server API supports
Maybe also update this TODO?

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

http://codereview.chromium.org/10828219/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.h:742: const FilePath&
src_file_path,
Why not get rid of src_file_path and dest_file_path? They are in |result|.

[satorux1, 2012-08-09 08:33:03]

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

http://codereview.chromium.org/10828219/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:955: bool
src_file_is_hosted_document = false;
On 2012/08/09 00:29:28, achuith.bhandarkar wrote:
> These auto vars seem unnecessary to me. I think it would be cleaner to inline
> these in the places where they are needed. If you want to keep these around,
can
> we move them to line 985 and make them const?

I'm all for minimized scope, but these cannot be moved. These are set inside of
the following if-else bodies, and used outside of them afterwards.

http://codereview.chromium.org/10828219/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:970: // TODO(benchan):
Implement copy for directories. In the interim,
On 2012/08/09 00:29:28, achuith.bhandarkar wrote:
> Can we update this TODO with an engineer current on webdrive and a bug id? If
we
> don't want to ever implement this, we should just get rid of this TODO.

Done.

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

http://codereview.chromium.org/10828219/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.h:742: const FilePath&
src_file_path,
On 2012/08/09 00:29:28, achuith.bhandarkar wrote:
> Why not get rid of src_file_path and dest_file_path? They are in |result|.

Removed src_file_path.

result.first.path == src_file_path but result.second.path ==
dest_file_path.DirName(), hence cannot remove dest_file_path

[achuithb, 2012-08-09 11:16:46]

lgtm

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

http://codereview.chromium.org/10828219/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:955: bool
src_file_is_hosted_document = false;
On 2012/08/09 08:33:03, satorux1 wrote:
> On 2012/08/09 00:29:28, achuith.bhandarkar wrote:
> > These auto vars seem unnecessary to me. I think it would be cleaner to
inline
> > these in the places where they are needed. If you want to keep these around,
> can
> > we move them to line 985 and make them const?
> 
> I'm all for minimized scope, but these cannot be moved. These are set inside
of
> the following if-else bodies, and used outside of them afterwards.

Hmm, what am I missing? It looks to me like these can be eliminated.
You'd have to remove the scoped_ptr assignment in the else block, which I think
is weird anyway. 

You should be able to do 
if (result->first->proto->file_specific_info().is_hosted_document()) {
  CopyDocumentToDirectory(dest_file_path.DirName(),
                                            result->first->proto->resource_id(),
etc.

Anyway, it's fine as it is.

http://codereview.chromium.org/10828219/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:963:
scoped_ptr<GDataEntryProto> src_file_proto = result->first.proto.Pass();
This seems rather weird to me. We are destroying the protos when we leave this
scope; I think they should only be destroyed at the end of this function. It
seems more natural to use
GDataEntryProto* src_file_proto = result->first.proto.get();

[satorux1, 2012-08-09 16:51:50]

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

http://codereview.chromium.org/10828219/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:955: bool
src_file_is_hosted_document = false;
On 2012/08/09 11:16:46, achuith.bhandarkar wrote:
> On 2012/08/09 08:33:03, satorux1 wrote:
> > On 2012/08/09 00:29:28, achuith.bhandarkar wrote:
> > > These auto vars seem unnecessary to me. I think it would be cleaner to
> inline
> > > these in the places where they are needed. If you want to keep these
around,
> > can
> > > we move them to line 985 and make them const?
> > 
> > I'm all for minimized scope, but these cannot be moved. These are set inside
> of
> > the following if-else bodies, and used outside of them afterwards.
> 
> Hmm, what am I missing? It looks to me like these can be eliminated.
> You'd have to remove the scoped_ptr assignment in the else block, which I
think
> is weird anyway. 
> 
> You should be able to do 
> if (result->first->proto->file_specific_info().is_hosted_document()) {
>   CopyDocumentToDirectory(dest_file_path.DirName(),
>                                            
result->first->proto->resource_id(),
> etc.
> 
> Anyway, it's fine as it is.

Now I see your point. It seems to me that the awkward if-else blocks were set up
in order to handle errors in one shot:

  960   if (error != GDATA_FILE_OK) {
  961     if (!callback.is_null())
  962       MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback,
error));
  963 	
  964     return;
  965   }

This doesn't make sense because 1) callback is never null here, 2) we no longer
need to deal with MessageLoop here.

I've just just reorganized the code and I think you'll like it.

Re 1), we should eliminate if (!callback.is_null()) from places where we know
callback is never null. we should make it clear if a callback parameter can be
null or not in function comments. I'll file a bug.

http://codereview.chromium.org/10828219/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:963:
scoped_ptr<GDataEntryProto> src_file_proto = result->first.proto.Pass();
On 2012/08/09 11:16:46, achuith.bhandarkar wrote:
> This seems rather weird to me. We are destroying the protos when we leave this
> scope; I think they should only be destroyed at the end of this function. It
> seems more natural to use
> GDataEntryProto* src_file_proto = result->first.proto.get();

That's a good point. With the new code, this weirdness is gone.

[satorux1, 2012-08-09 17:07:29]

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

http://codereview.chromium.org/10828219/diff/7004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1008: if
(!callback.is_null())
this was wrong! fixed.