Add support for more fields in the Crostini App Registry

This patch extends the Crostini App Registry to record:
- Localized names
- Localized comments
- Mime types
- VM/Container name

The launcher items are updated to use localized names, and the concierge
call now passes the actual VM and container names. We also change the
"app_id" we use for Crostini apps to also include these names, which
should allow us to have multiple icons for an app installed in multiple
containers.

Bug: 821662
Change-Id: I47e4da783c987a026b09decc6a67612fff3f55de
Reviewed-on: https://chromium-review.googlesource.com/995115
Commit-Queue: Timothy Loh <timloh@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548346}

chrome/browser/chromeos/crostini/crostini_registry_service.cc

@@ -5,12 +5,14 @@
 #include "chrome/browser/chromeos/crostini/crostini_registry_service.h"
 
 #include "base/values.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chromeos/dbus/vm_applications/apps.pb.h"
 #include "components/crx_file/id_util.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
+#include "ui/base/l10n/l10n_util.h"
 
 using vm_tools::apps::App;
 
@@ -20,21 +22,103 @@ namespace {
 
 constexpr char kCrostiniRegistryPref[] = "crostini.registry";
 
+// Keys for the Dictionary stored in prefs for each app.
 constexpr char kAppDesktopFileIdKey[] = "desktop_file_id";
+constexpr char kAppVmNameKey[] = "vm_name";
+constexpr char kAppContainerNameKey[] = "container_name";
+constexpr char kAppCommentKey[] = "comment";
+constexpr char kAppMimeTypesKey[] = "mime_types";
 constexpr char kAppNameKey[] = "name";
+constexpr char kAppNoDisplayKey[] = "no_display";
 
 constexpr char kCrostiniAppIdPrefix[] = "crostini:";
 
-std::string GenerateAppId(const std::string& desktop_file_id) {
-  return crx_file::id_util::GenerateId(kCrostiniAppIdPrefix + desktop_file_id);
+std::string GenerateAppId(const std::string& desktop_file_id,
+                          const std::string& vm_name,
+                          const std::string& container_name) {
+  // These can collide in theory because the user could choose VM and container
+  // names which contain slashes, but this will only result in apps missing from
+  // the launcher.
+  return crx_file::id_util::GenerateId(kCrostiniAppIdPrefix + vm_name + "/" +
+                                       container_name + "/" + desktop_file_id);
+}
+
+std::map<std::string, std::string> DictionaryToStringMap(
+    const base::Value* value) {
+  std::map<std::string, std::string> result;
+  for (const auto& item : value->DictItems())
+    result[item.first] = item.second.GetString();
+  return result;
+}
+
+base::Value ProtoToDictionary(const App::LocaleString& locale_string) {
+  base::Value result(base::Value::Type::DICTIONARY);
+  for (const App::LocaleString::Entry& entry : locale_string.values()) {
+    const std::string& locale = entry.locale();
+
+    std::string locale_with_dashes(locale);
+    std::replace(locale_with_dashes.begin(), locale_with_dashes.end(), '_',
+                 '-');
+    if (!locale.empty() && !l10n_util::IsValidLocaleSyntax(locale_with_dashes))
+      continue;
+
+    result.SetKey(locale, base::Value(entry.value()));
+  }
+  return result;
+}
+
+std::vector<std::string> ListToStringVector(const base::Value* list) {
+  std::vector<std::string> result;
+  for (const base::Value& value : list->GetList())
+    result.emplace_back(value.GetString());
+  return result;
+}
+
+base::Value ProtoToList(
+    const google::protobuf::RepeatedPtrField<std::string>& strings) {
+  base::Value result(base::Value::Type::LIST);
+  for (const std::string& string : strings)
+    result.GetList().emplace_back(string);
+  return result;
 }
 
 }  // namespace
 
 CrostiniRegistryService::Registration::Registration(
     const std::string& desktop_file_id,
-    const std::string& name)
-    : desktop_file_id(desktop_file_id), name(name) {}
+    const std::string& vm_name,
+    const std::string& container_name,
+    const LocaleString& name,
+    const LocaleString& comment,
+    const std::vector<std::string>& mime_types,
+    bool no_display)
+    : desktop_file_id(desktop_file_id),
+      vm_name(vm_name),
+      container_name(container_name),
+      name(name),
+      comment(comment),
+      mime_types(mime_types),
+      no_display(no_display) {
+  DCHECK(name.find(std::string()) != name.end());
+}
+
+CrostiniRegistryService::Registration::~Registration() = default;
+
+// static
+const std::string& CrostiniRegistryService::Registration::Localize(
+    const LocaleString& locale_string) {
+  std::string current_locale =
+      l10n_util::NormalizeLocale(g_browser_process->GetApplicationLocale());
+  std::vector<std::string> locales;
+  l10n_util::GetParentLocales(current_locale, &locales);
+
+  for (const std::string& locale : locales) {
+    LocaleString::const_iterator it = locale_string.find(locale);
+    if (it != locale_string.end())
+      return it->second;
+  }
+  return locale_string.at(std::string());
+}
 
 CrostiniRegistryService::CrostiniRegistryService(Profile* profile)
     : prefs_(profile->GetPrefs()) {}
