[Mac Fix-It] Changed OpenApplicationWithPath to use NSWorkspace.

base/mac/launch_services_util.mm

Index: base/mac/launch_services_util.mm
diff --git a/base/mac/launch_services_util.mm b/base/mac/launch_services_util.mm
new file mode 100644
index 0000000000000000000000000000000000000000..f5b9dae7679eb95331924b97c1a3601d65a6e447
--- /dev/null
+++ b/base/mac/launch_services_util.mm
@@ -0,0 +1,55 @@
+// Copyright 2013 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.
+
+#import "base/mac/launch_services_util.h"
+
+#include "base/logging.h"
+#include "base/strings/sys_string_conversions.h"
+
+namespace base {
+namespace mac {
+
+bool OpenApplicationWithPath(const base::FilePath& bundle_path,
+                             const CommandLine& command_line,
+                             NSWorkspaceLaunchOptions launch_options,
+                             pid_t* out_pid) {
+  NSString* bundle_url_spec = base::SysUTF8ToNSString(bundle_path.value());
+  NSURL* bundle_url = [NSURL fileURLWithPath:bundle_url_spec isDirectory:YES];
+  if (!bundle_url) {

[tapted, 2016/10/04 05:54:08]

apart from OOM, can this fail? Perhaps just DCHECK(bundle_url) so we can trim
some string data from the binary. 

[Eugene But (OOO till 7-30), 2016/10/04 07:02:49]

It can if bundle_path.value() is an empty string. I also missed early return
which was in the originally code. w/o early return launchApplicationAtURL: would
crash for nil URL, so I added it back.

+    LOG(ERROR) << "Cannot create NSURL for " << bundle_path.value();
+  }
+
+  std::vector<std::string> argv = command_line.argv();
+  int argc = argv.size();
+  NSMutableArray* launch_args = [NSMutableArray arrayWithCapacity:argc - 1];

[tapted, 2016/10/04 05:54:08]

NSWorkspaceLaunchConfigurationArguments says it's an NSArray of NSStrings passed
into the argv parameter, so should argv[0] be the command anyway? That would
allow for new-school for loops below too :). Otherwise we should comment here to
say why we need to remove it...

But LSApplicationParameters::argv seems to have the same documentation... so I'm
not sure how this ever worked :|

[Eugene But (OOO till 7-30), 2016/10/04 07:02:49]

argv[0] is a binary path, which automatically added by NSWorkspace (I checked).
Added comments to explain this.

+  for (int i = 1; i < argc; ++i) {
+    const std::string& arg(argv[i]);
+    [launch_args addObject:base::SysUTF8ToNSString(arg)];
+  }
+
+  NSDictionary* configuration = @{
+    NSWorkspaceLaunchConfigurationArguments : launch_args,
+  };

[tapted, 2016/10/04 05:54:08]

That // TODO(jeremya): this opens a new browser window if Chrome is already ...
probably still belongs here. 

And, not for this CL, but I think the TODO can actually be resolved by adding
`NSWorkspaceLaunchNewInstance` -- the code here was written before Chrome
supported process singletons, but we since added support for that, so Chrome now
*does* support launching multiple times, and we use IPC over a domain socket to
pass the args -- that would probably fix some bugs, since
NSWorkspaceLaunchConfigurationArguments is ignored when the app is already
running.

[Eugene But (OOO till 7-30), 2016/10/04 07:02:49]

Added back TODO.

+  NSError* launch_error = nil;
+  NSRunningApplication* app =
+      [[NSWorkspace sharedWorkspace] launchApplicationAtURL:bundle_url
+                                                    options:launch_options
+                                              configuration:configuration
+                                                      error:&launch_error];
+
+  if (out_pid) {
+    *out_pid = app.processIdentifier;

[tapted, 2016/10/04 05:54:08]

[app processIdentifier]

But also launchApplicationAtURL will return nil on error - I guess this is still
the right thing to do, but it looks a bit weird.

[Eugene But (OOO till 7-30), 2016/10/04 07:02:49]

Done. New code now correctly handles nil case.

+  }
+
+  if (launch_error) {
+    NSString* error_description =
+        [[launch_error userInfo] objectForKey:NSLocalizedDescriptionKey];
+    DLOG(ERROR) << base::SysNSStringToUTF8(error_description);

[tapted, 2016/10/04 05:54:08]

There's probably a good argument for making this a LOG rather than DLOG. But
also I'm not sure whether a localized string from userInfo has the best thing to
log. Does NSLog do something reasonable? Maybe that will give something with a
consistent format that we can look for on message boards/logs/bugs

[Eugene But (OOO till 7-30), 2016/10/04 07:02:49]

Localized description prints this: 

The application “NameOfTheapp.app” could not be launched because it was not
found.

Which sounds like a very good message. NSLog logs error description, which can
be logged as follows:
LOG(ERROR) << base::SysNSStringToUTF8([launch_error description]);

Do you think it is any better than localized description?

[tapted, 2016/10/05 01:40:47]

The localized description should be OK - I agree it's nicer than a fixed string.
The typical thinking for LOG output is that a user is very unlikely to see it
(better to just crash so we get a report). If a user does see it, they might
plug it into Google, in which case localizing it could result in fewer hits.
Chromium developers might see it locally or in tryjob output - in that case
localizing probably doesn't matter.

+    return false;
+  }
+  return true;
+}
+
+}  // namespace mac
+}  // namespace base