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

chrome/browser/chromeos/gdata/gdata_file_system.cc

Index: chrome/browser/chromeos/gdata/gdata_file_system.cc
diff --git a/chrome/browser/chromeos/gdata/gdata_file_system.cc b/chrome/browser/chromeos/gdata/gdata_file_system.cc
index c809f8643d1afb3b9623c2f83098e1dde97399d8..4b98c12ed7f2eef9375ba2f346ef20957f056384 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.cc
@@ -933,39 +933,58 @@ void GDataFileSystem::CopyOnUIThread(const FilePath& src_file_path,
                                      const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  GDataFileError error = GDATA_FILE_OK;
-  FilePath dest_parent_path = dest_file_path.DirName();
+  directory_service_->GetEntryInfoPairByPaths(
+      src_file_path,
+      dest_file_path.DirName(),
+      base::Bind(&GDataFileSystem::CopyOnUIThreadAfterGetEntryInfoPair,
+                 ui_weak_ptr_,
+                 src_file_path,
+                 dest_file_path,
+                 callback));
+}
 
-  std::string src_file_resource_id;
+void GDataFileSystem::CopyOnUIThreadAfterGetEntryInfoPair(
+    const FilePath& src_file_path,
+    const FilePath& dest_file_path,
+    const FileOperationCallback& callback,
+    scoped_ptr<EntryInfoPairResult> result) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(result.get());
+
+  GDataFileError error = GDATA_FILE_OK;
   bool src_file_is_hosted_document = false;

[achuithb, 2012/08/09 00:29:28]

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?

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

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.

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

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.

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

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.

+  std::string src_file_resource_id;
 
-  GDataEntry* src_entry = directory_service_->FindEntryByPathSync(
-      src_file_path);
-  GDataEntry* dest_parent = directory_service_->FindEntryByPathSync(
-      dest_parent_path);
-  if (!src_entry || !dest_parent) {
-    error = GDATA_FILE_ERROR_NOT_FOUND;
-  } else if (!dest_parent->AsGDataDirectory()) {
-    error = GDATA_FILE_ERROR_NOT_A_DIRECTORY;
-  } else if (!src_entry->AsGDataFile()) {
-    // TODO(benchan): Implement copy for directories. In the interim,
-    // we handle recursive directory copy in the file manager.
-    error = GDATA_FILE_ERROR_INVALID_OPERATION;
+  if (result->first.error != GDATA_FILE_OK) {
+    error = result->first.error;
+  } else if (result->second.error != GDATA_FILE_OK) {
+    error = result->second.error;
   } else {
-    src_file_resource_id = src_entry->resource_id();
-    src_file_is_hosted_document =
-        src_entry->AsGDataFile()->is_hosted_document();
+    scoped_ptr<GDataEntryProto> src_file_proto = result->first.proto.Pass();

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

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]

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

+    scoped_ptr<GDataEntryProto> dest_parent_proto =
+        result->second.proto.Pass();
+
+    if (!dest_parent_proto->file_info().is_directory()) {
+      error = GDATA_FILE_ERROR_NOT_A_DIRECTORY;
+    } else if (src_file_proto->file_info().is_directory()) {
+      // TODO(benchan): Implement copy for directories. In the interim,

[achuithb, 2012/08/09 00:29:28]

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.

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

Done.

+      // we handle recursive directory copy in the file manager.
+      error = GDATA_FILE_ERROR_INVALID_OPERATION;
+    } else {
+      src_file_resource_id = src_file_proto->resource_id();
+      src_file_is_hosted_document =
+          src_file_proto->file_specific_info().is_hosted_document();
+    }
   }
 
   if (error != GDATA_FILE_OK) {
     if (!callback.is_null())
-      MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback, error));
-
+      callback.Run(error);
     return;
   }
 
   if (src_file_is_hosted_document) {
-    CopyDocumentToDirectory(dest_parent_path,
+    CopyDocumentToDirectory(dest_file_path.DirName(),
                             src_file_resource_id,
                             // Drop the document extension, which should not be
                             // in the document title.