MacPWAs: Separate app focus from app reopen

This is towards allowing apps to run with no windows open.

Two different messages differentiated here:
- applicationWillBecomeActive (aka Focus)
  - happens when any window in the app is focused, or when the app
    is command-tabbed to be active
  - should focus existing windows
- applicationShouldHandleReopen (aka Reopen)
  - happens when the dock icon is clicked
  - should launch a new window if no window is open

It used to be that Focus was used by legacy apps and ignored by
PWAs. Also, Focus was used to re-open the app (inappropriately), and
also was used to close the app if it somehow was uninstalled while
still running.

Update the legacy app behavior to open new windows only on Reopen,
and remove the "close the app" behavior.

The next step here will be to handle Reopen for PWAs, and to allow
them to keep running even if no windows are open.

Bug: 1080729
Change-Id: I5bf86e7e7874b71c030d8dc9c5440ee7fec430d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2197231
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#769602}

chrome/app_shim/app_shim_delegate.mm

@@ -38,6 +38,12 @@
   return _appShimController->host()->FocusApp();
 }
 
+- (BOOL)applicationShouldHandleReopen:(NSApplication*)sender
+                    hasVisibleWindows:(BOOL)flag {
+  _appShimController->host()->ReopenApp();
+  return YES;
+}
+
 - (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
   return NO;
 }

chrome/browser/apps/app_shim/app_shim_host_mac.cc

@@ -149,6 +149,10 @@ void AppShimHost::FocusApp() {
   client_->OnShimFocus(this);
 }
 
+void AppShimHost::ReopenApp() {
+  client_->OnShimReopen(this);
+}
+
 void AppShimHost::FilesOpened(const std::vector<base::FilePath>& files) {
   client_->OnShimOpenedFiles(this, files);
 }

chrome/browser/apps/app_shim/app_shim_host_mac.h

