Skip to content

Commit

Permalink
Simplify DriveIntegrationService
Browse files Browse the repository at this point in the history 
Preparing the removal of the nested PrefWatcher class.
PrefWatcher does not implement NetworkStateHandlerObserver anymore.
Added DriveIntegrationService::registrar_.
DriveIntegrationService now inherits from NetworkStateHandlerObserver.
Renamed DriveIntegrationService::UpdateNetworkState() -> OnNetworkChanged().
Inlined DriveIntegrationService::GetPrefs()

BUG=b:298306347

Change-Id: Idf5aa5845529630ec3d6274d96c73cdefc5664c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4837607
Commit-Queue: François Degros <fdegros@chromium.org>
Commit-Queue: Ben Reich <benreich@chromium.org>
Auto-Submit: François Degros <fdegros@chromium.org>
Reviewed-by: Ben Reich <benreich@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1191963}
  • Loading branch information
@fdegros
fdegros authored and Chromium LUCI CQ committed Sep 4, 2023
1 parent 5daf763 commit 4a50d67
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 106 deletions.
164 changes: 70 additions & 94 deletions chrome/browser/ash/drive/drive_integration_service.cc
View file
Expand Up @@ -51,10 +51,6 @@
#include "chromeos/ash/components/drivefs/drivefs_bootstrap.h"
#include "chromeos/ash/components/drivefs/drivefs_pin_manager.h"
#include "chromeos/ash/components/drivefs/sync_status_tracker.h"
#include "chromeos/ash/components/network/network_handler.h"
#include "chromeos/ash/components/network/network_state.h"
#include "chromeos/ash/components/network/network_state_handler.h"
#include "chromeos/ash/components/network/network_state_handler_observer.h"
#include "chromeos/components/drivefs/mojom/drivefs_native_messaging.mojom.h"
#include "chromeos/constants/chromeos_features.h"
#include "chromeos/crosapi/mojom/drive_integration_service.mojom.h"
Expand Down Expand Up @@ -95,6 +91,7 @@ using drivefs::pinning::PinManager;
using network::NetworkConnectionTracker;
using network::mojom::ConnectionType;
using prefs::kDriveFsBulkPinningEnabled;
using util::ConnectionStatus;

// Name of the directory used to store metadata.
const base::FilePath::CharType kMetadataDirectory[] = FILE_PATH_LITERAL("meta");
Expand Down Expand Up @@ -384,108 +381,51 @@ void RecordBulkPinningMountFailureReason(const Profile* profile,
} // namespace

