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

chrome/browser/platform_util_mac.mm

Index: chrome/browser/platform_util_mac.mm
diff --git a/chrome/browser/platform_util_mac.mm b/chrome/browser/platform_util_mac.mm
index 15622f16ca3a51c53d94ca47adeac73e7be62609..c7c6a7649a6bc12d9834bbd9873600790ff12d6d 100644
--- a/chrome/browser/platform_util_mac.mm
+++ b/chrome/browser/platform_util_mac.mm
@@ -8,6 +8,8 @@
 #import <Cocoa/Cocoa.h>
 #include <CoreServices/CoreServices.h>
 
+#include "base/bind.h"
+#include "base/files/file_util.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/mac/mac_logging.h"
@@ -15,6 +17,8 @@
 #import "base/mac/sdk_forward_declarations.h"
 #include "base/mac/scoped_aedesc.h"
 #include "base/strings/sys_string_conversions.h"
+#include "chrome/browser/platform_util_internal.h"
+#include "content/public/browser/browser_thread.h"
 #include "url/gurl.h"
 
 namespace platform_util {
@@ -27,7 +31,14 @@ void ShowItemInFolder(Profile* profile, const base::FilePath& full_path) {
     LOG(WARNING) << "NSWorkspace failed to select file " << full_path.value();
 }
 
-void OpenItem(Profile* profile, const base::FilePath& full_path) {
+// This function opens a file.  This doesn't use LaunchServices or NSWorkspace

[Robert Sesek, 2015/02/05 00:52:29]

This comment isn't accurate anymore (c.f. lines 47-50), so you can remove it. (I
think it was a holdover from before I made the change below).

[asanka, 2015/02/05 18:07:25]

Comment removed.

+// because of two bugs:
+//  1. Incorrect app activation with com.apple.quarantine:
+//     http://crbug.com/32921
+//  2. Silent no-op for unassociated file types: http://crbug.com/50263
+// Instead, an AppleEvent is constructed to tell the Finder to open the
+// document.
+void OpenFileOnMainThread(const base::FilePath& full_path) {
   DCHECK([NSThread isMainThread]);
   NSString* path_string = base::SysUTF8ToNSString(full_path.value());
   if (!path_string)
@@ -64,7 +75,7 @@ void OpenItem(Profile* profile, const base::FilePath& full_path) {
                               sizeof(finderCreatorCode),  // dataSize
                               address.OutPointer());  // result
   if (status != noErr) {
-    OSSTATUS_LOG(WARNING, status) << "Could not create OpenItem() AE target";
+    OSSTATUS_LOG(WARNING, status) << "Could not create OpenFile() AE target";
     return;
   }
 
@@ -77,7 +88,7 @@ void OpenItem(Profile* profile, const base::FilePath& full_path) {
                               kAnyTransactionID,  // transactionID
                               theEvent.OutPointer());  // result
   if (status != noErr) {
-    OSSTATUS_LOG(WARNING, status) << "Could not create OpenItem() AE event";
+    OSSTATUS_LOG(WARNING, status) << "Could not create OpenFile() AE event";
     return;
   }
 
@@ -88,7 +99,7 @@ void OpenItem(Profile* profile, const base::FilePath& full_path) {
                         false,  // isRecord
                         fileList.OutPointer());  // resultList
   if (status != noErr) {
-    OSSTATUS_LOG(WARNING, status) << "Could not create OpenItem() AE file list";
+    OSSTATUS_LOG(WARNING, status) << "Could not create OpenFile() AE file list";
     return;
   }
 
@@ -104,11 +115,11 @@ void OpenItem(Profile* profile, const base::FilePath& full_path) {
                       sizeof(pathRef));  // dataSize
     if (status != noErr) {
       OSSTATUS_LOG(WARNING, status)
-          << "Could not add file path to AE list in OpenItem()";
+          << "Could not add file path to AE list in OpenFile()";
       return;
     }
   } else {
-    LOG(WARNING) << "Could not get FSRef for path URL in OpenItem()";
+    LOG(WARNING) << "Could not get FSRef for path URL in OpenFile()";
     return;
   }
 
@@ -118,7 +129,7 @@ void OpenItem(Profile* profile, const base::FilePath& full_path) {
                           fileList);  // theAEDesc
   if (status != noErr) {
     OSSTATUS_LOG(WARNING, status)
-        << "Could not put the AE file list the path in OpenItem()";
+        << "Could not put the AE file list the path in OpenFile()";
     return;
   }
 
@@ -133,10 +144,29 @@ void OpenItem(Profile* profile, const base::FilePath& full_path) {
                   NULL);  // filterProc
   if (status != noErr) {
     OSSTATUS_LOG(WARNING, status)
-        << "Could not send AE to Finder in OpenItem()";
+        << "Could not send AE to Finder in OpenFile()";
   }
 }
 
+namespace internal {
+
+void PlatformOpenVerifiedItem(const base::FilePath& path, OpenItemType type) {
+  switch (type) {
+    case OPEN_FILE:
+      content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                       base::Bind(&OpenFileOnMainThread, path));
+      return;
+    case OPEN_FOLDER:
+      NSString* path_string = base::SysUTF8ToNSString(path.value());
+      if (!path_string)
+        return;
+      [[NSWorkspace sharedWorkspace] openFile:path_string

[Robert Sesek, 2015/02/05 00:52:29]

Does:

  NSURL* url = [NSURL fileURLWithPath:path_string];
  [[NSWorkspace sharedWorkspace] activateFileViewerSelectingURLs:
      @[ url ]];

work? Users can technically have Finder alternatives installed.

[asanka, 2015/02/05 18:07:25]

I tried this, but it doesn't have the intended effect. This causes the
containing folder to the opened in Finder with the target folder item selected.
Using "/target/path/." also doesn't help.

By the way is this method considered canonical for opening a finder and
selecting an item? I can update ShowItemInFolder() above while I'm here since
the behavior matches what's expected there.

[Robert Sesek, 2015/02/05 23:37:55]

I think the only difference between this method and the current implementation
is selecting a single vs. multiple files, and based on URL vs file path.
Otherwise, I think they're equivalent.

For here, though, if you remove the withApplication:@"Finder" does this still
work as expected? Specifically, has |path| been validated to be a directory, or
is Finder explicitly being called out here to handle that?

+                              withApplication:@"Finder"];
+      return;
+  }
+}
+}

[Robert Sesek, 2015/02/05 00:52:29]

nit: blank line before, and "  // namespace internal"

[asanka, 2015/02/05 18:07:25]

Done.

+
 void OpenExternal(Profile* profile, const GURL& url) {
   DCHECK([NSThread isMainThread]);
   NSString* url_string = base::SysUTF8ToNSString(url.spec());