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

chrome/browser/chromeos/gdata/gdata_file_system.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/gdata/gdata_file_system.h"
 
 #include <set>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 915 matching lines @@
                                src_file_path,
                                dest_file_path,
                                CreateRelayCallback(callback)));
 }
 
 void GDataFileSystem::CopyOnUIThread(const FilePath& src_file_path,
                                      const FilePath& dest_file_path,
                                      const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  directory_service_->GetEntryInfoPairByPaths(
+      src_file_path,
+      dest_file_path.DirName(),
+      base::Bind(&GDataFileSystem::CopyOnUIThreadAfterGetEntryInfoPair,
+                 ui_weak_ptr_,
+                 src_file_path,
+                 dest_file_path,
+                 callback));
+}
+
+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;
-  FilePath dest_parent_path = dest_file_path.DirName();
+  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;
 
-  std::string src_file_resource_id;
-  bool src_file_is_hosted_document = false;
+  if (result->first.error != GDATA_FILE_OK) {
+    error = result->first.error;
+  } else if (result->second.error != GDATA_FILE_OK) {
+    error = result->second.error;
+  } else {
+    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();
 
-  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;
-  } else {
-    src_file_resource_id = src_entry->resource_id();
-    src_file_is_hosted_document =
-        src_entry->AsGDataFile()->is_hosted_document();
+    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.
                             dest_file_path.BaseName().RemoveExtension().value(),
                             callback);
     return;
   }
 
   // TODO(benchan): Reimplement this once the server API supports

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

Maybe also update this TODO?

   // copying of regular files directly on the server side.
   GetFileByPath(src_file_path,
                 base::Bind(&GDataFileSystem::OnGetFileCompleteForCopy,
                            ui_weak_ptr_,
                            dest_file_path,
                            callback),
                 GetDownloadDataCallback());
 }
 
 void GDataFileSystem::OnGetFileCompleteForCopy(
@@ skipping 2425 matching lines @@
   }
 
   PlatformFileInfoProto entry_file_info;
   GDataEntry::ConvertPlatformFileInfoToProto(*file_info, &entry_file_info);
   *entry_proto->mutable_file_info() = entry_file_info;
   if (!callback.is_null())
     callback.Run(GDATA_FILE_OK, entry_proto.Pass());
 }
 
 }  // namespace gdata