Be explicit about target type in platform_util::OpenItem()

chrome/browser/chromeos/file_manager/open_util.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/file_manager/open_util.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 109 matching lines @@
     }
   }
 
   ExecuteFileTaskForUrl(profile, chosen_task->task_descriptor(), url);
   callback.Run(true);
 }
 
 // Opens the file specified by |url| by finding and executing a file task for
 // the file. In case of success, calls |callback| with true. Otherwise the
 // returned value is false.
-void OpenFile(Profile* profile,
-              const base::FilePath& path,
-              const GURL& url,
-              const base::Callback<void(bool)>& callback) {
+void OpenFileInternal(Profile* profile,
+                      const base::FilePath& path,
+                      const GURL& url,
+                      const base::Callback<void(bool)>& callback) {
   extensions::app_file_handler_util::GetMimeTypeForLocalPath(
       profile,
       path,
       base::Bind(&OpenFileWithMimeType, profile, path, url, callback));
 }
 
+enum ExpectedOpenTarget {
+  OPEN_FILE,
+  OPEN_FOLDER
+};
+
 // Called when execution of ContinueOpenItem() is completed.
 void OnContinueOpenItemCompleted(Profile* profile,
                                  const base::FilePath& file_path,
                                  bool result) {
   if (!result) {
     int message;
     if (file_path.Extension() == FILE_PATH_LITERAL(".dmg"))
       message = IDS_FILE_BROWSER_ERROR_VIEWING_FILE_FOR_DMG;
     else if (file_path.Extension() == FILE_PATH_LITERAL(".exe") ||
              file_path.Extension() == FILE_PATH_LITERAL(".msi"))
       message = IDS_FILE_BROWSER_ERROR_VIEWING_FILE_FOR_EXECUTABLE;
     else
       message = IDS_FILE_BROWSER_ERROR_VIEWING_FILE;
     ShowWarningMessageBox(profile, file_path, message);
   }
 }
 
-// Used to implement OpenItem().
+// Used to implement OpenFile()/OpenFolder().
 void ContinueOpenItem(Profile* profile,

[hashimoto, 2014/09/25 02:34:05]

I think it's better to split this function into two.
One for opening files (this can be merged with OpenFileInternal) and another for
opening directories.

[asanka, 2014/09/25 22:24:20]

Done. Good call.

+                      ExpectedOpenTarget expected_target,
                       const base::FilePath& file_path,
                       const GURL& url,
-                      base::File::Error error) {
+                      base::File::Error check_directory_result) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  const bool target_is_a_directory =
+      (check_directory_result == base::File::FILE_OK);
 
-  if (error == base::File::FILE_OK) {
-    // A directory exists at |url|. Open it with the file manager.
-    OpenFileManagerWithInternalActionId(profile, url, "open");
-  } else {
-    // |url| should be a file. Open it.
-    OpenFile(profile,
-             file_path,
-             url,
-             base::Bind(&OnContinueOpenItemCompleted, profile, file_path));
+  switch (expected_target) {
+    case OPEN_FILE:
+      if (target_is_a_directory)
+        break;
+      OpenFileInternal(
+          profile,
+          file_path,
+          url,
+          base::Bind(&OnContinueOpenItemCompleted, profile, file_path));
+      return;
+
+    case OPEN_FOLDER:
+      if (target_is_a_directory) {
+        OpenFileManagerWithInternalActionId(profile, url, "open");
+        return;
+      }
+      break;
   }
+  OnContinueOpenItemCompleted(profile, file_path, false);
 }
 
 // Converts the |given_path| passed from external callers to the form that the
 // file manager can correctly handle. It first migrates old Drive/Download
 // folder path to the new formats, and then converts path to filesystem URL.
 //
 // When conversion fails, it shows a warning dialog UI and returns false.
 bool ConvertPath(Profile* profile,
                  const base::FilePath& given_path,
                  base::FilePath* path,
@@ skipping 19 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   base::FilePath converted_path;
   GURL url;
   if (!ConvertPath(profile, file_path, &converted_path, &url))
     return;
 
   OpenFileManagerWithInternalActionId(profile, url, "auto-open");
 }
 
-void OpenItem(Profile* profile, const base::FilePath& file_path) {
+void OpenFile(Profile* profile, const base::FilePath& file_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   base::FilePath converted_path;
+  GURL url;
+  if (!ConvertPath(profile, file_path, &converted_path, &url))
+    return;
+
+  CheckIfDirectoryExists(
+      GetFileSystemContextForExtensionId(profile, kFileManagerAppId),
+      url,
+      base::Bind(&ContinueOpenItem, profile, OPEN_FILE, converted_path, url));
+}
+
+void OpenFolder(Profile* profile, const base::FilePath& file_path) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  base::FilePath converted_path;
   GURL url;
   if (!ConvertPath(profile, file_path, &converted_path, &url))
     return;
 
   CheckIfDirectoryExists(
       GetFileSystemContextForExtensionId(profile, kFileManagerAppId),
       url,
-      base::Bind(&ContinueOpenItem, profile, converted_path, url));
+      base::Bind(&ContinueOpenItem, profile, OPEN_FOLDER, converted_path, url));
 }
 
 void ShowItemInFolder(Profile* profile, const base::FilePath& file_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   base::FilePath converted_path;
   GURL url;
   if (!ConvertPath(profile, file_path, &converted_path, &url))
     return;
 
   // This action changes the selection so we do not reuse existing tabs.
   OpenFileManagerWithInternalActionId(profile, url, "select");
 }
 
 }  // namespace util
 }  // namespace file_manager