Pass calls to GetDocuments through the scheduler

chrome/browser/chromeos/drive/drive_scheduler.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/drive/drive_scheduler.h"
 
 #include <math.h>
 
 #include "base/message_loop.h"
 #include "base/rand_util.h"
+#include "chrome/browser/chromeos/drive/drive_file_system_util.h"
 #include "chrome/browser/chromeos/drive/file_system/drive_operations.h"
 #include "chrome/browser/chromeos/drive/file_system/remove_operation.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 
 namespace drive {
 
 namespace {
 const int kMaxThrottleCount = 5;
 }
 
 DriveScheduler::JobInfo::JobInfo(JobType in_job_type, FilePath in_file_path)
     : job_type(in_job_type),
       job_id(-1),
       completed_bytes(0),
       total_bytes(0),
       file_path(in_file_path),
       state(STATE_NONE) {
 }
 
 DriveScheduler::QueueEntry::QueueEntry(JobType in_job_type,
-                                       FilePath in_file_path,
-                                       FileOperationCallback in_callback)
+                                       FilePath in_file_path)
     : job_info(in_job_type, in_file_path),
-      callback(in_callback),
       is_recursive(false) {
 }
 
 DriveScheduler::QueueEntry::~QueueEntry() {
 }
 
-DriveScheduler::DriveScheduler(Profile* profile,
-                               file_system::DriveOperations* drive_operations)
+DriveScheduler::DriveScheduler(
+    Profile* profile,
+    google_apis::DriveServiceInterface* drive_service,
+    file_system::DriveOperations* drive_operations)
     : job_loop_is_running_(false),
       next_job_id_(0),
       throttle_count_(0),
       disable_throttling_(false),
       drive_operations_(drive_operations),
+      drive_service_(drive_service),
       profile_(profile),
       weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       initialized_(false) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 DriveScheduler::~DriveScheduler() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(initialized_);
   net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
 }
 
 void DriveScheduler::Initialize() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Initialize() may be called more than once for the lifetime when the
   // file system is remounted.
   if (initialized_)
     return;
 
   net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
   initialized_ = true;
 }
 
 void DriveScheduler::Copy(const FilePath& src_file_path,
                           const FilePath& dest_file_path,
                           const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  scoped_ptr<QueueEntry> new_job(
-      new QueueEntry(TYPE_COPY, src_file_path, callback));
+  scoped_ptr<QueueEntry> new_job(new QueueEntry(TYPE_COPY, src_file_path));
   new_job->dest_file_path = dest_file_path;
+  new_job->file_operation_callback = callback;
 
   QueueJob(new_job.Pass());
 
+  StartJobLoop();
+}
+
+void DriveScheduler::GetDocuments(
+    const GURL& feed_url,
+    int64 start_changestamp,
+    const std::string& search_query,
+    bool shared_with_me,
+    const std::string& directory_resource_id,
+    const google_apis::GetDataCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  scoped_ptr<QueueEntry> new_job(
+      new QueueEntry(TYPE_GET_DOCUMENTS, FilePath()));
+  new_job->feed_url = feed_url;
+  new_job->start_changestamp = start_changestamp;
+  new_job->search_query = search_query;
+  new_job->shared_with_me = shared_with_me;
+  new_job->directory_resource_id = directory_resource_id;
+  new_job->get_data_callback = callback;
+
+  QueueJob(new_job.Pass());
+
   StartJobLoop();
 }
 
 void DriveScheduler::TransferFileFromRemoteToLocal(
     const FilePath& remote_src_file_path,
     const FilePath& local_dest_file_path,
     const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   scoped_ptr<QueueEntry> new_job(new QueueEntry(TYPE_TRANSFER_REMOTE_TO_LOCAL,
-                                                remote_src_file_path,
-                                                callback));
+                                                remote_src_file_path));
   new_job->dest_file_path = local_dest_file_path;
+  new_job->file_operation_callback = callback;
 
   QueueJob(new_job.Pass());
 
   StartJobLoop();
 }
 
 void DriveScheduler::TransferFileFromLocalToRemote(
     const FilePath& local_src_file_path,
     const FilePath& remote_dest_file_path,
     const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   scoped_ptr<QueueEntry> new_job(new QueueEntry(TYPE_TRANSFER_LOCAL_TO_REMOTE,
-                                                local_src_file_path,
-                                                callback));
+                                                local_src_file_path));
   new_job->dest_file_path = remote_dest_file_path;
+  new_job->file_operation_callback = callback;
 
   QueueJob(new_job.Pass());
 
   StartJobLoop();
 }
 
 void DriveScheduler::TransferRegularFile(
     const FilePath& local_src_file_path,
     const FilePath& remote_dest_file_path,
     const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   scoped_ptr<QueueEntry> new_job(new QueueEntry(TYPE_TRANSFER_REGULAR_FILE,
-                                                local_src_file_path,
-                                                callback));
+                                                local_src_file_path));
   new_job->dest_file_path = remote_dest_file_path;
