platform_util::OpenItem fixes

chrome/browser/platform_util_chromeos.cc

 // Copyright (c) 2011 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/platform_util.h"
 
+#include "base/bind.h"
 #include "base/file_util.h"
 #include "base/process_util.h"
 #include "base/task.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/file_manager_util.h"
 #include "chrome/browser/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "content/common/process_watcher.h"
 #include "content/public/browser/browser_thread.h"
 #include "googleurl/src/gurl.h"
 
 using content::BrowserThread;
 
 class Profile;
 
-namespace platform_util {
+namespace {
 
-static const std::string kGmailComposeUrl =
+const std::string kGmailComposeUrl =
     "https://mail.google.com/mail/?extsrc=mailto&url=";
 
+void OpenItemOnFileThread(const FilePath& full_path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  base::Closure callback;
+  if (file_util::DirectoryExists(full_path))
+    callback = base::Bind(&FileManagerUtil::ViewFolder, full_path);
+  else
+    callback = base::Bind(&FileManagerUtil::ViewItem, full_path, false);
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
+}
+
 // Opens file browser on UI thread.
-static
 void OpenFileBrowserOnUIThread(const FilePath& dir) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   Browser* browser = BrowserList::GetLastActive();
   if (!browser)
     return;
 
   FilePath virtual_path;
   if (!FileManagerUtil::ConvertFileToRelativeFileSystemPath(browser->profile(),
                                                             dir,
                                                             &virtual_path)) {
     return;
   }
 
   GURL url = FileManagerUtil::GetFileBrowserUrlWithParams(
      SelectFileDialog::SELECT_NONE, string16(), virtual_path, NULL, 0,
      FilePath::StringType());
   browser->ShowSingletonTab(url);
 }
 
+void OpenURL(const std::string& url) {
+  Browser* browser = BrowserList::GetLastActive();
+  browser->AddSelectedTabWithURL(GURL(url), content::PAGE_TRANSITION_LINK);
+}
+
+}  // namespace
+
+namespace platform_util {
+
 void ShowItemInFolder(const FilePath& full_path) {
-  FilePath dir = full_path.DirName();
-  if (!file_util::DirectoryExists(dir))
-    return;
+  if (BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
+    FilePath dir = full_path.DirName();
+    if (!file_util::DirectoryExists(dir))
+      return;
 
-  if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-    OpenFileBrowserOnUIThread(dir);
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            base::Bind(&OpenFileBrowserOnUIThread, dir));
   } else {
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE,
-        NewRunnableFunction(&OpenFileBrowserOnUIThread, dir));
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                            base::Bind(&ShowItemInFolder, full_path));

[Randy Smith (Not in Mondays), 2011/11/09 17:20:30]

nit, suggestion (i.e. up to you if you want to take it): The pattern I like
better here is to get the error/less important pathway out of the way at the
beginning, and leave the real code in the main function.  I.e.

if (!on file thread)
  Post task
  Return

Do file thread work


(I know that you may not keep the pattern of having a single function for both
off and on thread--this is just if you do.)

[achuithb, 2011/11/09 20:21:36]

I had your suggestion in this CL and then switched to the way I was doing it
(using if/else instead of the early return). My rationale for switching was that
the if/else was more compact, and I was trying to avoid early returns in the
middle of functions. However, your suggestion is clearer, so I'll go with that
in the future. In this CL, I've split the function by thread.

   }
 }
 
 void OpenItem(const FilePath& full_path) {
-  FileManagerUtil::ViewItem(full_path, false);
-}
-
-static void OpenURL(const std::string& url) {
-  Browser* browser = BrowserList::GetLastActive();
-  browser->AddSelectedTabWithURL(GURL(url), content::PAGE_TRANSITION_LINK);
+  if (BrowserThread::CurrentlyOn(BrowserThread::FILE))
+    OpenItemOnFileThread(full_path);
+  else
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,

[Mark Mentovai, 2011/11/09 14:22:50]

What’s the point of this if you ultimately need the work to be done on the UI
thread? OpenItemOnFileThread doesn’t seem to do any work that MUST be done on
the file thread. Why not jump straight to the UI thread? And if you start out on
the UI thread, why not just handle the request directly?

[achuithb, 2011/11/09 20:21:36]

file_util::DirectoryExists must be called on the FILE thread.

+                            base::Bind(&OpenItemOnFileThread, full_path));
 }
 
 void OpenExternal(const GURL& url) {
   if (url.SchemeIs("mailto")) {
     std::string string_url = kGmailComposeUrl;
     string_url.append(url.spec());
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE, NewRunnableFunction(OpenURL, string_url));
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            base::Bind(OpenURL, string_url));
   }
 }
 
 }  // namespace platform_util