Skip to content

Commit

Permalink
Move checks for MSBB and UMA consent into common dependencies.
Browse files Browse the repository at this point in the history
UMA is currently not required, I added this because we will soon need
it and digging up the pref key isn't straightforward.

This CL should not change behavior.

Bug: b/209384145
Change-Id: Ieb88f187bbb514b90c78154ccef395379146b724
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3785001
Reviewed-by: Luca Hunkeler <hluca@google.com>
Commit-Queue: Florian Gauger <fga@google.com>
Auto-Submit: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Florian Gauger <fga@google.com>
Cr-Commit-Position: refs/heads/main@{#1035027}
  • Loading branch information
Florian Gauger authored and Chromium LUCI CQ committed Aug 15, 2022
1 parent 3f06b58 commit bdd4518
Show file tree
Hide file tree
Showing 29 changed files with 240 additions and 134 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@
import org.chromium.chrome.browser.IntentHandler;
import org.chromium.chrome.browser.IntentHandler.ExternalAppId;
import org.chromium.chrome.browser.flags.ActivityType;
import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.signin.services.UnifiedConsentServiceBridge;
import org.chromium.chrome.browser.tab.EmptyTabObserver;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.components.autofill_assistant.AssistantFeatures;
Expand Down Expand Up @@ -87,10 +85,5 @@ public static boolean areDirectActionsAvailable(@ActivityType int activityType)
&& AssistantFeatures.AUTOFILL_ASSISTANT_DIRECT_ACTIONS.isEnabled();
}

public static boolean isMakeSearchesAndBrowsingBetterSettingEnabled() {
return UnifiedConsentServiceBridge.isUrlKeyedAnonymizedDataCollectionEnabled(
Profile.getLastUsedRegularProfile());
}

private AssistantDependencyUtilsChrome() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ public static void createForTab(Tab tab) {
-> TabUtils.getActivity(tab),
tab.getWebContents(), new AssistantStaticDependenciesChrome(),
AssistantDependencyUtilsChrome::isGsa,
AssistantDependencyUtilsChrome::isMakeSearchesAndBrowsingBetterSettingEnabled,
new AssistantModuleInstallUiProviderChrome(tab));
AssistantDependencyUtilsChrome.attachTabObserver(tab, starter);
tab.getUserDataHost().setUserData(USER_DATA_KEY, starter);
Expand Down
20 changes: 20 additions & 0 deletions chrome/browser/autofill_assistant/common_dependencies_chrome.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//
#include "chrome/browser/autofill_assistant/common_dependencies_chrome.h"

#include "base/values.h"
#include "chrome/browser/autofill/personal_data_manager_factory.h"
#include "chrome/browser/autofill_assistant/annotate_dom_model_service_factory.h"
#include "chrome/browser/autofill_assistant/assistant_field_trial_util_chrome.h"
Expand All @@ -16,11 +17,15 @@
#include "components/autofill_assistant/browser/assistant_field_trial_util.h"
#include "components/autofill_assistant/browser/dependencies_util.h"
#include "components/autofill_assistant/content/browser/annotate_dom_model_service.h"
#include "components/metrics/metrics_pref_names.h"
#include "components/prefs/pref_service.h"
#include "components/signin/public/identity_manager/account_info.h"
#include "components/signin/public/identity_manager/identity_manager.h"
#include "components/signin/public/identity_manager/tribool.h"
#include "components/unified_consent/pref_names.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/web_contents.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

using ::autofill::PersonalDataManager;
using ::content::WebContents;
Expand Down Expand Up @@ -126,4 +131,19 @@ version_info::Channel CommonDependenciesChrome::GetChannel() const {
return chrome::GetChannel();
}

bool CommonDependenciesChrome::GetMakeSearchesAndBrowsingBetterEnabled(
content::BrowserContext* browser_context) const {
return Profile::FromBrowserContext(browser_context)
->GetPrefs()
->GetBoolean(
unified_consent::prefs::kUrlKeyedAnonymizedDataCollectionEnabled);
}

bool CommonDependenciesChrome::GetMetricsReportingEnabled(
content::BrowserContext* browser_context) const {
return Profile::FromBrowserContext(browser_context)
->GetPrefs()
->GetBoolean(metrics::prefs::kMetricsReportingEnabled);
}

} // namespace autofill_assistant
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ class CommonDependenciesChrome : public CommonDependencies {
content::BrowserContext* browser_context) const override;

version_info::Channel GetChannel() const override;

bool GetMakeSearchesAndBrowsingBetterEnabled(
content::BrowserContext* browser_context) const override;

