Make FileTaskExecutor testable.

chrome/browser/chromeos/drive/file_task_executor.cc

Index: chrome/browser/chromeos/drive/file_task_executor.cc
diff --git a/chrome/browser/chromeos/drive/file_task_executor.cc b/chrome/browser/chromeos/drive/file_task_executor.cc
index 266b92ad3d0dec4b93c1dbd8f09a1b6b7a7c0fc8..37ea8aee013e7255c79e6554d326ccfb4595ab13 100644
--- a/chrome/browser/chromeos/drive/file_task_executor.cc
+++ b/chrome/browser/chromeos/drive/file_task_executor.cc
@@ -25,9 +25,49 @@ using fileapi::FileSystemURL;
 
 namespace drive {
 
-FileTaskExecutor::FileTaskExecutor(Profile* profile,
-                                   const std::string& app_id)
-  : profile_(profile),
+namespace {
+
+class FileTaskExecutorDelegateImpl : public FileTaskExecutorDelegate {
+ public:
+  explicit FileTaskExecutorDelegateImpl(Profile* profile) : profile_(profile) {
+  }
+
+  virtual FileSystemInterface* GetFileSystem() OVERRIDE {
+    return util::GetFileSystemByProfile(profile_);
+  }
+
+  virtual DriveServiceInterface* GetDriveService() OVERRIDE {
+    return util::GetDriveServiceByProfile(profile_);
+  }
+
+  virtual void OpenBrowserWindow(const GURL& open_link) OVERRIDE {
+    chrome::ScopedTabbedBrowserDisplayer displayer(
+         profile_, chrome::HOST_DESKTOP_TYPE_ASH);
+    chrome::AddSelectedTabWithURL(displayer.browser(), open_link,
+                                  content::PAGE_TRANSITION_LINK);
+    // Since the ScopedTabbedBrowserDisplayer does not guarantee that the
+    // browser will be shown on the active desktop, we ensure the visibility.
+    multi_user_util::MoveWindowToCurrentDesktop(
+        displayer.browser()->window()->GetNativeWindow());
+  }
+
+ private:
+  Profile* profile_;

[hirono, 2014/07/16 03:33:51]

nit: Profile* const

[kinaba, 2014/07/16 04:11:03]

Done.

+};
+
+}  // namespace
+
+FileTaskExecutor::FileTaskExecutor(Profile* profile, const std::string& app_id)
+  : delegate_(new FileTaskExecutorDelegateImpl(profile)),
+    app_id_(app_id),
+    current_index_(0),
+    weak_ptr_factory_(this) {
+}
+
+FileTaskExecutor::FileTaskExecutor(
+    scoped_ptr<FileTaskExecutorDelegate> delegate,
+    const std::string& app_id)
+  : delegate_(delegate.Pass()),
     app_id_(app_id),
     current_index_(0),
     weak_ptr_factory_(this) {
@@ -39,6 +79,8 @@ FileTaskExecutor::~FileTaskExecutor() {
 void FileTaskExecutor::Execute(
     const std::vector<FileSystemURL>& file_urls,
     const file_manager::file_tasks::FileTaskFinishedCallback& done) {
+  DCHECK(!file_urls.empty());
+
   done_ = done;
 
   std::vector<base::FilePath> paths;
@@ -51,7 +93,7 @@ void FileTaskExecutor::Execute(
     paths.push_back(path);
   }
 
-  FileSystemInterface* file_system = util::GetFileSystemByProfile(profile_);
+  FileSystemInterface* file_system = delegate_->GetFileSystem();

[hirono, 2014/07/16 03:33:51]

nit: FileSystemInterface* const

[kinaba, 2014/07/16 04:11:03]

Done.

   if (!file_system) {
     Done(false);
     return;
@@ -75,9 +117,7 @@ void FileTaskExecutor::OnFileEntryFetched(FileError error,
   if (entry.get() && !entry->has_file_specific_info())
     error = FILE_ERROR_NOT_FOUND;
 
-  DriveServiceInterface* drive_service =
-      util::GetDriveServiceByProfile(profile_);
-
+  DriveServiceInterface* drive_service = delegate_->GetDriveService();

[hirono, 2014/07/16 03:33:51]

nit: DriveServiceInterface* const

[kinaba, 2014/07/16 04:11:03]

Done.

   if (!drive_service || error != FILE_ERROR_OK) {
     Done(false);
     return;
@@ -98,24 +138,12 @@ void FileTaskExecutor::OnAppAuthorized(const std::string& resource_id,
                                        const GURL& open_link) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  DriveIntegrationService* service =
-      DriveIntegrationServiceFactory::FindForProfile(profile_);
-  if (!service || !service->IsMounted() ||

[hirono, 2014/07/16 03:33:51]

Can we remove IsMounted check?

[kinaba, 2014/07/16 04:11:03]

Yes, I believe so.

If IsMounted is false, GetDriveService() in OnFileEntryFetched returns NULL.
So basically, we don't reach this point in the case.

The only subtle case is that when the user disabled Drive integration during
this async call (which is the only possibility that turns IsMounted off).
The link can be opened safely even in this case. And I think it's ok to open
the link, since that's the user indicated before (s)he disabled. Of course,
re-checking IsMounted flag here is also a possible choice, but I'd like to
prefer
simplicity.

[hirono, 2014/07/16 04:12:46]

Got it. Thank you!

-      error != google_apis::HTTP_SUCCESS || open_link.is_empty()) {
+  if (error != google_apis::HTTP_SUCCESS || open_link.is_empty()) {
     Done(false);
     return;
   }
 
-  {
-    chrome::ScopedTabbedBrowserDisplayer displayer(
-         profile_, chrome::HOST_DESKTOP_TYPE_ASH);
-    chrome::AddSelectedTabWithURL(displayer.browser(), open_link,
-                                  content::PAGE_TRANSITION_LINK);
-    // Since the ScopedTabbedBrowserDisplayer does not guarantee that the
-    // browser will be shown on the active desktop, we ensure the visibility.
-    multi_user_util::MoveWindowToCurrentDesktop(
-        displayer.browser()->window()->GetNativeWindow());
-  }
+  delegate_->OpenBrowserWindow(open_link);
 
   // We're done with this file.  If this is the last one, then we're done.
   current_index_--;