[mac] App shims

chrome/app/chrome_main_app_mode_mac.mm

Index: chrome/app/chrome_main_app_mode_mac.mm
diff --git a/chrome/app/chrome_main_app_mode_mac.mm b/chrome/app/chrome_main_app_mode_mac.mm
index d102fc8f591e667bc0b2e00a5d67391d30f41729..f6d86fbcb486cd37b11ff3d7cd6d01100048db2c 100644
--- a/chrome/app/chrome_main_app_mode_mac.mm
+++ b/chrome/app/chrome_main_app_mode_mac.mm
@@ -7,20 +7,147 @@
 // passing the appropriate data. This is the entry point into the framework for
 // those app bundles.
 
-#include "base/basictypes.h"
-#include "base/files/file_path.h"
+#import <Cocoa/Cocoa.h>
+
+#include "base/at_exit.h"
 #include "base/logging.h"
-#include "base/mac/bundle_locations.h"
-#include "base/mac/foundation_util.h"
+#include "base/threading/thread.h"
 #include "base/mac/mac_logging.h"
 #include "base/mac/mac_util.h"
 #include "base/mac/scoped_nsautorelease_pool.h"
+#include "base/message_loop.h"
+#include "base/path_service.h"
 #include "base/sys_string_conversions.h"
-#include "chrome/browser/shell_integration.h"
-#include "chrome/common/chrome_constants.h"
-#include "chrome/common/chrome_paths_internal.h"
+#include "chrome/common/app_shim_messages.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/mac/app_mode_common.h"
+#include "ipc/ipc_channel_proxy.h"
+#include "ipc/ipc_listener.h"
+#include "ipc/ipc_message.h"
+
+const app_mode::ChromeAppModeInfo* g_info;
+base::Thread* g_io_thread = NULL;
+
+class AppShimController : public IPC::Listener {
+ public:
+  AppShimController();
+
+ private:
+  virtual bool OnMessageReceived(const IPC::Message& message);
+  virtual void OnChannelError();
+
+  void OnLaunchAppDone(bool success);
+  void OnDidActivateApplicationNotification(NSNotification* notification);
+
+  IPC::ChannelProxy* channel_;
+};

[Mark Mentovai, 2013/03/08 16:50:17]

DISALLOW_COPY_AND_ASSIGN?

[jeremya, 2013/03/11 02:47:33]

Done.

+
+AppShimController::AppShimController() {

[Mark Mentovai, 2013/03/08 16:50:17]

There’s enough stuff in this constructor that could fail that you’re probably
better off either

1. Having a minimal constructor and putting all of the real work into a
properly-named initialization method.

or

2. Since you seem to just want to die on error (as in OnChannelError), handle
the error cases right here in the constructor by dying.

[jeremya, 2013/03/11 02:47:33]

All the stuff that could fail here fails on a separate thread, so I can't die
straight away. I've split it out into an Init() method though.

+  base::FilePath user_data_dir;
+  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
+  base::FilePath socket_path =
+      user_data_dir.Append(app_mode::kAppShimSocketName);
+  IPC::ChannelHandle handle(socket_path.value());
+  channel_ = new IPC::ChannelProxy(handle, IPC::Channel::MODE_NAMED_CLIENT,
+      this, g_io_thread->message_loop_proxy());
+
+  channel_->Send(new AppShimHostMsg_LaunchApp(
+      g_info->profile_dir.value(), g_info->app_mode_id));
+}
+
+bool AppShimController::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(AppShimController, message)
+    IPC_MESSAGE_HANDLER(AppShimMsg_LaunchApp_Done, OnLaunchAppDone)
+
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+
+  return handled;
+}
+
+void AppShimController::OnChannelError() {
+  [NSApp terminate:nil];
+}
+
+void AppShimController::OnLaunchAppDone(bool success) {
+  if (!success)
+    [NSApp terminate:nil];
+}
+
+//-----------------------------------------------------------------------------
+
+@interface ReplyEventHandler : NSObject {

[Mark Mentovai, 2013/03/08 16:50:17]

Just because you’re defining this class in an .mm file instead of an .h file
doesn’t relieve you of the duty to provide proper documentation for methods and
member variables when they wouldn’t be obvious.

+  base::Closure onReply_;
+  AEDesc replyEvent_;
+}
++ (void)pingProcess:(const ProcessSerialNumber&)psn

[Mark Mentovai, 2013/03/08 16:50:17]