bool GetMetricsReportingEnabled(
content::BrowserContext* browser_context) const override;
};

} // namespace autofill_assistant
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ public class Starter implements AssistantTabObserver, UserData {

private final AssistantStaticDependencies mStaticDependencies;
private final AssistantIsGsaFunction mIsGsaFunction;
private final AssistantIsMsbbEnabledFunction mIsMsbbEnabledFunction;
private final AssistantModuleInstallUi.Provider mModuleInstallUiProvider;

/**
Expand Down Expand Up @@ -70,12 +69,10 @@ public class Starter implements AssistantTabObserver, UserData {
*/
public Starter(Supplier<Activity> activitySupplier, @Nullable WebContents webContents,
AssistantStaticDependencies staticDependencies, AssistantIsGsaFunction isGsaFunction,
AssistantIsMsbbEnabledFunction isMsbbEnabledFunction,
AssistantModuleInstallUi.Provider moduleInstallUiProvider) {
mActivitySupplier = activitySupplier;
mStaticDependencies = staticDependencies;
mIsGsaFunction = isGsaFunction;
mIsMsbbEnabledFunction = isMsbbEnabledFunction;
mModuleInstallUiProvider = moduleInstallUiProvider;
detectWebContentsChange(webContents);
}
Expand Down Expand Up @@ -285,11 +282,6 @@ private static void setProactiveHelpSettingEnabled(boolean enabled) {
AutofillAssistantPreferencesUtil.setProactiveHelpPreference(enabled);
}

@CalledByNative
private boolean getMakeSearchesAndBrowsingBetterSettingEnabled() {
return mIsMsbbEnabledFunction.getAsBoolean();
}

private AutofillAssistantModuleEntry getModuleOrThrow() {
if (!getFeatureModuleInstalled()) {
throw new RuntimeException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,17 +239,6 @@ void StarterDelegateAndroid::SetProactiveHelpSettingEnabled(bool enabled) {
base::android::AttachCurrentThread(), enabled);
}

bool StarterDelegateAndroid::GetMakeSearchesAndBrowsingBetterEnabled() const {
if (!java_object_) {
// Failsafe, should never happen.
NOTREACHED();
return false;
}

return Java_Starter_getMakeSearchesAndBrowsingBetterSettingEnabled(
base::android::AttachCurrentThread(), java_object_);
}

bool StarterDelegateAndroid::GetIsLoggedIn() {
return !GetCommonDependencies()
->GetSignedInEmail(GetWebContents().GetBrowserContext())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ class StarterDelegateAndroid
void HideOnboarding() override;
bool GetProactiveHelpSettingEnabled() const override;
void SetProactiveHelpSettingEnabled(bool enabled) override;
bool GetMakeSearchesAndBrowsingBetterEnabled() const override;
bool GetIsLoggedIn() override;
bool GetIsSupervisedUser() override;
bool GetIsAllowedForMachineLearning() override;
Expand Down
10 changes: 10 additions & 0 deletions components/autofill_assistant/browser/common_dependencies.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,14 @@ bool CommonDependencies::IsAllowedForMachineLearning(
return true;
}

bool CommonDependencies::GetMakeSearchesAndBrowsingBetterEnabled(
content::BrowserContext* browser_context) const {
return false;
}

bool CommonDependencies::GetMetricsReportingEnabled(
content::BrowserContext* browser_context) const {
return false;
}

} // namespace autofill_assistant
6 changes: 6 additions & 0 deletions components/autofill_assistant/browser/common_dependencies.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,12 @@ class CommonDependencies {
content::BrowserContext* browser_context) const = 0;

virtual version_info::Channel GetChannel() const = 0;

virtual bool GetMakeSearchesAndBrowsingBetterEnabled(
content::BrowserContext* browser_context) const;

virtual bool GetMetricsReportingEnabled(
content::BrowserContext* browser_context) const;
};

} // namespace autofill_assistant
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,6 @@ void StarterDelegateDesktop::SetProactiveHelpSettingEnabled(bool enabled) {
NOTREACHED();
}

bool StarterDelegateDesktop::GetMakeSearchesAndBrowsingBetterEnabled() const {
// Only relevant for trigger scripts, which don't exist in headless.
return false;
}

