Replace GetEntryByResourceId with GetEntryByResourceIdAsync in search and file upload

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 401 matching lines @@
 void RunTaskOnThread(scoped_refptr<base::MessageLoopProxy> relay_proxy,
                      const base::Closure& task) {
   if (relay_proxy->BelongsToCurrentThread()) {
     task.Run();
   } else {
     const bool posted = relay_proxy->PostTask(FROM_HERE, task);
     DCHECK(posted);
   }
 }
 
+// Callback for GetEntryByResourceIdAsync.
+// Removes stale entry upon upload of file.
+void RemoveStaleEntryOnUpload(const std::string& resource_id,
+                              GDataDirectory* parent_dir,
+                              GDataEntry* existing_entry) {
+  if (existing_entry &&
+      // This should always match, but just in case.
+      existing_entry->parent() == parent_dir) {
+    parent_dir->RemoveEntry(existing_entry);
+  } else {
+    LOG(ERROR) << "Entry for the existing file not found: " << resource_id;
+  }
+}
+
+// Callback for GetEntryByResourceIdAsync.
+// Adds |entry| to |results|. Runs |callback| with |results| when
+// |run_callback| is true.
+void AddEntryToSearchResults(
+    std::vector<SearchResultInfo>* results,
+    const SearchCallback& callback,
+    base::PlatformFileError error,
+    bool run_callback,
+    GDataEntry* entry) {
+  // If a result is not present in our local file system snapshot, ignore it.
+  // For example, this may happen if the entry has recently been added to the
+  // drive (and we still haven't received its delta feed).
+  if (entry) {
+    const bool is_directory = entry->AsGDataDirectory() != NULL;
+    results->push_back(SearchResultInfo(entry->GetFilePath(), is_directory));
+  }
+  if (run_callback && !callback.is_null())
+    callback.Run(error, scoped_ptr<std::vector<SearchResultInfo> >(results));

[hshi1, 2012/07/11 00:23:44]

Is it possible that |callback| is null? It seems that if callback is null, we
may leak memory associated with the std::vector<SearchResultInfo> |results|,
because the scoped_ptr would not be initialized with the |results|.

[achuithb, 2012/07/11 00:38:10]

I don't think the callback would be NULL since we're doing a lot of work to
collect the results. This is Toni's code so I'm not sure. I've added code to
defend against this potential leak.

+}
+
 // Runs task on UI thread.
 void RunTaskOnUIThread(const base::Closure& task) {
   RunTaskOnThread(
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI), task);
 }
 
 // RelayCallback relays arguments for callback running on the given thread.
 template<typename CallbackType>
 struct RelayCallback;
 
@@ skipping 2063 matching lines @@
 
   if (error != base::PLATFORM_FILE_OK) {
     if (!callback.is_null())
       callback.Run(error, scoped_ptr<std::vector<SearchResultInfo> >());
     return;
   }
 
   // The search results will be returned using virtual directory.
   // The directory is not really part of the file system, so it has no parent or
   // root.
-  scoped_ptr<std::vector<SearchResultInfo> > results(
-      new std::vector<SearchResultInfo>());
+  std::vector<SearchResultInfo>* results(new std::vector<SearchResultInfo>());
 
   DCHECK_EQ(1u, params->feed_list->size());
   DocumentFeed* feed = params->feed_list->at(0);
 
   // Go through all entires generated by the feed and add them to the search
   // result directory.
   for (size_t i = 0; i < feed->entries().size(); ++i) {
     DocumentEntry* doc = const_cast<DocumentEntry*>(feed->entries()[i]);
     scoped_ptr<GDataEntry> entry(
         GDataEntry::FromDocumentEntry(NULL, doc, root_.get()));
@@ skipping 11 matching lines @@
       scoped_ptr<GDataFile> entry_as_file(entry.release()->AsGDataFile());
       root_->RefreshFile(entry_as_file.Pass());
       // We shouldn't use entry object after this point.
       DCHECK(!entry.get());
     }
 
     // We will need information about result entry to create info for callback.
     // We can't use |entry| anymore, so we have to refetch entry from file
     // system. Also, |entry| doesn't have file path set before |RefreshFile|
     // call, so we can't get file path from there.
-    GDataEntry* saved_entry = root_->GetEntryByResourceId(entry_resource_id);
-
-    // If a result is not present in our local file system snapshot, ignore it.
-    // For example, this may happen if the entry has recently been added to the
-    // drive (and we still haven't received its delta feed).
-    if (!saved_entry)
-      continue;
-
-    bool is_directory = saved_entry->AsGDataDirectory() != NULL;
-    results->push_back(SearchResultInfo(saved_entry->GetFilePath(),
-                                        is_directory));
+    root_->GetEntryByResourceIdAsync(entry_resource_id,
+        base::Bind(&AddEntryToSearchResults,
+                   results,
+                   callback,
+                   error,
+                   i+1 == feed->entries().size()));
   }
-
-  if (!callback.is_null())
-    callback.Run(error, results.Pass());
 }
 
 void GDataFileSystem::Search(const std::string& search_query,
                              const SearchCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
          BrowserThread::CurrentlyOn(BrowserThread::IO));
   RunTaskOnUIThread(base::Bind(&GDataFileSystem::SearchAsyncOnUIThread,
                                ui_weak_ptr_,
                                search_query,
                                CreateRelayCallback(callback)));
@@ skipping 990 matching lines @@
 
   scoped_ptr<GDataEntry> new_entry(
       GDataEntry::FromDocumentEntry(parent_dir, entry, root_.get()));
   if (!new_entry.get()) {
     callback.Run();
     return;
   }
 
   if (upload_mode == UPLOAD_EXISTING_FILE) {
     // Remove an existing entry, which should be present.
-    GDataEntry* existing_entry = root_->GetEntryByResourceId(
-        new_entry->resource_id());
-    if (existing_entry &&
-        // This should always match, but just in case.
-        existing_entry->parent() == parent_dir) {
-      parent_dir->RemoveEntry(existing_entry);
-    } else {
-      LOG(ERROR) << "Entry for the existing file not found: "
-                 << new_entry->resource_id();
-    }
+    const std::string& resource_id = new_entry->resource_id();
+    root_->GetEntryByResourceIdAsync(resource_id,
+        base::Bind(&RemoveStaleEntryOnUpload, resource_id, parent_dir));
   }
 
   GDataFile* file = new_entry->AsGDataFile();
   DCHECK(file);
   const std::string& resource_id = file->resource_id();
   const std::string& md5 = file->file_md5();
   parent_dir->AddEntry(new_entry.release());
 
   NotifyDirectoryChanged(virtual_dir_path);
 
@@ skipping 337 matching lines @@
   // must go through here. Removes the |file_path| from the remembered set so
   // that subsequent operations can open the file again.
   open_files_.erase(file_path);
 
   // Then invokes the user-supplied callback function.
   if (!callback.is_null())
     callback.Run(result);
 }
 
 }  // namespace gdata