Skip to content

Commit

Permalink
CodeHealth: Add method to set fake subsystems in FakeWebAppProvider
Browse files Browse the repository at this point in the history 
Using an actual provider with fakes set on it means less boilerplate
setting up various managers in many unit tests, and less need to
manually set subsystems or make sure the right manager is used in code
under test.
Ideally fakes would be set by default in
FakeWebAppProvider::BuildDefault but that breaks some tests so it can be
done in a separate CL.

This makes FakeWebAppRegistryController redundant. We should remove it
in a follow-up CL so there's one correct way to do things.

As a trial/example, removed WebAppRegistryController from
web_app_install_finalizer_unittest.cc and
web_app_translation_manager_unittest.cc

Also fixed to IWYU in these classes.

Bug: 973324
Change-Id: Iaafef580f434d6839712a788d1ec74e07734b7f1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3526299
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: Glen Robertson <glenrob@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984678}
  • Loading branch information
Glen Robertson authored and Chromium LUCI CQ committed Mar 24, 2022
1 parent ebb8059 commit aaddfad
Show file tree
Hide file tree
Showing 8 changed files with 197 additions and 125 deletions.
1 change: 1 addition & 0 deletions chrome/browser/web_applications/extensions/externally_managed_app_install_task_unittest.cc
View file
Expand Up @@ -39,6 +39,7 @@
#include "chrome/browser/web_applications/web_app_install_manager.h"
#include "chrome/browser/web_applications/web_app_provider.h"
#include "chrome/browser/web_applications/web_app_registrar.h"
#include "chrome/browser/web_applications/web_app_sync_bridge.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/pref_names.h"
#include "chrome/test/base/chrome_render_view_host_test_harness.h"
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/web_applications/manifest_update_manager_browsertest.cc
View file
Expand Up @@ -47,6 +47,7 @@
#include "chrome/browser/web_applications/web_app_provider.h"
#include "chrome/browser/web_applications/web_app_registrar.h"
#include "chrome/browser/web_applications/web_app_registry_update.h"
#include "chrome/browser/web_applications/web_app_sync_bridge.h"
#include "chrome/browser/web_applications/web_app_utils.h"
#include "chrome/common/chrome_features.h"
#include "chrome/test/base/in_process_browser_test.h"
Expand Down
116 changes: 99 additions & 17 deletions chrome/browser/web_applications/test/fake_web_app_provider.cc
View file
Expand Up @@ -4,23 +4,41 @@

#include "chrome/browser/web_applications/test/fake_web_app_provider.h"

#include <memory>
#include <ostream>
#include <utility>

#include "base/bind.h"
#include "base/check.h"
#include "base/command_line.h"
#include "base/memory/scoped_refptr.h"
#include "base/one_shot_event.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/web_applications/externally_managed_app_manager.h"
#include "chrome/browser/web_applications/os_integration/os_integration_manager.h"
#include "chrome/browser/web_applications/policy/web_app_policy_manager.h"
#include "chrome/browser/web_applications/system_web_apps/system_web_app_manager.h"
#include "chrome/browser/web_applications/system_web_apps/test/test_system_web_app_manager.h"
#include "chrome/browser/web_applications/test/fake_externally_managed_app_manager.h"
#include "chrome/browser/web_applications/test/fake_os_integration_manager.h"
#include "chrome/browser/web_applications/test/fake_web_app_database_factory.h"
#include "chrome/browser/web_applications/test/fake_web_app_ui_manager.h"
#include "chrome/browser/web_applications/test/test_file_utils.h"
#include "chrome/browser/web_applications/web_app_database_factory.h"
#include "chrome/browser/web_applications/web_app_icon_manager.h"
#include "chrome/browser/web_applications/web_app_install_finalizer.h"
#include "chrome/browser/web_applications/web_app_install_manager.h"
#include "chrome/browser/web_applications/web_app_provider_factory.h"
#include "chrome/browser/web_applications/web_app_registrar.h"
#include "chrome/browser/web_applications/web_app_sync_bridge.h"
#include "chrome/browser/web_applications/web_app_translation_manager.h"
#include "chrome/browser/web_applications/web_app_ui_manager.h"
#include "chrome/browser/web_applications/web_app_utils.h"
#include "chrome/common/chrome_switches.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h"
#include "components/keyed_service/core/keyed_service.h"
#include "components/sync/test/model/mock_model_type_change_processor.h"
#include "testing/gmock/include/gmock/gmock.h"

namespace web_app {

Expand All @@ -33,7 +51,7 @@ std::unique_ptr<KeyedService> FakeWebAppProvider::BuildDefault(
// Do not call default production StartImpl if in TestingProfile.
provider->SetRunSubsystemStartupTasks(false);

// TODO(crbug.com/973324): Replace core subsystems with fakes by default.
// TODO(crbug.com/973324): `SetDefaultFakeSubsystems` by default.
provider->ConnectSubsystems();

return provider;
Expand Down Expand Up @@ -69,12 +87,36 @@ void FakeWebAppProvider::SetRegistrar(
registrar_ = std::move(registrar);
}

void FakeWebAppProvider::SetDatabaseFactory(
std::unique_ptr<AbstractWebAppDatabaseFactory> database_factory) {
CheckNotStarted();
database_factory_ = std::move(database_factory_);
}

void FakeWebAppProvider::SetSyncBridge(
std::unique_ptr<WebAppSyncBridge> sync_bridge) {
CheckNotStarted();
sync_bridge_ = std::move(sync_bridge);
}

void FakeWebAppProvider::SetIconManager(
std::unique_ptr<WebAppIconManager> icon_manager) {
CheckNotStarted();
icon_manager_ = std::move(icon_manager);
}

void FakeWebAppProvider::SetTranslationManager(
std::unique_ptr<WebAppTranslationManager> translation_manager) {
CheckNotStarted();
translation_manager_ = std::move(translation_manager);
}

void FakeWebAppProvider::SetOsIntegrationManager(
std::unique_ptr<OsIntegrationManager> os_integration_manager) {
CheckNotStarted();
os_integration_manager_ = std::move(os_integration_manager);
}

void FakeWebAppProvider::SetInstallManager(
std::unique_ptr<WebAppInstallManager> install_manager) {
CheckNotStarted();
Expand Down Expand Up @@ -112,10 +154,19 @@ void FakeWebAppProvider::SetWebAppPolicyManager(
web_app_policy_manager_ = std::move(web_app_policy_manager);
}

void FakeWebAppProvider::SetOsIntegrationManager(
std::unique_ptr<OsIntegrationManager> os_integration_manager) {
CheckNotStarted();
os_integration_manager_ = std::move(os_integration_manager);
WebAppRegistrarMutable& FakeWebAppProvider::GetRegistrarMutable() const {
DCHECK(registrar_);
return *static_cast<WebAppRegistrarMutable*>(registrar_.get());
}

WebAppIconManager& FakeWebAppProvider::GetIconManager() const {
DCHECK(icon_manager_);
return *icon_manager_;
}

AbstractWebAppDatabaseFactory& FakeWebAppProvider::GetDatabaseFactory() const {
DCHECK(database_factory_);
return *database_factory_;
}

void FakeWebAppProvider::SkipAwaitingExtensionSystem() {
Expand All @@ -128,24 +179,55 @@ void FakeWebAppProvider::StartWithSubsystems() {
SetRunSubsystemStartupTasks(true);
// Use a TestSystemWebAppManager to skip system web apps being
// auto-installed on |Start|.
// TODO(crbug.com/973324): This is set in `SetDefaultFakeSubsystems`. Remove
// it from here.
SetSystemWebAppManager(
std::make_unique<web_app::TestSystemWebAppManager>(profile_.get()));
std::make_unique<web_app::TestSystemWebAppManager>(profile_));
Start();
}

WebAppRegistrarMutable& FakeWebAppProvider::GetRegistrarMutable() const {
DCHECK(registrar_);
return *static_cast<WebAppRegistrarMutable*>(registrar_.get());
}
void FakeWebAppProvider::SetDefaultFakeSubsystems() {
// Disable preinstalled apps by default as they add noise and time to tests
// that don't need them.
base::CommandLine::ForCurrentProcess()->AppendSwitch(
switches::kDisablePreinstalledApps);

WebAppIconManager& FakeWebAppProvider::GetIconManager() const {
DCHECK(icon_manager_);
return *icon_manager_;
}
// Default to not wait for a test extension system, that is usually never
// started in web app tests.
SkipAwaitingExtensionSystem();

AbstractWebAppDatabaseFactory& FakeWebAppProvider::GetDatabaseFactory() const {
DCHECK(database_factory_);
return *database_factory_;
SetRegistrar(std::make_unique<WebAppRegistrarMutable>(profile_));
SetDatabaseFactory(std::make_unique<FakeWebAppDatabaseFactory>());

SetOsIntegrationManager(std::make_unique<FakeOsIntegrationManager>(
profile_, /*app_shortcut_manager=*/nullptr,
/*file_handler_manager=*/nullptr,
/*protocol_handler_manager=*/nullptr,
/*url_handler_manager=*/nullptr));

SetSyncBridge(std::make_unique<WebAppSyncBridge>(
&GetRegistrarMutable(), processor().CreateForwardingProcessor()));

SetIconManager(std::make_unique<WebAppIconManager>(
profile_, base::MakeRefCounted<TestFileUtils>()));

SetTranslationManager(std::make_unique<WebAppTranslationManager>(
profile_, base::MakeRefCounted<TestFileUtils>()));

SetWebAppUiManager(std::make_unique<FakeWebAppUiManager>());

SetExternallyManagedAppManager(
std::make_unique<FakeExternallyManagedAppManager>(profile_));

SetWebAppPolicyManager(std::make_unique<WebAppPolicyManager>(profile_));

// Use a TestSystemWebAppManager to skip system web apps being
// auto-installed on |Start|.
SetSystemWebAppManager(
std::make_unique<web_app::TestSystemWebAppManager>(profile_));

ON_CALL(processor(), IsTrackingMetadata())
.WillByDefault(testing::Return(true));
}

void FakeWebAppProvider::CheckNotStarted() const {
Expand Down
32 changes: 25 additions & 7 deletions chrome/browser/web_applications/test/fake_web_app_provider.h
View file
Expand Up @@ -8,11 +8,13 @@
#include <memory>

#include "base/callback.h"
#include "base/callback_forward.h"
#include "base/callback_list.h"
#include "chrome/browser/web_applications/web_app_provider.h"
#include "chrome/browser/web_applications/web_app_registrar.h"
#include "chrome/browser/web_applications/web_app_sync_bridge.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h"
#include "components/sync/test/model/mock_model_type_change_processor.h"
#include "testing/gmock/include/gmock/gmock.h"

class KeyedService;
class Profile;

namespace content {
Expand All @@ -30,6 +32,10 @@ class SystemWebAppManager;
class WebAppInstallManager;
class WebAppPolicyManager;
class WebAppIconManager;
class WebAppTranslationManager;
class WebAppRegistrarMutable;
class WebAppSyncBridge;
class WebAppUiManager;

class FakeWebAppProvider : public WebAppProvider {
public:
Expand Down Expand Up @@ -58,7 +64,12 @@ class FakeWebAppProvider : public WebAppProvider {
// NB: If you replace the Registrar, you also have to replace the SyncBridge
// accordingly.
void SetRegistrar(std::unique_ptr<WebAppRegistrar> registrar);
void SetDatabaseFactory(
std::unique_ptr<AbstractWebAppDatabaseFactory> database_factory);
void SetSyncBridge(std::unique_ptr<WebAppSyncBridge> sync_bridge);
void SetIconManager(std::unique_ptr<WebAppIconManager> icon_manager);
void SetTranslationManager(
std::unique_ptr<WebAppTranslationManager> translation_manager);
void SetOsIntegrationManager(
std::unique_ptr<OsIntegrationManager> os_integration_manager);
void SetInstallManager(std::unique_ptr<WebAppInstallManager> install_manager);
Expand All @@ -72,10 +83,6 @@ class FakeWebAppProvider : public WebAppProvider {
std::unique_ptr<SystemWebAppManager> system_web_app_manager);
void SetWebAppPolicyManager(
std::unique_ptr<WebAppPolicyManager> web_app_policy_manager);
void SkipAwaitingExtensionSystem();
// Starts this WebAppProvider and its subsystems. It does not wait for systems
// to be ready.
void StartWithSubsystems();

// These getters can be called at any time: no
// WebAppProvider::CheckIsConnected() check performed. See
Expand All @@ -86,6 +93,16 @@ class FakeWebAppProvider : public WebAppProvider {
WebAppIconManager& GetIconManager() const;
AbstractWebAppDatabaseFactory& GetDatabaseFactory() const;

void SkipAwaitingExtensionSystem();
// Starts this WebAppProvider and its subsystems. It does not wait for systems
// to be ready.
void StartWithSubsystems();

// Create and set default fake subsystems.
void SetDefaultFakeSubsystems();

syncer::MockModelTypeChangeProcessor& processor() { return mock_processor_; }

private:
void CheckNotStarted() const;

Expand All @@ -96,6 +113,7 @@ class FakeWebAppProvider : public WebAppProvider {
// WebAppProvider::StartImpl() and fire startup tasks like a real
// WebAppProvider.
bool run_subsystem_startup_tasks_ = true;
testing::NiceMock<syncer::MockModelTypeChangeProcessor> mock_processor_;
};

// Used in BrowserTests to ensure that the WebAppProvider that is create on
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/web_applications/test/fake_web_app_registry_controller.h
View file
Expand Up @@ -29,6 +29,7 @@ class WebAppTranslationManager;
class WebApp;
class WebAppPolicyManager;

// Deprecated: Prefer to use `FakeWebAppProvider::SetDefaultFakeSubsystems()`.
class FakeWebAppRegistryController : public SyncInstallDelegate {
public:
FakeWebAppRegistryController();
Expand Down

0 comments on commit aaddfad

Please sign in to comment.