For example, the memory management of ReplyEventHandler objects is a little
unconventional. You should say something about it.

[jeremya, 2013/03/11 02:47:33]

Done.

+            andCall:(base::Closure)replyFn;
+@end
+
+@interface ReplyEventHandler (PrivateMethods)
+- (id)initWithCallback:(base::Closure)replyFn;
+- (void)pingProcess:(const ProcessSerialNumber&)psn;
+- (void)message:(NSAppleEventDescriptor*)event
+      withReply:(NSAppleEventDescriptor*)reply;
+@end
+
+@implementation ReplyEventHandler
++ (void)pingProcess:(const ProcessSerialNumber&)psn
+            andCall:(base::Closure)replyFn {
+  ReplyEventHandler* handler = [[ReplyEventHandler alloc]

[Mark Mentovai, 2013/03/08 16:50:17]

This would be easier to read if you broke the line after the =. It still takes
two lines that way, but it’s a more natural place to break it.

[jeremya, 2013/03/11 02:47:33]

Done.

+      initWithCallback:replyFn];
+  [handler pingProcess:psn];
+}
+@end
+
+@implementation ReplyEventHandler (PrivateMethods)
+- (id)initWithCallback:(base::Closure)replyFn {
+  if (self = [super init]) {

[Mark Mentovai, 2013/03/08 16:50:17]

We always write
  if ((self = [super init]))
to avoid the compiler warning about it looking like you wanted == instead of =.
I’m surprised that this warning wasn’t triggered here.

[jeremya, 2013/03/11 02:47:33]

Done, I'm surprised too.

+    onReply_ = replyFn;
+  }
+  return self;
+}
+- (void)pingProcess:(const ProcessSerialNumber&)psn {

[Mark Mentovai, 2013/03/08 16:50:17]

Blank line between methods implementations, please.

[jeremya, 2013/03/11 02:47:33]

Done.

+  // Register the reply listener.
+  NSAppleEventManager* em = [NSAppleEventManager sharedAppleEventManager];
+  [em setEventHandler:self
+          andSelector:@selector(message:withReply:)
+        forEventClass:'aevt'
+           andEventID:'ansr'];
+  // Craft the Apple Event to send.
+  NSAppleEventDescriptor* target = [NSAppleEventDescriptor
+      descriptorWithDescriptorType:typeProcessSerialNumber
+                             bytes:&psn
+                            length:sizeof(ProcessSerialNumber)];

[Mark Mentovai, 2013/03/08 16:50:17]

Use sizeof(variable) and not sizeof(type) whenever possible.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=sizeof...

[jeremya, 2013/03/11 02:47:33]

Done.

+  NSAppleEventDescriptor* initial_event =
+      [NSAppleEventDescriptor
+          appleEventWithEventClass:app_mode::kAEChromeAppClass
+                           eventID:app_mode::kAEChromeAppPing
+                  targetDescriptor:target
+                          returnID:kAutoGenerateReturnID
+                     transactionID:kAnyTransactionID];
+  // And away we go.
+  // TODO(jeremya): if we don't care about the contents of the reply, can we
+  // pass NULL for the reply event parameter?
+  AESendMessage(

[Mark Mentovai, 2013/03/08 16:50:17]

Considering how much logging you did in the previous changelist, I’m surprised
to see things like this go unchecked. If you decide you want to log something
that returns an OSStatus or OSErr, you can use OSSTATUS_LOG from
"base/mac/mac_logging.h". PLOG : errno :: OSSTATUS_LOG : OSStatus.

[jeremya, 2013/03/11 02:47:33]

Done, plus the thing dies if AESendMessage fails.

+      [initial_event aeDesc], &replyEvent_, kAEQueueReply, kAEDefaultTimeout);
+}
+- (void)message:(NSAppleEventDescriptor*)event
+      withReply:(NSAppleEventDescriptor*)reply {
+  onReply_.Run();
+  NSAppleEventManager* em = [NSAppleEventManager sharedAppleEventManager];
+  [em removeEventHandlerForEventClass:'aevt' andEventID:'ansr'];
+  [self release];
+}
+@end
+
+//-----------------------------------------------------------------------------
+
+void Begin() {

[Mark Mentovai, 2013/03/08 16:50:17]

This is unnamespaced and non-static. Putting something called “Begin” into the
global namespace is a bad idea.

This one should be in an anonymous namespace or should be marked static. (And it
should have a better name even if you do that.)

[jeremya, 2013/03/11 02:47:33]

Done.

+  new AppShimController;
+}
 
 extern "C" {
 
@@ -33,6 +160,8 @@ int ChromeAppModeStart(const app_mode::ChromeAppModeInfo* info);
 
 int ChromeAppModeStart(const app_mode::ChromeAppModeInfo* info) {
   base::mac::ScopedNSAutoreleasePool scoped_pool;
+  base::AtExitManager exit_manager;
+  chrome::RegisterPathProvider();
 
   if (info->major_version < app_mode::kCurrentChromeAppModeInfoMajorVersion) {
     RAW_LOG(ERROR, "App Mode Loader too old.");
@@ -43,11 +172,21 @@ int ChromeAppModeStart(const app_mode::ChromeAppModeInfo* info) {
     return 1;
   }
 
+  g_info = info;
+
+  // Launch the IO thread.
+  base::Thread::Options io_thread_options;
+  io_thread_options.message_loop_type = MessageLoop::TYPE_IO;
+  base::Thread *io_thread = new base::Thread("CrAppShimIO");
+  io_thread->StartWithOptions(io_thread_options);
+  g_io_thread = io_thread;
+
+  // Launch Chrome if it isn't already running.
   FSRef app_fsref;
   if (!base::mac::FSRefFromPath(info->chrome_outer_bundle_path.value(),
         &app_fsref)) {
     PLOG(ERROR) << "base::mac::FSRefFromPath failed for "

[Mark Mentovai, 2013/03/08 16:50:17]

PLOG is wrong here, because base::mac::FSRefFromPath doesn’t have anything to do
with errno.

[jeremya, 2013/03/11 02:47:33]

-> LOG

-        << info->chrome_outer_bundle_path.value();
+                << info->chrome_outer_bundle_path.value();
     return 1;
   }
   std::string silent = std::string("--") + switches::kSilentLaunch;
@@ -63,27 +202,23 @@ int ChromeAppModeStart(const app_mode::ChromeAppModeInfo* info) {
     launch_args,
     NULL   // initialEvent
   };
-  NSAppleEventDescriptor* initial_event =
-      [NSAppleEventDescriptor
-          appleEventWithEventClass:app_mode::kAEChromeAppClass
-                           eventID:app_mode::kAEChromeAppLaunch
-                  targetDescriptor:nil
-                          returnID:kAutoGenerateReturnID
-                     transactionID:kAnyTransactionID];
-  NSAppleEventDescriptor* appid_descriptor = [NSAppleEventDescriptor
-      descriptorWithString:base::SysUTF8ToNSString(info->app_mode_id)];
-  [initial_event setParamDescriptor:appid_descriptor
-                         forKeyword:keyDirectObject];
-  NSAppleEventDescriptor* profile_dir_descriptor = [NSAppleEventDescriptor
-      descriptorWithString:base::SysUTF8ToNSString(info->profile_dir.value())];
-  [initial_event setParamDescriptor:profile_dir_descriptor
-                         forKeyword:app_mode::kAEProfileDirKey];
-  ls_parameters.initialEvent = const_cast<AEDesc*>([initial_event aeDesc]);
-  // Send the Apple Event using launch services, launching Chrome if necessary.
-  OSStatus status = LSOpenApplication(&ls_parameters, NULL);
+  ProcessSerialNumber psn;
+  // TODO(jeremya): this opens a new browser window if Chrome is already
+  // running without any windows open.
+  OSStatus status = LSOpenApplication(&ls_parameters, &psn);
   if (status != noErr) {
     OSSTATUS_LOG(ERROR, status) << "LSOpenApplication";
     return 1;
   }
+
+  // This code abuses the fact that Apple Events sent before the process is
+  // fully initialized don't receive a reply until its run loop starts. Once
+  // the reply is received, Chrome will have opened its IPC port, guaranteed.
+  [ReplyEventHandler pingProcess:psn andCall:base::Bind(&Begin)];
+
+  MessageLoopForUI main_message_loop;
+  main_message_loop.set_thread_name("MainThread");
+  base::PlatformThread::SetName("CrAppShimMain");

[Mark Mentovai, 2013/03/08 16:50:17]

I like that you’ve stuck with the appropriate naming convention.

[jeremya, 2013/03/11 02:47:33]

I did? Awesome!

+  main_message_loop.Run();
   return 0;
 }