chrome/browser/permissions/permission_context_base_unittest.cc - Issue 2684913002: Add UMA for recording embargo reasons and autoblocker interactions.

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

Issue 2684913002: Add UMA for recording embargo reasons and autoblocker interactions. (Closed)

Patch Set: rebase 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

« chrome/browser/permissions/permission_context_base.cc ('K') | « chrome/browser/permissions/permission_context_base.cc ('k') | chrome/browser/permissions/permission_decision_auto_blocker.cc » ('j') | chrome/browser/permissions/permission_uma_util.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | 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 888e537e1dc4832d2f21fca11f1e63e3ce7107f7..427bd1bbae297d6f09eeebc070fc84a02e5decff 100644

--- a/chrome/browser/permissions/permission_context_base_unittest.cc

+++ b/chrome/browser/permissions/permission_context_base_unittest.cc

@@ -25,6 +25,7 @@

#include "chrome/browser/permissions/permission_decision_auto_blocker.h"

#include "chrome/browser/permissions/permission_queue_controller.h"

#include "chrome/browser/permissions/permission_request_id.h"

+#include "chrome/browser/permissions/permission_uma_util.h"

#include "chrome/browser/permissions/permission_util.h"

#include "chrome/common/chrome_features.h"

#include "chrome/common/chrome_switches.h"

@@ -356,6 +357,18 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {

"Permissions.Prompt.Dismissed.PriorDismissCount." +

PermissionUtil::GetPermissionString(permission_type),

i, 1);

+ histograms.ExpectTotalCount("Permissions.AutoBlocker.EmbargoReason",

+ i + 1);

+ if (i < 2) {

+ histograms.ExpectUniqueSample("Permissions.AutoBlocker.EmbargoReason",

+ PermissionEmbargoReason::NOT_EMBARGOED,

+ i + 1);

+ } else {

+ histograms.ExpectBucketCount(

+ "Permissions.AutoBlocker.EmbargoReason",

+ PermissionEmbargoReason::REPEATED_DISMISSALS, 1);

+ }

ASSERT_EQ(1u, permission_context.decisions().size());

EXPECT_EQ(expected, permission_context.decisions()[0]);

EXPECT_TRUE(permission_context.tab_context_updated());

@@ -410,6 +423,10 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {

i + 1);

histograms.ExpectBucketCount(

"Permissions.Prompt.Dismissed.PriorDismissCount.Geolocation", i, 1);

+ histograms.ExpectUniqueSample("Permissions.AutoBlocker.EmbargoReason",

+ PermissionEmbargoReason::NOT_EMBARGOED,

+ i + 1);

ASSERT_EQ(1u, permission_context.decisions().size());

EXPECT_EQ(CONTENT_SETTING_ASK, permission_context.decisions()[0]);

EXPECT_TRUE(permission_context.tab_context_updated());

@@ -500,6 +517,18 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {

"Permissions.Prompt.Dismissed.PriorDismissCount.MidiSysEx", i + 1);

histograms.ExpectBucketCount(

"Permissions.Prompt.Dismissed.PriorDismissCount.MidiSysEx", i, 1);

+ histograms.ExpectTotalCount("Permissions.AutoBlocker.EmbargoReason",

+ i + 1);

+ if (i < 4) {

+ histograms.ExpectUniqueSample("Permissions.AutoBlocker.EmbargoReason",

+ PermissionEmbargoReason::NOT_EMBARGOED,

+ i + 1);

+ } else {

+ histograms.ExpectBucketCount(

+ "Permissions.AutoBlocker.EmbargoReason",

+ PermissionEmbargoReason::REPEATED_DISMISSALS, 1);

+ }

}

// Ensure that we finish in the block state.

@@ -646,8 +675,10 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {

scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,

const GURL& url,

int timeout,

- ContentSetting expected_permission_status) {

+ ContentSetting expected_permission_status,

+ PermissionEmbargoReason expected_embargo_reason) {

NavigateAndCommit(url);

+ base::HistogramTester histograms;

base::test::ScopedFeatureList scoped_feature_list;

scoped_feature_list.InitAndEnableFeature(features::kPermissionsBlacklist);

TestPermissionContext permission_context(profile(), permission_type,

@@ -680,6 +711,8 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {

ASSERT_EQ(1u, permission_context.decisions().size());

EXPECT_EQ(expected_permission_status, permission_context.decisions()[0]);

}

+ histograms.ExpectUniqueSample("Permissions.AutoBlocker.EmbargoReason",

+ expected_embargo_reason, 1);

}

private:

@@ -847,7 +880,8 @@ TEST_F(PermissionContextBaseTests, TestPermissionsBlacklistingBlocked) {

db_manager->BlacklistUrlPermissions(url, blacklisted_permissions);

TestPermissionsBlacklisting(content::PermissionType::GEOLOCATION,

CONTENT_SETTINGS_TYPE_GEOLOCATION, db_manager,

- url, 2000 /* timeout */, CONTENT_SETTING_BLOCK);

+ url, 2000 /* timeout */, CONTENT_SETTING_BLOCK,

+ PermissionEmbargoReason::BLACKLISTED);

}

// Tests that a URL that is blacklisted for one permission can still request

@@ -860,5 +894,6 @@ TEST_F(PermissionContextBaseTests, TestPermissionsBlacklistingAllowed) {

db_manager->BlacklistUrlPermissions(url, blacklisted_permissions);

TestPermissionsBlacklisting(content::PermissionType::NOTIFICATIONS,

CONTENT_SETTINGS_TYPE_NOTIFICATIONS, db_manager,

- url, 2000 /* timeout */, CONTENT_SETTING_ALLOW);

+ url, 2000 /* timeout */, CONTENT_SETTING_ALLOW,

+ PermissionEmbargoReason::NOT_EMBARGOED);

}

raymes 2017/02/08 23:20:09 Are we testing that NOT_EMBARGOED will be recorded

meredithl 2017/02/09 00:15:37 Not at all. I've added a histogram check into Test