MacPWAs: Add flag and enable new close behavior

Add a feature flag, AppShimNewCloseBehavior, to control the new close
behavior. With this flag, app shims will not close when all windows
close. Rather, the app will remain open, and clicking on it will cause
a new window to be created. Closing the app via cmd-Q, or Quit through
the menu bar will close the app.

Add tests to ensure new and old modes stay working.

Bug: 1080729
Change-Id: I057e038721d5c887f988f19e7f63ec33bcc429eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2427029Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809978}

chrome/browser/apps/app_shim/app_shim_manager_mac.cc

@@ -42,6 +42,7 @@
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/components/web_app_shortcut_mac.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
+#include "chrome/common/chrome_features.h"
 #include "components/crx_file/id_util.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/notification_details.h"
@@ -174,6 +175,10 @@ struct AppShimManager::AppState {
 
   bool IsMultiProfile() const;
 
+  // Return true if the app state should be deleted (e.g, because all profiles
+  // have closed).
+  bool ShouldDeleteAppState() const;
+
   // Mark the last-active profiles in AppShimRegistry, so that they will re-open
   // when the app is started next.
   void SaveLastActiveProfiles() const;
@@ -210,6 +215,22 @@ bool AppShimManager::AppState::IsMultiProfile() const {
   return multi_profile_host.get();
 }
 
+bool AppShimManager::AppState::ShouldDeleteAppState() const {
+  // The new behavior for multi-profile apps is to not close the app based on
+  // which windows are open. Rather, the app must be explicitly closed via
+  // the Quit menu, which will terminate the app (and the browser will be
+  // notified of the closed mojo pipe).
+  // https://crbug.com/1080729
+  if (IsMultiProfile() &&
+      base::FeatureList::IsEnabled(features::kAppShimNewCloseBehavior)) {
+    return false;
+  }
+
+  // The old behavior, and the behavior for single-profile apps, is to close
+  // only when all profiles are closed.
+  return profiles.empty();
+}
+
 void AppShimManager::AppState::SaveLastActiveProfiles() const {
   if (!IsMultiProfile())
     return;
@@ -609,30 +630,14 @@ void AppShimManager::CloseShimsForProfile(Profile* profile) {
   for (auto iter_app = apps_.begin(); iter_app != apps_.end();) {
     AppState* app_state = iter_app->second.get();
     app_state->profiles.erase(profile);
-    if (app_state->profiles.empty())
+
+    if (app_state->ShouldDeleteAppState())
       iter_app = apps_.erase(iter_app);
     else
       ++iter_app;
   }
 }
 
-void AppShimManager::CloseShimForApp(Profile* profile,
-                                     const web_app::AppId& app_id) {
-  auto found_app = apps_.find(app_id);
-  if (found_app == apps_.end())
-    return;
-  AppState* app_state = found_app->second.get();
-  auto found_profile = app_state->profiles.find(profile);
-  if (found_profile == app_state->profiles.end())
-    return;
-  if (app_state->profiles.size() == 1) {
-    app_state->SaveLastActiveProfiles();
-    apps_.erase(found_app);
-  } else {
-    app_state->profiles.erase(found_profile);
-  }
-}
-
 void AppShimManager::LoadProfileAndApp(const base::FilePath& profile_path,
                                        const web_app::AppId& app_id,
                                        LoadProfileAndAppCallback callback) {
@@ -785,6 +790,8 @@ void AppShimManager::OnShimProcessDisconnected(AppShimHost* host) {
     app_state->SaveLastActiveProfiles();
     DCHECK_EQ(host, app_state->multi_profile_host.get());
     apps_.erase(found_app);
+    if (apps_.empty())
+      MaybeTerminate();
     return;
   }
 
@@ -895,7 +902,20 @@ void AppShimManager::OnAppActivated(content::BrowserContext* context,
 
 void AppShimManager::OnAppDeactivated(content::BrowserContext* context,
                                       const std::string& app_id) {
-  CloseShimForApp(static_cast<Profile*>(context), app_id);
+  Profile* profile = static_cast<Profile*>(context);
+  auto found_app = apps_.find(app_id);
+  if (found_app != apps_.end()) {
+    AppState* app_state = found_app->second.get();
+    auto found_profile = app_state->profiles.find(profile);
+    if (found_profile != app_state->profiles.end()) {
+      if (app_state->profiles.size() == 1)
+        app_state->SaveLastActiveProfiles();
+      app_state->profiles.erase(found_profile);
+      if (app_state->ShouldDeleteAppState())
+        apps_.erase(found_app);
+    }
+  }
+
   if (apps_.empty())
     MaybeTerminate();
 }

chrome/browser/apps/app_shim/app_shim_manager_mac.h

@@ -234,9 +234,6 @@ class AppShimManager : public AppShimHostBootstrap::Client,
   // Close all app shims associated with the specified profile.
   void CloseShimsForProfile(Profile* profile);
 
-  // Close one specified app.
-  void CloseShimForApp(Profile* profile, const web_app::AppId& app_id);
-
   // This is called by OnShimProcessConnected if the app shim was launched by
   // Chrome, and should connect to an already-existing AppShimHost.
   void OnShimProcessConnectedForRegisterOnly(

chrome/browser/apps/app_shim/app_shim_manager_mac_unittest.cc

@@ -16,11 +16,13 @@
 #include "base/optional.h"
 #include "base/test/gmock_callback_support.h"
 #include "base/test/mock_callback.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.h"
 #include "chrome/browser/apps/app_shim/app_shim_host_mac.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/avatar_menu.h"
 #include "chrome/browser/web_applications/components/app_shim_registry_mac.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/common/mac/app_shim.mojom.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/prefs/testing_pref_service.h"
@@ -520,6 +522,8 @@ class AppShimManagerTest : public testing::Test {
   base::WeakPtr<TestHost> host_ba_;
   base::WeakPtr<TestHost> host_bb_;
 
+  base::test::ScopedFeatureList scoped_feature_list_;
+
  private:
   std::unique_ptr<TestingPrefServiceSimple> local_state_;
   DISALLOW_COPY_AND_ASSIGN(AppShimManagerTest);
@@ -614,6 +618,62 @@ TEST_F(AppShimManagerTest, LaunchAndCloseShim) {
 }
 
 TEST_F(AppShimManagerTest, AppLifetime) {
+  scoped_feature_list_.InitWithFeatures({features::kAppShimNewCloseBehavior},
+                                        {});
+
+  // When the app activates, a host is created. If there is no shim, one is
+  // launched.
+  manager_->SetHostForCreate(std::move(host_aa_unique_));
+  EXPECT_CALL(*delegate_, DoLaunchShim(&profile_a_, kTestAppIdA, false));
+  manager_->OnAppActivated(&profile_a_, kTestAppIdA);
+  EXPECT_EQ(host_aa_.get(), manager_->FindHost(&profile_a_, kTestAppIdA));
+
+  // Normal shim launch adds an entry in the map.
+  // App should not be launched here, but return success to the shim.
+  EXPECT_CALL(*delegate_, LaunchApp(&profile_a_, kTestAppIdA, _)).Times(0);
+  RegisterOnlyLaunch(bootstrap_aa_, nullptr);
+  EXPECT_EQ(chrome::mojom::AppShimLaunchResult::kSuccess,
+            *bootstrap_aa_result_);
+  EXPECT_EQ(host_aa_.get(), manager_->FindHost(&profile_a_, kTestAppIdA));
+
+  // 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(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);
+  host_aa_->ReopenApp();
+
+  // Open files should trigger a launch with those files.
+  std::vector<base::FilePath> some_file(1, base::FilePath("some_file"));
+  EXPECT_CALL(*delegate_, LaunchApp(&profile_a_, kTestAppIdA, some_file));
+  host_aa_->FilesOpened(some_file);
+
+  // OnAppDeactivated should not close the shim.
+  EXPECT_CALL(*manager_, MaybeTerminate()).Times(0);
+  manager_->OnAppDeactivated(&profile_a_, kTestAppIdA);
+  EXPECT_NE(nullptr, host_aa_.get());
+
+  // Process disconnect will cause the shim to close.
+  EXPECT_CALL(*manager_, MaybeTerminate()).WillOnce(Return());
+  manager_->OnShimProcessDisconnected(host_aa_.get());
+  EXPECT_EQ(nullptr, host_aa_.get());
+}
+
+TEST_F(AppShimManagerTest, AppLifetimeOld) {
+  scoped_feature_list_.InitWithFeatures({},
+                                        {features::kAppShimNewCloseBehavior});
+
   // When the app activates, a host is created. If there is no shim, one is
   // launched.
   manager_->SetHostForCreate(std::move(host_aa_unique_));
@@ -758,6 +818,33 @@ TEST_F(AppShimManagerTest, FailCodeSignature) {
 }
 
 TEST_F(AppShimManagerTest, MaybeTerminate) {
+  scoped_feature_list_.InitWithFeatures({features::kAppShimNewCloseBehavior},
+                                        {});
+
+  // Launch shims, adding entries in the map.
+  RegisterOnlyLaunch(bootstrap_aa_, std::move(host_aa_unique_));
+  EXPECT_EQ(chrome::mojom::AppShimLaunchResult::kSuccess,
+            *bootstrap_aa_result_);
+  EXPECT_EQ(host_aa_.get(), manager_->FindHost(&profile_a_, kTestAppIdA));
+
+  RegisterOnlyLaunch(bootstrap_ab_, std::move(host_ab_unique_));
+  EXPECT_EQ(chrome::mojom::AppShimLaunchResult::kSuccess,
+            *bootstrap_ab_result_);
+  EXPECT_EQ(host_ab_.get(), manager_->FindHost(&profile_a_, kTestAppIdB));
+
+  // Quitting when there's another shim should not terminate.
+  EXPECT_CALL(*manager_, MaybeTerminate()).Times(0);
+  manager_->OnAppDeactivated(&profile_a_, kTestAppIdA);
+
+  // Quitting when it's the last shim should terminate.
+  EXPECT_CALL(*manager_, MaybeTerminate()).Times(0);
+  manager_->OnAppDeactivated(&profile_a_, kTestAppIdB);
+}
+
+TEST_F(AppShimManagerTest, MaybeTerminateOld) {
+  scoped_feature_list_.InitWithFeatures({},
+                                        {features::kAppShimNewCloseBehavior});
+
   // Launch shims, adding entries in the map.
   RegisterOnlyLaunch(bootstrap_aa_, std::move(host_aa_unique_));
   EXPECT_EQ(chrome::mojom::AppShimLaunchResult::kSuccess,

chrome/common/chrome_features.cc

@@ -78,6 +78,12 @@ const base::Feature kAppServiceIntentHandling{"AppServiceIntentHandling",
 // process). For debugging purposes only.
 const base::Feature kAppShimRemoteCocoa{"AppShimRemoteCocoa",
                                         base::FEATURE_ENABLED_BY_DEFAULT};
+
+// This is used to control the new app close behavior on macOS wherein closing
+// all windows for an app leaves the app running.
+// https://crbug.com/1080729
+const base::Feature kAppShimNewCloseBehavior{"AppShimNewCloseBehavior",
+                                             base::FEATURE_ENABLED_BY_DEFAULT};
 #endif  // defined(OS_MAC)
 
 // Enables the built-in DNS resolver.

chrome/common/chrome_features.h

@@ -76,6 +76,8 @@ extern const base::Feature kAppServiceIntentHandling;
 #if defined(OS_MAC)
 COMPONENT_EXPORT(CHROME_FEATURES)
 extern const base::Feature kAppShimRemoteCocoa;
+COMPONENT_EXPORT(CHROME_FEATURES)
+extern const base::Feature kAppShimNewCloseBehavior;
 #endif  // defined(OS_MAC)
 
 COMPONENT_EXPORT(CHROME_FEATURES) extern const base::Feature kAsyncDns;