Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[dPWA] Create a command for OsIntegrationManager::Synchronize()
This CL creates a command for OsIntegrationManager::Synchronize() so that weak pointers to locks can be passed through to the Synchronize call. This also updates the only use-case in web_applications where we call Synchronize() outside of a command. Bug: b/261773920, b/262782347 Change-Id: I7ed4cc164b04cf0caf70e32ffe03dfeb030ce707 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4107828 Reviewed-by: Phillis Tang <phillis@chromium.org> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Commit-Queue: Dibyajyoti Pal <dibyapal@chromium.org> Cr-Commit-Position: refs/heads/main@{#1084610}
- Loading branch information
Dibyajyoti Pal
authored and
Chromium LUCI CQ
committed
Dec 17, 2022
1 parent
29ab5c5
commit 585c93f
Showing
7 changed files
with
316 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
66 changes: 66 additions & 0 deletions
66
chrome/browser/web_applications/commands/os_integration_synchronize_command.cc
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| // Copyright 2022 The Chromium Authors | ||
| // 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/commands/os_integration_synchronize_command.h" | ||
|
|
||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| #include "base/containers/flat_set.h" | ||
| #include "base/functional/callback.h" | ||
| #include "base/values.h" | ||
| #include "chrome/browser/web_applications/commands/web_app_command.h" | ||
| #include "chrome/browser/web_applications/locks/app_lock.h" | ||
| #include "chrome/browser/web_applications/os_integration/os_integration_manager.h" | ||
| #include "chrome/browser/web_applications/web_app_id.h" | ||
|
|
||
| namespace web_app { | ||
|
|
||
| OsIntegrationSynchronizeCommand::OsIntegrationSynchronizeCommand( | ||
| const AppId& app_id, | ||
| base::OnceClosure synchronize_callback) | ||
| : WebAppCommandTemplate<AppLock>("OsIntegrationSynchronizeCommand"), | ||
| app_lock_description_( | ||
| std::make_unique<AppLockDescription, base::flat_set<AppId>>( | ||
| {app_id})), | ||
| app_id_(app_id), | ||
| synchronize_callback_(std::move(synchronize_callback)) {} | ||
|
|
||
| OsIntegrationSynchronizeCommand::~OsIntegrationSynchronizeCommand() = default; | ||
|
|
||
| LockDescription& OsIntegrationSynchronizeCommand::lock_description() const { | ||
| return *app_lock_description_; | ||
| } | ||
|
|
||
| void OsIntegrationSynchronizeCommand::StartWithLock( | ||
| std::unique_ptr<AppLock> app_lock) { | ||
| app_lock_ = std::move(app_lock); | ||
|
|
||
| app_lock_->os_integration_manager().Synchronize( | ||
| app_id_, | ||
| base::BindOnce(&OsIntegrationSynchronizeCommand::OnSynchronizeComplete, | ||
| weak_factory_.GetWeakPtr())); | ||
| } | ||
|
|
||
| void OsIntegrationSynchronizeCommand::OnSynchronizeComplete() { | ||
| DCHECK(!synchronize_callback_.is_null()); | ||
| SignalCompletionAndSelfDestruct(CommandResult::kSuccess, | ||
| std::move(synchronize_callback_)); | ||
| } | ||
|
|
||
| void OsIntegrationSynchronizeCommand::OnSyncSourceRemoved() {} | ||
|
|
||
| void OsIntegrationSynchronizeCommand::OnShutdown() { | ||
| DCHECK(!synchronize_callback_.is_null()); | ||
| SignalCompletionAndSelfDestruct(CommandResult::kShutdown, | ||
| std::move(synchronize_callback_)); | ||
| } | ||
|
|
||
| base::Value OsIntegrationSynchronizeCommand::ToDebugValue() const { | ||
| base::Value::Dict value; | ||
| value.Set("app_id", app_id_); | ||
| return base::Value(std::move(value)); | ||
| } | ||
|
|
||
| } // namespace web_app |
51 changes: 51 additions & 0 deletions
51
chrome/browser/web_applications/commands/os_integration_synchronize_command.h
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| // Copyright 2022 The Chromium Authors | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| #ifndef CHROME_BROWSER_WEB_APPLICATIONS_COMMANDS_OS_INTEGRATION_SYNCHRONIZE_COMMAND_H_ | ||
| #define CHROME_BROWSER_WEB_APPLICATIONS_COMMANDS_OS_INTEGRATION_SYNCHRONIZE_COMMAND_H_ | ||
|
|
||
| #include <memory> | ||
|
|
||
| #include "base/functional/callback.h" | ||
| #include "base/memory/weak_ptr.h" | ||
| #include "chrome/browser/web_applications/commands/web_app_command.h" | ||
| #include "chrome/browser/web_applications/os_integration/os_integration_manager.h" | ||
| #include "chrome/browser/web_applications/web_app_id.h" | ||
|
|
||
| namespace web_app { | ||
|
|
||
| class LockDescription; | ||
| class AppLock; | ||
| class AppLockDescription; | ||
|
|
||
| // Used to call OsIntegrationManager::Synchronize() with an app_lock. | ||
| class OsIntegrationSynchronizeCommand : public WebAppCommandTemplate<AppLock> { | ||
| public: | ||
| OsIntegrationSynchronizeCommand(const AppId& app_id, | ||
| base::OnceClosure synchronize_callback); | ||
| ~OsIntegrationSynchronizeCommand() override; | ||
|
|
||
| LockDescription& lock_description() const override; | ||
|
|
||
| void StartWithLock(std::unique_ptr<AppLock> app_lock) override; | ||
| void OnSyncSourceRemoved() override; | ||
| void OnShutdown() override; | ||
|
|
||
| base::Value ToDebugValue() const override; | ||
|
|
||
| private: | ||
| void OnSynchronizeComplete(); | ||
|
|
||
| std::unique_ptr<AppLockDescription> app_lock_description_; | ||
| std::unique_ptr<AppLock> app_lock_; | ||
|
|
||
| AppId app_id_; | ||
| base::OnceClosure synchronize_callback_; | ||
|
|
||
| base::WeakPtrFactory<OsIntegrationSynchronizeCommand> weak_factory_{this}; | ||
| }; | ||
|
|
||
| } // namespace web_app | ||
|
|
||
| #endif // CHROME_BROWSER_WEB_APPLICATIONS_COMMANDS_OS_INTEGRATION_SYNCHRONIZE_COMMAND_H_ |
174 changes: 174 additions & 0 deletions
174
chrome/browser/web_applications/commands/os_integration_synchronize_command_unittest.cc
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| // Copyright 2022 The Chromium Authors | ||
| // 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/commands/os_integration_synchronize_command.h" | ||
|
|
||
| #include <memory> | ||
|
|
||
| #include "base/files/file_util.h" | ||
| #include "base/test/scoped_feature_list.h" | ||
| #include "base/test/test_future.h" | ||
| #include "chrome/browser/profiles/profile.h" | ||
| #include "chrome/browser/web_applications/os_integration/os_integration_manager.h" | ||
| #include "chrome/browser/web_applications/os_integration/web_app_file_handler_manager.h" | ||
| #include "chrome/browser/web_applications/os_integration/web_app_protocol_handler_manager.h" | ||
| #include "chrome/browser/web_applications/os_integration/web_app_shortcut.h" | ||
| #include "chrome/browser/web_applications/os_integration/web_app_shortcut_manager.h" | ||
| #include "chrome/browser/web_applications/proto/web_app_os_integration_state.pb.h" | ||
| #include "chrome/browser/web_applications/test/fake_web_app_provider.h" | ||
| #include "chrome/browser/web_applications/test/web_app_install_test_utils.h" | ||
| #include "chrome/browser/web_applications/test/web_app_test.h" | ||
| #include "chrome/browser/web_applications/test/web_app_test_utils.h" | ||
| #include "chrome/browser/web_applications/web_app_command_scheduler.h" | ||
| #include "chrome/browser/web_applications/web_app_install_info.h" | ||
| #include "chrome/browser/web_applications/web_app_provider.h" | ||
| #include "chrome/browser/web_applications/web_app_registrar.h" | ||
| #include "chrome/common/chrome_features.h" | ||
| #include "components/services/app_service/public/cpp/protocol_handler_info.h" | ||
| #include "components/webapps/browser/install_result_code.h" | ||
| #include "testing/gtest/include/gtest/gtest.h" | ||
| #include "url/gurl.h" | ||
|
|
||
| namespace web_app { | ||
| namespace { | ||
|
|
||
| using ::testing::Eq; | ||
|
|
||
| class OsIntegrationSynchronizeCommandTest | ||
| : public WebAppTest, | ||
| public ::testing::WithParamInterface<OsIntegrationSubManagersState> { | ||
| public: | ||
| const GURL kWebAppUrl = GURL("https://example.com"); | ||
|
|
||
| OsIntegrationSynchronizeCommandTest() { | ||
| if (GetParam() == OsIntegrationSubManagersState::kEnabled) { | ||
| scoped_feature_list_.InitWithFeaturesAndParameters( | ||
| {{features::kOsIntegrationSubManagers, {{"stage", "write_config"}}}}, | ||
| /*disabled_features=*/{}); | ||
| } else { | ||
| scoped_feature_list_.InitWithFeatures( | ||
| {}, {features::kOsIntegrationSubManagers}); | ||
| } | ||
| } | ||
|
|
||
| ~OsIntegrationSynchronizeCommandTest() override = default; | ||
|
|
||
| void SetUp() override { | ||
| WebAppTest::SetUp(); | ||
| { | ||
| base::ScopedAllowBlockingForTesting allow_blocking; | ||
| shortcut_override_ = | ||
| ShortcutOverrideForTesting::OverrideForTesting(base::GetHomeDir()); | ||
| } | ||
|
|
||
| provider_ = FakeWebAppProvider::Get(profile()); | ||
|
|
||
| auto file_handler_manager = | ||
| std::make_unique<WebAppFileHandlerManager>(profile()); | ||
| auto protocol_handler_manager = | ||
| std::make_unique<WebAppProtocolHandlerManager>(profile()); | ||
| auto shortcut_manager = std::make_unique<WebAppShortcutManager>( | ||
| profile(), /*icon_manager=*/nullptr, file_handler_manager.get(), | ||
| protocol_handler_manager.get()); | ||
| auto os_integration_manager = std::make_unique<OsIntegrationManager>( | ||
| profile(), std::move(shortcut_manager), std::move(file_handler_manager), | ||
| std::move(protocol_handler_manager), /*url_handler_manager=*/nullptr); | ||
|
|
||
| provider_->SetOsIntegrationManager(std::move(os_integration_manager)); | ||
|
|
||
| test::AwaitStartWebAppProviderAndSubsystems(profile()); | ||
| } | ||
|
|
||
| void TearDown() override { | ||
| EXPECT_TRUE(test::UninstallAllWebApps(profile())); | ||
| { | ||
| base::ScopedAllowBlockingForTesting allow_blocking; | ||
| shortcut_override_.reset(); | ||
| } | ||
| WebAppTest::TearDown(); | ||
| } | ||
|
|
||
| AppId InstallAppWithProtocolHandlers( | ||
| const std::vector<apps::ProtocolHandlerInfo>& protocol_handlers) { | ||
| auto info = std::make_unique<WebAppInstallInfo>(); | ||
| info->start_url = kWebAppUrl; | ||
| info->title = u"Test App"; | ||
| info->user_display_mode = web_app::UserDisplayMode::kStandalone; | ||
| info->protocol_handlers = protocol_handlers; | ||
|
|
||
| base::test::TestFuture<const AppId&, webapps::InstallResultCode> result; | ||
| // InstallFromInfo is used so that the DB states are updated but OS | ||
| // integration is not triggered. | ||
| provider()->scheduler().InstallFromInfo( | ||
| std::move(info), /*overwrite_existing_manifest_fields=*/true, | ||
| webapps::WebappInstallSource::OMNIBOX_INSTALL_ICON, | ||
| result.GetCallback()); | ||
| bool success = result.Wait(); | ||
| EXPECT_TRUE(success); | ||
| if (!success) { | ||
| return AppId(); | ||
| } | ||
| EXPECT_EQ(result.Get<webapps::InstallResultCode>(), | ||
| webapps::InstallResultCode::kSuccessNewInstall); | ||
| return result.Get<AppId>(); | ||
| } | ||
|
|
||
| protected: | ||
| WebAppProvider* provider() { return provider_; } | ||
|
|
||
| private: | ||
| raw_ptr<FakeWebAppProvider> provider_; | ||
| base::test::ScopedFeatureList scoped_feature_list_; | ||
| std::unique_ptr<ShortcutOverrideForTesting::BlockingRegistration> | ||
| shortcut_override_; | ||
| }; | ||
|
|
||
| TEST_P(OsIntegrationSynchronizeCommandTest, SynchronizeWorks) { | ||
| apps::ProtocolHandlerInfo protocol_handler; | ||
| const std::string handler_url = | ||
| std::string(kWebAppUrl.spec()) + "/testing=%s"; | ||
| protocol_handler.url = GURL(handler_url); | ||
| protocol_handler.protocol = "web+test"; | ||
| const AppId& app_id = InstallAppWithProtocolHandlers({protocol_handler}); | ||
|
|
||
| absl::optional<proto::WebAppOsIntegrationState> current_states = | ||
| provider()->registrar().GetAppCurrentOsIntegrationState(app_id); | ||
| ASSERT_FALSE(current_states.has_value()); | ||
|
|
||
| // OS Integration should be triggered now. | ||
| base::test::TestFuture<void> synchronize_future; | ||
| provider()->scheduler().SynchronizeOsIntegration( | ||
| app_id, synchronize_future.GetCallback()); | ||
| EXPECT_TRUE(synchronize_future.Wait()); | ||
|
|
||
| absl::optional<proto::WebAppOsIntegrationState> updated_states = | ||
| provider()->registrar().GetAppCurrentOsIntegrationState(app_id); | ||
| if (base::FeatureList::IsEnabled(features::kOsIntegrationSubManagers)) { | ||
| ASSERT_TRUE(updated_states.has_value()); | ||
| const proto::WebAppOsIntegrationState& os_integration_state = | ||
| updated_states.value(); | ||
|
|
||
| ASSERT_THAT(os_integration_state.manifest_protocol_handlers_states_size(), | ||
| testing::Eq(1)); | ||
|
|
||
| const proto::WebAppProtocolHandler& protocol_handler_state = | ||
| os_integration_state.manifest_protocol_handlers_states(0); | ||
|
|
||
| ASSERT_THAT(protocol_handler_state.protocol(), | ||
| testing::Eq(protocol_handler.protocol)); | ||
| ASSERT_THAT(protocol_handler_state.url(), testing::Eq(handler_url)); | ||
| } else { | ||
| ASSERT_FALSE(updated_states.has_value()); | ||
| } | ||
| } | ||
|
|
||
| INSTANTIATE_TEST_SUITE_P( | ||
| All, | ||
| OsIntegrationSynchronizeCommandTest, | ||
| ::testing::Values(OsIntegrationSubManagersState::kEnabled, | ||
| OsIntegrationSubManagersState::kDisabled), | ||
| test::GetOsIntegrationSubManagersTestName); | ||
|
|
||
| } // namespace | ||
| } // namespace web_app |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters