Mac app mode: locate Chrome + refactor

chrome/app/app_mode_loader_mac.mm

Index: chrome/app/app_mode_loader_mac.mm
diff --git a/chrome/app/app_mode_loader_mac.mm b/chrome/app/app_mode_loader_mac.mm
index 8ebd4f86823ee5e0430431f7286f7ea9f2efef93..fdd803399dc5fec30dba883b96346cc7fdf88073 100644
--- a/chrome/app/app_mode_loader_mac.mm
+++ b/chrome/app/app_mode_loader_mac.mm
@@ -9,140 +9,117 @@
 // framework side as possible).
 
 #include <dlfcn.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
 
 #include <CoreFoundation/CoreFoundation.h>
 #import <Foundation/Foundation.h>
 
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/mac/scoped_nsautorelease_pool.h"
+#include "base/sys_string_conversions.h"
+#import "chrome/common/mac/app_mode_chrome_locator.h"
 #include "chrome/common/mac/app_mode_common.h"
 
 namespace {
 
-// Checks that condition |x| is true. If not, outputs |msg| (together with
-// source filename and line number) and exits.
-#define CHECK_MSG(x, msg) if (!(x)) check_msg_helper(__FILE__, __LINE__, msg);
-void check_msg_helper(const char* file, int line, const char* msg) {
-  fprintf(stderr, "%s (%d): %s\n", file, line, msg);
-  exit(1);
-}
-
-// Converts an NSString to a UTF8 C string (which is allocated, and may be freed
-// using |free()|). If |s| is nil or can't produce such a string, this returns
-// |NULL|.
-char* NSStringToUTF8CString(NSString* s) {
-  CHECK_MSG([s isKindOfClass:[NSString class]], "expected an NSString");
-  const char* cstring = [s UTF8String];
-  return cstring ? strdup(cstring) : NULL;
-}
-
-// Converts an NSString to a file-system representation C string (which is
-// allocated, and may be freed using |free()|). If |s| is nil or can't produce
-// such a string, this returns |NULL|.
-char* NSStringToFSCString(NSString* s) {
-  CHECK_MSG([s isKindOfClass:[NSString class]], "expected an NSString");
-  const char* cstring = [s fileSystemRepresentation];
-  return cstring ? strdup(cstring) : NULL;
-}
-
-}  // namespace
-
-__attribute__((visibility("default")))
-int main(int argc, char** argv) {
-  app_mode::ChromeAppModeInfo info;
-  info.major_version = 0;  // v0.1
-  info.minor_version = 1;
-  info.argc = argc;
-  info.argv = argv;
+void LoadFramework(void** cr_dylib, app_mode::ChromeAppModeInfo* info) {
+  using base::SysNSStringToUTF8;
+  using base::SysNSStringToUTF16;
 
-  // The Cocoa APIs are a bit more convenient; for this an autorelease pool is
-  // needed.
-  NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
+  base::mac::ScopedNSAutoreleasePool scoped_pool;
 
   // Get the current main bundle, i.e., that of the app loader that's running.
   NSBundle* app_bundle = [NSBundle mainBundle];

[Mark Mentovai, 2012/02/07 17:28:06]

Didn’t you add your own functions to wrap this?

[jeremy, 2012/02/09 10:52:51]

Yes, but this code is called from the stub launcher and we really do want to use
[- mainBundle] here.

-  CHECK_MSG(app_bundle, "couldn't get loader bundle");
+  CHECK(app_bundle) << "couldn't get loader bundle";
+
+  // ** 1: Get path to outer Chrome bundle.
 
   // Get the bundle ID of the browser that created this app bundle.
   NSString* cr_bundle_id = [app_bundle
       objectForInfoDictionaryKey:(NSString*)app_mode::kBrowserBundleIDKey];
-  CHECK_MSG(cr_bundle_id, "couldn't get browser bundle ID");
+  CHECK(cr_bundle_id) << "couldn't get browser bundle ID";
 
-  // Get the browser bundle path.
-  // TODO(viettrungluu): more fun
-  NSString* cr_bundle_path =
+  // First check if Chrome exists at the last known location.
+  FilePath cr_bundle_path;
+  NSString* cr_bundle_path_ns =
       [(NSString*)CFPreferencesCopyAppValue(

[Mark Mentovai, 2012/02/07 17:28:06]

Don’t cast NS-to-CF like this. Use NSToCFCast<> and CFToNSCast<> from
base/mac/foundation_util.h.

[jeremy, 2012/02/09 10:52:51]

Done.

           app_mode::kLastRunAppBundlePathPrefsKey,
           (CFStringRef)cr_bundle_id) autorelease];
-  CHECK_MSG(cr_bundle_path, "couldn't get browser bundle path");
-
-  // Get the browser bundle.
-  NSBundle* cr_bundle = [NSBundle bundleWithPath:cr_bundle_path];
-  CHECK_MSG(cr_bundle, "couldn't get browser bundle");
-
-  // Get the current browser version.
-  NSString* cr_version =
-      [cr_bundle objectForInfoDictionaryKey:@"CFBundleShortVersionString"];
-  CHECK_MSG(cr_version, "couldn't get browser version");
-
-  // Get the current browser versioned directory.
-  NSArray* cr_versioned_path_components =
-      [NSArray arrayWithObjects:cr_bundle_path,
-                                @"Contents",
-                                @"Versions",
-                                cr_version,
-                                nil];
-  NSString* cr_versioned_path =
-      [[NSString pathWithComponents:cr_versioned_path_components]
-          stringByStandardizingPath];
-  CHECK_MSG(cr_versioned_path, "couldn't get browser versioned path");
-  // And copy it, since |cr_versioned_path| will go away with the pool.
-  info.chrome_versioned_path = NSStringToFSCString(cr_versioned_path);
-
-  // Optional, so okay if it's NULL.
-  info.app_mode_bundle_path = NSStringToFSCString([app_bundle bundlePath]);
+  cr_bundle_path = FilePath([cr_bundle_path_ns fileSystemRepresentation]);
+  bool found_bundle =
+      cr_bundle_path_ns && file_util::DirectoryExists(cr_bundle_path);
+
+  if (!found_bundle) {
+    // If no such bundle path exists, try to search by bundle ID.
+    if (!app_mode::FindBundleById(cr_bundle_id, &cr_bundle_path)) {
+      // TODO(jeremy): Display UI to allow user to manually locate the Chrome
+      // bundle.
+      LOG(FATAL) << "Failed to locate bundle by identifier";
+    }
+  }
+
+  // ** 2: Read information from the Chrome bundle.
+  string16 raw_version_str;
+  FilePath version_path;
+  FilePath framework_shlib_path;
+  if (!app_mode::GetChromeBundleInfo(cr_bundle_path, &raw_version_str,
+          &version_path, &framework_shlib_path)) {
+    LOG(FATAL) << "Couldn't ready Chrome bundle info";
+  }
+
+  // TODO(jeremy): Check that Chrome is new enough.
+  (void)raw_version_str;

[Mark Mentovai, 2012/02/07 17:28:06]

?

[jeremy, 2012/02/09 10:52:51]

Removed for now, the idea was to mark that we need to have a version check to
make sure the app shortcut doesn't try to launch a version of Chrome which is
too old to support .

+
+  // ** 3: Fill in ChromeAppModeInfo.
+  info->chrome_versioned_path = version_path;
+  info->app_mode_bundle_path =
+      FilePath([[app_bundle bundlePath] fileSystemRepresentation]);
 
   // Read information about the this app shortcut from the Info.plist.
   // Don't check for null-ness on optional items.
   NSDictionary* info_plist = [app_bundle infoDictionary];
-  CHECK_MSG(info_plist, "couldn't get loader Info.plist");
+  CHECK(info_plist) << "couldn't get loader Info.plist";
 
-  info.app_mode_id = NSStringToUTF8CString(
+  info->app_mode_id = SysNSStringToUTF8(
       [info_plist objectForKey:@"CrAppModeShortcutID"]);

[Mark Mentovai, 2012/02/07 17:28:06]

Do we not have symbolic constants for any of these?

[jeremy, 2012/02/09 10:52:51]

Sail has a patch ready to land that does that.

-  CHECK_MSG(info.app_mode_id, "couldn't get app shortcut ID");
+  CHECK(info->app_mode_id.size()) << "couldn't get app shortcut ID";
 
-  info.app_mode_short_name = NSStringToUTF8CString(
+  info->app_mode_short_name = SysNSStringToUTF16(
       [info_plist objectForKey:@"CrAppModeShortcutShortName"]);
 
-  info.app_mode_name = NSStringToUTF8CString(
+  info->app_mode_name = SysNSStringToUTF16(
       [info_plist objectForKey:@"CrAppModeShortcutName"]);
 
-  info.app_mode_url = NSStringToUTF8CString(
+  info->app_mode_url = SysNSStringToUTF8(
       [info_plist objectForKey:@"CrAppModeShortcutURL"]);
-  CHECK_MSG(info.app_mode_url, "couldn't get app shortcut URL");
-
-  // Get the framework path.
-  NSString* cr_bundle_exe =
-      [cr_bundle objectForInfoDictionaryKey:@"CFBundleExecutable"];
-  NSString* cr_framework_path =
-      [cr_versioned_path stringByAppendingPathComponent:
-          [cr_bundle_exe stringByAppendingString:@" Framework.framework"]];
-  cr_framework_path =
-      [cr_framework_path stringByAppendingPathComponent:
-          [cr_bundle_exe stringByAppendingString:@" Framework"]];
+  CHECK(info->app_mode_url.size()) << "couldn't get app shortcut URL";
 
   // Open the framework.
-  void* cr_dylib = dlopen([cr_framework_path fileSystemRepresentation],
+  *cr_dylib = dlopen(framework_shlib_path.value().c_str(),
                           RTLD_LAZY);

[Mark Mentovai, 2012/02/07 17:28:06]

This didn’t need to wrap.

[jeremy, 2012/02/09 10:52:51]

Done.

-  CHECK_MSG(cr_dylib, "couldn't load framework");
+  CHECK(cr_dylib) << "couldn't load framework";

[Mark Mentovai, 2012/02/07 17:28:06]

Print some more useful information from dlerror()?

[jeremy, 2012/02/09 10:52:51]

Done.

+}
+
+} // namespace
+
+__attribute__((visibility("default")))
+int main(int argc, char** argv) {
+  app_mode::ChromeAppModeInfo info;
+
+  // Hard coded info parameters.
+  info.major_version = 1;  // v1.0
+  info.minor_version = 0;
+  info.argc = argc;
+  info.argv = argv;
 
-  // Drain the pool as late as possible.
-  [pool drain];
+  // Load the Chrome framework.
+  void *cr_dylib;
+  LoadFramework(&cr_dylib, &info);
 
   typedef int (*StartFun)(const app_mode::ChromeAppModeInfo*);
   StartFun ChromeAppModeStart = (StartFun)dlsym(cr_dylib, "ChromeAppModeStart");
-  CHECK_MSG(ChromeAppModeStart, "couldn't get entry point");
+  CHECK(ChromeAppModeStart) << "couldn't get entry point";
 
   // Exit instead of returning to avoid the the removal of |main()| from stack
   // backtraces under tail call optimization.