Added Shields domain-specific ad/fingerprint block setting questions #12583

DJAndries · 2022-03-11T20:46:22Z

Resolves brave/brave-browser#21546

Resolves brave/brave-browser#22064
Resolves brave/brave-browser#22063

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Open browser with default settings
Verify that the P3A metrics Brave.Shields.DomainAdsSettingsAboveGlobal, Brave.Shields.DomainAdsSettingsBelowGlobal, Brave.Shields.DomainFingerprintSettingsAboveGlobal, Brave.Shields.DomainFingerprintSettingsBelowGlobal all have values of 0 on the brave://local-state page.
Access a website, update the domain-specific ad-blocking setting to "Aggressive"
Verify that the P3A metric Brave.Shields.DomainAdsSettingsAboveGlobal has a value of 1.
Access another website, update the domain-specific ad-blocking setting to "Allow"
Verify that the P3A metric Brave.Shields.DomainAdsSettingsBelowGlobal has a value of 1, and that Brave.Shields.DomainAdsSettingsAboveGlobal still has a value of 1.
Access a website, update the domain-specific fingerprint blocking setting to "Aggressive"
Verify that the P3A metric Brave.Shields.DomainFingerprintSettingsAboveGlobal has a value of 1.
Access another website, update the domain-specific fingerprint blocking setting to "Allow"
Verify that the P3A metric Brave.Shields.DomainFingerprintSettingsBelowGlobal has a value of 1, and that Brave.Shields.DomainFingerprintSettingsAboveGlobal still has a value of 1.

DJAndries · 2022-03-11T20:52:51Z

components/brave_shields/browser/brave_shields_p3a.cc

+  RecordShieldsFingerprintSetting(global_fp_setting);
+
+  ShieldsSettingCounts fp_counts = GetFPSettingCount(map);
+  ShieldsSettingCounts ads_counts = GetAdsSettingCount(map);


When performing the first recording of the metrics for a given profile, the domain-specific settings are counted by iterating through the HostContentSettingsMap rules. This will ensure that any settings made by the user before the browser upgrade will be accounted for in the metrics.

a comment in the code

DJAndries · 2022-03-11T20:58:17Z

components/brave_shields/browser/brave_shields_p3a.cc

+            << GetDomainSettingCount(profile_prefs, is_fingerprint,
+                                     *prev_setting);
+  }
+  UpdateDomainSettingCount(profile_prefs, is_fingerprint, new_setting, 1);


Any changes made to the domain-specific settings will be recorded in internal counts stored in the profile preferences. We keep a count of domains for each setting value. When a user changes a setting for a domain, we decrement the count for the previous setting value (if one existed), and increment the count for the new setting value. This is less costly compared to iterating through the HostContentSettingsMap rules every time a user changes a setting.

can you please put this comment into the code

DJAndries · 2022-03-11T21:00:37Z

components/brave_shields/browser/brave_shields_util.cc