@@ -53,6 +53,9 @@ class AppShimHost : public chrome::mojom::AppShimHost {
     // Invoked by the shim host when the shim process receives a focus event.
     virtual void OnShimFocus(AppShimHost* host) = 0;
 
+    // Invoked by the shim host when the shim process should reopen if needed.
+    virtual void OnShimReopen(AppShimHost* host) = 0;
+
     // Invoked by the shim host when the shim opens a file, e.g, by dragging
     // a file onto the dock icon.
     virtual void OnShimOpenedFiles(
@@ -112,6 +115,7 @@ class AppShimHost : public chrome::mojom::AppShimHost {
 
   // chrome::mojom::AppShimHost.
   void FocusApp() override;
+  void ReopenApp() override;
   void FilesOpened(const std::vector<base::FilePath>& files) override;
   void ProfileSelectedFromMenu(const base::FilePath& profile_path) override;
 

chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc

@@ -178,6 +178,7 @@ class AppShimHostTest : public testing::Test,
     ++close_count_;
   }
   void OnShimFocus(AppShimHost* host) override { ++focus_count_; }
+  void OnShimReopen(AppShimHost* host) override {}
   void OnShimOpenedFiles(AppShimHost* host,
                          const std::vector<base::FilePath>& files) override {}
   void OnShimSelectedProfile(AppShimHost* host,

chrome/browser/apps/app_shim/app_shim_listener_browsertest_mac.mm

@@ -143,6 +143,7 @@ class AppShimListenerBrowserTest : public InProcessBrowserTest,
  private:
   // chrome::mojom::AppShimHost.
   void FocusApp() override {}
+  void ReopenApp() override {}
   void FilesOpened(const std::vector<base::FilePath>& files) override {}
   void ProfileSelectedFromMenu(const base::FilePath& profile_path) override {}
 

chrome/browser/apps/app_shim/app_shim_manager_mac.cc

@@ -508,10 +508,6 @@ void AppShimManager::OnShimProcessConnectedAndAllLaunchesDone(
   // If we already have a host attached (e.g, due to multiple launches racing),
   // close down the app shim that didn't win the race.
   if (host->HasBootstrapConnected()) {
-    // If another app shim process has already connected to this (profile,
-    // app_id) pair, then focus the windows for the existing process. Note
-    // that this only does anything for non-RemoveCocoa apps.
-    OnShimFocus(profile_state->GetHost());
     bootstrap->OnFailedToConnectToHost(
         chrome::mojom::AppShimLaunchResult::kDuplicateHost);
     return;
@@ -721,12 +717,21 @@ void AppShimManager::OnShimFocus(AppShimHost* host) {
   if (host->UsesRemoteViews())
     return;
 
+  // Legacy apps don't own their own windows, so when we focus the app,
+  // what we really want to do is focus the Chrome windows.
   Profile* profile = ProfileForPath(host->GetProfilePath());
-  if (!delegate_->AppIsInstalled(profile, host->GetAppId())) {
-    CloseShimForApp(profile, host->GetAppId());
+  delegate_->ShowAppWindows(profile, host->GetAppId());
+}
+
+void AppShimManager::OnShimReopen(AppShimHost* host) {
+  // TODO(https://crbug.com/1080729): If the app has no windows open, this
+  // should trigger an app launch for PWAs as well.
+  if (host->UsesRemoteViews())
     return;
-  }
 
+  // Legacy apps don't own their own windows, so when we focus the app,
+  // what we really want to do is focus the Chrome windows.
+  Profile* profile = ProfileForPath(host->GetProfilePath());
   if (delegate_->ShowAppWindows(profile, host->GetAppId()))
     return;
 

chrome/browser/apps/app_shim/app_shim_manager_mac.h

@@ -139,6 +139,7 @@ class AppShimManager : public AppShimHostBootstrap::Client,
       base::OnceClosure terminated_callback) override;
   void OnShimProcessDisconnected(AppShimHost* host) override;
   void OnShimFocus(AppShimHost* host) override;
+  void OnShimReopen(AppShimHost* host) override;
   void OnShimOpenedFiles(AppShimHost* host,
                          const std::vector<base::FilePath>& files) override;
   void OnShimSelectedProfile(AppShimHost* host,

chrome/browser/apps/app_shim/app_shim_manager_mac_unittest.cc

@@ -268,6 +268,7 @@ class TestHost : public AppShimHost {
 
   using AppShimHost::FilesOpened;
   using AppShimHost::ProfileSelectedFromMenu;
+  using AppShimHost::ReopenApp;
 
   std::unique_ptr<TestAppShim> test_app_shim_;
 
@@ -594,10 +595,7 @@ TEST_F(AppShimManagerTest, LaunchAndCloseShim) {
   EXPECT_EQ(chrome::mojom::AppShimLaunchResult::kSuccess,
             *bootstrap_aa_result_);
 
-  // Starting and closing a second host just focuses the original host of the
-  // app.
-  EXPECT_CALL(*manager_, OnShimFocus(host_aa_.get()));
-
+  // Starting and closing a second host does nothing.
   DoShimLaunch(bootstrap_aa_duplicate_, std::move(host_aa_duplicate_unique_),
                chrome::mojom::AppShimLaunchType::kNormal, some_file);
   EXPECT_EQ(chrome::mojom::AppShimLaunchResult::kDuplicateHost,
@@ -626,17 +624,23 @@ TEST_F(AppShimManagerTest, AppLifetime) {
             *bootstrap_aa_result_);
   EXPECT_EQ(host_aa_.get(), manager_->FindHost(&profile_a_, kTestAppIdA));
 
-  // Return no app windows for OnShimFocus. This will result in a launch call.
+  // Return no app windows for OnShimFocus. This will do nothing.
   EXPECT_CALL(*delegate_, ShowAppWindows(&profile_a_, kTestAppIdA))
       .WillRepeatedly(Return(false));
-  EXPECT_CALL(*delegate_, LaunchApp(&profile_a_, kTestAppIdA, _)).Times(1);
+  EXPECT_CALL(*delegate_, LaunchApp(&profile_a_, kTestAppIdA, _)).Times(0);
   ShimNormalFocus(host_aa_.get());
 
+  // Return no app windows for OnShimReopen. This will result in a launch call.
+  EXPECT_CALL(*delegate_, ShowAppWindows(&profile_a_, kTestAppIdA))
+      .WillRepeatedly(Return(false));
+  EXPECT_CALL(*delegate_, LaunchApp(&profile_a_, kTestAppIdA, _)).Times(1);
+  host_aa_->ReopenApp();
+
   // Return one window. This should do nothing.
   EXPECT_CALL(*delegate_, ShowAppWindows(&profile_a_, kTestAppIdA))
       .WillRepeatedly(Return(true));
   EXPECT_CALL(*delegate_, LaunchApp(&profile_a_, kTestAppIdA, _)).Times(0);
-  ShimNormalFocus(host_aa_.get());
+  host_aa_->ReopenApp();
 
   // Open files should trigger a launch with those files.
   std::vector<base::FilePath> some_file(1, base::FilePath("some_file"));
@@ -821,10 +825,10 @@ TEST_F(AppShimManagerTest, ExtensionUninstalled) {
   EXPECT_CALL(*delegate_, AppIsInstalled(&profile_a_, kTestAppIdA))
       .WillRepeatedly(Return(false));
 
-  // Now trying to focus should automatically close the shim, and not try to
-  // get the window list.
+  // Trying to focus will do nothing -- the shim will have to be closed by
+  // the user manually.
   ShimNormalFocus(host_aa_.get());
-  EXPECT_EQ(nullptr, host_aa_.get());
+  EXPECT_NE(nullptr, host_aa_.get());
 }
 
 TEST_F(AppShimManagerTest, PreExistingHost) {
@@ -839,7 +843,6 @@ TEST_F(AppShimManagerTest, PreExistingHost) {
 
   // Launch the app for this host. It should find the pre-existing host, and the
   // pre-existing host's launch result should be set.
-  EXPECT_CALL(*manager_, OnShimFocus(host_aa_.get())).Times(1);
   EXPECT_CALL(*delegate_, LaunchApp(&profile_a_, kTestAppIdA, _)).Times(0);
   EXPECT_FALSE(host_aa_->did_connect_to_host());
   DoShimLaunch(bootstrap_aa_, nullptr,

chrome/browser/apps/platform_apps/app_shim_interactive_uitest_mac.mm

@@ -131,6 +131,7 @@ class WindowedAppShimLaunchObserver : public apps::AppShimManager {
       run_loop_->Quit();
   }
   void OnShimFocus(AppShimHost* host) override {}
+  void OnShimReopen(AppShimHost* host) override {}
   void OnShimOpenedFiles(AppShimHost* host,
                          const std::vector<base::FilePath>& files) override {}
   void OnShimSelectedProfile(AppShimHost* host,

chrome/common/mac/app_shim.mojom

@@ -89,6 +89,10 @@ interface AppShimHost {
   // clicking on the app's icon in the dock or by selecting it with Cmd+Tab.
   FocusApp();
 
+  // Sent when the application should launch if needed (e.g, when the dock
+  // icon is clicked).
+  ReopenApp();
+
   // Sent when files are opened by the app (e.g, by opening in Finder, or by
   // dragging on to the app in the dock).
   FilesOpened(array<mojo_base.mojom.FilePath> files);