+  new_job->file_operation_callback = callback;
 
   QueueJob(new_job.Pass());
 
   StartJobLoop();
 }
 
 void DriveScheduler::Move(const FilePath& src_file_path,
                           const FilePath& dest_file_path,
                           const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  scoped_ptr<QueueEntry> new_job(
-      new QueueEntry(TYPE_MOVE, src_file_path, callback));
+  scoped_ptr<QueueEntry> new_job(new QueueEntry(TYPE_MOVE, src_file_path));
   new_job->dest_file_path = dest_file_path;
+  new_job->file_operation_callback = callback;
 
   QueueJob(new_job.Pass());
 
   StartJobLoop();
 }
 
 void DriveScheduler::Remove(const FilePath& file_path,
                             bool is_recursive,
                             const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  scoped_ptr<QueueEntry> new_job(
-      new QueueEntry(TYPE_REMOVE, file_path, callback));
+  scoped_ptr<QueueEntry> new_job(new QueueEntry(TYPE_REMOVE, file_path));
   new_job->is_recursive = is_recursive;
+  new_job->file_operation_callback = callback;
 
   QueueJob(new_job.Pass());
 
   StartJobLoop();
 }
 
 int DriveScheduler::QueueJob(scoped_ptr<QueueEntry> job) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   int job_id = next_job_id_;
@@ skipping 25 matching lines @@
   }
   job_loop_is_running_ = true;
 
   // Should copy before calling queue_.pop_front().
   int job_id = queue_.front();
   queue_.pop_front();
 
   JobMap::iterator job_iter = job_info_map_.find(job_id);
   DCHECK(job_iter != job_info_map_.end());
 
   JobInfo& job_info = job_iter->second->job_info;

[satorux1, 2012/11/28 23:59:47]

Can you make it const? We rarely use a non-const reference.

[Zachary Kuznia, 2012/11/30 05:42:30]

Unfortunately, making it const makes the next line fail.  I think it's ok to use
a non-const reference, because it's not a function argument, so it shouldn't
violate the style guide.

   job_info.state = STATE_RUNNING;
 

[satorux1, 2012/11/28 23:59:47]

What about adding:

const QueueEntry& query_entry = *job_iter->second;

so we don't repeat  job_iter->second in many places below

[Zachary Kuznia, 2012/11/30 05:42:30]

