Add launch app button to CrostiniPackageNotification
When we finish installing a deb package from the UI, check if any
icons were added by the installation. If exactly one icon was added,
show an "Open App" button that allows the user to launch that app.
Bug: 864766
Change-Id: I8e2f03658a945f71aea5f1d1689b0a0dcfd548ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1652384
Commit-Queue: Fergus Dall <sidereal@google.com>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#673626}
diff --git a/chrome/app/chromeos_strings.grdp b/chrome/app/chromeos_strings.grdp
index b3c6281d..213c2e9 100644
--- a/chrome/app/chromeos_strings.grdp
+++ b/chrome/app/chromeos_strings.grdp
@@ -3630,6 +3630,9 @@
<message name="IDS_CROSTINI_PACKAGE_INSTALL_NOTIFICATION_COMPLETED_MESSAGE" desc="Message in the Notification for Linux package installation once installation has completed successfully.">
App is available in your terminal. There may also be an icon in your Launcher.
</message>
+ <message name="IDS_CROSTINI_PACKAGE_INSTALL_NOTIFICATION_COMPLETED_BUTTON" desc="Button text in the Notification for Linux package installation once installation has completed successfully.">
+ Launch
+ </message>
<message name="IDS_CROSTINI_PACKAGE_INSTALL_NOTIFICATION_ERROR_TITLE" desc="Title of the Notification for Linux package installation when there is an error during installation.">
Error while installing
</message>
diff --git a/chrome/app/chromeos_strings_grdp/IDS_CROSTINI_PACKAGE_INSTALL_NOTIFICATION_COMPLETED_BUTTON.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_CROSTINI_PACKAGE_INSTALL_NOTIFICATION_COMPLETED_BUTTON.png.sha1
new file mode 100644
index 0000000..80d4cbd
--- /dev/null
+++ b/chrome/app/chromeos_strings_grdp/IDS_CROSTINI_PACKAGE_INSTALL_NOTIFICATION_COMPLETED_BUTTON.png.sha1
@@ -0,0 +1 @@
+ef895f81021fa52f1c79f3a94127650343634620
\ No newline at end of file
diff --git a/chrome/browser/chromeos/BUILD.gn b/chrome/browser/chromeos/BUILD.gn
index 5d56a93d..df75a74 100644
--- a/chrome/browser/chromeos/BUILD.gn
+++ b/chrome/browser/chromeos/BUILD.gn
@@ -2369,6 +2369,7 @@
"crostini/crostini_export_import_unittest.cc",
"crostini/crostini_manager_unittest.cc",
"crostini/crostini_mime_types_service_unittest.cc",
+ "crostini/crostini_package_notification_unittest.cc",
"crostini/crostini_package_service_unittest.cc",
"crostini/crostini_registry_service_unittest.cc",
"crostini/crostini_reporting_util_unittest.cc",
diff --git a/chrome/browser/chromeos/crostini/crostini_package_notification.cc b/chrome/browser/chromeos/crostini/crostini_package_notification.cc
index c52bf6b..19b2f229 100644
--- a/chrome/browser/chromeos/crostini/crostini_package_notification.cc
+++ b/chrome/browser/chromeos/crostini/crostini_package_notification.cc
@@ -7,9 +7,13 @@
#include "ash/public/cpp/notification_utils.h"
#include "ash/public/cpp/vector_icons/vector_icons.h"
#include "chrome/browser/chromeos/crostini/crostini_package_service.h"
+#include "chrome/browser/chromeos/crostini/crostini_registry_service_factory.h"
#include "chrome/browser/notifications/notification_display_service.h"
+#include "chrome/browser/ui/app_list/app_list_client_impl.h"
#include "chrome/grit/generated_resources.h"
#include "ui/base/l10n/l10n_util.h"
+#include "ui/display/display.h"
+#include "ui/display/screen.h"
#include "ui/message_center/public/cpp/message_center_constants.h"
#include "ui/message_center/public/cpp/notification.h"
#include "ui/message_center/public/cpp/notification_delegate.h"
@@ -23,6 +27,10 @@
} // namespace
+int CrostiniPackageNotification::GetButtonCountForTesting() {
+ return notification_->buttons().size();
+}
+
CrostiniPackageNotification::NotificationSettings::NotificationSettings() {}
CrostiniPackageNotification::NotificationSettings::NotificationSettings(
const NotificationSettings& rhs) = default;
@@ -32,6 +40,7 @@
Profile* profile,
NotificationType notification_type,
PackageOperationStatus status,
+ const ContainerId& container_id,
const base::string16& app_name,
const std::string& notification_id,
CrostiniPackageService* package_service)
@@ -43,9 +52,11 @@
GetNotificationSettingsForTypeAndAppName(notification_type,
app_name)),
visible_(true),
+ container_id_(container_id),
weak_ptr_factory_(this) {
if (status == PackageOperationStatus::RUNNING) {
running_start_time_ = base::TimeTicks::Now();
+ CrostiniRegistryServiceFactory::GetForProfile(profile_)->AddObserver(this);
}
message_center::RichNotificationData rich_notification_data;
rich_notification_data.vector_small_image = &ash::kNotificationLinuxIcon;
@@ -68,12 +79,22 @@
UpdateProgress(status, 0 /*progress_percent*/);
}
-CrostiniPackageNotification::~CrostiniPackageNotification() = default;
+CrostiniPackageNotification::~CrostiniPackageNotification() {
+ CrostiniRegistryServiceFactory::GetForProfile(profile_)->RemoveObserver(this);
+}
PackageOperationStatus CrostiniPackageNotification::GetOperationStatus() const {
return current_status_;
}
+void CrostiniPackageNotification::OnRegistryUpdated(
+ CrostiniRegistryService* registry_service,
+ const std::vector<std::string>& updated_apps,
+ const std::vector<std::string>& removed_apps,
+ const std::vector<std::string>& inserted_apps) {
+ inserted_apps_.insert(inserted_apps.begin(), inserted_apps.end());
+}
+
// static
CrostiniPackageNotification::NotificationSettings
CrostiniPackageNotification::GetNotificationSettingsForTypeAndAppName(
@@ -136,19 +157,48 @@
if (status == PackageOperationStatus::RUNNING &&
current_status_ != PackageOperationStatus::RUNNING) {
running_start_time_ = base::TimeTicks::Now();
+ CrostiniRegistryServiceFactory::GetForProfile(profile_)->AddObserver(this);
}
current_status_ = status;
base::string16 title;
base::string16 body;
+ std::vector<message_center::ButtonInfo> buttons;
message_center::NotificationType notification_type =
message_center::NOTIFICATION_TYPE_SIMPLE;
bool never_timeout = false;
+ app_count_ = 0;
+ CrostiniRegistryService* crostini_registry_service =
+ CrostiniRegistryServiceFactory::GetForProfile(profile_);
switch (status) {
case PackageOperationStatus::SUCCEEDED:
title = notification_settings_.success_title;
body = notification_settings_.success_body;
+
+ if (notification_type_ == NotificationType::PACKAGE_INSTALL) {
+ // Try and match up launcher icons with the install we just finished. We
+ // don't have a perfect solution to this, but under normal circumstances
+ // we shouldn't see icons appearing during an install that aren't
+ // because of that install.
+ for (const std::string& app_id : inserted_apps_) {
+ auto registration =
+ crostini_registry_service->GetRegistration(app_id);
+ if (registration.has_value() &&
+ registration->VmName() == container_id_.first &&
+ registration->ContainerName() == container_id_.second) {
+ app_id_ = app_id;
+ app_count_++;
+ }
+ }
+ if (app_count_ == 1) {
+ buttons.push_back(
+ message_center::ButtonInfo(l10n_util::GetStringUTF16(
+ IDS_CROSTINI_PACKAGE_INSTALL_NOTIFICATION_COMPLETED_BUTTON)));
+ }
+ }
+ crostini_registry_service->RemoveObserver(this);
+
break;
case PackageOperationStatus::FAILED:
@@ -194,6 +244,7 @@
notification_->set_title(title);
notification_->set_message(body);
+ notification_->set_buttons(buttons);
notification_->set_type(notification_type);
notification_->set_progress(progress_percent);
notification_->set_never_timeout(never_timeout);
@@ -217,6 +268,21 @@
}
}
+void CrostiniPackageNotification::Click(
+ const base::Optional<int>& button_index,
+ const base::Optional<base::string16>& reply) {
+ if (app_count_ == 0) {
+ LaunchCrostiniApp(profile_, kCrostiniTerminalId,
+ display::Screen::GetScreen()->GetPrimaryDisplay().id());
+ } else if (app_count_ == 1) {
+ DCHECK(!app_id_.empty());
+ LaunchCrostiniApp(profile_, app_id_,
+ display::Screen::GetScreen()->GetPrimaryDisplay().id());
+ } else {
+ AppListClientImpl::GetInstance()->ShowAppList();
+ }
+}
+
void CrostiniPackageNotification::UpdateDisplayedNotification() {
if (current_status_ == PackageOperationStatus::SUCCEEDED ||
current_status_ == PackageOperationStatus::FAILED) {
diff --git a/chrome/browser/chromeos/crostini/crostini_package_notification.h b/chrome/browser/chromeos/crostini/crostini_package_notification.h
index 7675643..f5535ed 100644
--- a/chrome/browser/chromeos/crostini/crostini_package_notification.h
+++ b/chrome/browser/chromeos/crostini/crostini_package_notification.h
@@ -13,6 +13,7 @@
#include "base/time/time.h"
#include "chrome/browser/chromeos/crostini/crostini_manager.h"
#include "chrome/browser/chromeos/crostini/crostini_package_operation_status.h"
+#include "chrome/browser/chromeos/crostini/crostini_registry_service.h"
#include "ui/message_center/public/cpp/notification_delegate.h"
namespace message_center {
@@ -25,8 +26,8 @@
// Notification for various Crostini package operations, such as installing
// from a package or uninstalling an existing app.
-class CrostiniPackageNotification
- : public message_center::NotificationObserver {
+class CrostiniPackageNotification : public message_center::NotificationObserver,
+ public CrostiniRegistryService::Observer {
public:
enum class NotificationType { PACKAGE_INSTALL, APPLICATION_UNINSTALL };
@@ -35,10 +36,11 @@
CrostiniPackageNotification(Profile* profile,
NotificationType notification_type,
PackageOperationStatus status,
+ const ContainerId& container_id,
const base::string16& app_name,
const std::string& notification_id,
CrostiniPackageService* installer_service);
- virtual ~CrostiniPackageNotification();
+ ~CrostiniPackageNotification() override;
void UpdateProgress(PackageOperationStatus status, int progress_percent);
@@ -49,6 +51,18 @@
// message_center::NotificationObserver:
void Close(bool by_user) override;
+ void Click(const base::Optional<int>& button_index,
+ const base::Optional<base::string16>& reply) override;
+
+ // CrostiniRegistryService::Observer:
+ void OnRegistryUpdated(
+ CrostiniRegistryService* registry_service,
+ const std::vector<std::string>& updated_apps,
+ const std::vector<std::string>& removed_apps,
+ const std::vector<std::string>& inserted_apps) override;
+
+ int GetButtonCountForTesting();
+
private:
// A type giving the string, etc displayed for each notification type. Note
// that we have the complete strings here, not just the string IDs, because
@@ -93,6 +107,15 @@
// True if we think the notification is visible.
bool visible_;
+ // If we show a launch button on completion, this is the app that will be
+ // launched.
+ std::string app_id_;
+
+ ContainerId container_id_;
+
+ std::set<std::string> inserted_apps_;
+ int app_count_ = 0;
+
base::WeakPtrFactory<CrostiniPackageNotification> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(CrostiniPackageNotification);
diff --git a/chrome/browser/chromeos/crostini/crostini_package_notification_unittest.cc b/chrome/browser/chromeos/crostini/crostini_package_notification_unittest.cc
new file mode 100644
index 0000000..aca7e430
--- /dev/null
+++ b/chrome/browser/chromeos/crostini/crostini_package_notification_unittest.cc
@@ -0,0 +1,134 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/crostini/crostini_package_notification.h"
+
+#include <memory>
+#include <string>
+
+#include "base/strings/string16.h"
+#include "chrome/browser/chromeos/crostini/crostini_package_service.h"
+#include "chrome/browser/chromeos/crostini/crostini_registry_service.h"
+#include "chrome/browser/chromeos/crostini/crostini_registry_service_factory.h"
+#include "chrome/browser/chromeos/crostini/crostini_test_helper.h"
+#include "chrome/browser/chromeos/crostini/crostini_util.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace crostini {
+
+namespace {
+
+using ::chromeos::DBusThreadManager;
+
+constexpr char kDefaultAppFileId[] = "default_file_id";
+constexpr char kSecondAppFileId[] = "default_file_id2";
+constexpr char kNotificationId[] = "notification_id";
+
+class CrostiniPackageNotificationTest : public testing::Test {
+ public:
+ CrostiniPackageNotificationTest() {}
+
+ void SetUp() override {
+ DBusThreadManager::Initialize();
+ test_browser_thread_bundle_ =
+ std::make_unique<content::TestBrowserThreadBundle>(
+ base::test::ScopedTaskEnvironment::MainThreadType::UI,
+ base::test::ScopedTaskEnvironment::ThreadPoolExecutionMode::ASYNC,
+ content::TestBrowserThreadBundle::REAL_IO_THREAD);
+
+ profile_ = std::make_unique<TestingProfile>();
+ crostini_test_helper_ =
+ std::make_unique<CrostiniTestHelper>(profile_.get());
+ service_ = std::make_unique<CrostiniPackageService>(profile_.get());
+ }
+
+ void TearDown() override {
+ service_.reset();
+ crostini_test_helper_.reset();
+ profile_.reset();
+ test_browser_thread_bundle_.reset();
+ DBusThreadManager::Shutdown();
+ }
+
+ protected:
+ std::unique_ptr<content::TestBrowserThreadBundle> test_browser_thread_bundle_;
+ std::unique_ptr<CrostiniTestHelper> crostini_test_helper_;
+ std::unique_ptr<CrostiniPackageService> service_;
+ std::unique_ptr<TestingProfile> profile_;
+};
+
+TEST_F(CrostiniPackageNotificationTest, InstallWithNoIcons) {
+ CrostiniPackageNotification notification(
+ profile_.get(),
+ CrostiniPackageNotification::NotificationType::PACKAGE_INSTALL,
+ PackageOperationStatus::RUNNING,
+ std::pair<std::string, std::string>(kCrostiniDefaultVmName,
+ kCrostiniDefaultContainerName),
+ base::string16(), kNotificationId, service_.get());
+
+ notification.UpdateProgress(PackageOperationStatus::SUCCEEDED, 100);
+ EXPECT_EQ(notification.GetButtonCountForTesting(), 0);
+}
+
+TEST_F(CrostiniPackageNotificationTest, InstallWithOneIcon) {
+ CrostiniPackageNotification notification(
+ profile_.get(),
+ CrostiniPackageNotification::NotificationType::PACKAGE_INSTALL,
+ PackageOperationStatus::RUNNING,
+ std::pair<std::string, std::string>(kCrostiniDefaultVmName,
+ kCrostiniDefaultContainerName),
+ base::string16(), kNotificationId, service_.get());
+
+ auto app = CrostiniTestHelper::BasicApp(kDefaultAppFileId);
+ crostini_test_helper_->AddApp(app);
+
+ notification.UpdateProgress(PackageOperationStatus::SUCCEEDED, 100);
+ EXPECT_EQ(notification.GetButtonCountForTesting(), 1);
+}
+
+TEST_F(CrostiniPackageNotificationTest, InstallWithTwoIcons) {
+ CrostiniPackageNotification notification(
+ profile_.get(),
+ CrostiniPackageNotification::NotificationType::PACKAGE_INSTALL,
+ PackageOperationStatus::RUNNING,
+ std::pair<std::string, std::string>(kCrostiniDefaultVmName,
+ kCrostiniDefaultContainerName),
+ base::string16(), kNotificationId, service_.get());
+
+ auto app = CrostiniTestHelper::BasicApp(kDefaultAppFileId);
+ crostini_test_helper_->AddApp(app);
+
+ app = CrostiniTestHelper::BasicApp(kSecondAppFileId);
+ crostini_test_helper_->AddApp(app);
+
+ notification.UpdateProgress(PackageOperationStatus::SUCCEEDED, 100);
+ EXPECT_EQ(notification.GetButtonCountForTesting(), 0);
+}
+
+TEST_F(CrostiniPackageNotificationTest, InstallIgnorePreviousIcons) {
+ auto app = CrostiniTestHelper::BasicApp(kDefaultAppFileId);
+ crostini_test_helper_->AddApp(app);
+
+ CrostiniPackageNotification notification(
+ profile_.get(),
+ CrostiniPackageNotification::NotificationType::PACKAGE_INSTALL,
+ PackageOperationStatus::RUNNING,
+ std::pair<std::string, std::string>(kCrostiniDefaultVmName,
+ kCrostiniDefaultContainerName),
+ base::string16(), kNotificationId, service_.get());
+
+ app = CrostiniTestHelper::BasicApp(kSecondAppFileId);
+ crostini_test_helper_->AddApp(app);
+
+ notification.UpdateProgress(PackageOperationStatus::SUCCEEDED, 100);
+ EXPECT_EQ(notification.GetButtonCountForTesting(), 1);
+}
+
+} // namespace
+
+} // namespace crostini
diff --git a/chrome/browser/chromeos/crostini/crostini_package_service.cc b/chrome/browser/chromeos/crostini/crostini_package_service.cc
index 3d1ade5..e3833c9 100644
--- a/chrome/browser/chromeos/crostini/crostini_package_service.cc
+++ b/chrome/browser/chromeos/crostini/crostini_package_service.cc
@@ -119,6 +119,13 @@
manager->RemoveLinuxPackageOperationProgressObserver(this);
manager->RemovePendingAppListUpdatesObserver(this);
manager->RemoveVmShutdownObserver(this);
+
+ // CrostiniPackageNotification registers itself as a CrostiniRegistryService
+ // observer, so they need to be destroyed here while the
+ // CrostiniRegistryService still exists.
+ running_notifications_.clear();
+ queued_uninstalls_.clear();
+ finished_notifications_.clear();
}
void CrostiniPackageService::SetNotificationStateChangeCallbackForTesting(
@@ -276,7 +283,8 @@
running_notifications_[container_id] =
std::make_unique<CrostiniPackageNotification>(
profile_, notification_type, PackageOperationStatus::RUNNING,
- base::UTF8ToUTF16(app_name), GetUniqueNotificationId(), this);
+ container_id, base::UTF8ToUTF16(app_name), GetUniqueNotificationId(),
+ this);
}
void CrostiniPackageService::CreateQueuedUninstall(
@@ -288,8 +296,8 @@
std::make_unique<CrostiniPackageNotification>(
profile_,
CrostiniPackageNotification::NotificationType::APPLICATION_UNINSTALL,
- PackageOperationStatus::QUEUED, base::UTF8ToUTF16(app_name),
- GetUniqueNotificationId(), this));
+ PackageOperationStatus::QUEUED, container_id,
+ base::UTF8ToUTF16(app_name), GetUniqueNotificationId(), this));
}
void CrostiniPackageService::UpdatePackageOperationStatus(
