Remove the record from full restore data when the window is destroyed.

BUG=1146900

Change-Id: Iebbf4b53aef5723dfc71e394a302ff3f9557a17c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2611144
Commit-Queue: Nancy Wang <nancylingwang@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841167}

components/full_restore/app_launch_info.cc

@@ -9,14 +9,14 @@
 namespace full_restore {
 
 AppLaunchInfo::AppLaunchInfo(const std::string& app_id,
-                             int32_t session_id,
+                             int32_t window_id,
                              apps::mojom::LaunchContainer container,
                              WindowOpenDisposition disposition,
                              int64_t display_id,
                              std::vector<base::FilePath> launch_files,
                              apps::mojom::IntentPtr intent)
     : app_id(app_id),
-      id(session_id),
+      window_id(window_id),
       container(static_cast<int32_t>(container)),
       disposition(static_cast<int32_t>(disposition)),
       display_id(display_id),
@@ -24,7 +24,7 @@ AppLaunchInfo::AppLaunchInfo(const std::string& app_id,
       intent(std::move(intent)) {}
 
 AppLaunchInfo::AppLaunchInfo(const std::string& app_id, int32_t session_id)
-    : app_id(app_id), id(session_id) {}
+    : app_id(app_id), window_id(session_id) {}
 
 AppLaunchInfo::AppLaunchInfo(const std::string& app_id,
                              apps::mojom::LaunchContainer container,

components/full_restore/app_launch_info.h

@@ -20,14 +20,14 @@ namespace full_restore {
 // app launch information.
 struct COMPONENT_EXPORT(FULL_RESTORE) AppLaunchInfo {
   AppLaunchInfo(const std::string& app_id,
-                int32_t session_id,
+                int32_t window_id,
                 apps::mojom::LaunchContainer container,
                 WindowOpenDisposition disposition,
                 int64_t display_id,
                 std::vector<base::FilePath> launch_files,
                 apps::mojom::IntentPtr intent);
 
-  AppLaunchInfo(const std::string& app_id, int32_t session_id);
+  AppLaunchInfo(const std::string& app_id, int32_t window_id);
 
   AppLaunchInfo(const std::string& app_id,
                 apps::mojom::LaunchContainer container,
@@ -51,7 +51,7 @@ struct COMPONENT_EXPORT(FULL_RESTORE) AppLaunchInfo {
   ~AppLaunchInfo();
 
   std::string app_id;
-  base::Optional<int32_t> id;
+  base::Optional<int32_t> window_id;
   base::Optional<int32_t> event_flag;
   base::Optional<int32_t> container;
   base::Optional<int32_t> disposition;

components/full_restore/full_restore_save_handler.cc

@@ -17,6 +17,7 @@
 #include "components/full_restore/window_info.h"
 #include "components/sessions/core/session_id.h"
 #include "ui/aura/client/aura_constants.h"
+#include "ui/aura/env.h"
 #include "ui/aura/window.h"
 
 namespace full_restore {
@@ -34,9 +35,47 @@ FullRestoreSaveHandler* FullRestoreSaveHandler::GetInstance() {
   return full_restore_save_handler.get();
 }
 
-FullRestoreSaveHandler::FullRestoreSaveHandler() = default;
+FullRestoreSaveHandler::FullRestoreSaveHandler() {
+  aura::Env::GetInstance()->AddObserver(this);
+}
+
+FullRestoreSaveHandler::~FullRestoreSaveHandler() {
+  aura::Env::GetInstance()->RemoveObserver(this);
+}
+
+void FullRestoreSaveHandler::OnWindowInitialized(aura::Window* window) {
+  // TODO(crbug.com/1146900): Handle ARC app windows.
+
+  int32_t window_id = window->GetProperty(::full_restore::kWindowIdKey);
+
+  if (!SessionID::IsValidValue(window_id))
+    return;
+
+  observed_windows_.AddObservation(window);
+}
+
+void FullRestoreSaveHandler::OnWindowDestroying(aura::Window* window) {
+  // TODO(crbug.com/1146900): Handle ARC app windows.
 
-FullRestoreSaveHandler::~FullRestoreSaveHandler() = default;
+  DCHECK(observed_windows_.IsObservingSource(window));
+  observed_windows_.RemoveObservation(window);
+
+  int32_t window_id = window->GetProperty(::full_restore::kWindowIdKey);
+  DCHECK(SessionID::IsValidValue(window_id));
+
+  auto it = window_id_to_app_restore_info_.find(window_id);
+  if (it == window_id_to_app_restore_info_.end())
+    return;
+
+  profile_path_to_restore_data_[it->second.first].RemoveAppRestoreData(
+      it->second.second, window_id);
+
+  pending_save_profile_paths_.insert(it->second.first);
+
+  window_id_to_app_restore_info_.erase(it);
+
+  MaybeStartSaveTimer();
+}
 
 void FullRestoreSaveHandler::SaveAppLaunchInfo(
     const base::FilePath& profile_path,
@@ -44,12 +83,12 @@ void FullRestoreSaveHandler::SaveAppLaunchInfo(
   if (!app_launch_info)
     return;
 
-  if (!app_launch_info->id.has_value()) {
+  if (!app_launch_info->window_id.has_value()) {
     // TODO(crbug.com/1146900): Handle ARC app windows.
     return;
   }
 
-  window_id_to_app_restore_info_[app_launch_info->id.value()] =
+  window_id_to_app_restore_info_[app_launch_info->window_id.value()] =
       std::make_pair(profile_path, app_launch_info->app_id);
 
   // Each user should have one full restore file saving the restore data in the

components/full_restore/full_restore_save_handler.h

@@ -12,7 +12,11 @@
 #include "base/component_export.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_multi_source_observation.h"
 #include "base/timer/timer.h"
+#include "ui/aura/env_observer.h"
+#include "ui/aura/window.h"
+#include "ui/aura/window_observer.h"
 
 namespace base {
 class FilePath;
@@ -32,16 +36,24 @@ struct WindowInfo;
 // background task runner) for the actual writing. To minimize IO,
 // FullRestoreSaveHandler starts a timer that invokes restore data saving at a
 // later time.
-class COMPONENT_EXPORT(FULL_RESTORE) FullRestoreSaveHandler {
+class COMPONENT_EXPORT(FULL_RESTORE) FullRestoreSaveHandler
+    : public aura::EnvObserver,
+      public aura::WindowObserver {
  public:
   static FullRestoreSaveHandler* GetInstance();
 
   FullRestoreSaveHandler();
-  virtual ~FullRestoreSaveHandler();
+  ~FullRestoreSaveHandler() override;
 
   FullRestoreSaveHandler(const FullRestoreSaveHandler&) = delete;
   FullRestoreSaveHandler& operator=(const FullRestoreSaveHandler&) = delete;
 
+  // aura::EnvObserver:
+  void OnWindowInitialized(aura::Window* window) override;
+
+  // aura::WindowObserver:
+  void OnWindowDestroying(aura::Window* window) override;
+
   // Save |app_launch_info| to the full restore file in |profile_path|.
   void SaveAppLaunchInfo(const base::FilePath& profile_path,
                          std::unique_ptr<AppLaunchInfo> app_launch_info);
@@ -89,6 +101,9 @@ class COMPONENT_EXPORT(FULL_RESTORE) FullRestoreSaveHandler {
   // Records whether the saving process is running for a full restore file.
   std::set<base::FilePath> save_running_;
 
+  base::ScopedMultiSourceObservation<aura::Window, aura::WindowObserver>
+      observed_windows_{this};
+
   base::WeakPtrFactory<FullRestoreSaveHandler> weak_factory_{this};
 };
 

components/full_restore/restore_data.cc

@@ -39,14 +39,15 @@ RestoreData::RestoreData(std::unique_ptr<base::Value> restore_data_value) {
 
     for (base::DictionaryValue::Iterator data_iter(*data_dict);
          !data_iter.IsAtEnd(); data_iter.Advance()) {
-      int id = 0;
-      if (!base::StringToInt(data_iter.key(), &id)) {
+      int window_id = 0;
+      if (!base::StringToInt(data_iter.key(), &window_id)) {
         DVLOG(0) << "Fail to parse full restore data. "
                  << "Cannot find the valid id.";
         continue;
       }
-      app_id_to_launch_list_[app_id][id] = std::make_unique<AppRestoreData>(
-          std::move(*data_dict->FindDictKey(data_iter.key())));
+      app_id_to_launch_list_[app_id][window_id] =
+          std::make_unique<AppRestoreData>(
+              std::move(*data_dict->FindDictKey(data_iter.key())));
     }
   }
 }
@@ -83,27 +84,37 @@ base::Value RestoreData::ConvertToValue() const {
 
 void RestoreData::AddAppLaunchInfo(
     std::unique_ptr<AppLaunchInfo> app_launch_info) {
-  if (!app_launch_info || !app_launch_info->id.has_value())
+  if (!app_launch_info || !app_launch_info->window_id.has_value())
     return;
 
   const std::string app_id = app_launch_info->app_id;
-  const int32_t id = app_launch_info->id.value();
-  app_id_to_launch_list_[app_id][id] =
+  const int32_t window_id = app_launch_info->window_id.value();
+  app_id_to_launch_list_[app_id][window_id] =
       std::make_unique<AppRestoreData>(std::move(app_launch_info));
 }
 
 void RestoreData::ModifyWindowInfo(const std::string& app_id,
-                                   int32_t id,
+                                   int32_t window_id,
                                    const WindowInfo& window_info) {
   auto it = app_id_to_launch_list_.find(app_id);
   if (it == app_id_to_launch_list_.end())
     return;
 
-  auto data_it = it->second.find(id);
+  auto data_it = it->second.find(window_id);
   if (data_it == it->second.end())
     return;
 
   data_it->second->ModifyWindowInfo(window_info);
 }
 
+void RestoreData::RemoveAppRestoreData(const std::string& app_id,
+                                       int window_id) {
+  if (app_id_to_launch_list_.find(app_id) == app_id_to_launch_list_.end())
+    return;
+
+  app_id_to_launch_list_[app_id].erase(window_id);
+  if (app_id_to_launch_list_[app_id].empty())
+    app_id_to_launch_list_.erase(app_id);
+}
+
 }  // namespace full_restore

components/full_restore/restore_data.h

@@ -50,7 +50,7 @@ class COMPONENT_EXPORT(FULL_RESTORE) RestoreData {
   // {
   //   "odknhmnlageboeamepcngndbggdpaobj":    // app_id
   //     {
-  //       "403":                             // id
+  //       "403":                             // window_id
   //         {
   //           "container": 0,
   //           "disposition": 1,
@@ -64,14 +64,14 @@ class COMPONENT_EXPORT(FULL_RESTORE) RestoreData {
   //     },
   //   "pjibgclleladliembfgfagdaldikeohf":    // app_id
   //     {
-  //       "413":                             // id
+  //       "413":                             // window_id
   //         {
   //           "container": 0,
   //           "disposition": 3,
   //           "display_id": "22000000",
   //           ...
   //         },
-  //       "415":                             // id
+  //       "415":                             // window_id
   //         {
   //           ...
   //         },
@@ -83,11 +83,14 @@ class COMPONENT_EXPORT(FULL_RESTORE) RestoreData {
   void AddAppLaunchInfo(std::unique_ptr<AppLaunchInfo> app_launch_info);
 
   // Modify the window's information based on |window_info| for the window with
-  // |id| of the app with |app_id|.
+  // |window_id| of the app with |app_id|.
   void ModifyWindowInfo(const std::string& app_id,
-                        int32_t id,
+                        int32_t window_id,
                         const WindowInfo& window_info);
 
+  // Remove a AppRestoreData with |window_id| for |app_id|.
+  void RemoveAppRestoreData(const std::string& app_id, int window_id);
+
   const AppIdToLaunchList& app_id_to_launch_list() const {
     return app_id_to_launch_list_;
   }

components/full_restore/restore_data_unittest.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <utility>
 
+#include "base/containers/contains.h"
 #include "chromeos/ui/base/window_state_type.h"
 #include "components/full_restore/app_launch_info.h"
 #include "components/full_restore/app_restore_data.h"
@@ -23,9 +24,9 @@ namespace {
 const char kAppId1[] = "aaa";
 const char kAppId2[] = "bbb";
 
-const int32_t kId1 = 100;
-const int32_t kId2 = 200;
-const int32_t kId3 = 300;
+const int32_t kWindowId1 = 100;
+const int32_t kWindowId2 = 200;
+const int32_t kWindowId3 = 300;
 
 const int64_t kDisplayId1 = 22000000;
 const int64_t kDisplayId2 = 11000000;
@@ -88,7 +89,8 @@ class RestoreDataTest : public testing::Test {
   void AddAppLaunchInfos() {
     std::unique_ptr<AppLaunchInfo> app_launch_info1 =
         std::make_unique<AppLaunchInfo>(
-            kAppId1, kId1, apps::mojom::LaunchContainer::kLaunchContainerWindow,
+            kAppId1, kWindowId1,
+            apps::mojom::LaunchContainer::kLaunchContainerWindow,
             WindowOpenDisposition::NEW_WINDOW, kDisplayId1,
             std::vector<base::FilePath>{base::FilePath(kFilePath1),
                                         base::FilePath(kFilePath2)},
@@ -96,14 +98,16 @@ class RestoreDataTest : public testing::Test {
 
     std::unique_ptr<AppLaunchInfo> app_launch_info2 =
         std::make_unique<AppLaunchInfo>(
-            kAppId1, kId2, apps::mojom::LaunchContainer::kLaunchContainerTab,
+            kAppId1, kWindowId2,
+            apps::mojom::LaunchContainer::kLaunchContainerTab,
             WindowOpenDisposition::NEW_FOREGROUND_TAB, kDisplayId2,
             std::vector<base::FilePath>{base::FilePath(kFilePath2)},
             CreateIntent(kIntentActionView, kMimeType, kShareText2));
 
     std::unique_ptr<AppLaunchInfo> app_launch_info3 =
         std::make_unique<AppLaunchInfo>(
-            kAppId2, kId3, apps::mojom::LaunchContainer::kLaunchContainerNone,
+            kAppId2, kWindowId3,
+            apps::mojom::LaunchContainer::kLaunchContainerNone,
             WindowOpenDisposition::NEW_POPUP, kDisplayId2,
             std::vector<base::FilePath>{base::FilePath(kFilePath1)},
             CreateIntent(kIntentActionView, kMimeType, kShareText1));
@@ -135,9 +139,9 @@ class RestoreDataTest : public testing::Test {
     window_info3.current_bounds = kCurrentBounds3;
     window_info3.window_state_type = kWindowStateType3;
 
-    restore_data().ModifyWindowInfo(kAppId1, kId1, window_info1);
-    restore_data().ModifyWindowInfo(kAppId1, kId2, window_info2);
-    restore_data().ModifyWindowInfo(kAppId2, kId3, window_info3);
+    restore_data().ModifyWindowInfo(kAppId1, kWindowId1, window_info1);
+    restore_data().ModifyWindowInfo(kAppId1, kWindowId2, window_info2);
+    restore_data().ModifyWindowInfo(kAppId2, kWindowId3, window_info3);
   }
 
   void VerifyAppRestoreData(const std::unique_ptr<AppRestoreData>& data,
@@ -189,13 +193,13 @@ class RestoreDataTest : public testing::Test {
   void VerifyRestoreData(const RestoreData& restore_data) {
     EXPECT_EQ(2u, app_id_to_launch_list(restore_data).size());
 
-    // Verify for |kAppId1|
+    // Verify for |kAppId1|.
     const auto launch_list_it1 =
         app_id_to_launch_list(restore_data).find(kAppId1);
     EXPECT_TRUE(launch_list_it1 != app_id_to_launch_list(restore_data).end());
     EXPECT_EQ(2u, launch_list_it1->second.size());
 
-    const auto app_restore_data_it1 = launch_list_it1->second.find(kId1);
+    const auto app_restore_data_it1 = launch_list_it1->second.find(kWindowId1);
     EXPECT_TRUE(app_restore_data_it1 != launch_list_it1->second.end());
 
     VerifyAppRestoreData(
@@ -208,7 +212,7 @@ class RestoreDataTest : public testing::Test {
         kActivationIndex1, kDeskId1, kRestoreBounds1, kCurrentBounds1,
         kWindowStateType1);
 
-    const auto app_restore_data_it2 = launch_list_it1->second.find(kId2);
+    const auto app_restore_data_it2 = launch_list_it1->second.find(kWindowId2);
     EXPECT_TRUE(app_restore_data_it2 != launch_list_it1->second.end());
     VerifyAppRestoreData(
         app_restore_data_it2->second,
@@ -219,13 +223,13 @@ class RestoreDataTest : public testing::Test {
         kActivationIndex2, kDeskId2, kRestoreBounds2, kCurrentBounds2,
         kWindowStateType2);
 
-    // Verify for |kAppId2|
+    // Verify for |kAppId2|.
     const auto launch_list_it2 =
         app_id_to_launch_list(restore_data).find(kAppId2);
     EXPECT_TRUE(launch_list_it2 != app_id_to_launch_list(restore_data).end());
     EXPECT_EQ(1u, launch_list_it2->second.size());
 
-    EXPECT_EQ(kId3, launch_list_it2->second.begin()->first);
+    EXPECT_EQ(kWindowId3, launch_list_it2->second.begin()->first);
     VerifyAppRestoreData(
         launch_list_it2->second.begin()->second,
         apps::mojom::LaunchContainer::kLaunchContainerNone,
@@ -262,6 +266,52 @@ TEST_F(RestoreDataTest, AddAppLaunchInfos) {
   VerifyRestoreData(restore_data());
 }
 
+TEST_F(RestoreDataTest, RemoveAppRestoreData) {
+  AddAppLaunchInfos();
+  ModifyWindowInfos();
+  VerifyRestoreData(restore_data());
+
+  // Remove kAppId1's kId1.
+  restore_data().RemoveAppRestoreData(kAppId1, kWindowId1);
+
+  EXPECT_EQ(2u, app_id_to_launch_list().size());
+
+  // Verify for |kAppId1|.
+  auto launch_list_it1 = app_id_to_launch_list().find(kAppId1);
+  EXPECT_TRUE(launch_list_it1 != app_id_to_launch_list().end());
+  EXPECT_EQ(1u, launch_list_it1->second.size());
+
+  EXPECT_FALSE(base::Contains(launch_list_it1->second, kWindowId1));
+  EXPECT_TRUE(base::Contains(launch_list_it1->second, kWindowId2));
+
+  // Verify for |kAppId2|.
+  auto launch_list_it2 = app_id_to_launch_list().find(kAppId2);
+  EXPECT_TRUE(launch_list_it2 != app_id_to_launch_list().end());
+  EXPECT_EQ(1u, launch_list_it2->second.size());
+
+  EXPECT_TRUE(base::Contains(launch_list_it2->second, kWindowId3));
+
+  // Remove kAppId1's kId2.
+  restore_data().RemoveAppRestoreData(kAppId1, kWindowId2);
+
+  EXPECT_EQ(1u, app_id_to_launch_list().size());
+
+  // Verify for |kAppId1|.
+  EXPECT_FALSE(base::Contains(app_id_to_launch_list(), kAppId1));
+
+  // Verify for |kAppId2|.
+  launch_list_it2 = app_id_to_launch_list().find(kAppId2);
+  EXPECT_TRUE(launch_list_it2 != app_id_to_launch_list().end());
+  EXPECT_EQ(1u, launch_list_it2->second.size());
+
+  EXPECT_TRUE(base::Contains(launch_list_it2->second, kWindowId3));
+
+  // Remove kAppId2's kId3.
+  restore_data().RemoveAppRestoreData(kAppId2, kWindowId3);
+
+  EXPECT_EQ(0u, app_id_to_launch_list().size());
+}
+
 TEST_F(RestoreDataTest, Convert) {
   AddAppLaunchInfos();
   ModifyWindowInfos();