// Observes changes in Drive's Preferences and network connections.
class DriveIntegrationService::PrefWatcher : ash::NetworkStateHandlerObserver {
class DriveIntegrationService::PrefWatcher {
public:
using NetworkHandler = ash::NetworkHandler;
using NetworkState = ash::NetworkState;
using NetworkStateHandler = ash::NetworkStateHandler;

explicit PrefWatcher(DriveIntegrationService* const service)
: service_(service) {
DCHECK(service_);
pref_change_registrar_.Init(service_->GetPrefs());
pref_change_registrar_.Add(
service_->registrar_.Init(service_->GetPrefs());
service_->registrar_.Add(
prefs::kDisableDrive,
base::BindRepeating(&PrefWatcher::ToggleDrive,
base::BindRepeating(&PrefWatcher::OnDrivePrefChanged,
weak_ptr_factory_.GetWeakPtr()));
pref_change_registrar_.Add(
service_->registrar_.Add(
prefs::kDisableDriveOverCellular,
base::BindRepeating(&PrefWatcher::ToggleMetered,
weak_ptr_factory_.GetWeakPtr()));
base::BindRepeating(&DriveIntegrationService::OnNetworkChanged,
service_->weak_ptr_factory_.GetWeakPtr()));
if (ash::features::IsDriveFsMirroringEnabled()) {
pref_change_registrar_.Add(
service_->registrar_.Add(
prefs::kDriveFsEnableMirrorSync,
base::BindRepeating(&PrefWatcher::ToggleMirroring,
base::BindRepeating(&PrefWatcher::OnMirroringPrefChanged,
weak_ptr_factory_.GetWeakPtr()));
}

if (util::IsDriveFsBulkPinningEnabled(service_->profile_)) {
pref_change_registrar_.Add(
service_->registrar_.Add(
kDriveFsBulkPinningEnabled,
base::BindRepeating(&PrefWatcher::ToggleBulkPinning,
weak_ptr_factory_.GetWeakPtr()));
}

DCHECK(!network_state_handler_);
if (!NetworkHandler::IsInitialized()) {
DCHECK(!service_->network_state_handler_);
if (!ash::NetworkHandler::IsInitialized()) {
return; // Test environment.
}

network_state_handler_ = NetworkHandler::Get()->network_state_handler();
DCHECK(network_state_handler_);
network_state_handler_->AddObserver(this);
}

~PrefWatcher() override {
if (network_state_handler_) {
network_state_handler_->RemoveObserver(this);
}
service_->network_state_handler_ =
ash::NetworkHandler::Get()->network_state_handler();
DCHECK(service_->network_state_handler_);
service_->network_state_handler_->AddObserver(service_);
}

private:
void ToggleMetered() {
VLOG(1) << "ToggleMetered";
service_->UpdateNetworkState();
}

void ToggleDrive() {
VLOG(1) << "ToggleDrive";
service_->SetEnabled(
!service_->GetPrefs()->GetBoolean(prefs::kDisableDrive));
}

void ToggleMirroring() {
VLOG(1) << "ToggleMirroring";
if (!ash::features::IsDriveFsMirroringEnabled()) {
return;
}

if (service_->GetPrefs()->GetBoolean(prefs::kDriveFsEnableMirrorSync)) {
service_->ToggleMirroring(
true, base::BindOnce(
&DriveIntegrationService::OnEnableMirroringStatusUpdate,
service_->weak_ptr_factory_.GetWeakPtr()));
} else {
service_->ToggleMirroring(
false, base::BindOnce(
&DriveIntegrationService::OnDisableMirroringStatusUpdate,
service_->weak_ptr_factory_.GetWeakPtr()));
}
}

void ToggleBulkPinning() {
VLOG(1) << "ToggleBulkPinning";
service_->ToggleBulkPinning();
}

void DefaultNetworkChanged(const NetworkState*) override {
VLOG(1) << "DefaultNetworkChanged";
service_->UpdateNetworkState();
}

void OnShuttingDown() override {
VLOG(1) << "OnShuttingDown";
if (network_state_handler_) {
network_state_handler_->RemoveObserver(this);
network_state_handler_ = nullptr;
}
}
void OnDrivePrefChanged() { service_->OnDrivePrefChanged(); }
void OnMirroringPrefChanged() { service_->OnMirroringPrefChanged(); }
void ToggleBulkPinning() { service_->ToggleBulkPinning(); }

const raw_ptr<DriveIntegrationService, ExperimentalAsh> service_;
PrefChangeRegistrar pref_change_registrar_;
raw_ptr<NetworkStateHandler, ExperimentalAsh> network_state_handler_ =
nullptr;

base::WeakPtrFactory<PrefWatcher> weak_ptr_factory_{this};
};

Expand Down Expand Up @@ -680,7 +620,7 @@ class DriveIntegrationService::DriveFsHolder
};

DriveIntegrationService::DriveIntegrationService(
Profile* profile,
Profile* const profile,
const std::string& test_mount_point_name,
const base::FilePath& test_cache_root,
DriveFsMojoListenerFactory test_drivefs_mojo_listener_factory)
Expand All @@ -690,7 +630,7 @@ DriveIntegrationService::DriveIntegrationService(
? test_cache_root
: util::GetCacheRootPath(profile)),
drivefs_holder_(std::make_unique<DriveFsHolder>(
profile_,
profile,
this,
std::move(test_drivefs_mojo_listener_factory))) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
Expand All @@ -701,7 +641,7 @@ DriveIntegrationService::DriveIntegrationService(
base::WithBaseSyncPrimitives()});

if (util::IsDriveAvailableForProfile(profile)) {
pref_watcher_ = std::make_unique<PrefWatcher>(this);
pref_watcher_ = std::make_unique<const PrefWatcher>(this);
}

bool migrated_to_drivefs =
Expand All @@ -714,15 +654,14 @@ DriveIntegrationService::DriveIntegrationService(
blocking_task_runner_.get()));
}

SetEnabled(drive::util::IsDriveEnabledForProfile(profile));
SetEnabled(util::IsDriveEnabledForProfile(profile));
}

DriveIntegrationService::~DriveIntegrationService() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
}

PrefService* DriveIntegrationService::GetPrefs() const {
return profile_->GetPrefs();
if (network_state_handler_) {
network_state_handler_->RemoveObserver(this);
}
}

void DriveIntegrationService::Shutdown() {
Expand Down Expand Up @@ -1022,7 +961,7 @@ bool DriveIntegrationService::AddDriveMountPointAfterMounted() {
}
}

UpdateNetworkState();
OnNetworkChanged();

if (!GetPrefs()->GetBoolean(prefs::kDriveFsPinnedMigrated)) {
MigratePinnedFiles();
Expand Down Expand Up @@ -1302,6 +1241,7 @@ void DriveIntegrationService::PinFiles(
}

void DriveIntegrationService::ToggleBulkPinning() {
VLOG(1) << "ToggleBulkPinning";
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (!pin_manager_) {
VLOG(1) << "No bulk-pinning manager";
Expand Down Expand Up @@ -1743,12 +1683,11 @@ void DriveIntegrationService::GetDocsOfflineStats(
GetDriveFsInterface()->GetDocsOfflineStats(std::move(callback));
}

void DriveIntegrationService::UpdateNetworkState() {
const util::ConnectionStatus status =
util::GetDriveConnectionStatus(profile_);
VLOG(1) << "UpdateNetworkState: " << status;
void DriveIntegrationService::OnNetworkChanged() {
const ConnectionStatus status = util::GetDriveConnectionStatus(profile_);
VLOG(1) << "OnNetworkChanged: " << status;

using enum util::ConnectionStatus;
using enum ConnectionStatus;
is_online_ = status == kMetered || status == kConnected;

if (DriveFs* const drivefs = GetDriveFsInterface()) {
Expand All @@ -1771,6 +1710,43 @@ void DriveIntegrationService::UpdateNetworkState() {
}
}

void DriveIntegrationService::OnDrivePrefChanged() {
VLOG(1) << "OnDrivePrefChanged";
SetEnabled(!GetPrefs()->GetBoolean(prefs::kDisableDrive));
}

void DriveIntegrationService::OnMirroringPrefChanged() {
VLOG(1) << "OnMirroringPrefChanged";
if (!ash::features::IsDriveFsMirroringEnabled()) {
return;
}

if (GetPrefs()->GetBoolean(prefs::kDriveFsEnableMirrorSync)) {
ToggleMirroring(
true,
base::BindOnce(&DriveIntegrationService::OnEnableMirroringStatusUpdate,
weak_ptr_factory_.GetWeakPtr()));
} else {
ToggleMirroring(
false,
base::BindOnce(&DriveIntegrationService::OnDisableMirroringStatusUpdate,
weak_ptr_factory_.GetWeakPtr()));
}
}

void DriveIntegrationService::DefaultNetworkChanged(const ash::NetworkState*) {
VLOG(1) << "DefaultNetworkChanged";
OnNetworkChanged();
}

void DriveIntegrationService::OnShuttingDown() {
VLOG(1) << "OnShuttingDown";
if (network_state_handler_) {
network_state_handler_->RemoveObserver(this);
network_state_handler_ = nullptr;
}
}

//===================== DriveIntegrationServiceFactory =======================

DriveIntegrationServiceFactory::FactoryCallback*
Expand Down
37 changes: 26 additions & 11 deletions chrome/browser/ash/drive/drive_integration_service.h
View file
Expand Up @@ -18,20 +18,24 @@
#include "base/observer_list_types.h"
#include "base/time/time.h"
#include "chrome/browser/ash/drive/file_system_util.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_keyed_service_factory.h"
#include "chromeos/ash/components/drivefs/drivefs_host.h"
#include "chromeos/ash/components/drivefs/drivefs_pin_manager.h"
#include "chromeos/ash/components/drivefs/sync_status_tracker.h"
#include "chromeos/ash/components/network/network_state.h"
#include "chromeos/ash/components/network/network_state_handler.h"
#include "chromeos/ash/components/network/network_state_handler_observer.h"
#include "chromeos/crosapi/mojom/drive_integration_service.mojom.h"
#include "components/drive/event_logger.h"
#include "components/drive/file_errors.h"
#include "components/drive/file_system_core_util.h"
#include "components/keyed_service/core/keyed_service.h"
#include "components/prefs/pref_change_registrar.h"
#include "google_apis/common/api_error_codes.h"
#include "google_apis/common/auth_service_interface.h"
#include "mojo/public/cpp/bindings/pending_remote.h"

class Profile;
class PrefService;

namespace base {
Expand Down Expand Up @@ -113,7 +117,8 @@ class DriveIntegrationServiceObserver : public base::CheckedObserver {
// created per-profile.
class DriveIntegrationService : public KeyedService,
public drivefs::DriveFsHost::MountObserver,
public drivefs::pinning::PinManager::Observer {
drivefs::pinning::PinManager::Observer,
ash::NetworkStateHandlerObserver {
public:
using DriveFsMojoListenerFactory = base::RepeatingCallback<
std::unique_ptr<drivefs::DriveFsBootstrapListener>()>;
Expand Down Expand Up @@ -143,7 +148,7 @@ class DriveIntegrationService : public KeyedService,

~DriveIntegrationService() override;

// KeyedService override:
// KeyedService override.
void Shutdown() override;

void SetEnabled(bool enabled);
Expand Down Expand Up @@ -336,7 +341,7 @@ class DriveIntegrationService : public KeyedService,
void GetDocsOfflineStats(
drivefs::mojom::DriveFs::GetDocsOfflineStatsCallback callback);

void UpdateNetworkState();
void OnNetworkChanged();

private:
enum class State {
Expand All @@ -348,7 +353,7 @@ class DriveIntegrationService : public KeyedService,

class DriveFsHolder;

PrefService* GetPrefs() const;
PrefService* GetPrefs() const { return profile_->GetPrefs(); }

// Returns true if Drive is enabled.
// Must be called on UI thread.
Expand Down Expand Up @@ -457,22 +462,30 @@ class DriveIntegrationService : public KeyedService,
drive::FileError error,
int64_t total_size);

void OnDrivePrefChanged();
void OnMirroringPrefChanged();

// NetworkStateHandlerObserver implementation.
void DefaultNetworkChanged(const ash::NetworkState*) override;
void OnShuttingDown() override;

friend class DriveIntegrationServiceFactory;

const raw_ptr<Profile, ExperimentalAsh> profile_;

State state_ = State::kNone;
bool enabled_ = false;
bool mount_failed_ = false;
bool in_clear_cache_ = false;

// Is the bulk-pinning preference sampling task currently scheduled?
bool bulk_pinning_pref_sampling_ = false;
bool mirroring_enabled_ = false;
bool is_online_ = true;
bool remount_when_online_ = false;

// Custom mount point name that can be injected for testing in constructor.
std::string mount_point_name_;

bool mirroring_enabled_ = false;

base::FilePath cache_root_directory_;
EventLogger logger_;
scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_;
Expand All @@ -483,14 +496,16 @@ class DriveIntegrationService : public KeyedService,

std::unique_ptr<DriveFsHolder> drivefs_holder_;

PrefChangeRegistrar registrar_;
raw_ptr<ash::NetworkStateHandler, ExperimentalAsh> network_state_handler_ =
nullptr;

class PrefWatcher;
std::unique_ptr<PrefWatcher> pref_watcher_;
std::unique_ptr<const PrefWatcher> pref_watcher_;
std::unique_ptr<PinManager> pin_manager_;

int drivefs_total_failures_count_ = 0;
int drivefs_consecutive_failures_count_ = 0;
bool is_online_ = true;
bool remount_when_online_ = false;

// Used to fetch authentication and refresh tokens from Drive.
std::unique_ptr<google_apis::AuthServiceInterface> auth_service_;
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/webui/ash/cloud_upload/drive_upload_handler_browsertest.cc
View file
Expand Up @@ -513,7 +513,7 @@ IN_PROC_BROWSER_TEST_F(DriveUploadHandlerTest,
content::NetworkConnectionChangeSimulator().SetConnectionType(
CONNECTION_NONE);
SetDriveConnectionStatusForTesting(ConnectionStatus::kNoNetwork);
drive_integration_service()->UpdateNetworkState();
drive_integration_service()->OnNetworkChanged();
});

base::RunLoop run_loop;
Expand Down

0 comments on commit 4a50d67

Please sign in to comment.