Done.

   switch (job_info.job_type) {
     case TYPE_COPY: {
       drive_operations_->Copy(
           job_info.file_path,
           job_iter->second->dest_file_path,
-          base::Bind(&DriveScheduler::OnJobDone,
+          base::Bind(&DriveScheduler::OnFileOperationJobDone,
                      weak_ptr_factory_.GetWeakPtr(),
                      job_id));
     }
+    break;
+
+    case TYPE_GET_DOCUMENTS: {
+      drive_service_->GetDocuments(
+          job_iter->second->feed_url,
+          job_iter->second->start_changestamp,
+          job_iter->second->search_query,
+          job_iter->second->shared_with_me,
+          job_iter->second->directory_resource_id,
+          base::Bind(&DriveScheduler::OnGetDataJobDone,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     job_id));
+    }
     break;
 
     case TYPE_MOVE: {
       drive_operations_->Move(
           job_info.file_path,
           job_iter->second->dest_file_path,
-          base::Bind(&DriveScheduler::OnJobDone,
+          base::Bind(&DriveScheduler::OnFileOperationJobDone,
                      weak_ptr_factory_.GetWeakPtr(),
                      job_id));
     }
     break;
 
     case TYPE_REMOVE: {
       drive_operations_->Remove(
           job_info.file_path,
           job_iter->second->is_recursive,
-          base::Bind(&DriveScheduler::OnJobDone,
+          base::Bind(&DriveScheduler::OnFileOperationJobDone,
                      weak_ptr_factory_.GetWeakPtr(),
                      job_id));
     }
     break;
 
     case TYPE_TRANSFER_LOCAL_TO_REMOTE: {
       drive_operations_->TransferFileFromLocalToRemote(
           job_info.file_path,
           job_iter->second->dest_file_path,
-          base::Bind(&DriveScheduler::OnJobDone,
+          base::Bind(&DriveScheduler::OnFileOperationJobDone,
                      weak_ptr_factory_.GetWeakPtr(),
                      job_id));
     }
     break;
 
     case TYPE_TRANSFER_REGULAR_FILE: {
       drive_operations_->TransferRegularFile(
           job_info.file_path,
           job_iter->second->dest_file_path,
-          base::Bind(&DriveScheduler::OnJobDone,
+          base::Bind(&DriveScheduler::OnFileOperationJobDone,
                      weak_ptr_factory_.GetWeakPtr(),
                      job_id));
     }
     break;
 
     case TYPE_TRANSFER_REMOTE_TO_LOCAL: {
       drive_operations_->TransferFileFromRemoteToLocal(
           job_info.file_path,
           job_iter->second->dest_file_path,
-          base::Bind(&DriveScheduler::OnJobDone,
+          base::Bind(&DriveScheduler::OnFileOperationJobDone,
                      weak_ptr_factory_.GetWeakPtr(),
                      job_id));
     }
     break;
 
     // There is no default case so that there will be a compiler error if a type
     // is added but unhandled.
   }
 }
 
@@ skipping 38 matching lines @@
       base::Bind(&DriveScheduler::DoJobLoop,
                  weak_ptr_factory_.GetWeakPtr()),
       delay);
   DCHECK(posted);
 }
 
 void DriveScheduler::ResetThrottleAndContinueJobLoop() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   throttle_count_ = 0;
-  DoJobLoop();
+  base::MessageLoopProxy::current()->PostNonNestableTask(

[satorux1, 2012/11/28 23:59:47]

Could you add some comment about why you don't call DoJobLoop() directly here?

Besides, why not just PostTask()?

[Zachary Kuznia, 2012/11/30 05:42:30]

Changed.  Done.

+      FROM_HERE,
+      base::Bind(&DriveScheduler::DoJobLoop,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
-void DriveScheduler::OnJobDone(int job_id, DriveFileError error) {
+scoped_ptr<DriveScheduler::QueueEntry> DriveScheduler::OnJobDone(
+    int job_id,
+    DriveFileError error) {
   JobMap::iterator job_iter = job_info_map_.find(job_id);
   DCHECK(job_iter != job_info_map_.end());
 
   // Retry, depending on the error.
-  if (error == DRIVE_FILE_ERROR_THROTTLED ||
-      error == DRIVE_FILE_ERROR_NO_CONNECTION) {
+  if (error == DRIVE_FILE_ERROR_THROTTLED) {
     job_iter->second->job_info.state = STATE_RETRY;
 
     // Requeue the job.
     queue_.push_back(job_id);
     ThrottleAndContinueJobLoop();
+
+    return scoped_ptr<DriveScheduler::QueueEntry>();
   } else {
-    // Handle the callback.
-    if (!job_iter->second->callback.is_null()) {
-      MessageLoop::current()->PostTask(FROM_HERE,
-          base::Bind(job_iter->second->callback, error));
-    }
+    scoped_ptr<DriveScheduler::QueueEntry> job_info(job_iter->second.release());
 
     // Delete the job.
     job_info_map_.erase(job_id);
     ResetThrottleAndContinueJobLoop();
+
+    return job_info.Pass();
   }
 }
 
+void DriveScheduler::OnFileOperationJobDone(int job_id, DriveFileError error) {
+  scoped_ptr<DriveScheduler::QueueEntry> job_info(OnJobDone(job_id, error));

[satorux1, 2012/11/28 23:59:47]

nit: When taking an object from a function returning scoped_ptr<>, please use
'='. This makes it clear that the function is returning a scoped_ptr (otherwise,
it won't compile)

scoped_ptr<DriveScheduler::QueueEntry> job_info = OnJobDone(job_id, error);

[Zachary Kuznia, 2012/11/30 05:42:30]

Done.

+
+  if (job_info.get()) {

[satorux1, 2012/11/28 23:59:47]

nit:

if (!job_info)
  return;

to save one indent level.

Please omit .get() from boolean context. That's a new preferred manner.

[Zachary Kuznia, 2012/11/30 05:42:30]

Done.

+    // Handle the callback.
+    if (!job_info->file_operation_callback.is_null()) {
+      MessageLoop::current()->PostTask(FROM_HERE,
+          base::Bind(job_info->file_operation_callback, error));
+    }
+  }
+}
+
+void DriveScheduler::OnGetDataJobDone(int job_id,
+                                      google_apis::GDataErrorCode error,
+                                      scoped_ptr<base::Value> feed_data) {
+  DriveFileError drive_error(util::GDataToDriveFileError(error));
+
+  scoped_ptr<QueueEntry> job_info(OnJobDone(job_id, drive_error));

[satorux1, 2012/11/28 23:59:47]

ditto.

[Zachary Kuznia, 2012/11/30 05:42:30]

Done.

+
+  if (job_info.get()) {

[satorux1, 2012/11/28 23:59:47]

ditto.

[Zachary Kuznia, 2012/11/30 05:42:30]

Done.

+    // Handle the callback.
+    if (!job_info->get_data_callback.is_null()) {
+      MessageLoop::current()->PostTask(FROM_HERE,
+          base::Bind(job_info->get_data_callback,
+                     error,
+                     base::Passed(&feed_data)));
+    }
+  }
+}
+
 void DriveScheduler::OnConnectionTypeChanged(
     net::NetworkChangeNotifier::ConnectionType type) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Resume the job loop if the network is back online. Note that we don't
   // need to check the type of the network as it will be checked in
   // ShouldStopJobLoop() as soon as the loop is resumed.
   if (!net::NetworkChangeNotifier::IsOffline())
     StartJobLoop();
 }
 
 }  // namespace drive