bool StarterDelegateDesktop::GetIsLoggedIn() {
return !common_dependencies_
->GetSignedInEmail(GetWebContents().GetBrowserContext())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ class StarterDelegateDesktop
void HideOnboarding() override;
bool GetProactiveHelpSettingEnabled() const override;
void SetProactiveHelpSettingEnabled(bool enabled) override;
bool GetMakeSearchesAndBrowsingBetterEnabled() const override;
bool GetIsLoggedIn() override;
bool GetIsSupervisedUser() override;
bool GetIsAllowedForMachineLearning() override;
Expand Down
10 changes: 10 additions & 0 deletions components/autofill_assistant/browser/fake_common_dependencies.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,14 @@ version_info::Channel FakeCommonDependencies::GetChannel() const {
return channel_;
}

bool FakeCommonDependencies::GetMakeSearchesAndBrowsingBetterEnabled(
content::BrowserContext* browser_context) const {
return msbb_enabled_;
}

bool FakeCommonDependencies::GetMetricsReportingEnabled(
content::BrowserContext* browser_context) const {
return uma_enabled_;
}

} // namespace autofill_assistant
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ class FakeCommonDependencies : public CommonDependencies {
signin::IdentityManager* GetIdentityManager(
content::BrowserContext* browser_context) const override;
version_info::Channel GetChannel() const override;
bool GetMakeSearchesAndBrowsingBetterEnabled(
content::BrowserContext* browser_context) const override;
bool GetMetricsReportingEnabled(
content::BrowserContext* browser_context) const override;

// Intentionally public to allow tests direct access.
std::string locale_;
Expand All @@ -45,6 +49,8 @@ class FakeCommonDependencies : public CommonDependencies {
bool is_allowed_for_machine_learning_ = true;
bool is_weblayer_ = false;
version_info::Channel channel_ = version_info::Channel::UNKNOWN;
bool msbb_enabled_ = true;
bool uma_enabled_ = true;
};

} // namespace autofill_assistant
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,6 @@ void FakeStarterPlatformDelegate::SetProactiveHelpSettingEnabled(bool enabled) {
proactive_help_enabled_ = enabled;
}

bool FakeStarterPlatformDelegate::GetMakeSearchesAndBrowsingBetterEnabled()
const {
return msbb_enabled_;
}

