Skip to content

Commit

Permalink
Remove more Scout references from safe_browsing_prefs
Browse files Browse the repository at this point in the history 
This includes things like old prefs, metrics and finch features.

Bug: 662944
Change-Id: Ib7199895bb3b588efcb88c8898eb696b75717fa8
Reviewed-on: https://chromium-review.googlesource.com/c/1294130
Commit-Queue: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603250}
  • Loading branch information
@LukeZielinski
LukeZielinski authored and Commit Bot committed Oct 26, 2018
1 parent e9222e9 commit d49f95c
Show file tree
Hide file tree
Showing 6 changed files with 11 additions and 164 deletions.
92 changes: 1 addition & 91 deletions components/safe_browsing/common/safe_browsing_prefs.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,38 +18,6 @@

namespace {

// Name of the Scout Transition UMA metric.
const char kScoutTransitionMetricName[] = "SafeBrowsing.Pref.Scout.Transition";

// Reasons that a state transition for Scout was performed.
// These values are written to logs. New enum values can be added, but
// existing enums must never be renumbered or deleted and reused.
enum ScoutTransitionReason {
// Flag forced Scout Group to true
FORCE_SCOUT_FLAG_TRUE = 0,
// Flag forced Scout Group to false
FORCE_SCOUT_FLAG_FALSE = 1,
// User in OnlyShowScout group, enters Scout Group
ONLY_SHOW_SCOUT_OPT_IN = 2,
// User in CanShowScout group, enters Scout Group immediately
CAN_SHOW_SCOUT_OPT_IN_SCOUT_GROUP_ON = 3,
// User in CanShowScout group, waiting for interstitial to enter Scout Group
CAN_SHOW_SCOUT_OPT_IN_WAIT_FOR_INTERSTITIAL = 4,
// User in CanShowScout group saw the first interstitial and entered the Scout
// Group
CAN_SHOW_SCOUT_OPT_IN_SAW_FIRST_INTERSTITIAL = 5,
// User in Control group
CONTROL = 6,
// Rollback: SBER2 on on implies SBER1 can turn on
ROLLBACK_SBER2_IMPLIES_SBER1 = 7,
// Rollback: SBER2 off so SBER1 must be turned off
ROLLBACK_NO_SBER2_SET_SBER1_FALSE = 8,
// Rollback: SBER2 absent so SBER1 must be cleared
ROLLBACK_NO_SBER2_CLEAR_SBER1 = 9,
// New reasons must be added BEFORE MAX_REASONS
MAX_REASONS
};

// The Extended Reporting pref that is currently active, used for UMA metrics.
// These values are written to logs. New enum values can be added, but
// existing enums must never be renumbered or deleted and reused.
Expand All @@ -60,25 +28,6 @@ enum ActiveExtendedReportingPref {
MAX_SBER_PREF
};

// A histogram for tracking a nullable boolean, which can be false, true or
// null. These values are written to logs. New enum values can be added, but
// existing enums must never be renumbered or deleted and reused.
enum NullableBoolean {
NULLABLE_BOOLEAN_FALSE = 0,
NULLABLE_BOOLEAN_TRUE = 1,
NULLABLE_BOOLEAN_NULL = 2,
MAX_NULLABLE_BOOLEAN
};

NullableBoolean GetPrefValueOrNull(const PrefService& prefs,
const std::string& pref_name) {
if (!prefs.HasPrefPath(pref_name)) {
return NULLABLE_BOOLEAN_NULL;
}
return prefs.GetBoolean(pref_name) ? NULLABLE_BOOLEAN_TRUE
: NULLABLE_BOOLEAN_FALSE;
}

// Update the correct UMA metric based on which pref was changed and which UI
// the change was made on.
void RecordExtendedReportingPrefChanged(
Expand Down Expand Up @@ -137,12 +86,8 @@ const char kSafeBrowsingExtendedReportingOptInAllowed[] =
const char kSafeBrowsingIncidentsSent[] = "safebrowsing.incidents_sent";
const char kSafeBrowsingProceedAnywayDisabled[] =
"safebrowsing.proceed_anyway_disabled";
const char kSafeBrowsingSawInterstitialExtendedReporting[] =
"safebrowsing.saw_interstitial_sber1";
const char kSafeBrowsingSawInterstitialScoutReporting[] =
"safebrowsing.saw_interstitial_sber2";
const char kSafeBrowsingScoutGroupSelected[] =
"safebrowsing.scout_group_selected";
const char kSafeBrowsingScoutReportingEnabled[] =
"safebrowsing.scout_reporting_enabled";
const char kSafeBrowsingTriggerEventTimestamps[] =
Expand All @@ -165,9 +110,6 @@ const char kAdvancedProtectionLastRefreshInUs[] =

namespace safe_browsing {

const base::Feature kCanShowScoutOptIn{"CanShowScoutOptIn",
base::FEATURE_ENABLED_BY_DEFAULT};

bool ExtendedReportingPrefExists(const PrefService& prefs) {
return prefs.HasPrefPath(prefs::kSafeBrowsingScoutReportingEnabled);
}
Expand Down Expand Up @@ -196,36 +138,14 @@ void RecordExtendedReportingMetrics(const PrefService& prefs) {
IsExtendedReportingEnabled(prefs));

// Track whether this user has ever seen a security interstitial.
UMA_HISTOGRAM_BOOLEAN(
"SafeBrowsing.Pref.SawInterstitial.SBER1Pref",
prefs.GetBoolean(prefs::kSafeBrowsingSawInterstitialExtendedReporting));
UMA_HISTOGRAM_BOOLEAN(
"SafeBrowsing.Pref.SawInterstitial.SBER2Pref",
prefs.GetBoolean(prefs::kSafeBrowsingSawInterstitialScoutReporting));

// These metrics track the Scout transition.
if (prefs.GetBoolean(prefs::kSafeBrowsingScoutGroupSelected)) {
UMA_HISTOGRAM_ENUMERATION(
"SafeBrowsing.Pref.Scout.ScoutGroup.SBER2Pref",
GetPrefValueOrNull(prefs, prefs::kSafeBrowsingScoutReportingEnabled),
MAX_NULLABLE_BOOLEAN);
} else {
// The following metric is a corner case. User was previously in the
// Scout group and was able to opt-in to the Scout pref, but was since
// removed from the Scout group (eg: by rolling back a Scout experiment).
UMA_HISTOGRAM_ENUMERATION(
"SafeBrowsing.Pref.Scout.NoScoutGroup.SBER2Pref",
GetPrefValueOrNull(prefs, prefs::kSafeBrowsingScoutReportingEnabled),
MAX_NULLABLE_BOOLEAN);
}
}

void RegisterProfilePrefs(PrefRegistrySimple* registry) {
registry->RegisterBooleanPref(prefs::kSafeBrowsingScoutReportingEnabled,
false);
registry->RegisterBooleanPref(prefs::kSafeBrowsingScoutGroupSelected, false);
registry->RegisterBooleanPref(
prefs::kSafeBrowsingSawInterstitialExtendedReporting, false);
registry->RegisterBooleanPref(
prefs::kSafeBrowsingSawInterstitialScoutReporting, false);
registry->RegisterBooleanPref(
Expand Down Expand Up @@ -327,17 +247,7 @@ void UpdateMetricsAfterSecurityInterstitial(const PrefService& prefs,
}

void UpdatePrefsBeforeSecurityInterstitial(PrefService* prefs) {
// Move the user into the Scout Group if the CanShowScoutOptIn experiment is
// enabled and they're not in the group already.
if (base::FeatureList::IsEnabled(kCanShowScoutOptIn) &&
!prefs->GetBoolean(prefs::kSafeBrowsingScoutGroupSelected)) {
prefs->SetBoolean(prefs::kSafeBrowsingScoutGroupSelected, true);
UMA_HISTOGRAM_ENUMERATION(kScoutTransitionMetricName,
CAN_SHOW_SCOUT_OPT_IN_SAW_FIRST_INTERSTITIAL,
MAX_REASONS);
}

// Remember that this user saw an interstitial with the current opt-in text.
// Remember that this user saw an interstitial.
prefs->SetBoolean(prefs::kSafeBrowsingSawInterstitialScoutReporting, true);
}

Expand Down
17 changes: 1 addition & 16 deletions components/safe_browsing/common/safe_browsing_prefs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,9 @@ extern const char kSafeBrowsingIncidentsSent[];
// users to proceed anyway.
extern const char kSafeBrowsingProceedAnywayDisabled[];

// Boolean indicating whether the user has ever seen a security interstitial
// containing the legacy Extended Reporting opt-in.
extern const char kSafeBrowsingSawInterstitialExtendedReporting[];

// Boolean indicating whether the user has ever seen a security interstitial
// containing the new Scout opt-in.
// Boolean indicating whether the user has ever seen a security interstitial.
extern const char kSafeBrowsingSawInterstitialScoutReporting[];

// Boolean indicating whether the Scout reporting workflow is enabled. This
// affects which of SafeBrowsingExtendedReporting or SafeBrowsingScoutReporting
// is used.
extern const char kSafeBrowsingScoutGroupSelected[];

// Boolean indicating whether Safe Browsing Scout reporting is enabled, which
// collects data for malware detection.
extern const char kSafeBrowsingScoutReportingEnabled[];
Expand Down Expand Up @@ -94,11 +84,6 @@ extern const char kAdvancedProtectionLastRefreshInUs[];

namespace safe_browsing {

// When this feature is enabled, the Scout opt-in text will be displayed as of
// the next security incident. Until then, the legacy SBER text will appear.
// TODO: this is temporary (crbug.com/662944)
extern const base::Feature kCanShowScoutOptIn;

// Enumerates the level of Safe Browsing Extended Reporting that is currently
// available.
enum ExtendedReportingLevel {
Expand Down
63 changes: 9 additions & 54 deletions components/safe_browsing/common/safe_browsing_prefs_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ class SafeBrowsingPrefsTest : public ::testing::Test {
void SetUp() override {
prefs_.registry()->RegisterBooleanPref(
prefs::kSafeBrowsingScoutReportingEnabled, false);
prefs_.registry()->RegisterBooleanPref(
prefs::kSafeBrowsingScoutGroupSelected, false);
prefs_.registry()->RegisterBooleanPref(
prefs::kSafeBrowsingSawInterstitialExtendedReporting, false);
prefs_.registry()->RegisterBooleanPref(
prefs::kSafeBrowsingSawInterstitialScoutReporting, false);
prefs_.registry()->RegisterStringPref(
Expand All @@ -37,55 +33,27 @@ class SafeBrowsingPrefsTest : public ::testing::Test {
prefs_.registry()->RegisterBooleanPref(
prefs::kSafeBrowsingExtendedReportingOptInAllowed, true);
prefs_.registry()->RegisterListPref(prefs::kSafeBrowsingWhitelistDomains);

ResetExperiments(/*can_show_scout=*/false);
}

void ResetPrefs(bool scout_reporting, bool scout_group) {
void ResetPrefs(bool scout_reporting) {
prefs_.SetBoolean(prefs::kSafeBrowsingScoutReportingEnabled,
scout_reporting);
prefs_.SetBoolean(prefs::kSafeBrowsingScoutGroupSelected, scout_group);
}

void ResetExperiments(bool can_show_scout) {
std::vector<base::StringPiece> enabled_features;
std::vector<base::StringPiece> disabled_features;

auto* target_vector =
can_show_scout ? &enabled_features : &disabled_features;
target_vector->push_back(kCanShowScoutOptIn.name);

feature_list_.reset(new base::test::ScopedFeatureList);
feature_list_->InitFromCommandLine(
base::JoinString(enabled_features, ","),
base::JoinString(disabled_features, ","));
}

bool IsScoutGroupSelected() {
return prefs_.GetBoolean(prefs::kSafeBrowsingScoutGroupSelected);
}

void ExpectPrefs(bool scout_reporting, bool scout_group) {
LOG(INFO) << "Pref values: scout=" << scout_reporting
<< " scout_group=" << scout_group;
void ExpectPrefs(bool scout_reporting) {
LOG(INFO) << "Pref values: scout=" << scout_reporting;
EXPECT_EQ(scout_reporting,
prefs_.GetBoolean(prefs::kSafeBrowsingScoutReportingEnabled));
EXPECT_EQ(scout_group,
prefs_.GetBoolean(prefs::kSafeBrowsingScoutGroupSelected));
}

void ExpectPrefsExist(bool scout_reporting, bool scout_group) {
LOG(INFO) << "Prefs exist: scout=" << scout_reporting
<< " scout_group=" << scout_group;
void ExpectPrefsExist(bool scout_reporting) {
LOG(INFO) << "Prefs exist: scout=" << scout_reporting;
EXPECT_EQ(scout_reporting,
prefs_.HasPrefPath(prefs::kSafeBrowsingScoutReportingEnabled));
EXPECT_EQ(scout_group,
prefs_.HasPrefPath(prefs::kSafeBrowsingScoutGroupSelected));
}
TestingPrefServiceSimple prefs_;

private:
std::unique_ptr<base::test::ScopedFeatureList> feature_list_;
content::TestBrowserThreadBundle thread_bundle_;
};

Expand All @@ -98,28 +66,15 @@ class SafeBrowsingPrefsTest : public ::testing::Test {
GetSafeBrowsingExtendedReportingLevel
#endif
TEST_F(SafeBrowsingPrefsTest, MAYBE_GetSafeBrowsingExtendedReportingLevel) {
// By Default, extneded reporting is off.
// By Default, extended reporting is off.
EXPECT_EQ(SBER_LEVEL_OFF, GetExtendedReportingLevel(prefs_));

// The value of the Scout pref affects the reporting level directly,
// regardless of the experiment configuration since Scout is the only level
// we are using.
// No scout group.
ResetPrefs(/*scout_reporting=*/true, /*scout_group=*/false);
// The value of the Scout pref affects the reporting level directly.
ResetPrefs(/*scout_reporting=*/true);
EXPECT_EQ(SBER_LEVEL_SCOUT, GetExtendedReportingLevel(prefs_));
// Scout group but no experiment.
ResetPrefs(/*scout_reporting=*/true, /*scout_group=*/true);
EXPECT_EQ(SBER_LEVEL_SCOUT, GetExtendedReportingLevel(prefs_));
ResetExperiments(/*can_show_scout=*/true);
// Scout pref off, so reporting is off.
ResetPrefs(/*scout_reporting=*/false, /*scout_group=*/true);
EXPECT_EQ(SBER_LEVEL_OFF, GetExtendedReportingLevel(prefs_));
// Scout pref off with the experiment group off, so reporting remains off.
ResetPrefs(/*scout_reporting=*/false, /*scout_group=*/true);
ResetPrefs(/*scout_reporting=*/false);
EXPECT_EQ(SBER_LEVEL_OFF, GetExtendedReportingLevel(prefs_));
// Turning on Scout gives us Scout level reporting
ResetPrefs(/*scout_reporting=*/true, /*scout_group=*/true);
EXPECT_EQ(SBER_LEVEL_SCOUT, GetExtendedReportingLevel(prefs_));
}

// TODO(crbug.com/881476) disabled for flaky crashes.
Expand Down
1 change: 0 additions & 1 deletion components/safe_browsing/triggers/ad_sampler_trigger_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ class AdSamplerTriggerTest : public content::RenderViewHostTestHarness {
safe_browsing::RegisterProfilePrefs(prefs_.registry());
prefs_.SetBoolean(prefs::kSafeBrowsingExtendedReportingOptInAllowed, true);
prefs_.SetBoolean(prefs::kSafeBrowsingScoutReportingEnabled, true);
prefs_.SetBoolean(prefs::kSafeBrowsingScoutGroupSelected, true);
}

void CreateTriggerWithFrequency(const size_t denominator) {
Expand Down
1 change: 0 additions & 1 deletion components/safe_browsing/triggers/suspicious_site_trigger_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ class SuspiciousSiteTriggerTest : public content::RenderViewHostTestHarness {
safe_browsing::RegisterProfilePrefs(prefs_.registry());
prefs_.SetBoolean(prefs::kSafeBrowsingExtendedReportingOptInAllowed, true);
prefs_.SetBoolean(prefs::kSafeBrowsingScoutReportingEnabled, true);
prefs_.SetBoolean(prefs::kSafeBrowsingScoutGroupSelected, true);
}

void CreateTrigger(bool monitor_mode) {
Expand Down
1 change: 0 additions & 1 deletion components/safe_browsing/triggers/trigger_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ class TriggerManagerTest : public ::testing::Test {
safe_browsing::RegisterProfilePrefs(pref_service_.registry());
SetPref(prefs::kSafeBrowsingExtendedReportingOptInAllowed, true);
SetPref(prefs::kSafeBrowsingScoutReportingEnabled, true);
SetPref(prefs::kSafeBrowsingScoutGroupSelected, true);

MockTriggerThrottler* mock_throttler = new MockTriggerThrottler();
ON_CALL(*mock_throttler, TriggerCanFire(_)).WillByDefault(Return(true));
Expand Down

0 comments on commit d49f95c

Please sign in to comment.