Add install and last launch time fields to the Crostini registry

This patch adds install_time and last_launch_time fields to the Crostini
registry. These will be used in the app launcher's recently launched
apps list, where currently Crostini apps do not appear.

As we would also like the Terminal to appear in the recents list, this
patch makes the registry support the Terminal, returning it in both
GetRegistration and GetRegisteredAppIds. We don't need to keep track of
Terminal install time as it is just a fallback for apps that haven't
been launched. Internally, we only store the last launched time, so
methods in the registry need to be careful they don't expect all pref
entries will have all the standard fields.

Bug: 821662, 836137
Change-Id: I43f34434afe61bd80c165c3a42047b184f96fcbe
Reviewed-on: https://chromium-review.googlesource.com/1027292
Commit-Queue: Timothy Loh <timloh@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553945}

chrome/browser/chromeos/crostini/crostini_registry_service.cc

@@ -4,7 +4,11 @@
 
 #include "chrome/browser/chromeos/crostini/crostini_registry_service.h"
 
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "base/time/clock.h"
+#include "base/time/default_clock.h"
+#include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
@@ -44,6 +48,8 @@ constexpr char kAppNameKey[] = "name";
 constexpr char kAppNoDisplayKey[] = "no_display";
 constexpr char kAppStartupWMClassKey[] = "startup_wm_class";
 constexpr char kAppStartupNotifyKey[] = "startup_notify";