bool FakeStarterPlatformDelegate::GetIsLoggedIn() {
return is_logged_in_;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ class FakeStarterPlatformDelegate : public StarterPlatformDelegate {
void HideOnboarding() override;
bool GetProactiveHelpSettingEnabled() const override;
void SetProactiveHelpSettingEnabled(bool enabled) override;
bool GetMakeSearchesAndBrowsingBetterEnabled() const override;
bool GetIsLoggedIn() override;
bool GetIsSupervisedUser() override;
bool GetIsAllowedForMachineLearning() override;
Expand Down Expand Up @@ -82,7 +81,6 @@ class FakeStarterPlatformDelegate : public StarterPlatformDelegate {
base::OnceCallback<void(bool, OnboardingResult)> result_callback)>
on_show_onboarding_callback_;
bool proactive_help_enabled_ = true;
bool msbb_enabled_ = true;
bool is_logged_in_ = true;
bool is_supervised_user_ = false;
bool is_allowed_for_machine_learning_ = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ class MockCommonDependencies : public CommonDependencies {
(content::BrowserContext*),
(const override));
MOCK_METHOD(version_info::Channel, GetChannel, (), (const override));
MOCK_METHOD(bool,
GetMakeSearchesAndBrowsingBetterEnabled,
(content::BrowserContext*),
(const override));
MOCK_METHOD(bool,
GetMetricsReportingEnabled,
(content::BrowserContext*),
(const override));
};

} // namespace autofill_assistant
Expand Down
15 changes: 10 additions & 5 deletions components/autofill_assistant/browser/starter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "base/no_destructor.h"
#include "base/strings/string_util.h"
#include "components/autofill_assistant/browser/assistant_field_trial_util.h"
#include "components/autofill_assistant/browser/common_dependencies.h"
#include "components/autofill_assistant/browser/features.h"
#include "components/autofill_assistant/browser/intent_strings.h"
#include "components/autofill_assistant/browser/service/api_key_fetcher.h"
Expand Down Expand Up @@ -386,15 +387,17 @@ void Starter::Init() {
bool switched_from_cct_to_tab = prev_is_custom_tab && !is_custom_tab_;
bool proactive_help_setting_enabled =
platform_delegate_->GetProactiveHelpSettingEnabled();
bool msbb_setting_enabled =
platform_delegate_->GetMakeSearchesAndBrowsingBetterEnabled();
bool msbb_setting_enabled = platform_delegate_->GetCommonDependencies()
->GetMakeSearchesAndBrowsingBetterEnabled(
web_contents()->GetBrowserContext());
bool feature_module_installed =
platform_delegate_->GetFeatureModuleInstalled();
bool prev_fetch_trigger_scripts_on_navigation =
fetch_trigger_scripts_on_navigation_;

starter_heuristic_->InitFromHeuristicConfigs(heuristic_configs_,
platform_delegate_.get());
starter_heuristic_->InitFromHeuristicConfigs(
heuristic_configs_, platform_delegate_.get(),
web_contents()->GetBrowserContext());
fetch_trigger_scripts_on_navigation_ = starter_heuristic_->HasConditionSets();

// If there is a pending startup, re-check that the settings are still
Expand Down Expand Up @@ -491,7 +494,9 @@ void Starter::Start(std::unique_ptr<TriggerContext> trigger_context) {

StartupMode startup_mode = StartupUtil().ChooseStartupModeForIntent(
*pending_trigger_context_,
{platform_delegate_->GetMakeSearchesAndBrowsingBetterEnabled(),
{platform_delegate_->GetCommonDependencies()
->GetMakeSearchesAndBrowsingBetterEnabled(
web_contents()->GetBrowserContext()),
platform_delegate_->GetProactiveHelpSettingEnabled(),
platform_delegate_->GetFeatureModuleInstalled()});
Metrics::RecordStartRequest(ukm_recorder_, current_ukm_source_id_,
Expand Down
7 changes: 4 additions & 3 deletions components/autofill_assistant/browser/starter_heuristic.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,17 @@ StarterHeuristic::~StarterHeuristic() = default;

void StarterHeuristic::InitFromHeuristicConfigs(
const std::vector<std::unique_ptr<StarterHeuristicConfig>>& configs,
StarterPlatformDelegate* platform_delegate) {
StarterPlatformDelegate* platform_delegate,
content::BrowserContext* browser_context) {
url_matcher_ = std::make_unique<url_matcher::URLMatcher>();
matcher_id_to_config_map_.clear();

url_matcher::URLMatcherConditionSet::Vector condition_sets;
base::flat_map<base::MatcherStringPattern::ID, HeuristicConfigEntry> mapping;
base::MatcherStringPattern::ID next_condition_set_id = 0;
for (const auto& config : configs) {
for (const auto& condition_set :
config->GetConditionSetsForClientState(platform_delegate)) {
for (const auto& condition_set : config->GetConditionSetsForClientState(
platform_delegate, browser_context)) {
if (!condition_set.is_dict()) {
LOG(ERROR) << "Invalid heuristic config: expected a dictionary for "
"each condition set, but got "
Expand Down
7 changes: 6 additions & 1 deletion components/autofill_assistant/browser/starter_heuristic.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
#include "components/url_matcher/url_matcher_factory.h"
#include "url/gurl.h"

namespace content {
class BrowserContext;
} // namespace content

namespace autofill_assistant {

// Utility that implements a heuristic for autofill-assistant URLs.
Expand All @@ -33,7 +37,8 @@ class StarterHeuristic : public base::RefCountedThreadSafe<StarterHeuristic> {
// the current client state.
void InitFromHeuristicConfigs(
const std::vector<std::unique_ptr<StarterHeuristicConfig>>& configs,
StarterPlatformDelegate* platform_delegate);
StarterPlatformDelegate* platform_delegate,
content::BrowserContext* browser_context);

// Returns true if at least one condition set is available. There is no point
// in running the heuristic otherwise.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ const std::string& FinchStarterHeuristicConfig::GetIntent() const {

const base::Value::List&
FinchStarterHeuristicConfig::GetConditionSetsForClientState(
StarterPlatformDelegate* platform_delegate) const {
StarterPlatformDelegate* platform_delegate,
content::BrowserContext* browser_context) const {
static const base::NoDestructor<base::Value> empty_list(
base::Value::Type::LIST);
if (platform_delegate->GetIsSupervisedUser() ||
Expand Down Expand Up @@ -52,7 +53,8 @@ FinchStarterHeuristicConfig::GetConditionSetsForClientState(
return empty_list->GetList();
}

if (!platform_delegate->GetMakeSearchesAndBrowsingBetterEnabled() &&
if (!platform_delegate->GetCommonDependencies()
->GetMakeSearchesAndBrowsingBetterEnabled(browser_context) &&
!enabled_without_msbb_) {
return empty_list->GetList();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ class FinchStarterHeuristicConfig : public StarterHeuristicConfig {
// Overrides HeuristicConfig:
const std::string& GetIntent() const override;
const base::Value::List& GetConditionSetsForClientState(
StarterPlatformDelegate* platform_delegate) const override;
StarterPlatformDelegate* platform_delegate,
content::BrowserContext* browser_context) const override;
const base::flat_set<std::string>& GetDenylistedDomains() const override;

private:
Expand Down

0 comments on commit bdd4518

Please sign in to comment.