Skip to content

Commit

Permalink
Revert "CrOS Settings: Clearing pref when UMA consent is revoked."
Browse files Browse the repository at this point in the history
This reverts commit 72dd284.

Reason for revert: Failed CHECK() for GAIA test accounts b/288018127

Original change's description:
> CrOS Settings: Clearing pref when UMA consent is revoked.
>
> Clearing the kTotalUniqueOsSettingsChanged pref when the user has
> revoked consent for UMA. We will not store the unique Settings, and we
> will not record data to the histograms.
> If the user grants consent for UMA, we will keep the state that the user
> has revoked consent before and we will not record their data in the
> histogram
> ChromeOS.Settings.NumUniqueSettingsChanged.DeviceLifetime.{time}.
> We will not record the data again once the user has granted consent
> to increase the accuracy of the data that we are recording.
>
> dd: go/cros-settings-revamp-metrics-design-doc
>
> NOTE: This CL will have a follow-up that will convert the
> unittests per_session_settings_user_action_tracker_unittest.cc and
> settings_user_action_tracker_unittest.cc to browser tests. They need
> to get converted as we are now using g_browser_process in
> PerSessionSettingsUserActionTracker which can only be tested using
> browser tests.
>
> Bug= b/285021579
> TEST=
> Manual:
>   All tests must be taken with a user that has run OOBE.
>
>   Histograms:
>    ChromeOS.Settings.NumUniqueSettingsChanged.DeviceLifetime.{time}
>         {time} = one of {FirstWeek, SubsequentWeeks, Total}
>         {time} is based on the time passed since the user has taken
> 	OOBE, .FirstWeek and .SubsequentWeeks. The total of first and
>         subsequent weeks will be recorded in .Total histogram.
>   A) Open Settings, grant UMA consent. Histogram will record
>   the data if any.
>   B) Open Settings, revoke UMA consent. Histogram will no
>   longer record the data, until the user grants UMA consent again.
>   C) Open Settings, grant UMA consent again. Case B will repeat again
>   since we will not record data if the user has revoked their consent at
>   least once in the lifetime of the device.
>
> Automated: Unittest: DISPLAY=:20 out/Default/unit_tests
>   --gtest_filter="*SettingsUserActionTrackerTest*"
>
> Change-Id: Ie973044ab375082d2e5272da4ce26b6f858ae08d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4574214
> Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Commit-Queue: Nikki Moteva <moteva@google.com>
> Cr-Commit-Position: refs/heads/main@{#1159212}

(cherry picked from commit 4cff84f)

Bug: b:285021579, b:288201067
No-Try: true
Change-Id: I187ed28fd47face86435aa7fe1ff1685e27a9fd7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4630790
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Fergus Dall <sidereal@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Auto-Submit: Wes Okuhara <wesokuhara@google.com>
Commit-Queue: Fergus Dall <sidereal@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1160502}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4643168
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5845@{#61}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
  • Loading branch information
Wes Okuhara authored and Chromium LUCI CQ committed Jun 24, 2023
1 parent b259efa commit ecc71d2
Show file tree
Hide file tree
Showing 12 changed files with 888 additions and 113 deletions.
2 changes: 2 additions & 0 deletions chrome/browser/ash/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -4792,10 +4792,12 @@ source_set("unit_tests") {
# when PDF OCR becomes available on windows, macOS, and linux.
"../ui/webui/settings/ash/pdf_ocr_handler_unittest.cc",
"../ui/webui/settings/ash/privacy_hub_handler_unittest.cc",
"../ui/webui/settings/ash/search/per_session_settings_user_action_tracker_unittest.cc",
"../ui/webui/settings/ash/search/search_handler_unittest.cc",
"../ui/webui/settings/ash/search/search_tag_registry_unittest.cc",
"../ui/webui/settings/ash/search_engines_handler_unittest.cc",
"../ui/webui/settings/ash/send_search_feedback_handler_unittest.cc",
"../ui/webui/settings/ash/settings_user_action_tracker_unittest.cc",
"../ui/webui/settings/chromeos/constants/routes_util_unittest.cc",
"accessibility/pumpkin_installer_unittest.cc",
"account_manager/account_apps_availability_unittest.cc",
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/ash/preferences.cc
Original file line number Diff line number Diff line change
Expand Up @@ -603,8 +603,6 @@ void Preferences::RegisterProfilePrefs(

registry->RegisterBooleanPref(::prefs::kHasResetFirst7DaysSettingsUsedCount,
false);

registry->RegisterBooleanPref(::prefs::kHasEverRevokedMetricsConsent, true);
}

void Preferences::InitUserPrefs(sync_preferences::PrefServiceSyncable* prefs) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
#include "base/metrics/histogram_functions.h"
#include "base/strings/string_number_conversions.h"
#include "chrome/browser/ash/login/login_pref_names.h"
#include "chrome/browser/browser_process.h"
#include "chrome/common/pref_names.h"
#include "components/metrics/metrics_service.h"
#include "components/prefs/scoped_user_pref_update.h"

namespace ash::settings {
Expand Down Expand Up @@ -46,9 +44,7 @@ PerSessionSettingsUserActionTracker::PerSessionSettingsUserActionTracker(
PrefService* pref_service)
: metric_start_time_(base::TimeTicks::Now()),
window_last_active_timestamp_(base::TimeTicks::Now()),
pref_service_(pref_service) {
DCHECK(pref_service_);
}
pref_service_(pref_service) {}

PerSessionSettingsUserActionTracker::~PerSessionSettingsUserActionTracker() {
RecordPageActiveTime();
Expand All @@ -58,35 +54,6 @@ PerSessionSettingsUserActionTracker::~PerSessionSettingsUserActionTracker() {
base::UmaHistogramCounts1000(
"ChromeOS.Settings.NumUniqueSettingsChanged.PerSession",
changed_settings_.size());
}

void PerSessionSettingsUserActionTracker::RecordTotalLifetimeMetric() {
metrics::MetricsService* metrics_service_ =
g_browser_process->metrics_service();
CHECK(metrics_service_->GetCurrentUserMetricsConsent().has_value());

// We will not keep a record of the user's total number of unique Settings
// changed if they have turned off UMA. We will not transmit the old data when
// the user revokes their consent for UMA. The pref gets cleared and will
// never record the Device Lifetime metric again.
if (pref_service_->FindPreference(::prefs::kHasEverRevokedMetricsConsent)
->IsDefaultValue()) {
bool current_metric_consent_value =
metrics_service_->GetCurrentUserMetricsConsent().value();
pref_service_->SetBoolean(::prefs::kHasEverRevokedMetricsConsent,
!current_metric_consent_value);
} else if (!metrics_service_->GetCurrentUserMetricsConsent().value()) {
pref_service_->SetBoolean(::prefs::kHasEverRevokedMetricsConsent, true);
}

if (pref_service_->GetBoolean(::prefs::kHasEverRevokedMetricsConsent)) {
// If the pref has been turned off at least once in the user's lifetime,
// clear the pref kTotalUniqueOsSettingsChanged.
ClearTotalUniqueSettingsChangedPref();

// Return early before any recording takes place.
return;
}

// The pref kHasResetFirst7DaysSettingsUsedCount indicates whether the pref
// kTotalUniqueOsSettingsChanged has been cleared once after 1 week has passed
Expand All @@ -108,28 +75,29 @@ void PerSessionSettingsUserActionTracker::RecordTotalLifetimeMetric() {
}

// Record number of unique settings changed in this session.
int total_unique_settings_changed_count =
absl::optional<int> total_unique_settings_changed_count =
UpdateSettingsPrefTotalUniqueChanged();

// If the number of total unique setting used increased, flagged by the
// optional variable total_unique_settings_changed_count having a value, add
// the datapoint to the histogram.
// NOTE: prefs::kOobeOnboardingTime does not exist for users in guest mode.
if (pref_service_->HasPrefPath(prefs::kOobeOnboardingTime)) {
if (pref_service_->HasPrefPath(prefs::kOobeOnboardingTime) &&
total_unique_settings_changed_count.has_value()) {
if (IsTodayInFirst7Days()) {
base::UmaHistogramCounts1000(
"ChromeOS.Settings.NumUniqueSettingsChanged.DeviceLifetime.FirstWeek",
total_unique_settings_changed_count);
total_unique_settings_changed_count.value());
} else {
base::UmaHistogramCounts1000(
"ChromeOS.Settings.NumUniqueSettingsChanged.DeviceLifetime."
"SubsequentWeeks",
total_unique_settings_changed_count);
total_unique_settings_changed_count.value());
}
// Store the total unique Settings changed in .DeviceLifetime histogram.
base::UmaHistogramCounts1000(
"ChromeOS.Settings.NumUniqueSettingsChanged.DeviceLifetime.Total",
total_unique_settings_changed_count);
total_unique_settings_changed_count.value());
}
}

Expand All @@ -145,10 +113,7 @@ bool PerSessionSettingsUserActionTracker::IsTodayInFirst7Days() {

void PerSessionSettingsUserActionTracker::
ClearTotalUniqueSettingsChangedPref() {
if (pref_service_->GetDict(::prefs::kTotalUniqueOsSettingsChanged).size() !=
0) {
pref_service_->ClearPref(::prefs::kTotalUniqueOsSettingsChanged);
}
pref_service_->ClearPref(::prefs::kTotalUniqueOsSettingsChanged);
}

void PerSessionSettingsUserActionTracker::RecordPageFocus() {
Expand Down Expand Up @@ -202,10 +167,6 @@ void PerSessionSettingsUserActionTracker::RecordSettingChange(
if (setting.has_value()) {
changed_settings_.insert(
base::NumberToString(static_cast<int>(setting.value())));

// Record the total unique Settings changed to the histogram
// ChromeOS.Settings.NumUniqueSettingsChanged.DeviceLifetime.{Time}.
RecordTotalLifetimeMetric();
}
base::TimeTicks now = base::TimeTicks::Now();

Expand Down Expand Up @@ -252,12 +213,13 @@ void PerSessionSettingsUserActionTracker::ResetMetricsCountersAndTimestamp() {
num_searches_since_start_time_ = 0u;
}

int PerSessionSettingsUserActionTracker::
UpdateSettingsPrefTotalUniqueChanged() {
absl::optional<int>
PerSessionSettingsUserActionTracker::UpdateSettingsPrefTotalUniqueChanged() {
// Fetch the dictionary from the pref.
ScopedDictPrefUpdate total_unique_settings_changed_(
pref_service_, ::prefs::kTotalUniqueOsSettingsChanged);
base::Value::Dict& pref_data = total_unique_settings_changed_.Get();
int current_count = pref_data.size();

// Set the dictionary.
// Value is a constant 1 since we only want to know which Setting has been
Expand All @@ -269,14 +231,16 @@ int PerSessionSettingsUserActionTracker::
}
}

// We still want to record the data in UMA even if no new unique Settings has
// changed because UMA clears the data after a certain timeframe. We will
// record the pref kTotalUniqueOsSettingsChanged's size to UMA every time that
// a user changes a Setting, whether it was a unique change or not.
int new_count = pref_data.size();

// If the new size of the pref dictionary is the same as before, we do not
// want to record that in UMA so we will return a nullopt to flag not to add
// to histogram bucket.
//
// The value of pref_data will automatically get stored to pref_service_ upon
// destruction.
return pref_data.size();
return current_count == new_count ? absl::nullopt
: absl::optional<int>{new_count};
}

} // namespace ash::settings
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,12 @@ class PerSessionSettingsUserActionTracker {
// which is set once the user finished the OOBE.
bool IsTodayInFirst7Days();

void RecordTotalLifetimeMetric();

void ResetMetricsCountersAndTimestamp();

// Returns the size of the pref dict ::prefs::kTotalUniqueOsSettingsChanged
// after user used unique Settings have been added to it.
int UpdateSettingsPrefTotalUniqueChanged();
// Returns the size of the pref dict if it changes. Otherwise, no value will
// get returned if if there were no new unique settings changed in the
// session.
absl::optional<int> UpdateSettingsPrefTotalUniqueChanged();

// Time at which the last setting change metric was recorded since the window
// has been focused, or null if no setting change has been recorded since the
Expand Down

0 comments on commit ecc71d2

Please sign in to comment.