-  if (url.is_empty()) {
-    // If global setting changed, report to P3A
-    RecordShieldsFingerprintSetting(type);
+  if (!map->IsOffTheRecord()) {


It is important that we ignore any domain-specific changes made in the guest/incognito profile.

pes10k

The policy looks good to me, but others would be better than me to review the implementation

iefremov · 2022-03-24T16:43:59Z

components/p3a/brave_p3a_service.cc

@@ -92,6 +92,10 @@ constexpr const char* kCollectedHistograms[] = {
    "Brave.Shields.UsageStatus",
    "Brave.Shields.AdBlockSetting",
    "Brave.Shields.FingerprintBlockSetting",
+    "Brave.Shields.DomainAdsSettingsAboveGlobal",


let's keep it sorted

iefremov · 2022-04-05T13:43:17Z

components/brave_shields/browser/ad_block_pref_service.cc

@@ -56,6 +61,10 @@ AdBlockPrefService::AdBlockPrefService(AdBlockService* ad_block_service,
  OnPreferenceChanged(prefs::kFBEmbedControlType);
  OnPreferenceChanged(prefs::kTwitterEmbedControlType);
  OnPreferenceChanged(prefs::kLinkedInEmbedControlType);
+
+  if (is_regular_profile) {
+    MaybeRecordInitialShieldsSettings(prefs, host_content_settings_map);


I don't really follow why do we need to pass all the stuff (HostContentSettingsMap, is_regular_profile) to this service (why this one?) just to record P3A. Am I missing something important about this specific location? Maybe we could do that around BraveProfileManager that already has some P3A initialization?

now that you point it out, i agree. moved the function call to browser/profiles/profile_util.cc

iefremov · 2022-04-05T13:44:30Z

components/brave_shields/browser/ad_block_service.cc

@@ -335,9 +334,6 @@ bool AdBlockService::Start() {
  regional_service_manager();
  subscription_service_manager();

-  MaybeRecordDefaultShieldsAdsSetting(local_state_);


why recording here doesn't work?

no access to profile

iefremov · 2022-04-05T13:51:01Z

browser/brave_shields/ad_block_pref_service_factory.cc

-      Profile::FromBrowserContext(context)->GetPrefs());
+      g_brave_browser_process->ad_block_service(), profile->GetPrefs(),
+      HostContentSettingsMapFactory::GetForProfile(profile),
+      profile->IsRegularProfile() && !profile->IsGuestSession());


i guess GuestSession is never Regular (see comments for IsRegularProfile())

In general I'm not in favor of passing new things to this service just for P3A

agreed, removed

iefremov · 2022-04-05T15:41:59Z

components/brave_shields/browser/brave_shields_util.cc

@@ -335,12 +355,21 @@ bool AllowReferrers(HostContentSettingsMap* map, const GURL& url) {
 void SetFingerprintingControlType(HostContentSettingsMap* map,
                                  ControlType type,
                                  const GURL& url,
-                                  PrefService* local_state) {
+                                  PrefService* local_state,
+                                  PrefService* profile_state) {


why not use local_state?

or vice versa, erase local state and use only prefs

Shields settings are per profile, so it may be best to store global/domain-specific setting counts on a per profile basis. The info from the last used profile will be transmitted at P3A report time.

The only shields P3A function that uses local_state is MaybeRecordShieldsUsageP3A, which was present before this PR. Perhaps it would be best to refactor that function to use profile prefs in a separate issue/PR

iefremov · 2022-04-05T15:44:16Z

components/brave_shields/common/brave_shield_utils.cc

+    const ContentSettingsForOneType& ads_rules) {
+  ShieldsSettingCounts result = {};
+
+  std::unordered_set<std::string> block_set;


using unordered_set is discouraged in chromium, see https://chromium.googlesource.com/chromium/src/+show/master/base/containers/README.md#32

switched to set

iefremov · 2022-04-08T13:04:09Z

components/brave_shields/browser/ad_block_pref_service.h

@@ -15,6 +15,7 @@

 class PrefChangeRegistrar;
 class PrefService;
+class HostContentSettingsMap;


iefremov · 2022-04-08T13:04:15Z

components/brave_shields/browser/ad_block_pref_service.cc

@@ -10,6 +10,7 @@
 #include "brave/components/brave_shields/browser/ad_block_regional_service_manager.h"
 #include "brave/components/brave_shields/browser/ad_block_service.h"
 #include "brave/components/brave_shields/browser/ad_block_subscription_service_manager.h"
+#include "brave/components/brave_shields/browser/brave_shields_p3a.h"


iefremov · 2022-04-08T16:27:22Z

browser/brave_shields/ad_block_pref_service_factory.cc

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

 #include "brave/browser/brave_browser_process.h"
 #include "brave/components/brave_shields/browser/ad_block_pref_service.h"
+#include "chrome/browser/content_settings/host_content_settings_map_factory.h"


iefremov · 2022-04-08T19:04:44Z

browser/ui/brave_shields_data_controller.cc

@@ -239,16 +239,21 @@ void BraveShieldsDataController::SetAdBlockMode(AdBlockMode mode) {

  if (mode == AdBlockMode::AGGRESSIVE) {
    control_type_cosmetic = ControlType::BLOCK;  // aggressive
-  } else {
+  } else if (mode == AdBlockMode::STANDARD) {


cc @antonok-edm

also confirmed with @nullhook yesterday

bridiver

chromium_src change looks ok

user setting is set to allow

DJAndries · 2022-04-11T23:44:29Z

force-pushed a rebase

GeetaSarvadnya · 2022-04-13T04:33:19Z

Verification PASSED on

Brave | 1.39.53 Chromium: 100.0.4896.88 (Official Build) nightly (64-bit)
-- | --
Revision | 4450653bfa91182e85723d8f1dee64dd6ce40ed4-refs/branch-heads/4896@{#1086}
OS | Windows 10 Version 21H2 (Build 19044.1586)

Verified the test plan mentioned in the description Shields cosmetic filitering preference is not set to "allow", when the user setting is set to "allow" brave-browser#22249 (comment)

[Note: Discussed with @DJAndries, the fix is only for the adblocking "Allow". The other two settings "Aggressive" and "Block" give the same internal preferences values in both 1.39.47 and 1.39.53 ]

Case 1:Verify cosmetic filtering preference_ad blocking settings set to "Allow"

Confirmed that cosmetic filtering preference settings is set to 1 when the ad-blocking setting is set to "Allow"

Before the fix - `1.39.47`	After the fix - `1.39.53`

Case 2:Verify cosmetic filtering preference_ad blocking settings set to "Aggressive"

Confirmed that cosmetic filtering preference settings is set to 2 when the ad-blocking setting is set to "Aggressive"

Case 3:Verify cosmetic filtering preference_ad blocking settings set to "Block trackers and ads"

Confirmed that cosmetic filtering preference settings is set to 2 when the ad-blocking setting is set to "Block trackers and ads"

GeetaSarvadnya · 2022-04-13T05:26:08Z

@DJAndries I don't see any difference in cosmetic filtering preference for shields v1 and v2 when ad blocking settings are set to "Allow" or "Aggressive" or "Block". Compared the internal cosmetic filtering preferences in 1.37.x and 1.39.x. Just trying to understand what is fixed as part of this PR.

Also, this PR is only for ad-blocking cosmetic filtering internal preferences? does it not applicable for Fingerprint and block cookies settings?

v1 - `1.37.113`	v2 - `1.39.53`

DJAndries · 2022-04-13T17:05:10Z

@GeetaSarvadnya it would be best to compare 1.39.53 against a slightly older version of nightly, since this bug is related to the new Shields UI, which is not available in 1.37.x.

This PR involves both the ad-block/cosmetic filtering, and fingerprint blocking settings. However, the commit that addresses brave/brave-browser#22249 only involves the ad-block/cosmetic setting.

DJAndries requested review from pes10k and rillian March 11, 2022 20:46

DJAndries requested review from iefremov and a team as code owners March 11, 2022 20:46

github-actions bot assigned DJAndries Mar 11, 2022

DJAndries added this to the 1.38.x - Nightly milestone Mar 11, 2022

DJAndries commented Mar 11, 2022

View reviewed changes

pes10k approved these changes Mar 11, 2022

View reviewed changes

DJAndries force-pushed the shields-p3a-multiple-domains branch from 4bd6752 to d6d353e Compare March 11, 2022 22:40

iefremov reviewed Mar 24, 2022

View reviewed changes

DJAndries force-pushed the shields-p3a-multiple-domains branch from d6d353e to b0a4e8e Compare March 26, 2022 00:00

DJAndries requested a review from iefremov March 26, 2022 00:41

iefremov reviewed Apr 5, 2022

View reviewed changes

DJAndries force-pushed the shields-p3a-multiple-domains branch from b0a4e8e to 67de76a Compare April 5, 2022 21:54

DJAndries requested a review from iefremov April 5, 2022 22:44

DJAndries force-pushed the shields-p3a-multiple-domains branch from c8f09f5 to dfa29c6 Compare April 5, 2022 22:53

DJAndries modified the milestones: 1.38.x - Beta, 1.39.x - Nightly Apr 5, 2022

DJAndries force-pushed the shields-p3a-multiple-domains branch from dfa29c6 to d9ebe58 Compare April 6, 2022 16:14

iefremov reviewed Apr 8, 2022

View reviewed changes

DJAndries force-pushed the shields-p3a-multiple-domains branch from d9ebe58 to cb01fb3 Compare April 8, 2022 17:19

iefremov reviewed Apr 8, 2022

View reviewed changes

iefremov approved these changes Apr 8, 2022

View reviewed changes

bridiver approved these changes Apr 11, 2022

View reviewed changes

DJAndries force-pushed the shields-p3a-multiple-domains branch from cb01fb3 to 2e08e18 Compare April 11, 2022 23:42

DJAndries added 3 commits April 11, 2022 16:43

Refactor initial shields P3A recording

acee49f

Added Shields domain-specific ad/fingerprint block setting questions

6a1aab5

Shields UI data controller: set cosmetic filtering setting to allow if

066a3dd

user setting is set to allow

DJAndries force-pushed the shields-p3a-multiple-domains branch from 2e08e18 to 066a3dd Compare April 11, 2022 23:43

DJAndries merged commit 2af3710 into master Apr 12, 2022

DJAndries deleted the shields-p3a-multiple-domains branch April 12, 2022 00:33

GeetaSarvadnya mentioned this pull request Apr 29, 2022

Add Shields domain-specific ad/fingerprint block setting questions brave/brave-browser#21546

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Shields domain-specific ad/fingerprint block setting questions #12583

Added Shields domain-specific ad/fingerprint block setting questions #12583

DJAndries commented Mar 11, 2022 •

edited

DJAndries Mar 11, 2022 •

edited

iefremov Apr 8, 2022

DJAndries Mar 11, 2022 •

edited

iefremov Apr 8, 2022

DJAndries Mar 11, 2022

pes10k left a comment

iefremov Mar 24, 2022

iefremov Apr 5, 2022

DJAndries Apr 5, 2022

iefremov Apr 5, 2022

DJAndries Apr 5, 2022

iefremov Apr 5, 2022

iefremov Apr 5, 2022

DJAndries Apr 5, 2022

iefremov Apr 5, 2022

iefremov Apr 5, 2022

DJAndries Apr 5, 2022 •

edited

DJAndries Apr 5, 2022

iefremov Apr 5, 2022

DJAndries Apr 5, 2022

iefremov Apr 8, 2022

iefremov Apr 8, 2022

iefremov Apr 8, 2022

iefremov Apr 8, 2022

DJAndries Apr 9, 2022

bridiver left a comment

DJAndries commented Apr 11, 2022

GeetaSarvadnya commented Apr 13, 2022 •

edited

GeetaSarvadnya commented Apr 13, 2022 •

edited

DJAndries commented Apr 13, 2022

Added Shields domain-specific ad/fingerprint block setting questions #12583

Added Shields domain-specific ad/fingerprint block setting questions #12583

Conversation

DJAndries commented Mar 11, 2022 • edited

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

DJAndries Mar 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJAndries Mar 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pes10k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJAndries Apr 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

DJAndries commented Apr 11, 2022

GeetaSarvadnya commented Apr 13, 2022 • edited

GeetaSarvadnya commented Apr 13, 2022 • edited

DJAndries commented Apr 13, 2022

DJAndries commented Mar 11, 2022 •

edited

DJAndries Mar 11, 2022 •

edited

DJAndries Mar 11, 2022 •

edited

DJAndries Apr 5, 2022 •

edited

GeetaSarvadnya commented Apr 13, 2022 •

edited

GeetaSarvadnya commented Apr 13, 2022 •

edited