Commit 5bee35ff authored by Phillis Tang's avatar Phillis Tang Committed by Commit Bot

DPWA: improve barrier callback for os hooks

Adds a CallbackFactory that creates callback for each os hook, and
DCHECK on destruction to make sure that all os hooks type have created
callback.

Bug: 1129193
Change-Id: I82a44dc7fa8a1c40c7caba8fcaa6134620e80d4f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2517438
Commit-Queue: Phillis Tang <phillis@chromium.org>
Reviewed-by: default avatarDaniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826457}
parent 968b98ed
......@@ -162,6 +162,7 @@ source_set("unit_tests") {
sources = [
"file_handler_manager_unittest.cc",
"install_finalizer_unittest.cc",
"os_integration_manager_unittest.cc",
"pending_app_manager_unittest.cc",
"web_app_constants_unittest.cc",
"web_app_data_retriever_unittest.cc",
......
......@@ -14,6 +14,9 @@
#include "base/threading/sequenced_task_runner_handle.h"
#include "build/build_config.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/web_applications/components/web_app_constants.h"
#include "chrome/browser/web_applications/components/web_app_id.h"
#include "chrome/browser/web_applications/components/web_app_shortcut.h"
#include "chrome/browser/web_applications/components/web_app_ui_manager.h"
#include "chrome/common/chrome_features.h"
#include "content/public/browser/browser_thread.h"
......@@ -58,6 +61,22 @@ class OsHooksBarrierInfo {
InstallOsHooksCallback done_callback_;
};
class CallbackFactory {
public:
explicit CallbackFactory(BarrierCallback barrier_callback)
: barrier_callback_(std::move(barrier_callback)) {}
~CallbackFactory() { DCHECK(callback_created_.all()); }
base::OnceCallback<void(bool)> CreateCallack(OsHookType::Type os_hook) {
callback_created_[os_hook] = true;
return base::BindOnce(barrier_callback_, os_hook);
}
private:
BarrierCallback barrier_callback_;
OsHooksResults callback_created_{false};
};
OsIntegrationManager::OsIntegrationManager(
Profile* profile,
std::unique_ptr<AppShortcutManager> shortcut_manager,
......@@ -109,18 +128,14 @@ void OsIntegrationManager::InstallOsHooks(
FROM_HERE, base::BindOnce(std::move(callback), os_hooks_results));
return;
}
#if defined(OS_MAC)
AppShimRegistry::Get()->OnAppInstalledForProfile(app_id, profile_->GetPath());
#endif
MacAppShimOnAppInstalledForProfile(app_id);
// Note: This barrier protects against multiple calls on the same type, but
// it doesn't protect against the case where we fail to call Run / create a
// callback for every type. Developers should double check that Run is
// called for every OsHookType::Type. If there is any missing type, the
// InstallOsHooksCallback will not get run.
base::RepeatingCallback<void(OsHookType::Type os_hook, bool completed)>
barrier = base::BindRepeating(
BarrierCallback barrier = base::BindRepeating(
&OsHooksBarrierInfo::Run,
base::Owned(new OsHooksBarrierInfo(std::move(callback))));
......@@ -130,20 +145,19 @@ void OsIntegrationManager::InstallOsHooks(
// TODO(ortuno): Make adding a shortcut to the applications menu independent
// from adding a shortcut to desktop.
if (options.os_hooks[OsHookType::kShortcuts] &&
shortcut_manager_->CanCreateShortcuts()) {
if (options.os_hooks[OsHookType::kShortcuts] && CanCreateShortcuts()) {
const bool add_to_desktop = options.add_to_desktop;
shortcut_manager_->CreateShortcuts(
app_id, add_to_desktop,
base::BindOnce(&OsIntegrationManager::OnShortcutsCreated,
weak_ptr_factory_.GetWeakPtr(), app_id,
std::move(web_app_info), std::move(options), barrier));
CreateShortcutsCallback shortcuts_callback = base::BindOnce(
&OsIntegrationManager::OnShortcutsCreated,
weak_ptr_factory_.GetWeakPtr(), app_id, std::move(web_app_info),
std::move(options), std::move(barrier));
CreateShortcuts(app_id, add_to_desktop, std::move(shortcuts_callback));
} else {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(&OsIntegrationManager::OnShortcutsCreated,
FROM_HERE, base::BindOnce(&OsIntegrationManager::OnShortcutsCreated,
weak_ptr_factory_.GetWeakPtr(), app_id,
std::move(web_app_info), std::move(options), barrier,
std::move(web_app_info), std::move(options),
std::move(barrier),
/*shortcuts_created=*/false));
}
}
......@@ -172,13 +186,15 @@ void OsIntegrationManager::UninstallOsHooks(const AppId& app_id,
barrier = base::BindRepeating(
&OsHooksBarrierInfo::Run,
base::Owned(new OsHooksBarrierInfo(std::move(callback))));
CallbackFactory callback_factory = CallbackFactory(barrier);
if (os_hooks[OsHookType::kShortcutsMenu] &&
ShouldRegisterShortcutsMenuWithOs()) {
barrier.Run(OsHookType::kShortcutsMenu,
UnregisterShortcutsMenuWithOs(app_id, profile_->GetPath()));
callback_factory.CreateCallack(OsHookType::kShortcutsMenu)
.Run(UnregisterShortcutsMenuWithOs(app_id, profile_->GetPath()));
} else {
barrier.Run(OsHookType::kShortcutsMenu, /*completed=*/true);
callback_factory.CreateCallack(OsHookType::kShortcutsMenu)
.Run(/*completed=*/true);
}
if (os_hooks[OsHookType::kShortcuts] || os_hooks[OsHookType::kRunOnOsLogin]) {
......@@ -191,29 +207,38 @@ void OsIntegrationManager::UninstallOsHooks(const AppId& app_id,
base::FeatureList::IsEnabled(features::kDesktopPWAsRunOnOsLogin)) {
ScheduleUnregisterRunOnOsLogin(
shortcut_info->profile_path, shortcut_info->title,
base::BindOnce(barrier, OsHookType::kRunOnOsLogin));
base::BindOnce(
callback_factory.CreateCallack(OsHookType::kRunOnOsLogin)));
} else {
barrier.Run(OsHookType::kRunOnOsLogin, /*completed=*/true);
callback_factory.CreateCallack(OsHookType::kRunOnOsLogin)
.Run(/*completed=*/true);
}
if (os_hooks[OsHookType::kShortcuts]) {
internals::ScheduleDeletePlatformShortcuts(
shortcut_data_dir, std::move(shortcut_info),
base::BindOnce(&OsIntegrationManager::OnShortcutsDeleted,
weak_ptr_factory_.GetWeakPtr(), app_id, barrier));
base::BindOnce(
&OsIntegrationManager::OnShortcutsDeleted,
weak_ptr_factory_.GetWeakPtr(), app_id,
callback_factory.CreateCallack(OsHookType::kShortcuts)));
} else {
barrier.Run(OsHookType::kShortcuts, /*completed=*/true);
callback_factory.CreateCallack(OsHookType::kShortcuts)
.Run(/*completed=*/true);
}
} else {
barrier.Run(OsHookType::kShortcuts, /*completed=*/true);
barrier.Run(OsHookType::kRunOnOsLogin, /*completed=*/true);
callback_factory.CreateCallack(OsHookType::kShortcuts)
.Run(/*completed=*/true);
callback_factory.CreateCallack(OsHookType::kRunOnOsLogin)
.Run(/*completed=*/true);
}
// TODO(https://crbug.com/1108109) we should return the result of file handler
// unregistration and record errors during unregistration.
if (os_hooks[OsHookType::kFileHandlers])
file_handler_manager_->DisableAndUnregisterOsFileHandlers(app_id);
barrier.Run(OsHookType::kFileHandlers, /*completed=*/true);
callback_factory.CreateCallack(OsHookType::kFileHandlers)
.Run(/*completed=*/true);
}
void OsIntegrationManager::UpdateOsHooks(
......@@ -241,6 +266,9 @@ void OsIntegrationManager::UpdateOsHooks(
}
bool OsIntegrationManager::CanCreateShortcuts() const {
if (suppress_os_managers_for_testing_)
return true;
DCHECK(shortcut_manager_);
return shortcut_manager_->CanCreateShortcuts();
}
......@@ -308,94 +336,177 @@ ScopedOsHooksSuppress OsIntegrationManager::ScopedSuppressOsHooksForTesting() {
#endif
}
void OsIntegrationManager::SuppressOsManagersForTesting() {
suppress_os_managers_for_testing_ = true;
}
void OsIntegrationManager::CreateShortcuts(const AppId& app_id,
bool add_to_desktop,
CreateShortcutsCallback callback) {
if (suppress_os_managers_for_testing_) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), true));
return;
}
shortcut_manager_->CreateShortcuts(app_id, add_to_desktop,
std::move(callback));
}
void OsIntegrationManager::RegisterFileHandlers(
const AppId& app_id,
base::OnceCallback<void(bool success)> callback) {
if (suppress_os_managers_for_testing_)
return;
file_handler_manager_->EnableAndRegisterOsFileHandlers(app_id);
// TODO(crbug.com/1087219): callback should be run after all hooks are
// deployed, need to refactor filehandler to allow this.
std::move(callback).Run(true);
}
void OsIntegrationManager::RegisterShortcutsMenu(
const AppId& app_id,
const std::vector<WebApplicationShortcutsMenuItemInfo>&
shortcuts_menu_item_infos,
const ShortcutsMenuIconsBitmaps& shortcuts_menu_icons_bitmaps,
base::OnceCallback<void(bool success)> callback) {
if (suppress_os_managers_for_testing_)
return;
shortcut_manager_->RegisterShortcutsMenuWithOs(
app_id, shortcuts_menu_item_infos, shortcuts_menu_icons_bitmaps);
// TODO(https://crbug.com/1098471): fix RegisterShortcutsMenuWithOs to
// take callback.
std::move(callback).Run(true);
}
void OsIntegrationManager::ReadAllShortcutsMenuIconsAndRegisterShortcutsMenu(
const AppId& app_id,
base::OnceCallback<void(bool success)> callback) {
if (suppress_os_managers_for_testing_) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), true));
return;
}
shortcut_manager_->ReadAllShortcutsMenuIconsAndRegisterShortcutsMenu(
app_id, std::move(callback));
}
void OsIntegrationManager::RegisterRunOnOsLogin(
const AppId& app_id,
RegisterRunOnOsLoginCallback callback) {
if (suppress_os_managers_for_testing_) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), true));
return;
}
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
shortcut_manager_->GetShortcutInfoForApp(
app_id,
base::BindOnce(
&OsIntegrationManager::OnShortcutInfoRetrievedRegisterRunOnOsLogin,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
}
void OsIntegrationManager::MacAppShimOnAppInstalledForProfile(
const AppId& app_id) {
if (suppress_os_managers_for_testing_)
return;
#if defined(OS_MAC)
AppShimRegistry::Get()->OnAppInstalledForProfile(app_id, profile_->GetPath());
#endif
}
void OsIntegrationManager::AddAppToQuickLaunchBar(const AppId& app_id) {
if (suppress_os_managers_for_testing_)
return;
DCHECK(ui_manager_);
if (ui_manager_->CanAddAppToQuickLaunchBar()) {
ui_manager_->AddAppToQuickLaunchBar(app_id);
}
}
void OsIntegrationManager::OnShortcutsCreated(
const AppId& app_id,
std::unique_ptr<WebApplicationInfo> web_app_info,
InstallOsHooksOptions options,
base::RepeatingCallback<void(OsHookType::Type os_hook, bool created)>
barrier_callback,
BarrierCallback barrier,
bool shortcuts_created) {
DCHECK(file_handler_manager_);
DCHECK(ui_manager_);
CallbackFactory callback_factory = CallbackFactory(barrier);
barrier_callback.Run(OsHookType::kShortcuts, /*completed=*/true);
callback_factory.CreateCallack(OsHookType::kShortcuts)
.Run(/*completed=*/shortcuts_created);
// TODO(crbug.com/1087219): callback should be run after all hooks are
// deployed, need to refactor filehandler to allow this.
if (options.os_hooks[OsHookType::kFileHandlers])
file_handler_manager_->EnableAndRegisterOsFileHandlers(app_id);
barrier_callback.Run(OsHookType::kFileHandlers, /*completed=*/true);
if (options.os_hooks[OsHookType::kFileHandlers]) {
RegisterFileHandlers(
app_id, callback_factory.CreateCallack(OsHookType::kFileHandlers));
} else {
callback_factory.CreateCallack(OsHookType::kFileHandlers)
.Run(/*completed=*/false);
}
if (options.os_hooks[OsHookType::kShortcuts] &&
options.add_to_quick_launch_bar &&
ui_manager_->CanAddAppToQuickLaunchBar()) {
ui_manager_->AddAppToQuickLaunchBar(app_id);
options.add_to_quick_launch_bar) {
AddAppToQuickLaunchBar(app_id);
}
if (shortcuts_created && options.os_hooks[OsHookType::kShortcutsMenu] &&
base::FeatureList::IsEnabled(
features::kDesktopPWAsAppIconShortcutsMenu)) {
if (web_app_info) {
if (web_app_info->shortcuts_menu_item_infos.empty()) {
barrier_callback.Run(OsHookType::kShortcutsMenu, /*completed=*/false);
callback_factory.CreateCallack(OsHookType::kShortcutsMenu)
.Run(/*completed=*/false);
} else {
shortcut_manager_->RegisterShortcutsMenuWithOs(
RegisterShortcutsMenu(
app_id, web_app_info->shortcuts_menu_item_infos,
web_app_info->shortcuts_menu_icons_bitmaps);
// TODO(https://crbug.com/1098471): fix RegisterShortcutsMenuWithOs to
// take callback.
barrier_callback.Run(OsHookType::kShortcutsMenu, /*completed=*/true);
web_app_info->shortcuts_menu_icons_bitmaps,
callback_factory.CreateCallack(OsHookType::kShortcutsMenu));
}
} else {
shortcut_manager_->ReadAllShortcutsMenuIconsAndRegisterShortcutsMenu(
app_id, base::BindOnce(barrier_callback, OsHookType::kShortcutsMenu));
ReadAllShortcutsMenuIconsAndRegisterShortcutsMenu(
app_id, callback_factory.CreateCallack(OsHookType::kShortcutsMenu));
}
} else {
barrier_callback.Run(OsHookType::kShortcutsMenu, /*completed=*/false);
callback_factory.CreateCallack(OsHookType::kShortcutsMenu)
.Run(/*completed=*/false);
}
if (options.os_hooks[OsHookType::kRunOnOsLogin] &&
base::FeatureList::IsEnabled(features::kDesktopPWAsRunOnOsLogin)) {
// TODO(crbug.com/897302): Implement Run on OS Login mode selection.
// Currently it is set to be the default: RunOnOsLoginMode::kWindowed
RegisterRunOnOsLogin(
app_id, base::BindOnce(barrier_callback, OsHookType::kRunOnOsLogin));
RegisterRunOnOsLogin(app_id, base::BindOnce(callback_factory.CreateCallack(
OsHookType::kRunOnOsLogin)));
} else {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(barrier_callback, OsHookType::kRunOnOsLogin,
FROM_HERE, base::BindOnce(callback_factory.CreateCallack(
OsHookType::kRunOnOsLogin),
/*completed=*/false));
}
}
void OsIntegrationManager::OnShortcutsDeleted(
const AppId& app_id,
base::RepeatingCallback<void(OsHookType::Type os_hook, bool deleted)>
barrier_callback,
void OsIntegrationManager::OnShortcutsDeleted(const AppId& app_id,
DeleteShortcutsCallback callback,
bool shortcuts_deleted) {
#if defined(OS_MAC)
bool delete_multi_profile_shortcuts =
AppShimRegistry::Get()->OnAppUninstalledForProfile(app_id,
profile_->GetPath());
if (delete_multi_profile_shortcuts) {
internals::ScheduleDeleteMultiProfileShortcutsForApp(
app_id, base::BindOnce(barrier_callback, OsHookType::kShortcuts));
internals::ScheduleDeleteMultiProfileShortcutsForApp(app_id,
std::move(callback));
}
#else
barrier_callback.Run(OsHookType::kShortcuts, /*completed=*/shortcuts_deleted);
std::move(callback).Run(shortcuts_deleted);
#endif
}
void OsIntegrationManager::RegisterRunOnOsLogin(
const AppId& app_id,
RegisterRunOnOsLoginCallback callback) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
shortcut_manager_->GetShortcutInfoForApp(
app_id,
base::BindOnce(
&OsIntegrationManager::OnShortcutInfoRetrievedRegisterRunOnOsLogin,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
}
void OsIntegrationManager::OnShortcutInfoRetrievedRegisterRunOnOsLogin(
RegisterRunOnOsLoginCallback callback,
std::unique_ptr<ShortcutInfo> info) {
......
......@@ -57,6 +57,9 @@ using UninstallOsHooksCallback =
// Used to suppress OS hooks within this object's lifetime.
using ScopedOsHooksSuppress = std::unique_ptr<base::AutoReset<bool>>;
using BarrierCallback =
base::RepeatingCallback<void(OsHookType::Type os_hook, bool completed)>;
// OsIntegrationManager is responsible of creating/updating/deleting
// all OS hooks during Web App lifecycle.
// It contains individual OS integration managers and takes
......@@ -128,6 +131,9 @@ class OsIntegrationManager {
static ScopedOsHooksSuppress ScopedSuppressOsHooksForTesting();
// Suppress calling individual OS managers for testing.
void SuppressOsManagersForTesting();
protected:
AppShortcutManager* shortcut_manager() { return shortcut_manager_.get(); }
FileHandlerManager* file_handler_manager() {
......@@ -142,23 +148,38 @@ class OsIntegrationManager {
file_handler_manager_ = std::move(file_handler_manager);
}
virtual void CreateShortcuts(const AppId& app_id,
bool add_to_desktop,
CreateShortcutsCallback callback);
virtual void RegisterFileHandlers(
const AppId& app_id,
base::OnceCallback<void(bool success)> callback);
virtual void RegisterShortcutsMenu(
const AppId& app_id,
const std::vector<WebApplicationShortcutsMenuItemInfo>&
shortcuts_menu_item_infos,
const ShortcutsMenuIconsBitmaps& shortcuts_menu_icons_bitmaps,
base::OnceCallback<void(bool success)> callback);
virtual void ReadAllShortcutsMenuIconsAndRegisterShortcutsMenu(
const AppId& app_id,
base::OnceCallback<void(bool success)> callback);
virtual void RegisterRunOnOsLogin(const AppId& app_id,
RegisterRunOnOsLoginCallback callback);
virtual void MacAppShimOnAppInstalledForProfile(const AppId& app_id);
virtual void AddAppToQuickLaunchBar(const AppId& app_id);
private:
void OnShortcutsCreated(const AppId& app_id,
std::unique_ptr<WebApplicationInfo> web_app_info,
InstallOsHooksOptions options,
base::RepeatingCallback<void(OsHookType::Type os_hook,
bool created)> callback,
BarrierCallback barrier,
bool shortcuts_created);
void OnShortcutsDeleted(
const AppId& app_id,
base::RepeatingCallback<void(OsHookType::Type os_hook, bool deleted)>
barrier_callback,
void OnShortcutsDeleted(const AppId& app_id,
DeleteShortcutsCallback callback,
bool shortcuts_deleted);
void RegisterRunOnOsLogin(const AppId& app_id,
RegisterRunOnOsLoginCallback callback);
void OnShortcutInfoRetrievedRegisterRunOnOsLogin(
RegisterRunOnOsLoginCallback callback,
std::unique_ptr<ShortcutInfo> info);
......@@ -169,6 +190,7 @@ class OsIntegrationManager {
std::unique_ptr<AppShortcutManager> shortcut_manager_;
std::unique_ptr<FileHandlerManager> file_handler_manager_;
bool suppress_os_managers_for_testing_ = false;
base::WeakPtrFactory<OsIntegrationManager> weak_ptr_factory_{this};
};
......
// Copyright 2020 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/web_applications/components/os_integration_manager.h"
#include <memory>
#include "base/bind.h"
#include "base/run_loop.h"
#include "base/test/bind.h"
#include "chrome/browser/web_applications/components/web_app_constants.h"
#include "chrome/browser/web_applications/components/web_app_helpers.h"
#include "chrome/browser/web_applications/components/web_app_ui_manager.h"
#include "chrome/browser/web_applications/test/test_file_handler_manager.h"
#include "chrome/browser/web_applications/test/test_os_integration_manager.h"
#include "chrome/browser/web_applications/test/web_app_test.h"
#include "chrome/browser/web_applications/web_app.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/features.h"
namespace web_app {
class OsIntegrationManagerTest : public WebAppTest {
public:
void SetUp() override {
WebAppTest::SetUp();
auto shortcut_manager = std::make_unique<TestShortcutManager>(profile());
auto file_handler_manager =
std::make_unique<TestFileHandlerManager>(profile());
os_integration_manager_ = std::make_unique<OsIntegrationManager>(
profile(), std::move(shortcut_manager),
std::move(file_handler_manager));
os_integration_manager_->SuppressOsManagersForTesting();
}
OsIntegrationManager* os_integration_manager() {
return os_integration_manager_.get();
}
private:
std::unique_ptr<WebAppUiManager> ui_manager_;
std::unique_ptr<OsIntegrationManager> os_integration_manager_;
};
TEST_F(OsIntegrationManagerTest, InstallOsHooksCallbackCalled) {
base::RunLoop run_loop;
OsHooksResults install_results;
InstallOsHooksCallback callback =
base::BindLambdaForTesting([&](OsHooksResults results) {
install_results = results;
run_loop.Quit();
});
const AppId app_id = "test";
InstallOsHooksOptions options;
options.os_hooks = OsHooksResults{true};
os_integration_manager()->InstallOsHooks(app_id, std::move(callback), nullptr,
options);
run_loop.Run();
ASSERT_TRUE(install_results[0]);
}
} // namespace web_app
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