[Mac] Add interactive App Shim test.

apps/app_shim/app_shim_interactive_uitest_mac.mm

Index: apps/app_shim/app_shim_interactive_uitest_mac.mm
diff --git a/apps/app_shim/app_shim_interactive_uitest_mac.mm b/apps/app_shim/app_shim_interactive_uitest_mac.mm
new file mode 100644
index 0000000000000000000000000000000000000000..ca13b3f224937fa51186e71a2f91eb31b74b1f73
--- /dev/null
+++ b/apps/app_shim/app_shim_interactive_uitest_mac.mm
@@ -0,0 +1,227 @@
+// Copyright 2014 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 <Cocoa/Cocoa.h>
+#include <vector>
+
+#include "apps/app_shim/app_shim_host_manager_mac.h"
+#include "apps/app_shim/extension_app_shim_handler_mac.h"
+#include "apps/switches.h"
+#include "apps/ui/native_app_window.h"
+#include "base/auto_reset.h"
+#include "base/callback.h"
+#include "base/files/file_path_watcher.h"
+#include "base/mac/foundation_util.h"
+#include "base/mac/launch_services_util.h"
+#include "base/mac/scoped_nsobject.h"
+#include "base/path_service.h"
+#include "base/process/launch.h"
+#include "base/strings/sys_string_conversions.h"
+#include "base/test/test_timeouts.h"
+#include "chrome/browser/apps/app_browsertest_util.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/extension_test_message_listener.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/web_applications/web_app_mac.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/mac/app_mode_common.h"
+#include "content/public/test/test_utils.h"
+#include "extensions/browser/extension_registry.h"
+#import "ui/events/test/cocoa_test_event_utils.h"
+
+namespace {
+
+// General end-to-end test for app shims.
+class AppShimInteractiveTest : public extensions::PlatformAppBrowserTest {
+ protected:
+  AppShimInteractiveTest()
+      : auto_reset_(&g_app_shims_allow_update_and_launch_in_tests, true) {}
+
+  // Temporarily enable app shims.
+  base::AutoReset<bool> auto_reset_;
+
+  DISALLOW_COPY_AND_ASSIGN(AppShimInteractiveTest);
+};
+
+// Watches for changes to a file. This is designed to be used from the the UI
+// thread.
+class WindowedFilePathWatcher
+    : public base::RefCountedThreadSafe<WindowedFilePathWatcher> {
+ public:
+  WindowedFilePathWatcher(const base::FilePath& path)
+      : observed_(false) {
+    content::BrowserThread::PostTask(
+      content::BrowserThread::FILE,

[tapted, 2014/06/04 09:36:03]

nit: indent 2 more spaces

[jackhou1, 2014/06/05 02:57:11]

Done.

+      FROM_HERE,
+      base::Bind(&WindowedFilePathWatcher::Watch, this, path));
+  }
+
+  void Wait() {
+    if (observed_)
+      return;
+
+    run_loop_.reset(new base::RunLoop);
+    run_loop_->Run();
+  }
+
+ protected:
+  friend class base::RefCountedThreadSafe<WindowedFilePathWatcher>;
+  virtual ~WindowedFilePathWatcher() {}
+
+  void Watch(const base::FilePath& path) {
+    watcher_.Watch(
+        path, false, base::Bind(&WindowedFilePathWatcher::Observe, this));
+  }
+
+  void Observe(const base::FilePath& path, bool error) {
+    observed_ = true;
+    content::BrowserThread::PostTask(
+      content::BrowserThread::UI,

[tapted, 2014/06/04 09:36:03]

nit: indent

[jackhou1, 2014/06/05 02:57:11]

Done.

+      FROM_HERE,
+      base::Bind(&WindowedFilePathWatcher::StopRunLoop, this));
+  }
+
+  void StopRunLoop() {
+    if (run_loop_.get())
+      run_loop_->Quit();
+  }
+
+  base::FilePathWatcher watcher_;
+  bool observed_;
+  scoped_ptr<base::RunLoop> run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(WindowedFilePathWatcher);
+};
+
+NSString* GetBundleID(const base::FilePath& shim_path) {
+  base::FilePath plist_path = shim_path.Append("Contents").Append("Info.plist");
+  NSMutableDictionary* plist = [NSMutableDictionary
+      dictionaryWithContentsOfFile:base::mac::FilePathToNSString(plist_path)];
+  return [plist objectForKey:base::mac::CFToNSCast(kCFBundleIdentifierKey)];
+}
+
+bool HasAppShimHost(Profile* profile, const std::string& app_id) {
+  return g_browser_process->platform_part()->app_shim_host_manager()->
+      extension_app_shim_handler()->FindHost(profile, app_id);
+}
+
+}  // namespace
+
+// Watches for NSNotifications from the shared workspace.
+@interface WindowedNSNotificationObserver : NSObject {
+ @private
+  BOOL notificationReceived_;
+  scoped_ptr<base::RunLoop> runLoop_;
+}
+
+-(id) initForNotification:(NSString*)name;

[tapted, 2014/06/04 09:36:03]

nit: spacing on all these should be like

- (id)initFor...

[jackhou1, 2014/06/05 02:57:11]

Done.

+-(void) observe:(NSNotification*)notification;
+-(void) wait;
+@end
+
+@implementation WindowedNSNotificationObserver
+
+-(id) initForNotification:(NSString*)name {
+  if (self = [super init]) {
+    [[[NSWorkspace sharedWorkspace] notificationCenter]
+      addObserver:self

[tapted, 2014/06/04 09:36:03]

nit: min 4 spaces indent

[jackhou1, 2014/06/05 02:57:11]

Done.

+         selector:@selector(observe:)
+             name:name
+           object:nil];
+  }
+  return self;
+}
+
+-(void) observe:(NSNotification*)notification {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  [[[NSWorkspace sharedWorkspace] notificationCenter] removeObserver:self];
+  notificationReceived_ = YES;

[tapted, 2014/06/04 09:36:03]

I think this will quit once any application is started - how hard is it to
extract the application from the notification object and return early from here
if it's not the shim?

[jackhou1, 2014/06/05 02:57:11]

Done.

+  if (runLoop_.get())
+    runLoop_->Quit();
+}
+
+-(void) wait {
+  if (notificationReceived_)
+    return;
+
+  runLoop_.reset(new base::RunLoop);
+  runLoop_->Run();
+}
+
+@end
+
+namespace apps {
+
+// Test that launching the shim for an app starts the app, and vice versa.
+// These two cases are combined because the time to run the test is dominated
+// by loading the extension and creating the shim.
+IN_PROC_BROWSER_TEST_F(AppShimInteractiveTest, Launch) {
+  // Create the internal app shim.
+  const extensions::Extension* app = InstallPlatformApp("minimal");
+
+  // Use a WebAppShortcutCreator to get the path.
+  web_app::WebAppShortcutCreator shortcut_creator(
+      web_app::GetWebAppDataDirectory(profile()->GetPath(), app->id(), GURL()),
+      web_app::ShortcutInfoForExtensionAndProfile(app, profile()),
+      extensions::FileHandlersInfo());
+  base::FilePath shim_path = shortcut_creator.GetInternalShortcutPath();
+  EXPECT_FALSE(base::PathExists(shim_path));
+
+  // Simulate updating the app. FilePathWatcher is used to wait for file
+  // operations on the shim to be finished before attempting to launch it.
+  scoped_refptr<WindowedFilePathWatcher> file_watcher =
+      new WindowedFilePathWatcher(shim_path);

[tapted, 2014/06/04 09:36:03]

I think this is watching the .app root directory? I'm not sure it will guarantee
all the contents of the directory have been copied in at that point, which might
make the launch flaky. Typically only renames (of files or directories) are
atomic on POSIX systems (and then only if the destination path already exists).

Perhaps you could watch for the temp directory being deleted? (except the
ScopedTempDir path isn't currently exposed). That is, in Observe, check each
time whether PathExists(tempdir) and only stop the run loop when it's gone.

[jackhou1, 2014/06/05 02:57:11]

It's guaranteed but in a non-obvious way. Added a comment.

[tapted, 2014/06/05 04:08:01]

Ah - cool makes sense. FilePathWatcher's documentation about what happens on
what thread isn't super clear :/

+  web_app::UpdateAllShortcuts(base::string16(), profile(), app);
+  file_watcher->Wait();
+  NSString* bundle_id = GetBundleID(shim_path);
+
+  // Case 1: Launch the shim, it should start the app.
+  {
+    ExtensionTestMessageListener launched_listener("Launched", false);
+    CommandLine shim_cmdline(CommandLine::NO_PROGRAM);
+    shim_cmdline.AppendSwitch(app_mode::kLaunchedForTest);
+    ProcessSerialNumber shim_psn;
+    ASSERT_TRUE(base::mac::OpenApplicationWithPath(
+        shim_path, shim_cmdline, kLSLaunchDefaults, &shim_psn));
+    ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
+
+    EXPECT_TRUE(HasAppShimHost(profile(), app->id()));
+
+    // If the window is closed, the shim should quit.
+    pid_t shim_pid;
+    EXPECT_EQ(noErr, GetProcessPID(&shim_psn, &shim_pid));
+    GetFirstAppWindow()->GetBaseWindow()->Close();
+    ASSERT_TRUE(base::WaitForSingleProcess(
+         shim_pid, TestTimeouts::action_timeout()));
+
+    EXPECT_FALSE(HasAppShimHost(profile(), app->id()));
+  }
+
+  // Case 2: Launch the app, it should start the shim.
+  {
+    base::scoped_nsobject<WindowedNSNotificationObserver> observer;
+    observer.reset([[WindowedNSNotificationObserver alloc]
+        initForNotification:NSWorkspaceDidLaunchApplicationNotification]);
+    LaunchPlatformApp(app);
+    [observer wait];
+
+    // Quitting the shim eventually closes the app.
+    NSArray* running_shim = [NSRunningApplication

[tapted, 2014/06/04 09:36:03]

git cl format might format this line differently too

[jackhou1, 2014/06/05 02:57:11]

Ran git cl format. It's ok with this, but it puts -> at the start of lines
instead of the end.

+        runningApplicationsWithBundleIdentifier:bundle_id];
+    ASSERT_EQ(1u, [running_shim count]);
+
+    observer.reset([[WindowedNSNotificationObserver alloc]
+        initForNotification:NSWorkspaceDidTerminateApplicationNotification]);
+    [base::mac::ObjCCastStrict<NSRunningApplication>(
+        [running_shim objectAtIndex:0]) terminate];
+    [observer wait];

[tapted, 2014/06/04 09:36:03]

I *think* this is not racy, but it might need a comment. Something along the
lines of ~"When the shim process receives a terminate request, it first sends
NSTerminateLater, then waits for the IPC socket to be closed. This only only
happens after ExtensionAppShimHandler::OnShimQuit has closed all windows for the
app."

[jackhou1, 2014/06/05 02:57:11]

I think this is only fired when the shim actually terminates. The documentation
says:
"Posted when an app finishes executing."
https://developer.apple.com/library/mac/documentation/cocoa/reference/applica...

[tapted, 2014/06/05 04:08:01]

Yep - I just meant it's subtle that [NSRunnignApplicaiton terminate] on the line
above does not immediately terminate like it would "by default". So a comment
about the shim sending NSTerminateLater would make it clear that this will not
be racy.

[jackhou1, 2014/06/05 05:11:01]

Done.

+
+    EXPECT_FALSE(GetFirstAppWindow());
+    EXPECT_FALSE(HasAppShimHost(profile(), app->id()));

[tapted, 2014/06/04 09:36:03]

add a matching EXPECT_TRUE(HasAppShimHost../window) for this block?

[jackhou1, 2014/06/05 02:57:11]

Looks like I need to wait for the shim to connect with ExtensionAppShimHandler.
More WindowedSomethingObservers!

+  }
+}
+
+}  // namespace apps