From 26452c344aca1cd63563a81c3d13ffdee1d832d6 Mon Sep 17 00:00:00 2001 From: Kyle Hickinson Date: Wed, 7 Dec 2022 10:36:56 -0500 Subject: [PATCH 1/2] Fix #6592: Answer shields p3a question with initial state once This matches desktops behaviour where they seem to record the answer only once --- .../Browser/BrowserViewController.swift | 2 + .../BrowserViewController+P3A.swift | 48 +++++++++++++++++++ ...rowserViewController+ToolbarDelegate.swift | 8 +++- .../Shields/ShieldsViewController.swift | 46 ++---------------- Sources/BraveShared/Preferences.swift | 2 + 5 files changed, 62 insertions(+), 44 deletions(-) create mode 100644 Client/Frontend/Browser/BrowserViewController/BrowserViewController+P3A.swift diff --git a/Client/Frontend/Browser/BrowserViewController.swift b/Client/Frontend/Browser/BrowserViewController.swift index f54e2c77b92..0ff1fddb097 100644 --- a/Client/Frontend/Browser/BrowserViewController.swift +++ b/Client/Frontend/Browser/BrowserViewController.swift @@ -576,6 +576,8 @@ public class BrowserViewController: UIViewController { // Eliminate the older usage days // Used in App Rating criteria AppReviewManager.shared.processMainCriteria(for: .daysInUse) + + maybeRecordInitialShieldsP3A() } private func setupAdsNotificationHandler() { diff --git a/Client/Frontend/Browser/BrowserViewController/BrowserViewController+P3A.swift b/Client/Frontend/Browser/BrowserViewController/BrowserViewController+P3A.swift new file mode 100644 index 00000000000..9b0be8cc603 --- /dev/null +++ b/Client/Frontend/Browser/BrowserViewController/BrowserViewController+P3A.swift @@ -0,0 +1,48 @@ +// Copyright 2022 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import Foundation +import BraveShared +import Growth +import Data + +extension BrowserViewController { + + func maybeRecordInitialShieldsP3A() { + if Preferences.Shields.initialP3AStateReported.value { return } + defer { Preferences.Shields.initialP3AStateReported.value = true } + recordShieldsUpdateP3A(shield: .AdblockAndTp) + recordShieldsUpdateP3A(shield: .FpProtection) + } + + func recordShieldsUpdateP3A(shield: BraveShield) { + let buckets: [Bucket] = [ + 0, + .r(1...5), + .r(6...10), + .r(11...20), + .r(21...30), + .r(31...), + ] + switch shield { + case .AdblockAndTp: + // Q51 On how many domains has the user set the adblock setting to be lower (block less) than the default? + let adsBelowGlobalCount = Domain.totalDomainsWithAdblockShieldsLoweredFromGlobal() + UmaHistogramRecordValueToBucket("Brave.Shields.DomainAdsSettingsBelowGlobal", buckets: buckets, value: adsBelowGlobalCount) + // Q52 On how many domains has the user set the adblock setting to be higher (block more) than the default? + let adsAboveGlobalCount = Domain.totalDomainsWithAdblockShieldsIncreasedFromGlobal() + UmaHistogramRecordValueToBucket("Brave.Shields.DomainAdsSettingsAboveGlobal", buckets: buckets, value: adsBelowGlobalCount) + case .FpProtection: + // Q53 On how many domains has the user set the FP setting to be lower (block less) than the default? + let fingerprintingBelowGlobalCount = Domain.totalDomainsWithFingerprintingProtectionLoweredFromGlobal() + UmaHistogramRecordValueToBucket("Brave.Shields.DomainFingerprintSettingsBelowGlobal", buckets: buckets, value: fingerprintingBelowGlobalCount) + // Q54 On how many domains has the user set the FP setting to be higher (block more) than the default? + let fingerprintingAboveGlobalCount = Domain.totalDomainsWithFingerprintingProtectionIncreasedFromGlobal() + UmaHistogramRecordValueToBucket("Brave.Shields.DomainFingerprintSettingsAboveGlobal", buckets: buckets, value: fingerprintingAboveGlobalCount) + case .AllOff, .NoScript, .SafeBrowsing: + break + } + } +} diff --git a/Client/Frontend/Browser/BrowserViewController/BrowserViewController+ToolbarDelegate.swift b/Client/Frontend/Browser/BrowserViewController/BrowserViewController+ToolbarDelegate.swift index edc5208a08a..a7fb08facae 100644 --- a/Client/Frontend/Browser/BrowserViewController/BrowserViewController+ToolbarDelegate.swift +++ b/Client/Frontend/Browser/BrowserViewController/BrowserViewController+ToolbarDelegate.swift @@ -292,13 +292,19 @@ extension BrowserViewController: TopToolbarDelegate { } let shields = ShieldsViewController(tab: selectedTab) - shields.shieldsSettingsChanged = { [unowned self] _ in + shields.shieldsSettingsChanged = { [unowned self] _, shield in // Update the shields status immediately self.topToolbar.refreshShieldsStatus() // Reload this tab. This will also trigger an update of the brave icon in `TabLocationView` if // the setting changed is the global `.AllOff` shield self.tabManager.selectedTab?.reload() + + // Record P3A shield changes + DispatchQueue.main.asyncAfter(deadline: .now() + 1) { + // Record shields & FP related hisotgrams, wait a sec for CoreData to sync contexts + self.recordShieldsUpdateP3A(shield: shield) + } // In 1.6 we "reload" the whole web view state, dumping caches, etc. (reload():BraveWebView.swift:495) // BRAVE TODO: Port over proper tab reloading with Shields diff --git a/Client/Frontend/Shields/ShieldsViewController.swift b/Client/Frontend/Shields/ShieldsViewController.swift index 60e96f49e09..f121186c083 100644 --- a/Client/Frontend/Shields/ShieldsViewController.swift +++ b/Client/Frontend/Shields/ShieldsViewController.swift @@ -27,7 +27,7 @@ class ShieldsViewController: UIViewController, PopoverContentComponent { return _url }() - var shieldsSettingsChanged: ((ShieldsViewController) -> Void)? + var shieldsSettingsChanged: ((ShieldsViewController, BraveShield) -> Void)? var showGlobalShieldsSettings: ((ShieldsViewController) -> Void)? private var statsUpdateObservable: AnyObject? @@ -105,10 +105,6 @@ class ShieldsViewController: UIViewController, PopoverContentComponent { Domain.setBraveShield( forUrl: url, shield: shield, isOn: isOn, isPrivateBrowsing: PrivateBrowsingManager.shared.isPrivateBrowsing) - DispatchQueue.main.asyncAfter(deadline: .now() + 1) { - // Record shields & FP related hisotgrams, wait a sec for CoreData to sync contexts - self.recordShieldsUpdateP3A(shield: shield) - } } private func updateGlobalShieldState(_ on: Bool, animated: Bool = false) { @@ -273,7 +269,7 @@ class ShieldsViewController: UIViewController, PopoverContentComponent { // Wait a fraction of a second to allow DB write to complete otherwise it will not use the // updated shield settings when reloading the page DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) { - self.shieldsSettingsChanged?(self) + self.shieldsSettingsChanged?(self, shield) } } } @@ -286,7 +282,7 @@ class ShieldsViewController: UIViewController, PopoverContentComponent { // Wait a fraction of a second to allow DB write to complete otherwise it will not use the updated // shield settings when reloading the page DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) { - self.shieldsSettingsChanged?(self) + self.shieldsSettingsChanged?(self, .AllOff) } } @@ -344,40 +340,4 @@ class ShieldsViewController: UIViewController, PopoverContentComponent { required init?(coder aDecoder: NSCoder) { fatalError() } - - // MARK: - P3A - - private enum P3AShieldUpdateKind { - case adblock - case fingerprintingProtection - } - - private func recordShieldsUpdateP3A(shield: BraveShield) { - let buckets: [Bucket] = [ - 0, - .r(1...5), - .r(6...10), - .r(11...20), - .r(21...30), - .r(31...), - ] - switch shield { - case .AdblockAndTp: - // Q51 On how many domains has the user set the adblock setting to be lower (block less) than the default? - let adsBelowGlobalCount = Domain.totalDomainsWithAdblockShieldsLoweredFromGlobal() - UmaHistogramRecordValueToBucket("Brave.Shields.DomainAdsSettingsBelowGlobal", buckets: buckets, value: adsBelowGlobalCount) - // Q52 On how many domains has the user set the adblock setting to be higher (block more) than the default? - let adsAboveGlobalCount = Domain.totalDomainsWithAdblockShieldsIncreasedFromGlobal() - UmaHistogramRecordValueToBucket("Brave.Shields.DomainAdsSettingsAboveGlobal", buckets: buckets, value: adsBelowGlobalCount) - case .FpProtection: - // Q53 On how many domains has the user set the FP setting to be lower (block less) than the default? - let fingerprintingBelowGlobalCount = Domain.totalDomainsWithFingerprintingProtectionLoweredFromGlobal() - UmaHistogramRecordValueToBucket("Brave.Shields.DomainFingerprintSettingsBelowGlobal", buckets: buckets, value: fingerprintingBelowGlobalCount) - // Q54 On how many domains has the user set the FP setting to be higher (block more) than the default? - let fingerprintingAboveGlobalCount = Domain.totalDomainsWithFingerprintingProtectionIncreasedFromGlobal() - UmaHistogramRecordValueToBucket("Brave.Shields.DomainFingerprintSettingsAboveGlobal", buckets: buckets, value: fingerprintingAboveGlobalCount) - case .AllOff, .NoScript, .SafeBrowsing: - break - } - } } diff --git a/Sources/BraveShared/Preferences.swift b/Sources/BraveShared/Preferences.swift index a79d6e74fbb..0b61307ab7d 100644 --- a/Sources/BraveShared/Preferences.swift +++ b/Sources/BraveShared/Preferences.swift @@ -87,6 +87,8 @@ extension Preferences { public static let adblockStatsDataVersion = Option(key: "stats.adblock-data-version", default: nil) /// Whether or not advanced controls in the shields UI are visible by default public static let advancedControlsVisible = Option(key: "shields.advanced-controls-visible", default: false) + /// Whether or not we've reported the initial state of shields for p3a + public static let initialP3AStateReported = Option(key: "shields.initial-p3a-state-reported", default: false) } public final class Rewards { From 11391e02e0f2d3c8e0786b112082a7dbc0c6bb41 Mon Sep 17 00:00:00 2001 From: Kyle Hickinson Date: Wed, 7 Dec 2022 11:06:46 -0500 Subject: [PATCH 2/2] Fix #6591: Report correct count for domains above global p3a question --- .../BrowserViewController/BrowserViewController+P3A.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Client/Frontend/Browser/BrowserViewController/BrowserViewController+P3A.swift b/Client/Frontend/Browser/BrowserViewController/BrowserViewController+P3A.swift index 9b0be8cc603..c601b9facb5 100644 --- a/Client/Frontend/Browser/BrowserViewController/BrowserViewController+P3A.swift +++ b/Client/Frontend/Browser/BrowserViewController/BrowserViewController+P3A.swift @@ -33,7 +33,7 @@ extension BrowserViewController { UmaHistogramRecordValueToBucket("Brave.Shields.DomainAdsSettingsBelowGlobal", buckets: buckets, value: adsBelowGlobalCount) // Q52 On how many domains has the user set the adblock setting to be higher (block more) than the default? let adsAboveGlobalCount = Domain.totalDomainsWithAdblockShieldsIncreasedFromGlobal() - UmaHistogramRecordValueToBucket("Brave.Shields.DomainAdsSettingsAboveGlobal", buckets: buckets, value: adsBelowGlobalCount) + UmaHistogramRecordValueToBucket("Brave.Shields.DomainAdsSettingsAboveGlobal", buckets: buckets, value: adsAboveGlobalCount) case .FpProtection: // Q53 On how many domains has the user set the FP setting to be lower (block less) than the default? let fingerprintingBelowGlobalCount = Domain.totalDomainsWithFingerprintingProtectionLoweredFromGlobal()