borealis: Migrate window manager to use the InstanceRegistry

The InstanceRegistry is the new/preferred way for application providers
like borealis to observe their apps' windows.

When using the instanceregistry, we also avoid the (potential) pitfall
due to us performing all of our window registration in a method called
"GetShelfAppId".

Bug: b/175152663
Change-Id: I1f7893428d76edc507e6841ebd94ffbb68024867
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2586445
Commit-Queue: Nic Hollingum <hollingum@google.com>
Reviewed-by: Nancy Wang <nancylingwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837451}

chrome/browser/chromeos/borealis/borealis_window_manager.cc

@@ -10,10 +10,11 @@
 #include "base/containers/flat_set.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
+#include "chrome/browser/apps/app_service/app_service_proxy.h"
+#include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
 #include "chrome/browser/chromeos/borealis/borealis_util.h"
 #include "chrome/browser/chromeos/crostini/crostini_shelf_utils.h"
 #include "components/exo/shell_surface_util.h"
-#include "ui/aura/window.h"
 
 namespace {
 
@@ -75,7 +76,7 @@ bool BorealisWindowManager::IsBorealisWindow(aura::Window* window) {
 }
 
 BorealisWindowManager::BorealisWindowManager(Profile* profile)
-    : profile_(profile) {}
+    : profile_(profile), instance_registry_observation_(this) {}
 
 BorealisWindowManager::~BorealisWindowManager() {
   for (auto& observer : anon_observers_) {
@@ -109,14 +110,37 @@ std::string BorealisWindowManager::GetShelfAppId(aura::Window* window) {
   if (!IsBorealisWindow(window))
     return {};
 
-  std::string app_id = WindowToAppId(profile_, window);
-  HandleWindow(window, app_id);
+  // We delay the observation until the first time we actually see a borealis
+  // window, which prevents unnecessary messages being sent and breaks an
+  // init-cycle.
+  if (!instance_registry_observation_.IsObserving()) {
+    instance_registry_observation_.Observe(
+        &apps::AppServiceProxyFactory::GetForProfile(profile_)
+             ->InstanceRegistry());
+  }
+
+  return WindowToAppId(profile_, window);
+}
+
+void BorealisWindowManager::OnInstanceUpdate(
+    const apps::InstanceUpdate& update) {
+  if (!IsBorealisWindow(update.Window()))
+    return;
+  if (update.IsCreation()) {
+    HandleWindowCreation(update.Window(), update.AppId());
+  } else if (update.IsDestruction()) {
+    HandleWindowDestruction(update.Window(), update.AppId());
+  }
+}
 
-  return app_id;
+void BorealisWindowManager::OnInstanceRegistryWillBeDestroyed(
+    apps::InstanceRegistry* cache) {
+  DCHECK(instance_registry_observation_.IsObservingSource(cache));
+  instance_registry_observation_.Reset();
 }
 
-void BorealisWindowManager::OnWindowDestroying(aura::Window* window) {
-  std::string app_id = WindowToAppId(profile_, window);
+void BorealisWindowManager::HandleWindowDestruction(aura::Window* window,
+                                                    const std::string& app_id) {
   for (auto& observer : lifetime_observers_) {
     observer.OnWindowFinished(app_id, window);
   }
@@ -143,8 +167,8 @@ void BorealisWindowManager::OnWindowDestroying(aura::Window* window) {
     observer.OnSessionFinished();
 }
 
-void BorealisWindowManager::HandleWindow(aura::Window* window,
-                                         const std::string& app_id) {
+void BorealisWindowManager::HandleWindowCreation(aura::Window* window,
+                                                 const std::string& app_id) {
   // If this is the first window, the session has started.
   if (ids_to_windows_.empty()) {
     for (auto& observer : lifetime_observers_)
@@ -160,10 +184,8 @@ void BorealisWindowManager::HandleWindow(aura::Window* window,
         observer.OnAnonymousAppAdded(app_id, anon_name);
     }
   }
-  // If this window was not already in the set, observe it and notify our
-  // observers about it.
+  // If this window was not already in the set, notify our observers about it.
   if (ids_to_windows_[app_id].emplace(window).second) {
-    window->AddObserver(this);
     for (auto& observer : lifetime_observers_)
       observer.OnWindowStarted(app_id, window);
   }

chrome/browser/chromeos/borealis/borealis_window_manager.h

@@ -11,7 +11,8 @@
 #include "base/containers/flat_set.h"
 #include "base/observer_list.h"
 #include "base/observer_list_types.h"
-#include "ui/aura/window_observer.h"
+#include "base/scoped_observation.h"
+#include "components/services/app_service/public/cpp/instance_registry.h"
 
 class Profile;
 
@@ -25,7 +26,7 @@ namespace borealis {
 // borealis apps. This includes determining which windows belong to a borealis
 // app, what the lifetime of the app is relative to its windows, and the
 // presence of borealis windows with an unknown app (see go/anonymous-apps).
-class BorealisWindowManager : public aura::WindowObserver {
+class BorealisWindowManager : public apps::InstanceRegistry::Observer {
  public:
   // Returns true if this window belongs to a borealis VM (based on its app_id
   // and startup_id).
@@ -104,20 +105,22 @@ class BorealisWindowManager : public aura::WindowObserver {
   void RemoveObserver(AppWindowLifetimeObserver* observer);
 
   // Returns the application ID for the given window, or "" if the window does
-  // not belong to borealis. If the window does belong to borealis, this call
-  // will also cause the manager to track the window.
-  //
-  // TODO(b/175152663): Refactor this into two methods so that it is clear which
-  // one has side-effects.
+  // not belong to borealis.
   std::string GetShelfAppId(aura::Window* window);
 
- private:
-  // aura::WindowObserver overrides.
-  void OnWindowDestroying(aura::Window* window) override;
+  // apps::InstanceRegistry::Observer overrides.
+  void OnInstanceUpdate(const apps::InstanceUpdate& update) override;
+  void OnInstanceRegistryWillBeDestroyed(
+      apps::InstanceRegistry* cache) override;
 
-  void HandleWindow(aura::Window* window, const std::string& app_id);
+ private:
+  void HandleWindowDestruction(aura::Window* window, const std::string& app_id);
+  void HandleWindowCreation(aura::Window* window, const std::string& app_id);
 
   Profile* const profile_;
+  base::ScopedObservation<apps::InstanceRegistry,
+                          apps::InstanceRegistry::Observer>
+      instance_registry_observation_;
   base::flat_map<std::string, base::flat_set<aura::Window*>> ids_to_windows_;
   base::ObserverList<AnonymousAppObserver> anon_observers_;
   base::ObserverList<AppWindowLifetimeObserver> lifetime_observers_;

chrome/browser/chromeos/borealis/borealis_window_manager_unittest.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/chromeos/guest_os/guest_os_registry_service_factory.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/exo/shell_surface_util.h"
+#include "components/services/app_service/public/cpp/instance.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -23,6 +24,31 @@ namespace {
 
 class BorealisWindowManagerTest : public testing::Test {
  protected:
+  // A helper class used to emulate the behaviour of the InstanceRegistry when
+  // windows are created/destroyed.
+  class ScopedTestWindow {
+   public:
+    ScopedTestWindow(std::unique_ptr<aura::Window> window,
+                     BorealisWindowManager* manager)
+        : window_(std::move(window)), manager_(manager) {
+      apps::Instance instance(manager_->GetShelfAppId(window_.get()),
+                              window_.get());
+      manager_->OnInstanceUpdate(apps::InstanceUpdate(nullptr, &instance));
+    }
+
+    ~ScopedTestWindow() {
+      apps::Instance instance(manager_->GetShelfAppId(window_.get()),
+                              window_.get());
+      std::unique_ptr<apps::Instance> delta = instance.Clone();
+      delta->UpdateState(apps::InstanceState::kDestroyed, base::Time::Now());
+      manager_->OnInstanceUpdate(apps::InstanceUpdate(&instance, delta.get()));
+    }
+
+   private:
+    std::unique_ptr<aura::Window> window_;
+    BorealisWindowManager* manager_;
+  };
+
   Profile* profile() { return &profile_; }
   // Creates a widget for use in testing.
   std::unique_ptr<aura::Window> MakeWindow(std::string name) {
@@ -32,6 +58,13 @@ class BorealisWindowManagerTest : public testing::Test {
     return win;
   }
 
+  std::unique_ptr<ScopedTestWindow> MakeAndTrackWindow(
+      std::string name,
+      BorealisWindowManager* manager) {
+    return std::make_unique<ScopedTestWindow>(MakeWindow(std::move(name)),
+                                              manager);
+  }
+
  private:
   content::BrowserTaskEnvironment task_environment_;
   TestingProfile profile_;
@@ -50,6 +83,22 @@ TEST_F(BorealisWindowManagerTest, BorealisWindowHasAnId) {
   EXPECT_NE(window_manager.GetShelfAppId(window.get()), "");
 }
 
+TEST_F(BorealisWindowManagerTest, IdDetectionDoesNotImplyTracking) {
+  BorealisWindowManager window_manager(profile());
+
+  testing::StrictMock<MockAnonObserver> anon_observer;
+  testing::StrictMock<MockLifetimeObserver> life_observer;
+  window_manager.AddObserver(&anon_observer);
+  window_manager.AddObserver(&life_observer);
+
+  std::unique_ptr<aura::Window> window =
+      MakeWindow("org.chromium.borealis.foobarbaz");
+  window_manager.GetShelfAppId(window.get());
+
+  window_manager.RemoveObserver(&anon_observer);
+  window_manager.RemoveObserver(&life_observer);
+}
+
 TEST_F(BorealisWindowManagerTest, ObserversNotifiedOnManagerShutdown) {
   testing::StrictMock<MockAnonObserver> anon_observer;
   testing::StrictMock<MockLifetimeObserver> life_observer;
@@ -75,9 +124,8 @@ TEST_F(BorealisWindowManagerTest, ObserverCalledForAnonymousApp) {
 
   BorealisWindowManager window_manager(profile());
   window_manager.AddObserver(&observer);
-  std::unique_ptr<aura::Window> window =
-      MakeWindow("org.chromium.borealis.anonymous_app");
-  window_manager.GetShelfAppId(window.get());
+  std::unique_ptr<ScopedTestWindow> window = MakeAndTrackWindow(
+      "org.chromium.borealis.anonymous_app", &window_manager);
 
   EXPECT_CALL(observer,
               OnAnonymousAppRemoved(testing::ContainsRegex("anonymous_app")));
@@ -99,22 +147,19 @@ TEST_F(BorealisWindowManagerTest, LifetimeObserverTracksWindows) {
   EXPECT_CALL(observer, OnSessionStarted());
   EXPECT_CALL(observer, OnAppStarted(_));
   EXPECT_CALL(observer, OnWindowStarted(_, _));
-  std::unique_ptr<aura::Window> first_foo =
-      MakeWindow("org.chromium.borealis.foo");
-  window_manager.GetShelfAppId(first_foo.get());
+  std::unique_ptr<ScopedTestWindow> first_foo =
+      MakeAndTrackWindow("org.chromium.borealis.foo", &window_manager);
 
   // A window for the same app only starts that window.
   EXPECT_CALL(observer, OnWindowStarted(_, _));
-  std::unique_ptr<aura::Window> second_foo =
-      MakeWindow("org.chromium.borealis.foo");
-  window_manager.GetShelfAppId(second_foo.get());
+  std::unique_ptr<ScopedTestWindow> second_foo =
+      MakeAndTrackWindow("org.chromium.borealis.foo", &window_manager);
 
   // Whereas a new app starts both the app and the window.
   EXPECT_CALL(observer, OnAppStarted(_));
   EXPECT_CALL(observer, OnWindowStarted(_, _));
-  std::unique_ptr<aura::Window> only_bar =
-      MakeWindow("org.chromium.borealis.bar");
-  window_manager.GetShelfAppId(only_bar.get());
+  std::unique_ptr<ScopedTestWindow> only_bar =
+      MakeAndTrackWindow("org.chromium.borealis.bar", &window_manager);
 
   // Deleting an app window while one still exists does not end the app.
   EXPECT_CALL(observer, OnWindowFinished(_, _));
@@ -144,12 +189,10 @@ TEST_F(BorealisWindowManagerTest, HandlesMultipleAnonymousWindows) {
   // one observer call.
   EXPECT_CALL(observer, OnAnonymousAppAdded(_, _)).Times(1);
 
-  std::unique_ptr<aura::Window> window1 =
-      MakeWindow("org.chromium.borealis.anonymous_app");
-  window_manager.GetShelfAppId(window1.get());
-  std::unique_ptr<aura::Window> window2 =
-      MakeWindow("org.chromium.borealis.anonymous_app");
-  window_manager.GetShelfAppId(window2.get());
+  std::unique_ptr<ScopedTestWindow> window1 = MakeAndTrackWindow(
+      "org.chromium.borealis.anonymous_app", &window_manager);
+  std::unique_ptr<ScopedTestWindow> window2 = MakeAndTrackWindow(
+      "org.chromium.borealis.anonymous_app", &window_manager);
 
   // We only expect to see the app removed after the last window closes.
   window1.reset();
@@ -176,9 +219,8 @@ TEST_F(BorealisWindowManagerTest, AnonymousObserverNotCalledForKnownApp) {
 
   BorealisWindowManager window_manager(profile());
   window_manager.AddObserver(&observer);
-  std::unique_ptr<aura::Window> window =
-      MakeWindow("org.chromium.borealis.wmclass.foo");
-  window_manager.GetShelfAppId(window.get());
+  std::unique_ptr<ScopedTestWindow> window =
+      MakeAndTrackWindow("org.chromium.borealis.wmclass.foo", &window_manager);
 
   window_manager.RemoveObserver(&observer);
 }

chrome/browser/ui/ash/launcher/app_service/app_service_app_window_launcher_controller.cc

@@ -320,8 +320,7 @@ void AppServiceAppWindowLauncherController::OnWindowActivated(
 
 void AppServiceAppWindowLauncherController::OnInstanceUpdate(
     const apps::InstanceUpdate& update) {
-  if (update.StateChanged() &&
-      update.State() == apps::InstanceState::kDestroyed) {
+  if (update.IsDestruction()) {
     // For Chrome apps edge case, it could be added for the inactive users, and
     // then removed. Since it is not registered we don't need to do anything
     // anyways. As such, all which is left to do here is to get rid of our own
@@ -340,9 +339,7 @@ void AppServiceAppWindowLauncherController::OnInstanceUpdate(
   ash::ShelfID shelf_id(update.AppId(), update.LaunchId());
 
   // This is the first update for the given window.
-  if (update.StateIsNull() &&
-      (update.State() & apps::InstanceState::kDestroyed) ==
-          apps::InstanceState::kUnknown) {
+  if (update.IsCreation()) {
     std::string app_id = update.AppId();
     if (GetAppType(app_id) == apps::mojom::AppType::kCrostini ||
         crostini::IsUnmatchedCrostiniShelfAppId(app_id)) {

components/services/app_service/public/cpp/instance_update.cc

@@ -89,6 +89,15 @@ bool InstanceUpdate::StateIsNull() const {
   return state_ == nullptr;
 }
 
+bool InstanceUpdate::IsCreation() const {
+  return StateIsNull() && (State() & apps::InstanceState::kDestroyed) ==
+                              apps::InstanceState::kUnknown;
+}
+
+bool InstanceUpdate::IsDestruction() const {
+  return StateChanged() && State() == apps::InstanceState::kDestroyed;
+}
+
 const std::string& InstanceUpdate::AppId() const {
   return delta_ ? delta_->AppId() : state_->AppId();
 }

components/services/app_service/public/cpp/instance_update.h

@@ -53,6 +53,13 @@ class InstanceUpdate {
   // Equivalently, there are no previous deltas for the window.
   bool StateIsNull() const;
 
+  // Returns true if this update represents the creation of an instance, which
+  // will now be visible to the user.
+  bool IsCreation() const;
+
+  // Returns true if this update represents the destruction of an instance.
+  bool IsDestruction() const;
+
   const std::string& AppId() const;
 
   aura::Window* Window() const;