+constexpr char kAppInstallTimeKey[] = "install_time";
+constexpr char kAppLastLaunchTimeKey[] = "last_launch_time";
 
 std::string GenerateAppId(const std::string& desktop_file_id,
                           const std::string& vm_name,
@@ -104,6 +110,9 @@ FindAppIdResult FindAppId(const base::DictionaryValue* prefs,
                           bool require_startup_notify = false) {
   result->clear();
   for (const auto& item : prefs->DictItems()) {
+    if (item.first == kCrostiniTerminalId)
+      continue;
+
     if (require_startup_notify &&
         !item.second
              .FindKeyOfType(kAppStartupNotifyKey, base::Value::Type::BOOLEAN)
@@ -126,6 +135,31 @@ FindAppIdResult FindAppId(const base::DictionaryValue* prefs,
   return FindAppIdResult::NoMatch;
 }
 
+bool EqualsExcludingTimestamps(const base::Value& left,
+                               const base::Value& right) {
+  auto left_items = left.DictItems();
+  auto right_items = right.DictItems();
+  auto left_iter = left_items.begin();
+  auto right_iter = right_items.begin();
+  while (left_iter != left_items.end() && right_iter != right_items.end()) {
+    if (left_iter->first == kAppInstallTimeKey ||
+        left_iter->first == kAppLastLaunchTimeKey) {
+      ++left_iter;
+      continue;
+    }
+    if (right_iter->first == kAppInstallTimeKey ||
+        right_iter->first == kAppLastLaunchTimeKey) {
+      ++right_iter;
+      continue;
+    }
+    if (*left_iter != *right_iter)
+      return false;
+    ++left_iter;
+    ++right_iter;
+  }
+  return left_iter == left_items.end() && right_iter == right_items.end();
+}
+
 }  // namespace
 
 CrostiniRegistryService::Registration::Registration(
@@ -137,7 +171,9 @@ CrostiniRegistryService::Registration::Registration(
     const std::vector<std::string>& mime_types,
     bool no_display,
     const std::string& startup_wm_class,
-    bool startup_notify)
+    bool startup_notify,
+    base::Time install_time,
+    base::Time last_launch_time)
     : desktop_file_id(desktop_file_id),
       vm_name(vm_name),
       container_name(container_name),
@@ -146,7 +182,9 @@ CrostiniRegistryService::Registration::Registration(
       mime_types(mime_types),
       no_display(no_display),
       startup_wm_class(startup_wm_class),
-      startup_notify(startup_notify) {
+      startup_notify(startup_notify),
+      install_time(install_time),
+      last_launch_time(last_launch_time) {
   DCHECK(name.find(std::string()) != name.end());
 }
 
@@ -169,7 +207,7 @@ const std::string& CrostiniRegistryService::Registration::Localize(
 }
 
 CrostiniRegistryService::CrostiniRegistryService(Profile* profile)
-    : prefs_(profile->GetPrefs()) {}
+    : prefs_(profile->GetPrefs()), clock_(base::DefaultClock::GetInstance()) {}
 
 CrostiniRegistryService::~CrostiniRegistryService() = default;
 
@@ -257,7 +295,8 @@ std::vector<std::string> CrostiniRegistryService::GetRegisteredAppIds() const {
   std::vector<std::string> result;
   for (const auto& item : apps->DictItems())
     result.push_back(item.first);
-
+  if (!apps->FindKey(kCrostiniTerminalId))
+    result.push_back(kCrostiniTerminalId);
   return result;
 }
 
@@ -267,6 +306,18 @@ CrostiniRegistryService::GetRegistration(const std::string& app_id) const {
       prefs_->GetDictionary(kCrostiniRegistryPref);
   const base::Value* pref_registration =
       apps->FindKeyOfType(app_id, base::Value::Type::DICTIONARY);
+
+  if (app_id == kCrostiniTerminalId) {
+    std::map<std::string, std::string> name = {
+        {std::string(), kCrostiniTerminalAppName}};
+    return std::make_unique<Registration>(
+        "", kCrostiniDefaultVmName, kCrostiniDefaultContainerName, name,
+        Registration::LocaleString(), std::vector<std::string>(), false, "",
+        false, base::Time(),
+        pref_registration ? GetTime(*pref_registration, kAppLastLaunchTimeKey)
+                          : base::Time());
+  }
+
   if (!pref_registration)
     return nullptr;
 
@@ -296,7 +347,9 @@ CrostiniRegistryService::GetRegistration(const std::string& app_id) const {
       DictionaryToStringMap(comment), ListToStringVector(mime_types),
       no_display->GetBool(),
       startup_wm_class ? startup_wm_class->GetString() : "",
-      startup_notify && startup_notify->GetBool());
+      startup_notify && startup_notify->GetBool(),
+      GetTime(*pref_registration, kAppInstallTimeKey),
+      GetTime(*pref_registration, kAppLastLaunchTimeKey));
 }
 
 void CrostiniRegistryService::UpdateApplicationList(
@@ -356,18 +409,35 @@ void CrostiniRegistryService::UpdateApplicationList(
                                base::Value(app.startup_notify()));
 
       base::Value* old_app = apps->FindKey(app_id);
-      if (old_app && pref_registration == *old_app)
+      if (old_app && EqualsExcludingTimestamps(pref_registration, *old_app))
         continue;
 
-      if (old_app)
+      base::Value* old_install_time = nullptr;
+      base::Value* old_last_launch_time = nullptr;
+      if (old_app) {
         updated_apps.push_back(app_id);
-      else
+        old_install_time = old_app->FindKey(kAppInstallTimeKey);
+        old_last_launch_time = old_app->FindKey(kAppLastLaunchTimeKey);
+      } else {
         inserted_apps.push_back(app_id);
+      }
+
+      if (old_install_time)
+        pref_registration.SetKey(kAppInstallTimeKey, old_install_time->Clone());
+      else
+        SetCurrentTime(&pref_registration, kAppInstallTimeKey);
+
+      if (old_last_launch_time) {
+        pref_registration.SetKey(kAppLastLaunchTimeKey,
+                                 old_last_launch_time->Clone());
+      }
 
       apps->SetKey(app_id, std::move(pref_registration));
     }
 
     for (const auto& item : apps->DictItems()) {
+      if (item.first == kCrostiniTerminalId)
+        continue;
       if (item.second.FindKey(kAppVmNameKey)->GetString() ==
               app_list.vm_name() &&
           item.second.FindKey(kAppContainerNameKey)->GetString() ==
@@ -381,8 +451,11 @@ void CrostiniRegistryService::UpdateApplicationList(
       apps->RemoveKey(removed_app);
   }
 
-  for (Observer& obs : observers_)
-    obs.OnRegistryUpdated(this, updated_apps, removed_apps, inserted_apps);
+  if (!updated_apps.empty() || !removed_apps.empty() ||
+      !inserted_apps.empty()) {
+    for (Observer& obs : observers_)
+      obs.OnRegistryUpdated(this, updated_apps, removed_apps, inserted_apps);
+  }
 }
 
 void CrostiniRegistryService::AddObserver(Observer* observer) {
@@ -393,6 +466,40 @@ void CrostiniRegistryService::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void CrostiniRegistryService::AppLaunched(const std::string& app_id) {
+  DictionaryPrefUpdate update(prefs_, kCrostiniRegistryPref);
+  base::DictionaryValue* apps = update.Get();
+
+  base::Value* app = apps->FindKey(app_id);
+  if (!app) {
+    DCHECK_EQ(app_id, kCrostiniTerminalId);
+    base::Value pref(base::Value::Type::DICTIONARY);
+    SetCurrentTime(&pref, kAppLastLaunchTimeKey);
+    apps->SetKey(app_id, std::move(pref));
+    return;
+  }
+
+  SetCurrentTime(app, kAppLastLaunchTimeKey);
+}
+
+void CrostiniRegistryService::SetCurrentTime(base::Value* dictionary,
+                                             const char* key) const {
+  DCHECK(dictionary);
+  int64_t time = clock_->Now().ToDeltaSinceWindowsEpoch().InMicroseconds();
+  dictionary->SetKey(key, base::Value(base::Int64ToString(time)));
+}
+
+base::Time CrostiniRegistryService::GetTime(const base::Value& dictionary,
+                                            const char* key) const {
+  const base::Value* value =
+      dictionary.FindKeyOfType(key, base::Value::Type::STRING);
+  int64_t time;
+  if (!value || !base::StringToInt64(value->GetString(), &time))
+    return base::Time();
+  return base::Time::FromDeltaSinceWindowsEpoch(
+      base::TimeDelta::FromMicroseconds(time));
+}
+
 // static
 void CrostiniRegistryService::RegisterProfilePrefs(
     PrefRegistrySimple* registry) {

chrome/browser/chromeos/crostini/crostini_registry_service.h

@@ -18,6 +18,12 @@ class Profile;
 class PrefRegistrySimple;
 class PrefService;
 
+namespace base {
+class Clock;
+class Time;
+class Value;
+}  // namespace base
+
 namespace vm_tools {
 namespace apps {
 class ApplicationList;
@@ -38,8 +44,7 @@ namespace crostini {
 // 2) Crostini App List Ids (app_id):
 //    - Valid extensions ids for apps stored in the registry, derived from the
 //    desktop file id, vm name, and container name.
-//    - The default Terminal app is a special case app list item, without an
-//    entry in the registry.
+//    - The Terminal is a special case, using kCrostiniTerminalId (see below).
 // 3) Exo Window App Ids (window_app_id):
 //    - Retrieved from exo::ShellSurface::GetApplicationId()
 //    - For Wayland apps, this is the surface class of the app
@@ -50,6 +55,11 @@ namespace crostini {
 //    - Used in ash::ShelfID::app_id
 //    - Either a Window App Id prefixed by "crostini:" or a Crostini App Id.
 //    - For pinned apps, this is a Crostini App Id.
+
+// The default Terminal app does not correspond to a desktop file, but users
+// of the registry can treat it as a regular app that is always installed.
+// Internal to the registry, the pref entry only contains the last launch time
+// so some care is required.
 class CrostiniRegistryService : public KeyedService {
  public:
   struct Registration {
@@ -66,7 +76,9 @@ class CrostiniRegistryService : public KeyedService {
                  const std::vector<std::string>& mime_types,
                  bool no_display,
                  const std::string& startup_wm_class,
-                 bool startup_notify);
+                 bool startup_notify,
+                 base::Time install_time,
+                 base::Time last_launch_time);
     ~Registration();
 
     static const std::string& Localize(const LocaleString& locale_string);
@@ -83,13 +95,17 @@ class CrostiniRegistryService : public KeyedService {
     std::string startup_wm_class;
     bool startup_notify;
 
+    base::Time install_time;
+    base::Time last_launch_time;
+
     DISALLOW_COPY_AND_ASSIGN(Registration);
   };
 
   class Observer {
    public:
     // Called at the end of UpdateApplicationList() with lists of app_ids for
-    // apps which have been updated, removed, and inserted.
+    // apps which have been updated, removed, and inserted. Not called when the
+    // last_launch_time field is updated.
     virtual void OnRegistryUpdated(
         CrostiniRegistryService* registry_service,
         const std::vector<std::string>& updated_apps,
@@ -117,10 +133,10 @@ class CrostiniRegistryService : public KeyedService {
   // Returns whether the app_id is a Crostini app id.
   bool IsCrostiniShelfAppId(const std::string& shelf_app_id);
 
+  // Return all installed apps. This always includes the Terminal app.
   std::vector<std::string> GetRegisteredAppIds() const;
 
   // Return null if |app_id| is not found in the registry.
-  // TODO(timloh): We should probably have an entry for the Terminal app.
   std::unique_ptr<CrostiniRegistryService::Registration> GetRegistration(
       const std::string& app_id) const;
 
@@ -130,6 +146,15 @@ class CrostiniRegistryService : public KeyedService {
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // Notify the registry to update the last_launched field.
+  void AppLaunched(const std::string& app_id);
+
+  // Serializes the current time and stores it in |dictionary|.
+  void SetCurrentTime(base::Value* dictionary, const char* key) const;
+  // Deserializes a time from |dictionary|.
+  base::Time GetTime(const base::Value& dictionary, const char* key) const;
+  void SetClockForTesting(base::Clock* clock) { clock_ = clock; }
+
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
  private:
@@ -138,6 +163,8 @@ class CrostiniRegistryService : public KeyedService {
 
   base::ObserverList<Observer> observers_;
 
+  const base::Clock* clock_;
+
   DISALLOW_COPY_AND_ASSIGN(CrostiniRegistryService);
 };
 

chrome/browser/chromeos/crostini/crostini_registry_service_unittest.cc

@@ -7,6 +7,8 @@
 #include <stddef.h>
 
 #include "base/macros.h"
+#include "base/test/simple_test_clock.h"
+#include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chromeos/dbus/vm_applications/apps.pb.h"
 #include "components/crx_file/id_util.h"
@@ -14,6 +16,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using testing::_;
 using vm_tools::apps::App;
 using vm_tools::apps::ApplicationList;
 
@@ -26,6 +29,7 @@ class CrostiniRegistryServiceTest : public testing::Test {
   // testing::Test:
   void SetUp() override {
     service_ = std::make_unique<CrostiniRegistryService>(&profile_);
+    service_->SetClockForTesting(&test_clock_);
   }
 
  protected:
@@ -71,6 +75,9 @@ class CrostiniRegistryServiceTest : public testing::Test {
 
   CrostiniRegistryService* service() { return service_.get(); }
 
+ protected:
+  base::SimpleTestClock test_clock_;
+
  private:
   content::TestBrowserThreadBundle thread_bundle_;
   TestingProfile profile_;
@@ -159,6 +166,53 @@ TEST_F(CrostiniRegistryServiceTest, Observer) {
   service()->UpdateApplicationList(app_list);
 }
 
+TEST_F(CrostiniRegistryServiceTest, InstallAndLaunchTime) {
+  ApplicationList app_list = BasicAppList("app", "vm", "container");
+  std::string app_id = GenerateAppId("app", "vm", "container");
+  test_clock_.Advance(base::TimeDelta::FromHours(1));
+
+  Observer observer;
+  service()->AddObserver(&observer);
+  EXPECT_CALL(observer, OnRegistryUpdated(service(), testing::IsEmpty(),
+                                          testing::IsEmpty(),
+                                          testing::ElementsAre(app_id)));
+  service()->UpdateApplicationList(app_list);
+
+  auto result = service()->GetRegistration(app_id);
+  base::Time install_time = test_clock_.Now();
+  EXPECT_EQ(result->install_time, install_time);
+  EXPECT_EQ(result->last_launch_time, base::Time());
+
+  // UpdateApplicationList with nothing changed. Times shouldn't be updated and
+  // the observer shouldn't fire.
+  test_clock_.Advance(base::TimeDelta::FromHours(1));
+  EXPECT_CALL(observer, OnRegistryUpdated(_, _, _, _)).Times(0);
+  service()->UpdateApplicationList(app_list);
+  result = service()->GetRegistration(app_id);
+  EXPECT_EQ(result->install_time, install_time);
+  EXPECT_EQ(result->last_launch_time, base::Time());
+
+  // Launch the app
+  test_clock_.Advance(base::TimeDelta::FromHours(1));
+  service()->AppLaunched(app_id);
+  result = service()->GetRegistration(app_id);
+  EXPECT_EQ(result->install_time, install_time);
+  EXPECT_EQ(result->last_launch_time,
+            base::Time() + base::TimeDelta::FromHours(3));
+
+  // The install time shouldn't change if fields change.
+  test_clock_.Advance(base::TimeDelta::FromHours(1));
+  app_list.mutable_apps(0)->set_no_display(true);
+  EXPECT_CALL(observer,
+              OnRegistryUpdated(service(), testing::ElementsAre(app_id),
+                                testing::IsEmpty(), testing::IsEmpty()));
+  service()->UpdateApplicationList(app_list);
+  result = service()->GetRegistration(app_id);
+  EXPECT_EQ(result->install_time, install_time);
+  EXPECT_EQ(result->last_launch_time,
+            base::Time() + base::TimeDelta::FromHours(3));
+}
+
 // Test that UpdateApplicationList doesn't clobber apps from different VMs or
 // containers.
 TEST_F(CrostiniRegistryServiceTest, MultipleContainers) {
@@ -170,7 +224,8 @@ TEST_F(CrostiniRegistryServiceTest, MultipleContainers) {
   std::string app_id_3 = GenerateAppId("app", "vm 2", "container 1");
 
   EXPECT_THAT(service()->GetRegisteredAppIds(),
-              testing::UnorderedElementsAre(app_id_1, app_id_2, app_id_3));
+              testing::UnorderedElementsAre(app_id_1, app_id_2, app_id_3,
+                                            kCrostiniTerminalId));
 
   // Clobber app_id_2
   service()->UpdateApplicationList(
@@ -178,7 +233,8 @@ TEST_F(CrostiniRegistryServiceTest, MultipleContainers) {
   std::string new_app_id = GenerateAppId("app 2", "vm 1", "container 2");
 
   EXPECT_THAT(service()->GetRegisteredAppIds(),
-              testing::UnorderedElementsAre(app_id_1, app_id_3, new_app_id));
+              testing::UnorderedElementsAre(app_id_1, app_id_3, new_app_id,
+                                            kCrostiniTerminalId));
 }
 
 TEST_F(CrostiniRegistryServiceTest, GetCrostiniAppIdNoStartupID) {
@@ -189,7 +245,7 @@ TEST_F(CrostiniRegistryServiceTest, GetCrostiniAppIdNoStartupID) {
 
   service()->UpdateApplicationList(BasicAppList("super", "vm 2", "container"));
 
-  EXPECT_THAT(service()->GetRegisteredAppIds(), testing::SizeIs(4));
+  EXPECT_THAT(service()->GetRegisteredAppIds(), testing::SizeIs(5));
 
   EXPECT_EQ(
       service()->GetCrostiniShelfAppId(WindowIdForWMClass("App"), nullptr),
@@ -226,7 +282,7 @@ TEST_F(CrostiniRegistryServiceTest, GetCrostiniAppIdStartupWMClass) {
   app_list.mutable_apps(2)->set_startup_wm_class("app2");
   service()->UpdateApplicationList(app_list);
 
-  EXPECT_THAT(service()->GetRegisteredAppIds(), testing::SizeIs(3));
+  EXPECT_THAT(service()->GetRegisteredAppIds(), testing::SizeIs(4));
 
   EXPECT_EQ(service()->GetCrostiniShelfAppId(WindowIdForWMClass("app_start"),
                                              nullptr),

chrome/browser/chromeos/crostini/crostini_util.cc

@@ -75,6 +75,8 @@ bool IsExperimentalCrostiniUIAvailable() {
 
 void LaunchCrostiniApp(Profile* profile, const std::string& app_id) {
   auto* crostini_manager = crostini::CrostiniManager::GetInstance();
+  crostini::CrostiniRegistryService* registry_service =
+      crostini::CrostiniRegistryServiceFactory::GetForProfile(profile);
 
   if (app_id == kCrostiniTerminalId) {
     RecordAppLaunchHistogram(CrostiniAppLaunchAppType::kTerminal);
@@ -87,11 +89,10 @@ void LaunchCrostiniApp(Profile* profile, const std::string& app_id) {
           profile, kCrostiniDefaultVmName, kCrostiniDefaultContainerName,
           base::BindOnce(&MaybeLaunchTerminal, profile));
     }
+    registry_service->AppLaunched(app_id);
     return;
   }
 
-  crostini::CrostiniRegistryService* registry_service =
-      crostini::CrostiniRegistryServiceFactory::GetForProfile(profile);
   std::unique_ptr<crostini::CrostiniRegistryService::Registration>
       registration = registry_service->GetRegistration(app_id);
   if (!registration) {
@@ -105,4 +106,5 @@ void LaunchCrostiniApp(Profile* profile, const std::string& app_id) {
       profile, registration->vm_name, registration->container_name,
       base::BindOnce(&MaybeLaunchContainerAppplication,
                      std::move(registration)));
+  registry_service->AppLaunched(app_id);
 }

chrome/browser/ui/app_list/crostini/crostini_app_model_builder.cc

@@ -24,12 +24,6 @@ CrostiniAppModelBuilder::~CrostiniAppModelBuilder() {
 }
 
 void CrostiniAppModelBuilder::BuildModel() {
-  InsertApp(std::make_unique<CrostiniAppItem>(
-      profile(), GetSyncItem(kCrostiniTerminalId), kCrostiniTerminalId,
-      kCrostiniTerminalAppName,
-      ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
-          IDR_LOGO_CROSTINI_TERMINAL)));
-
   crostini::CrostiniRegistryService* registry_service =
       crostini::CrostiniRegistryServiceFactory::GetForProfile(profile());
   for (const std::string& app_id : registry_service->GetRegisteredAppIds()) {
@@ -54,7 +48,8 @@ void CrostiniAppModelBuilder::InsertCrostiniAppItem(
   InsertApp(std::make_unique<CrostiniAppItem>(
       profile(), GetSyncItem(app_id), app_id, localized_name,
       ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
-          IDR_LOGO_CROSTINI_DEFAULT)));
+          app_id == kCrostiniTerminalId ? IDR_LOGO_CROSTINI_TERMINAL
+                                        : IDR_LOGO_CROSTINI_DEFAULT)));
 }
 
 void CrostiniAppModelBuilder::OnRegistryUpdated(