@@ -63,15 +147,38 @@ CrostiniRegistryService::GetRegistration(const std::string& app_id) const {
 
   const base::Value* desktop_file_id = pref_registration->FindKeyOfType(
       kAppDesktopFileIdKey, base::Value::Type::STRING);
-  const base::Value* name =
-      pref_registration->FindKeyOfType(kAppNameKey, base::Value::Type::STRING);
-
-  return std::make_unique<Registration>(desktop_file_id->GetString(),
-                                        name->GetString());
+  const base::Value* vm_name = pref_registration->FindKeyOfType(
+      kAppVmNameKey, base::Value::Type::STRING);
+  const base::Value* container_name = pref_registration->FindKeyOfType(
+      kAppContainerNameKey, base::Value::Type::STRING);
+
+  const base::Value* name = pref_registration->FindKeyOfType(
+      kAppNameKey, base::Value::Type::DICTIONARY);
+  const base::Value* comment = pref_registration->FindKeyOfType(
+      kAppCommentKey, base::Value::Type::DICTIONARY);
+  const base::Value* mime_types = pref_registration->FindKeyOfType(
+      kAppMimeTypesKey, base::Value::Type::LIST);
+  const base::Value* no_display = pref_registration->FindKeyOfType(
+      kAppNoDisplayKey, base::Value::Type::BOOLEAN);
+
+  return std::make_unique<Registration>(
+      desktop_file_id->GetString(), vm_name->GetString(),
+      container_name->GetString(), DictionaryToStringMap(name),
+      DictionaryToStringMap(comment), ListToStringVector(mime_types),
+      no_display->GetBool());
 }
 
 void CrostiniRegistryService::UpdateApplicationList(
     const vm_tools::apps::ApplicationList& app_list) {
+  if (app_list.vm_name().empty()) {
+    LOG(WARNING) << "Received app list with missing VM name";
+    return;
+  }
+  if (app_list.container_name().empty()) {
+    LOG(WARNING) << "Received app list with missing container name";
+    return;
+  }
+
   DictionaryPrefUpdate update(prefs_, kCrostiniRegistryPref);
   base::DictionaryValue* apps = update.Get();
   apps->Clear();
@@ -82,14 +189,8 @@ void CrostiniRegistryService::UpdateApplicationList(
       continue;
     }
 
-    std::string default_name;
-    for (const App::LocaleString::Entry& localized_name : app.name().values()) {
-      if (localized_name.locale().empty()) {
-        default_name = localized_name.value();
-        break;
-      }
-    }
-    if (default_name.empty()) {
+    base::Value name = ProtoToDictionary(app.name());
+    if (name.FindKey(base::StringPiece()) == nullptr) {
       LOG(WARNING) << "Received app '" << app.desktop_file_id()
                    << "' with missing unlocalized name";
       continue;
@@ -98,8 +199,15 @@ void CrostiniRegistryService::UpdateApplicationList(
     base::Value pref_registration(base::Value::Type::DICTIONARY);
     pref_registration.SetKey(kAppDesktopFileIdKey,
                              base::Value(app.desktop_file_id()));
-    pref_registration.SetKey(kAppNameKey, base::Value(default_name));
-    apps->SetKey(GenerateAppId(app.desktop_file_id()),
+    pref_registration.SetKey(kAppVmNameKey, base::Value(app_list.vm_name()));
+    pref_registration.SetKey(kAppContainerNameKey,
+                             base::Value(app_list.container_name()));
+    pref_registration.SetKey(kAppNameKey, std::move(name));
+    pref_registration.SetKey(kAppCommentKey, ProtoToDictionary(app.comment()));
+    pref_registration.SetKey(kAppMimeTypesKey, ProtoToList(app.mime_types()));
+    pref_registration.SetKey(kAppNoDisplayKey, base::Value(app.no_display()));
+    apps->SetKey(GenerateAppId(app.desktop_file_id(), app_list.vm_name(),
+                               app_list.container_name()),
                  std::move(pref_registration));
   }
 }

chrome/browser/chromeos/crostini/crostini_registry_service.h

@@ -5,8 +5,10 @@
 #ifndef CHROME_BROWSER_CHROMEOS_CROSTINI_CROSTINI_REGISTRY_SERVICE_H_
 #define CHROME_BROWSER_CHROMEOS_CROSTINI_CROSTINI_REGISTRY_SERVICE_H_
 
+#include <map>
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "components/keyed_service/core/keyed_service.h"
@@ -31,16 +33,31 @@ namespace chromeos {
 class CrostiniRegistryService : public KeyedService {
  public:
   struct Registration {
-    Registration(const std::string& desktop_file_id, const std::string& name);
-    ~Registration() = default;
+    // Maps from locale to localized string, where the default string is always
+    // present with an empty string key. Locales strings are formatted with
+    // underscores and not hyphens (e.g. 'fr', 'en_US').
+    using LocaleString = std::map<std::string, std::string>;
+
+    Registration(const std::string& desktop_file_id,
+                 const std::string& vm_name,
+                 const std::string& container_name,
+                 const LocaleString& name,
+                 const LocaleString& comment,
+                 const std::vector<std::string>& mime_types,
+                 bool no_display);
+    ~Registration();
+
+    static const std::string& Localize(const LocaleString& locale_string);
 
     std::string desktop_file_id;
-
-    // TODO(timloh): Add other relevant fields from the Desktop Entry Spec, in
-    // particular: Icon, Comment, MimeType, NoDisplay
-    // TODO(timloh): .desktop files allow localization of this string. We need
-    // to expand this to support those too.
-    std::string name;
+    std::string vm_name;
+    std::string container_name;
+
+    // TODO(timloh): Support icons.
+    LocaleString name;
+    LocaleString comment;
+    std::vector<std::string> mime_types;
+    bool no_display;
 
     DISALLOW_COPY_AND_ASSIGN(Registration);
   };

chrome/browser/chromeos/crostini/crostini_registry_service_unittest.cc

@@ -28,8 +28,11 @@ class CrostiniRegistryServiceTest : public testing::Test {
   }
 
  protected:
-  std::string GenerateAppId(const std::string& desktop_file_id) {
-    return crx_file::id_util::GenerateId("crostini:" + desktop_file_id);
+  std::string GenerateAppId(const std::string& desktop_file_id,
+                            const std::string& vm_name,
+                            const std::string& container_name) {
+    return crx_file::id_util::GenerateId(
+        "crostini:" + vm_name + "/" + container_name + "/" + desktop_file_id);
   }
 
   CrostiniRegistryService* service() { return service_.get(); }
@@ -45,23 +48,52 @@ class CrostiniRegistryServiceTest : public testing::Test {
 
 TEST_F(CrostiniRegistryServiceTest, SetAndGetRegistration) {
   std::string desktop_file_id = "vim";
-  std::string name = "Vim";
-  std::string app_id = GenerateAppId(desktop_file_id);
+  std::string vm_name = "awesomevm";
+  std::string container_name = "awesomecontainer";
+  std::map<std::string, std::string> name = {{"", "Vim"}};
+  std::map<std::string, std::string> comment = {
+      {"", "Edit text files"},
+      {"en_GB", "Modify files containing textual content"},
+  };
+  std::vector<std::string> mime_types = {"text/plain", "text/x-python"};
+  bool no_display = true;
+
+  std::string app_id = GenerateAppId(desktop_file_id, vm_name, container_name);
   EXPECT_EQ(nullptr, service()->GetRegistration(app_id));
 
   ApplicationList app_list;
+  app_list.set_vm_name(vm_name);
+  app_list.set_container_name(container_name);
+
   App* app = app_list.add_apps();
   app->set_desktop_file_id(desktop_file_id);
-  App::LocaleString::Entry* name_with_locale =
-      app->mutable_name()->add_values();
-  name_with_locale->set_locale("");
-  name_with_locale->set_value(name);
+  app->set_no_display(no_display);
+
+  for (const auto& localized_name : name) {
+    App::LocaleString::Entry* entry = app->mutable_name()->add_values();
+    entry->set_locale(localized_name.first);
+    entry->set_value(localized_name.second);
+  }
+
+  for (const auto& localized_comment : comment) {
+    App::LocaleString::Entry* entry = app->mutable_comment()->add_values();
+    entry->set_locale(localized_comment.first);
+    entry->set_value(localized_comment.second);
+  }
+
+  for (const std::string& mime_type : mime_types)
+    app->add_mime_types(mime_type);
 
   service()->UpdateApplicationList(app_list);
   auto result = service()->GetRegistration(app_id);
   ASSERT_NE(nullptr, result);
   EXPECT_EQ(result->desktop_file_id, desktop_file_id);
+  EXPECT_EQ(result->vm_name, vm_name);
+  EXPECT_EQ(result->container_name, container_name);
   EXPECT_EQ(result->name, name);
+  EXPECT_EQ(result->comment, comment);
+  EXPECT_EQ(result->mime_types, mime_types);
+  EXPECT_EQ(result->no_display, no_display);
 }
 
 }  // namespace chromeos

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

@@ -43,11 +43,11 @@ void CrostiniAppItem::Activate(int event_flags) {
   std::unique_ptr<chromeos::CrostiniRegistryService::Registration>
       registration = registry_service->GetRegistration(id());
   if (registration) {
-    // TODO(timloh): Store the VM and container names in the registration
     // TODO(timloh): Do something if launching failed, as otherwise the app
     // launcher remains open and there's no feedback.
     crostini::CrostiniManager::GetInstance()->LaunchContainerApplication(
-        "termina", "penguin", registration->desktop_file_id,
+        registration->vm_name, registration->container_name,
+        registration->desktop_file_id,
         base::BindOnce([](crostini::ConciergeClientResult result) {}));
     return;
   }

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

@@ -38,9 +38,14 @@ void CrostiniAppModelBuilder::BuildModel() {
     std::unique_ptr<chromeos::CrostiniRegistryService::Registration>
         registration = registry_service->GetRegistration(app_id);
     DCHECK(registration);
-    // TODO(timloh): Use a real icon and a localized name
+    if (registration->no_display)
+      continue;
+    const std::string& localized_name =
+        chromeos::CrostiniRegistryService::Registration::Localize(
+            registration->name);
+    // TODO(timloh): Use a real icon
     InsertApp(std::make_unique<CrostiniAppItem>(
-        profile(), GetSyncItem(app_id), app_id, registration->name,
+        profile(), GetSyncItem(app_id), app_id, localized_name,
         ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
             IDR_LOGO_CROSTINI_TERMINAL)));
   }