Skip to content

GitLab

T
tangled
Commit 5ca4558a authored by Mihai Sardarescu's avatar Mihai Sardarescu Committed by Commit Bot
Browse files
Options

Revert SyncDisableObserver to its pre-Unity version. 

This CL reverts all code in the SyncDisableObserver to its state
before http://crrev.com/c/1152744. It also disables all UKMBrowserTests
for Unified Consent enabled feature as this not supported anymore.

This is an attempt to verify whether http://crrev.com/c/1152744 is
the culprit from the drop in UKM collected metrics seen in bug 891777.

For reference - here is the diff of file sync_disable_observer.cc with the version
before http://crrev.com/c/1152744:
$ git diff 8f1b6d09 --  components/ukm/observers/sync_disable_observer.cc
diff --git a/components/ukm/observers/sync_disable_observer.cc b/components/ukm/observers/sync_disable_observer.cc
index f6d72866cfdd..beb5dac78eb5 100644
--- a/components/ukm/observers/sync_disable_observer.cc
+++ b/components/ukm/observers/sync_disable_observer.cc
@@ -68,7 +68,8 @@ SyncDisableObserver::SyncState SyncDisableObserver::GetSyncState(
 }

 void SyncDisableObserver::ObserveServiceForSyncDisables(
-    syncer::SyncService* sync_service) {
+    syncer::SyncService* sync_service,
+    PrefService* pref_service) {
   previous_states_[sync_service] = GetSyncState(sync_service);
   sync_observer_.Add(sync_service);
   UpdateAllProfileEnabled(false);

$ git diff 8f1b6d09 --  components/ukm/observers/sync_disable_observer.h
diff --git a/components/ukm/observers/sync_disable_observer.h b/components/ukm/observers/sync_disable_observer.h
index 2de46181f0f5..2b89842addcb 100644
--- a/components/ukm/observers/sync_disable_observer.h
+++ b/components/ukm/observers/sync_disable_observer.h
@@ -11,6 +11,8 @@
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/driver/sync_service_observer.h"

+class PrefService;
+
 namespace ukm {

 // Observes the state of a set of SyncServices for changes to history sync
@@ -23,7 +25,8 @@ class SyncDisableObserver : public syncer::SyncServiceObserver {
   ~SyncDisableObserver() override;

   // Starts observing a service for sync disables.
-  void ObserveServiceForSyncDisables(syncer::SyncService* sync_service);
+  void ObserveServiceForSyncDisables(syncer::SyncService* sync_service,
+                                     PrefService* pref_service);

   // Returns true iff sync is in a state that allows UKM to be enabled.
   // This means that for all profiles, sync is initialized, connected, has the

Bug: 891777

Change-Id: Ie55d4df1a66454918fa4931b3cc5000dade1d3e9
Reviewed-on: https://chromium-review.googlesource.com/c/1264781Reviewed-by: default avatarRobert Kaplow (sloooow) <rkaplow@chromium.org>
Reviewed-by: default avatarAlexei Svitkine <asvitkine@chromium.org>
Reviewed-by: default avatarBrian White <bcwhite@chromium.org>
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597298}
parent 28c2bad9
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 66 additions and 218 deletions
chrome/browser/metrics/ukm_browsertest.cc
View file @ 5ca4558a
......@@ -992,7 +992,10 @@ IN_PROC_BROWSER_TEST_P(UkmBrowserTest, HistoryDeleteCheck) {
}
// Run UKM browser test suite with Unified Consent enabled and disabled.
INSTANTIATE_TEST_CASE_P(, UkmBrowserTest, testing::Bool());
//
// Tests with Unified Consent enabled were temporarely disabled.
// (see http://crbug.com/891777).
INSTANTIATE_TEST_CASE_P(, UkmBrowserTest, ::testing::Values(false));
IN_PROC_BROWSER_TEST_P(UkmConsentParamBrowserTest, GroupPolicyConsentCheck) {
// Note we are not using the synthetic MetricsConsentOverride since we are
......@@ -1017,8 +1020,11 @@ IN_PROC_BROWSER_TEST_P(UkmConsentParamBrowserTest, GroupPolicyConsentCheck) {
}
// Verify UKM is enabled/disabled for both potential settings of group policy.
//
// Tests with Unified Consent enabled were temporarely disabled.
// (see http://crbug.com/891777).
INSTANTIATE_TEST_CASE_P(UkmConsentParamBrowserTests,
UkmConsentParamBrowserTest,
testing::Bool());
::testing::Values(false));
} // namespace metrics
components/ukm/observers/sync_disable_observer.h
View file @ 5ca4558a
......@@ -10,22 +10,16 @@
#include "base/scoped_observer.h"
#include "components/sync/driver/sync_service.h"
#include "components/sync/driver/sync_service_observer.h"
#include "components/unified_consent/url_keyed_data_collection_consent_helper.h"
class PrefService;
namespace ukm {
// Observer that monitors whether UKM is allowed for all profiles.
//
// For one profile, UKM is allowed under the following conditions:
// * If unified consent is disabled, then UKM is allowed for the profile iff
// sync history is active;
// * If unified consent is enabled, then UKM is allowed for the profile iff
// URL-keyed anonymized data collectiion is enabled.
class SyncDisableObserver
: public syncer::SyncServiceObserver,
public unified_consent::UrlKeyedDataCollectionConsentHelper::Observer {
// Observes the state of a set of SyncServices for changes to history sync
// preferences. This is for used to trigger purging of local state when
// sync is disabled on a profile and disabling recording when any non-syncing
// profiles are active.
class SyncDisableObserver : public syncer::SyncServiceObserver {
public:
SyncDisableObserver();
~SyncDisableObserver() override;
......@@ -34,13 +28,10 @@ class SyncDisableObserver
void ObserveServiceForSyncDisables(syncer::SyncService* sync_service,
PrefService* pref_service);
// Returns true iff all sync states alllow UKM to be enabled. This means that
// for all profiles:
// * If unified consent is disabled, then sync is initialized, connected, has
// the HISTORY_DELETE_DIRECTIVES data type enabled, and does not have a
// secondary passphrase enabled.
// * If unified consent is enabled, then URL-keyed anonymized data collection
// is enabled for that profile.
// Returns true iff sync is in a state that allows UKM to be enabled.
// This means that for all profiles, sync is initialized, connected, has the
// HISTORY_DELETE_DIRECTIVES data type enabled, and does not have a secondary
// passphrase enabled.
virtual bool SyncStateAllowsUkm();
// Returns true iff sync is in a state that allows UKM to capture extensions.
......@@ -58,11 +49,6 @@ class SyncDisableObserver
void OnStateChanged(syncer::SyncService* sync) override;
void OnSyncShutdown(syncer::SyncService* sync) override;
// unified_consent::UrlKeyedDataCollectionConsentHelper::Observer:
void OnUrlKeyedDataCollectionConsentStateChanged(
unified_consent::UrlKeyedDataCollectionConsentHelper* consent_helper)
override;
// Recomputes all_profiles_enabled_ state from previous_states_;
void UpdateAllProfileEnabled(bool must_purge);
......@@ -78,29 +64,8 @@ class SyncDisableObserver
ScopedObserver<syncer::SyncService, syncer::SyncServiceObserver>
sync_observer_;
enum class DataCollectionState {
// Matches the case when unified consent feature is disabled
kIgnored,
// Unified consent feature is enabled and the user has disabled URL-keyed
// anonymized data collection.
kDisabled,
// Unified consent feature is enabled and the user has enabled URL-keyed
// anonymized data collection.
kEnabled
};
// State data about sync services that we need to remember.
struct SyncState {
// Returns true if this sync state allows UKM:
// * If unified consent is disabled, then sync is initialized, connected,
// has history data type enabled, and does not have a secondary passphrase
// enabled.
// * If unified consent is enabled, then URL-keyed anonymized data
// collection is enabled.
bool AllowsUkm() const;
// Returns true if |AllowUkm| and if sync extensions are enabled.
bool AllowsUkmWithExtension() const;
// If the user has history sync enabled.
bool history_enabled = false;
// If the user has extension sync enabled.
......@@ -112,45 +77,21 @@ class SyncDisableObserver
// Whether user data is hidden by a secondary passphrase.
// This is not valid if the state is not initialized.
bool passphrase_protected = false;
// Whether anonymized data collection is enabled.
// Note: This is not managed by sync service. It was added in this enum
// for convenience.
DataCollectionState anonymized_data_collection_state =
DataCollectionState::kIgnored;
};
// Updates the sync state for |sync| service. Updates all profiles if needed.
void UpdateSyncState(
syncer::SyncService* sync,
unified_consent::UrlKeyedDataCollectionConsentHelper* consent_helper);
// Gets the current state of a SyncService.
// A non-null |consent_helper| implies that Unified Consent is enabled.
static SyncState GetSyncState(
syncer::SyncService* sync,
unified_consent::UrlKeyedDataCollectionConsentHelper* consent_helper);
static SyncState GetSyncState(syncer::SyncService* sync);
// The state of the sync services being observed.
// The list of services that had sync enabled when we last checked.
std::map<syncer::SyncService*, SyncState> previous_states_;
// The list of URL-keyed anonymized data collection consent helpers.
//
// Note: UrlKeyedDataCollectionConsentHelper do not rely on sync when
// unified consent feature is enabled but there must be exactly one per
// Chromium profile. As there is a single sync service per profile, it is safe
// to key them by sync service instead of introducing an additional map.
std::map<
syncer::SyncService*,
std::unique_ptr<unified_consent::UrlKeyedDataCollectionConsentHelper>>
consent_helpers_;
// Tracks if UKM is allowed on all profiles after the last state change.
bool all_sync_states_allow_ukm_ = false;
// Tracks if history sync was enabled on all profiles after the last state
// change.
bool all_histories_enabled_;
// Tracks if extension sync was enabled on all profiles after the last state
// change.
bool all_sync_states_allow_extension_ukm_ = false;
bool all_extensions_enabled_;
DISALLOW_COPY_AND_ASSIGN(SyncDisableObserver);
};
......
components/ukm/observers/sync_disable_observer_unittest.cc
View file @ 5ca4558a
......@@ -154,7 +154,7 @@ TEST_F(SyncDisableObserverTest, OneEnabled_UnifiedConsentDisabled) {
EXPECT_FALSE(observer.ResetPurged());
}
TEST_F(SyncDisableObserverTest, OneEnabled_UnifiedConsentEnabled) {
TEST_F(SyncDisableObserverTest, DISABLED_OneEnabled_UnifiedConsentEnabled) {
ScopedUnifiedConsent scoped_unified_consent(
UnifiedConsentFeatureState::kEnabledNoBump);
sync_preferences::TestingPrefServiceSyncable prefs;
......@@ -247,7 +247,7 @@ TEST_F(SyncDisableObserverTest, MixedProfiles2_UnifiedConsentDisabled) {
EXPECT_FALSE(observer.ResetPurged());
}
TEST_F(SyncDisableObserverTest, MixedProfiles_UnifiedConsentEnabled) {
TEST_F(SyncDisableObserverTest, DISABLED_MixedProfiles_UnifiedConsentEnabled) {
ScopedUnifiedConsent scoped_unified_consent(
UnifiedConsentFeatureState::kEnabledNoBump);
sync_preferences::TestingPrefServiceSyncable prefs1;
......@@ -283,7 +283,7 @@ TEST_F(SyncDisableObserverTest, TwoEnabled_UnifiedConsentDisabled) {
EXPECT_FALSE(observer.ResetPurged());
}
TEST_F(SyncDisableObserverTest, TwoEnabled_UnifiedConsentEnabled) {
TEST_F(SyncDisableObserverTest, DISABLED_TwoEnabled_UnifiedConsentEnabled) {
ScopedUnifiedConsent scoped_unified_consent(
UnifiedConsentFeatureState::kEnabledNoBump);
sync_preferences::TestingPrefServiceSyncable prefs1;
......@@ -323,7 +323,7 @@ TEST_F(SyncDisableObserverTest, OneAddRemove_UnifiedConsentDisabled) {
EXPECT_FALSE(observer.ResetPurged());
}
TEST_F(SyncDisableObserverTest, OneAddRemove_UnifiedConsentEnabled) {
TEST_F(SyncDisableObserverTest, DISABLED_OneAddRemove_UnifiedConsentEnabled) {
ScopedUnifiedConsent scoped_unified_consent(
UnifiedConsentFeatureState::kEnabledNoBump);
sync_preferences::TestingPrefServiceSyncable prefs;
......@@ -364,7 +364,7 @@ TEST_F(SyncDisableObserverTest, PurgeOnDisable_UnifiedConsentDisabled) {
EXPECT_FALSE(observer.ResetPurged());
}
TEST_F(SyncDisableObserverTest, PurgeOnDisable_UnifiedConsentEnabled) {
TEST_F(SyncDisableObserverTest, DISABLED_PurgeOnDisable_UnifiedConsentEnabled) {
ScopedUnifiedConsent scoped_unified_consent(
UnifiedConsentFeatureState::kEnabledNoBump);
......
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