Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3947)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: chrome/browser/permissions/permission_context_base_unittest.cc

Issue 2701343002: Implement permission embargo suppression metrics. (Closed)
Patch Set: Clean up Created 3 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« chrome/browser/permissions/permission_context_base.cc ('K') | « chrome/browser/permissions/permission_context_base.cc ('k') | chrome/browser/permissions/permission_decision_auto_blocker.h » ('j') | chrome/browser/permissions/permission_decision_auto_blocker.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/browser/permissions/permission_context_base_unittest.cc
diff --git a/chrome/browser/permissions/permission_context_base_unittest.cc b/chrome/browser/permissions/permission_context_base_unittest.cc
index 9dd2f2ae8f1e05ccc5333457fb6dbe3f0e4da124..8566940878c88c7cfc38fa41431b11ef9c291a6c 100644
--- a/chrome/browser/permissions/permission_context_base_unittest.cc
+++ b/chrome/browser/permissions/permission_context_base_unittest.cc
@@ -317,6 +317,9 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
permission_context.GetContentSettingFromMap(url, url));
}
+ histograms.ExpectUniqueSample(
+ "Permissions.AutoBlocker.EmbargoPromptSuppression",
+ PermissionEmbargoStatus::NOT_EMBARGOED, 1);
histograms.ExpectUniqueSample("Permissions.AutoBlocker.EmbargoStatus",
PermissionEmbargoStatus::NOT_EMBARGOED, 1);
}
@@ -330,8 +333,6 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
// Dismiss |iterations| times. The final dismiss should change the decision
// from dismiss to block, and hence change the persisted content setting.
for (uint32_t i = 0; i < iterations; ++i) {
- ContentSetting expected =
- (i < 2) ? CONTENT_SETTING_ASK : CONTENT_SETTING_BLOCK;
TestPermissionContext permission_context(profile(),
content_settings_type);
const PermissionRequestID id(
@@ -355,25 +356,45 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
"Permissions.Prompt.Dismissed.PriorDismissCount." +
PermissionUtil::GetPermissionString(content_settings_type),
i, 1);
+
+// On Android, repeatedly requesting and deciding permissions has the side
+// effect of overcounting any metrics recorded in the PermissionInfoBarDelegate
+// destructor. This is because we directly call
+// PermissionQueueController::OnPermissionSet without setting the action_taken
+// bit in PermissionInfoBarDelegate. When PermissionQueueController is deleted
+// all OS_ANDROID ifdefs in this test can be removed.
+#if !defined(OS_ANDROID)
histograms.ExpectTotalCount("Permissions.AutoBlocker.EmbargoStatus",
i + 1);
+#endif
+
+ PermissionResult result =
+ permission_context.GetPermissionStatus(url, url);
+
+ histograms.ExpectUniqueSample(
+ "Permissions.AutoBlocker.EmbargoPromptSuppression",
+ PermissionEmbargoStatus::NOT_EMBARGOED, i + 1);
if (i < 2) {
+ EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
+ EXPECT_EQ(CONTENT_SETTING_ASK, result.content_setting);
+#if !defined(OS_ANDROID)
histograms.ExpectUniqueSample("Permissions.AutoBlocker.EmbargoStatus",
PermissionEmbargoStatus::NOT_EMBARGOED,
i + 1);
+#endif
} else {
+ EXPECT_EQ(PermissionStatusSource::MULTIPLE_DISMISSALS, result.source);
+ EXPECT_EQ(CONTENT_SETTING_BLOCK, result.content_setting);
+#if !defined(OS_ANDROID)
histograms.ExpectBucketCount(
"Permissions.AutoBlocker.EmbargoStatus",
PermissionEmbargoStatus::REPEATED_DISMISSALS, 1);
+#endif
}
ASSERT_EQ(1u, permission_context.decisions().size());
EXPECT_EQ(CONTENT_SETTING_ASK, permission_context.decisions()[0]);
EXPECT_TRUE(permission_context.tab_context_updated());
- PermissionResult result =
- permission_context.GetPermissionStatus(url, url);
- EXPECT_EQ(expected, result.content_setting);
- EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
}
TestPermissionContext permission_context(profile(), content_settings_type);
@@ -393,7 +414,10 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
PermissionResult result = permission_context.GetPermissionStatus(url, url);
EXPECT_EQ(CONTENT_SETTING_BLOCK, result.content_setting);
- EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
+ EXPECT_EQ(PermissionStatusSource::MULTIPLE_DISMISSALS, result.source);
+ histograms.ExpectBucketCount(
+ "Permissions.AutoBlocker.EmbargoPromptSuppression",
+ PermissionEmbargoStatus::REPEATED_DISMISSALS, 1);
}
void TestBlockOnSeveralDismissals_TestContent() {
@@ -423,9 +447,21 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
i + 1);
histograms.ExpectBucketCount(
"Permissions.Prompt.Dismissed.PriorDismissCount.Geolocation", i, 1);
+ histograms.ExpectUniqueSample(
+ "Permissions.AutoBlocker.EmbargoPromptSuppression",
+ PermissionEmbargoStatus::NOT_EMBARGOED, i + 1);
+
+// On Android, repeatedly requesting and deciding permissions has the side
+// effect of overcounting any metrics recorded in the PermissionInfoBarDelegate
+// destructor. This is because we directly call
+// PermissionQueueController::OnPermissionSet without setting the action_taken
+// bit in PermissionInfoBarDelegate. When PermissionQueueController is deleted
+// all OS_ANDROID ifdefs in this test can be removed.
+#if !defined(OS_ANDROID)
histograms.ExpectUniqueSample("Permissions.AutoBlocker.EmbargoStatus",
PermissionEmbargoStatus::NOT_EMBARGOED,
i + 1);
+#endif
ASSERT_EQ(1u, permission_context.decisions().size());
EXPECT_EQ(CONTENT_SETTING_ASK, permission_context.decisions()[0]);
@@ -491,8 +527,6 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
TestPermissionContext permission_context(
profile(), CONTENT_SETTINGS_TYPE_MIDI_SYSEX);
- ContentSetting expected =
- (i < 4) ? CONTENT_SETTING_ASK : CONTENT_SETTING_BLOCK;
const PermissionRequestID id(
web_contents()->GetRenderProcessHost()->GetID(),
web_contents()->GetMainFrame()->GetRoutingID(), i);
@@ -510,24 +544,41 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
EXPECT_TRUE(permission_context.tab_context_updated());
PermissionResult result =
permission_context.GetPermissionStatus(url, url);
- EXPECT_EQ(expected, result.content_setting);
- EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
histograms.ExpectTotalCount(
"Permissions.Prompt.Dismissed.PriorDismissCount.MidiSysEx", i + 1);
histograms.ExpectBucketCount(
"Permissions.Prompt.Dismissed.PriorDismissCount.MidiSysEx", i, 1);
-
+ histograms.ExpectUniqueSample(
+ "Permissions.AutoBlocker.EmbargoPromptSuppression",
+ PermissionEmbargoStatus::NOT_EMBARGOED, i + 1);
+
+// On Android, repeatedly requesting and deciding permissions has the side
+// effect of overcounting any metrics recorded in the PermissionInfoBarDelegate
+// destructor. This is because we directly call
+// PermissionQueueController::OnPermissionSet without setting the action_taken
+// bit in PermissionInfoBarDelegate. When PermissionQueueController is deleted
+// all OS_ANDROID ifdefs in this test can be removed.
+#if !defined(OS_ANDROID)
histograms.ExpectTotalCount("Permissions.AutoBlocker.EmbargoStatus",
i + 1);
+#endif
if (i < 4) {
+ EXPECT_EQ(CONTENT_SETTING_ASK, result.content_setting);
+ EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
+#if !defined(OS_ANDROID)
histograms.ExpectUniqueSample("Permissions.AutoBlocker.EmbargoStatus",
PermissionEmbargoStatus::NOT_EMBARGOED,
i + 1);
+#endif
} else {
+ EXPECT_EQ(CONTENT_SETTING_BLOCK, result.content_setting);
+ EXPECT_EQ(PermissionStatusSource::MULTIPLE_DISMISSALS, result.source);
+#if !defined(OS_ANDROID)
histograms.ExpectBucketCount(
"Permissions.AutoBlocker.EmbargoStatus",
PermissionEmbargoStatus::REPEATED_DISMISSALS, 1);
+#endif
}
}
@@ -536,13 +587,13 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
CONTENT_SETTINGS_TYPE_MIDI_SYSEX);
PermissionResult result = permission_context.GetPermissionStatus(url, url);
EXPECT_EQ(CONTENT_SETTING_BLOCK, result.content_setting);
- EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
-
+ EXPECT_EQ(PermissionStatusSource::MULTIPLE_DISMISSALS, result.source);
variations::testing::ClearAllVariationParams();
}
void TestRequestPermissionInvalidUrl(
ContentSettingsType content_settings_type) {
+ base::HistogramTester histograms;
TestPermissionContext permission_context(profile(), content_settings_type);
GURL url;
ASSERT_FALSE(url.is_valid());
@@ -563,6 +614,8 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
EXPECT_TRUE(permission_context.tab_context_updated());
EXPECT_EQ(CONTENT_SETTING_ASK,
permission_context.GetContentSettingFromMap(url, url));
+ histograms.ExpectTotalCount(
+ "Permissions.AutoBlocker.EmbargoPromptSuppression", 0);
}
void TestGrantAndRevoke_TestContent(ContentSettingsType content_settings_type,
@@ -623,6 +676,7 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
// Don't call this more than once in the same test, as it persists data to
// HostContentSettingsMap.
void TestParallelRequests(ContentSetting response) {
+ base::HistogramTester histograms;
TestPermissionContext permission_context(
profile(), CONTENT_SETTINGS_TYPE_NOTIFICATIONS);
GURL url("http://www.google.com");
@@ -662,6 +716,10 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
EXPECT_TRUE(permission_context.tab_context_updated());
EXPECT_EQ(response, permission_context.GetContentSettingFromMap(url, url));
+
+ histograms.ExpectUniqueSample(
+ "Permissions.AutoBlocker.EmbargoPromptSuppression",
+ PermissionEmbargoStatus::NOT_EMBARGOED, 2);
raymes 2017/02/22 23:13:50 I think the autoblocking stuff is fairly orthogona
dominickn 2017/02/22 23:38:54 Done.
}
void TestPermissionsBlacklisting(
@@ -699,12 +757,15 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
base::Unretained(&permission_context)));
PermissionResult result = permission_context.GetPermissionStatus(url, url);
EXPECT_EQ(expected_permission_status, result.content_setting);
- EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
if (expected_permission_status == CONTENT_SETTING_ALLOW) {
ASSERT_EQ(1u, permission_context.decisions().size());
EXPECT_EQ(expected_permission_status, permission_context.decisions()[0]);
+ EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
raymes 2017/02/22 23:13:50 should we check the source in the else case?
dominickn 2017/02/22 23:38:54 Done.
}
+ histograms.ExpectUniqueSample(
+ "Permissions.AutoBlocker.EmbargoPromptSuppression",
+ expected_embargo_reason, 1);
histograms.ExpectUniqueSample("Permissions.AutoBlocker.EmbargoStatus",
expected_embargo_reason, 1);
}
« chrome/browser/permissions/permission_context_base.cc ('K') | « chrome/browser/permissions/permission_context_base.cc ('k') | chrome/browser/permissions/permission_decision_auto_blocker.h » ('j') | chrome/browser/permissions/permission_decision_auto_blocker.cc » ('J')

Powered by Google App Engine
This is Rietveld 408576698