Skip to content

GitLab

T
tangled
Commit c90764f4 authored by Julian Watson's avatar Julian Watson Committed by Commit Bot
Browse files
Options

crostini: decouple lifetime of export import controller and notification 

The lifetime of CrostiniExportImportNotificationController used to be
1to1 with the lifetime of the NotificationUI. This is unnecessary and
makes it harder to reuse this functionality more broadly (see
https://chromium-review.googlesource.com/c/chromium/src/+/1985621/3 for
an example which has very complicated lifetime management due to this).

This CL separates the Controller's lifetime from the UI's lifetime by
extracting the Controller's UI event handling logic (clicking/closing)
into a delegate which the controller creates, such that its lifetime
is 1to1 with the UI, thus allowing the Controller to be destroyed at
anytime without affecting the behaviour of the UI.

Bug: 1024693
Change-Id: I6c67016f862e80dcd1c92a12d507f484c28fe4de
tbr: stevenjb@chromium.org
tbr: tengs@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1991060
Commit-Queue: Julian Watson <juwa@google.com>
Reviewed-by: default avatarDavid Munro <davidmunro@google.com>
Cr-Commit-Position: refs/heads/master@{#731788}
parent 9fe1b4d7
Expand all Show whitespace changes
Inline Side-by-side
Showing with 433 additions and 313 deletions
chrome/browser/chromeos/crostini/crostini_export_import.cc
View file @ c90764f4
......@@ -106,8 +106,15 @@ CrostiniExportImport::OperationData* CrostiniExportImport::NewOperationData(
CrostiniExportImport::OperationData* CrostiniExportImport::NewOperationData(
ExportImportType type,
ContainerId container_id) {
TrackerFactory factory =
base::BindOnce(&CrostiniExportImportNotificationController::Create,
TrackerFactory factory = base::BindOnce(
[](Profile* profile, ContainerId container_id,
std::string notification_id, ExportImportType type,
base::FilePath path)
-> std::unique_ptr<CrostiniExportImportStatusTracker> {
return std::make_unique<CrostiniExportImportNotificationController>(
profile, type, std::move(notification_id), std::move(path),
std::move(container_id));
},
profile_, container_id, GetUniqueNotificationId());
return NewOperationData(type, std::move(container_id), std::move(factory));
}
......@@ -205,7 +212,7 @@ void CrostiniExportImport::Start(
return std::move(callback).Run(CrostiniResult::NOT_ALLOWED);
}
auto* status_tracker = std::move(operation_data->tracker_factory)
auto status_tracker = std::move(operation_data->tracker_factory)
.Run(operation_data->type, path);
auto it = status_trackers_.find(operation_data->container_id);
......@@ -218,7 +225,7 @@ void CrostiniExportImport::Start(
return;
} else {
status_trackers_.emplace_hint(it, operation_data->container_id,
status_tracker);
std::move(status_tracker));
for (auto& observer : observers_) {
observer.OnCrostiniExportImportOperationStatusChanged(true);
}
......@@ -268,7 +275,7 @@ void CrostiniExportImport::ExportAfterSharing(
<< failure_reason;
auto it = status_trackers_.find(container_id);
if (it != status_trackers_.end()) {
RemoveTracker(it).SetStatusFailed();
RemoveTracker(it)->SetStatusFailed();
} else {
NOTREACHED() << container_id << " has no status_tracker to update";
}
......@@ -306,7 +313,7 @@ void CrostiniExportImport::OnExportComplete(
base::TaskPriority::BEST_EFFORT},
base::BindOnce(base::IgnoreResult(&base::DeleteFile),
it->second->path(), false));
RemoveTracker(it).SetStatusCancelled();
RemoveTracker(it)->SetStatusCancelled();
break;
}
case CrostiniExportImportStatusTracker::Status::RUNNING:
......@@ -324,7 +331,7 @@ void CrostiniExportImport::OnExportComplete(
"Crostini.BackupSizeRatio",
std::round(compressed_size * 100.0 / container_size));
}
RemoveTracker(it).SetStatusDone();
RemoveTracker(it)->SetStatusDone();
break;
default:
NOTREACHED();
......@@ -340,7 +347,7 @@ void CrostiniExportImport::OnExportComplete(
base::TaskPriority::BEST_EFFORT},
base::BindOnce(base::IgnoreResult(&base::DeleteFile),
it->second->path(), false));
RemoveTracker(it).SetStatusCancelled();
RemoveTracker(it)->SetStatusCancelled();
break;
}
default:
......@@ -370,7 +377,7 @@ void CrostiniExportImport::OnExportComplete(
CrostiniExportImportStatusTracker::Status::RUNNING ||
it->second->status() ==
CrostiniExportImportStatusTracker::Status::CANCELLING);
RemoveTracker(it).SetStatusFailed();
RemoveTracker(it)->SetStatusFailed();
}
UMA_HISTOGRAM_ENUMERATION("Crostini.Backup", enum_hist_result);
std::move(callback).Run(result);
......@@ -431,7 +438,7 @@ void CrostiniExportImport::ImportAfterSharing(
<< failure_reason;
auto it = status_trackers_.find(container_id);
if (it != status_trackers_.end()) {
RemoveTracker(it).SetStatusFailed();
RemoveTracker(it)->SetStatusFailed();
} else {
NOTREACHED() << container_id << " has no status_tracker to update";
}
......@@ -465,7 +472,7 @@ void CrostiniExportImport::OnImportComplete(
// natural to pretend the cancel did not happen, and instead display
// success.
case CrostiniExportImportStatusTracker::Status::CANCELLING:
RemoveTracker(it).SetStatusDone();
RemoveTracker(it)->SetStatusDone();
break;
default:
NOTREACHED();
......@@ -478,7 +485,7 @@ void CrostiniExportImport::OnImportComplete(
if (it != status_trackers_.end()) {
switch (it->second->status()) {
case CrostiniExportImportStatusTracker::Status::CANCELLING:
RemoveTracker(it).SetStatusCancelled();
RemoveTracker(it)->SetStatusCancelled();
break;
default:
NOTREACHED();
......@@ -514,7 +521,7 @@ void CrostiniExportImport::OnImportComplete(
if (it != status_trackers_.end()) {
DCHECK(it->second->status() ==
CrostiniExportImportStatusTracker::Status::RUNNING);
RemoveTracker(it).SetStatusFailed();
RemoveTracker(it)->SetStatusFailed();
} else {
NOTREACHED() << container_id << " has no status_tracker to update";
}
......@@ -558,12 +565,12 @@ void CrostiniExportImport::OnImportContainerProgress(
break;
// Failure, set error message.
case ImportContainerProgressStatus::FAILURE_ARCHITECTURE:
RemoveTracker(it).SetStatusFailedArchitectureMismatch(
RemoveTracker(it)->SetStatusFailedArchitectureMismatch(
architecture_container, architecture_device);
break;
case ImportContainerProgressStatus::FAILURE_SPACE:
DCHECK_GE(minimum_required_space, available_space);
RemoveTracker(it).SetStatusFailedInsufficientSpace(
RemoveTracker(it)->SetStatusFailedInsufficientSpace(
minimum_required_space - available_space);
break;
default:
......@@ -576,10 +583,12 @@ std::string CrostiniExportImport::GetUniqueNotificationId() {
next_status_tracker_id_++);
}
CrostiniExportImportStatusTracker& CrostiniExportImport::RemoveTracker(
std::map<ContainerId, CrostiniExportImportStatusTracker*>::iterator it) {
std::unique_ptr<CrostiniExportImportStatusTracker>
CrostiniExportImport::RemoveTracker(
std::map<ContainerId,
std::unique_ptr<CrostiniExportImportStatusTracker>>::iterator it) {
DCHECK(it != status_trackers_.end());
auto& status_tracker = *it->second;
auto status_tracker = std::move(it->second);
status_trackers_.erase(it);
for (auto& observer : observers_) {
observer.OnCrostiniExportImportOperationStatusChanged(false);
......@@ -623,7 +632,8 @@ CrostiniExportImport::GetNotificationControllerForTesting(
if (it == status_trackers_.end()) {
return nullptr;
}
return static_cast<CrostiniExportImportNotificationController*>(it->second)
return static_cast<CrostiniExportImportNotificationController*>(
it->second.get())
->GetWeakPtr();
}
......
chrome/browser/chromeos/crostini/crostini_export_import.h
View file @ c90764f4
......@@ -68,9 +68,8 @@ class CrostiniExportImport : public KeyedService,
bool in_progress) = 0;
};
using TrackerFactory =
base::OnceCallback<CrostiniExportImportStatusTracker*(ExportImportType,
base::FilePath)>;
using TrackerFactory = base::OnceCallback<std::unique_ptr<
CrostiniExportImportStatusTracker>(ExportImportType, base::FilePath)>;
struct OperationData {
OperationData(ExportImportType type,
......@@ -201,12 +200,15 @@ class CrostiniExportImport : public KeyedService,
std::string GetUniqueNotificationId();
CrostiniExportImportStatusTracker& RemoveTracker(
std::map<ContainerId, CrostiniExportImportStatusTracker*>::iterator it);
std::unique_ptr<CrostiniExportImportStatusTracker> RemoveTracker(
std::map<ContainerId,
std::unique_ptr<CrostiniExportImportStatusTracker>>::iterator
it);
Profile* profile_;
scoped_refptr<ui::SelectFileDialog> select_folder_dialog_;
std::map<ContainerId, CrostiniExportImportStatusTracker*> status_trackers_;
std::map<ContainerId, std::unique_ptr<CrostiniExportImportStatusTracker>>
status_trackers_;
// |operation_data_storage_| persists the data required to complete an
// operation while the file selection dialog is open/operation is in progress.
std::unordered_map<OperationData*, std::unique_ptr<OperationData>>
......
chrome/browser/chromeos/crostini/crostini_export_import_notification_controller.cc
View file @ c90764f4
......@@ -32,6 +32,18 @@ constexpr char kNotifierCrostiniExportImportOperation[] =
} // namespace
CrostiniExportImportClickCloseDelegate::CrostiniExportImportClickCloseDelegate()
: HandleNotificationClickDelegate(
HandleNotificationClickDelegate::ButtonClickCallback()) {}
void CrostiniExportImportClickCloseDelegate::Close(bool by_user) {
if (!close_closure_.is_null())
close_closure_.Run();
}
CrostiniExportImportClickCloseDelegate::
~CrostiniExportImportClickCloseDelegate() = default;
CrostiniExportImportNotificationController::
CrostiniExportImportNotificationController(
Profile* profile,
......@@ -41,7 +53,13 @@ CrostiniExportImportNotificationController::
ContainerId container_id)
: CrostiniExportImportStatusTracker(type, std::move(path)),
profile_(profile),
container_id_(std::move(container_id)) {
container_id_(std::move(container_id)),
delegate_(
base::MakeRefCounted<CrostiniExportImportClickCloseDelegate>()) {
delegate_->SetCloseCallback(base::BindRepeating(
&CrostiniExportImportNotificationController::on_notification_closed,
weak_ptr_factory_.GetWeakPtr()));
message_center::RichNotificationData rich_notification_data;
rich_notification_data.vector_small_image = &kNotificationLinuxIcon;
rich_notification_data.accent_color = ash::kSystemNotificationColorNormal;
......@@ -55,9 +73,7 @@ CrostiniExportImportNotificationController::
GURL(), // origin_url
message_center::NotifierId(message_center::NotifierType::SYSTEM_COMPONENT,
kNotifierCrostiniExportImportOperation),
rich_notification_data,
base::MakeRefCounted<message_center::ThunkNotificationDelegate>(
weak_ptr_factory_.GetWeakPtr()));
rich_notification_data, delegate_);
SetStatusRunning(0);
}
......@@ -78,6 +94,18 @@ void CrostiniExportImportNotificationController::SetStatusRunningUI(
return;
}
delegate_->SetCallback(base::BindRepeating(
[](Profile* profile, ExportImportType type, ContainerId container_id,
base::Optional<int> button_index) {
if (!button_index.has_value()) {
return;
}
DCHECK(*button_index == 1);
CrostiniExportImport::GetForProfile(profile)->CancelOperation(
type, container_id);
},
profile_, type(), container_id_));
notification_->set_type(message_center::NOTIFICATION_TYPE_PROGRESS);
notification_->set_accent_color(ash::kSystemNotificationColorNormal);
notification_->set_title(l10n_util::GetStringUTF16(
......@@ -100,6 +128,9 @@ void CrostiniExportImportNotificationController::SetStatusCancellingUI() {
return;
}
delegate_->SetCallback(
CrostiniExportImportClickCloseDelegate::ButtonClickCallback());
notification_->set_type(message_center::NOTIFICATION_TYPE_PROGRESS);
notification_->set_accent_color(ash::kSystemNotificationColorNormal);
notification_->set_title(l10n_util::GetStringUTF16(
......@@ -116,6 +147,17 @@ void CrostiniExportImportNotificationController::SetStatusCancellingUI() {
}
void CrostiniExportImportNotificationController::SetStatusDoneUI() {
if (type() == ExportImportType::EXPORT) {
delegate_->SetCallback(base::BindRepeating(
[](Profile* profile, base::FilePath path) {
platform_util::ShowItemInFolder(profile, path);
},
profile_, path()));
} else {
delegate_->SetCallback(
CrostiniExportImportClickCloseDelegate::ButtonClickCallback());
}
notification_->set_type(message_center::NOTIFICATION_TYPE_SIMPLE);
notification_->set_accent_color(ash::kSystemNotificationColorNormal);
notification_->set_title(l10n_util::GetStringUTF16(
......@@ -134,6 +176,9 @@ void CrostiniExportImportNotificationController::SetStatusDoneUI() {
}
void CrostiniExportImportNotificationController::SetStatusCancelledUI() {
delegate_->SetCallback(
CrostiniExportImportClickCloseDelegate::ButtonClickCallback());
NotificationDisplayService::GetForProfile(profile_)->Close(
NotificationHandler::Type::TRANSIENT, notification_->id());
}
......@@ -141,6 +186,42 @@ void CrostiniExportImportNotificationController::SetStatusCancelledUI() {
void CrostiniExportImportNotificationController::SetStatusFailedWithMessageUI(
Status status,
const base::string16& message) {
switch (status) {
case Status::FAILED_UNKNOWN_REASON:
delegate_->SetCallback(base::BindRepeating(
[](Profile* profile) {
chrome::SettingsWindowManager::GetInstance()->ShowOSSettings(
profile, chrome::kCrostiniExportImportSubPage);
},
profile_));
break;
case Status::FAILED_ARCHITECTURE_MISMATCH:
delegate_->SetCallback(base::BindRepeating(
[](Profile* profile) {
NavigateParams params(profile,
GURL(chrome::kLinuxExportImportHelpURL),
ui::PAGE_TRANSITION_LINK);
params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
params.window_action = NavigateParams::SHOW_WINDOW;
Navigate(&params);
},
profile_));
break;
case Status::FAILED_INSUFFICIENT_SPACE:
delegate_->SetCallback(base::BindRepeating(
[](Profile* profile) {
chrome::SettingsWindowManager::GetInstance()->ShowOSSettings(
profile, chrome::kStorageSubPage);
},
profile_));
break;
default:
LOG(ERROR) << "Unexpected failure status "
<< static_cast<std::underlying_type_t<Status>>(status);
delegate_->SetCallback(
CrostiniExportImportClickCloseDelegate::ButtonClickCallback());
}
notification_->set_type(message_center::NOTIFICATION_TYPE_SIMPLE);
notification_->set_accent_color(ash::kSystemNotificationColorCriticalWarning);
notification_->set_title(l10n_util::GetStringUTF16(
......@@ -155,64 +236,4 @@ void CrostiniExportImportNotificationController::SetStatusFailedWithMessageUI(
ForceRedisplay();
}
void CrostiniExportImportNotificationController::Close(bool by_user) {
switch (status()) {
case Status::RUNNING:
case Status::CANCELLING:
hidden_ = true;
return;
case Status::DONE:
case Status::CANCELLED:
case Status::FAILED_UNKNOWN_REASON:
case Status::FAILED_ARCHITECTURE_MISMATCH:
case Status::FAILED_INSUFFICIENT_SPACE:
case Status::FAILED_CONCURRENT_OPERATION:
delete this;
return;
default:
NOTREACHED();
}
}
void CrostiniExportImportNotificationController::Click(
const base::Optional<int>& button_index,
const base::Optional<base::string16>&) {
switch (status()) {
case Status::RUNNING:
if (button_index) {
DCHECK(*button_index == 1);
CrostiniExportImport::GetForProfile(profile_)->CancelOperation(
type(), container_id_);
}
return;
case Status::DONE:
DCHECK(!button_index);
if (type() == ExportImportType::EXPORT) {
platform_util::ShowItemInFolder(profile_, path());
}
return;
case Status::FAILED_UNKNOWN_REASON:
DCHECK(!button_index);
chrome::SettingsWindowManager::GetInstance()->ShowOSSettings(
profile_, chrome::kCrostiniExportImportSubPage);
return;
case Status::FAILED_ARCHITECTURE_MISMATCH: {
DCHECK(!button_index);
NavigateParams params(profile_, GURL(chrome::kLinuxExportImportHelpURL),
ui::PAGE_TRANSITION_LINK);
params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
params.window_action = NavigateParams::SHOW_WINDOW;
Navigate(&params);
}
return;
case Status::FAILED_INSUFFICIENT_SPACE:
DCHECK(!button_index);
chrome::SettingsWindowManager::GetInstance()->ShowOSSettings(
profile_, chrome::kStorageSubPage);
return;
default:
DCHECK(!button_index);
}
}
} // namespace crostini
chrome/browser/chromeos/crostini/crostini_export_import_notification_controller.h
View file @ c90764f4
......@@ -22,26 +22,40 @@ class Notification;
namespace crostini {
class CrostiniExportImportClickCloseDelegate
: public message_center::HandleNotificationClickDelegate {
public:
using HandleNotificationClickDelegate::ButtonClickCallback;
CrostiniExportImportClickCloseDelegate();
void SetCloseCallback(base::RepeatingClosure close_closure) {
close_closure_ = std::move(close_closure);
}
// NotificationDelegate overrides:
void Close(bool by_user) override;
private:
~CrostiniExportImportClickCloseDelegate() override;
base::RepeatingClosure close_closure_{};
};
enum class ExportImportType;
// Controller for Crostini's Export Import Notification UI.
// It can be used to change the look of the UI, and handles actions from the UI.
// Controller for Crostini's Export Import notification UI.
// Upon construction the Controller will create a new notification, displaying
// the StatusRunning UI. If the controller is freed the notification will
// continue to be shown and handle click events until the user closes it.
class CrostiniExportImportNotificationController
: public CrostiniExportImportStatusTracker,
public message_center::NotificationObserver {
: public CrostiniExportImportStatusTracker {
public:
// Used to construct CrostiniExportImportNotificationController to ensure it
// controls its lifetime.
static CrostiniExportImportStatusTracker* Create(
Profile* profile,
ContainerId container_id,
const std::string& notification_id,
CrostiniExportImportNotificationController(Profile* profile,
ExportImportType type,
base::FilePath path) {
return new CrostiniExportImportNotificationController(
profile, type, notification_id, std::move(path),
std::move(container_id));
}
const std::string& notification_id,
base::FilePath path,
ContainerId container_id);
~CrostiniExportImportNotificationController() override;
......@@ -52,19 +66,11 @@ class CrostiniExportImportNotificationController
base::WeakPtr<CrostiniExportImportNotificationController> GetWeakPtr() {
return weak_ptr_factory_.GetWeakPtr();
}
// message_center::NotificationObserver:
void Close(bool by_user) override;
void Click(const base::Optional<int>& button_index,
const base::Optional<base::string16>& reply) override;
message_center::NotificationObserver* get_delegate() {
return delegate_.get();
}
private:
CrostiniExportImportNotificationController(Profile* profile,
ExportImportType type,
const std::string& notification_id,
base::FilePath path,
ContainerId container_id);
// CrostiniExportImportStatusTracker:
void ForceRedisplay() override;
void SetStatusRunningUI(int progress_percent) override;
......@@ -74,15 +80,26 @@ class CrostiniExportImportNotificationController
void SetStatusFailedWithMessageUI(Status status,
const base::string16& message) override;
void on_notification_closed() { hidden_ = true; }
Profile* profile_; // Not owned.
ContainerId container_id_;
// |delegate_| is responsible for handling click events. It is separate from
// the controller because it needs to live as long as the notification is in
// the UI, but the controller's lifetime ends once the notification is in a
// final state (done, canceled, or failed).
scoped_refptr<CrostiniExportImportClickCloseDelegate> delegate_;
// Time when the operation started. Used for estimating time remaining.
base::TimeTicks started_ = base::TimeTicks::Now();
// |notification_| acts as a handle to the notification UI, it is used to
// update the UI's progress/message/buttons. Freeing |notification_| doesn't
// close the notification UI; the UI system is responsible for closing it.
std::unique_ptr<message_center::Notification> notification_;
bool hidden_ = false;
base::WeakPtrFactory<CrostiniExportImportNotificationController>
weak_ptr_factory_{this};
DISALLOW_COPY_AND_ASSIGN(CrostiniExportImportNotificationController);
};
......
ui/message_center/public/cpp/notification_delegate.cc
View file @ c90764f4
......@@ -43,7 +43,21 @@ ThunkNotificationDelegate::~ThunkNotificationDelegate() = default;
HandleNotificationClickDelegate::HandleNotificationClickDelegate(
const base::RepeatingClosure& callback) {
if (!callback.is_null()) {
SetCallback(callback);
}
HandleNotificationClickDelegate::HandleNotificationClickDelegate(
const ButtonClickCallback& callback)
: callback_(callback) {}
void HandleNotificationClickDelegate::SetCallback(
const ButtonClickCallback& callback) {
callback_ = callback;
}
void HandleNotificationClickDelegate::SetCallback(
const base::RepeatingClosure& closure) {
if (!closure.is_null()) {
// Create a callback that consumes and ignores the button index parameter,
// and just runs the provided closure.
callback_ = base::BindRepeating(
......@@ -52,14 +66,10 @@ HandleNotificationClickDelegate::HandleNotificationClickDelegate(
DCHECK(!button_index);
closure.Run();
},
callback);
closure);
}
}
HandleNotificationClickDelegate::HandleNotificationClickDelegate(
const ButtonClickCallback& callback)
: callback_(callback) {}
HandleNotificationClickDelegate::~HandleNotificationClickDelegate() {}
void HandleNotificationClickDelegate::Click(
......
ui/message_center/public/cpp/notification_delegate.h
View file @ c90764f4
......@@ -95,6 +95,14 @@ class MESSAGE_CENTER_PUBLIC_EXPORT HandleNotificationClickDelegate
explicit HandleNotificationClickDelegate(
const base::RepeatingClosure& closure);
// Overrides the callback with one that handles clicks on a button or on the
// body.
void SetCallback(const ButtonClickCallback& callback);
// Overrides the callback with one that only handles clicks on the body of the
// notification.
void SetCallback(const base::RepeatingClosure& closure);
// NotificationDelegate overrides:
void Click(const base::Optional<int>& button_index,
const base::Optional<base::string16>& reply) override;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment