Enforce settings dialog for simple override extensions [chromium/src : main]
Christopher Grant (Gerrit)
Christopher Grant added 1 comment![]( "Open in Gerrit")
Is this CL being posted as a proof-of-concept? This would almost certainly need to be flag-gated.
Related details
- Daniel Soromou
- Darryl James
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou added 1 comment![]( "Open in Gerrit")
Is this CL being posted as a proof-of-concept? This would almost certainly need to be flag-gated.
- Christopher Grant
- Darryl James
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
sources = [ "features.h" ]My gut is telling me that these files should be included in one of the if blocks below, like `enable_extensions` or `enable_extensions_core`. Unclear which one at a glance
If it's not a problem with the build system disregard 👍
optionally, the feature files can go in a separate target name `extensions_features` might be more than we need but is an option.
Related details
- Christopher Grant
- Daniel Soromou
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christopher Grant (Gerrit)
Christopher Grant added 3 comments![]( "Open in Gerrit")
"extensions.simple_override_enforcement_timestamp";Suggestions:
- Might want "begin" in there to be more clear
- Enforcement -> Confirmation?
Ie, maybe "extensions.simple_override_begin_confirmation_timestamp"
Will defer to code owners on this though.
if (!base::FeatureList::IsEnabled(Style: This new block of logic could be pulled out to a separate method for readability, like ShouldShowForSimpleOverrideExtension().
extensions::features::kGrandfatheredSimpleOverrideDialog)) {Suggestion: The feature flag probably doesn't need the Grandfathering aspect. The design doc suggests "SearchEngineUnconditionalDialog", ie, covers the entire change in its naming.
Related details
- Daniel Soromou
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
overall lgtm! % waiting on +1 from Devlin as extensions ONWER before stamping 🙏
Related details
- Daniel Soromou
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou added 4 comments![]( "Open in Gerrit")
sources = [ "features.h" ]My gut is telling me that these files should be included in one of the if blocks below, like `enable_extensions` or `enable_extensions_core`. Unclear which one at a glance
If it's not a problem with the build system disregard 👍
optionally, the feature files can go in a separate target name `extensions_features` might be more than we need but is an option.
Done
"extensions.simple_override_enforcement_timestamp";Suggestions:
- Might want "begin" in there to be more clear
- Enforcement -> Confirmation?
Ie, maybe "extensions.simple_override_begin_confirmation_timestamp"
Will defer to code owners on this though.
Done
if (!base::FeatureList::IsEnabled(Style: This new block of logic could be pulled out to a separate method for readability, like ShouldShowForSimpleOverrideExtension().
Done
extensions::features::kGrandfatheredSimpleOverrideDialog)) {Suggestion: The feature flag probably doesn't need the Grandfathering aspect. The design doc suggests "SearchEngineUnconditionalDialog", ie, covers the entire change in its naming.
- Christopher Grant
- Darryl James
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
sources = [ "features.h" ]Daniel SoromouMy gut is telling me that these files should be included in one of the if blocks below, like `enable_extensions` or `enable_extensions_core`. Unclear which one at a glance
If it's not a problem with the build system disregard 👍
optionally, the feature files can go in a separate target name `extensions_features` might be more than we need but is an option.
Done
- Christopher Grant
- Daniel Soromou
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | -1 |
1 comment
Thanks, Daniel. I've kicked off a thread about this offline; please respond there. The linked bug also doesn't appear to have sufficient information.
For now, not lgtm since this is a sensitive area in the code and we should be careful about changes going in.
Related details
- Christopher Grant
- Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christopher Grant (Gerrit)
Christopher Grant added 1 comment![]( "Open in Gerrit")
Dropping myself from Review until we've got the design reviewed and have discussed details with Devlin and his team.
Related details
- Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou added 2 comments![]( "Open in Gerrit")
Dropping myself from Review until we've got the design reviewed and have discussed details with Devlin and his team.
Done
Thanks, Daniel. I've kicked off a thread about this offline; please respond there. The linked bug also doesn't appear to have sufficient information.
For now, not lgtm since this is a sensitive area in the code and we should be careful about changes going in.
- Christopher Grant
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Emilia Paz added 1 comment![]( "Open in Gerrit")
Daniel SoromouThanks, Daniel. I've kicked off a thread about this offline; please respond there. The linked bug also doesn't appear to have sufficient information.
For now, not lgtm since this is a sensitive area in the code and we should be careful about changes going in.
Acknowledged
Has this been approved? If so, would be good to link the doc to the bug
Related details
- Christopher Grant
- Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou added 1 comment![]( "Open in Gerrit")
Daniel SoromouThanks, Daniel. I've kicked off a thread about this offline; please respond there. The linked bug also doesn't appear to have sufficient information.
For now, not lgtm since this is a sensitive area in the code and we should be careful about changes going in.
Emilia PazAcknowledged
Has this been approved? If so, would be good to link the doc to the bug
I have updated the description with the doc link: go/chrome-dse-selection
Related details
- Christopher Grant
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Emilia Paz added 8 comments![]( "Open in Gerrit")
Thanks Daniel! Overall lgtm, just some comments
// Historically, "Simple Overrides" were exempt from this dialog. We are
// removing that exemption, but we grandfather in extensions installed
// before the policy change was enabled to prevent spamming existing users.nit: Lets add the bug number here, and add a description to the bug (basically this [section](https://docs.google.com/document/d/1ZeYWHCL-9tELnmBH33UBjoK9uINhpK8aySeipK-PSQ4/edit?tab=t.0#bookmark=id.h16zg5ufedlg) on the design doc)
if (!base::FeatureList::IsEnabled(
extensions::features::kSearchEngineUnconditionalDialog)) {
PrefService* prefs = profile_->GetPrefs();
prefs->ClearPref(kSimpleOverrideBeginConfirmationTimestamp);
return false;
}nit: explain why we do this. My understanding is to handling if the feature is enabled, then disabled, and then re-enabled, the grandfathering timestamp will be reset to the time of re-enabling. Any extensions installed while the feature was disabled will be grandfathered
// If the extension was installed before the enforcement logic began,
// do not show the dialog (grandfathered).
if (install_time < enforcement_time) {
return false;
}
return true;Good point. This can be simplified to:
```suggestion
// If the extension was installed after the enforcement logic began,
// show the dialog.
return install_time >= enforcement_time;
```
// Tests that simple override extensions don't trigger the settings overridden
// dialog.
TEST_F(ExtensionSettingsOverriddenDialogUnitTest,
SimpleOverrideExtensionDoesntTriggerDialog) {
const extensions::Extension* extension =
AddExtension("alpha", extensions::mojom::ManifestLocation::kInternal,
/*include_extra_perms=*/false);
ExtensionSettingsOverriddenDialog controller(
CreateTestDialogParams(extension->id()), profile());
EXPECT_FALSE(controller.ShouldShow());
// The the extension should not be acknowledged. The latter is important to
// re-assess the extension in case it updates.
EXPECT_FALSE(IsExtensionAcknowledged(extension->id()));
}Lets leave this test while the feature is disabled. We can remove it when the feature rolls out.
// Copyright 2026 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CHROME_BROWSER_UI_EXTENSIONS_FEATURES_H_
#define CHROME_BROWSER_UI_EXTENSIONS_FEATURES_H_
#include "base/feature_list.h"
#include "build/build_config.h"
#include "extensions/buildflags/buildflags.h"
namespace extensions::features {
#if BUILDFLAG(ENABLE_EXTENSIONS)
// When enabled, all search extensions will unconditionally get the search
// engine override dialog.
BASE_DECLARE_FEATURE(kSearchEngineUnconditionalDialog);
#endif // BUILDFLAG(ENABLE_EXTENSIONS)
} // namespace extensions::features
#endif // CHROME_BROWSER_UI_EXTENSIONS_FEATURES_H_nit: We could add it to extensions/common/extension_features.h instead of creating a new file
Related details
- Christopher Grant
- Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou added 8 comments![]( "Open in Gerrit")
Thanks, Daniel. I've kicked off a thread about this offline; please respond there. The linked bug also doesn't appear to have sufficient information.
For now, not lgtm since this is a sensitive area in the code and we should be careful about changes going in.
Emilia PazAcknowledged
Daniel SoromouHas this been approved? If so, would be good to link the doc to the bug
I have updated the description with the doc link: go/chrome-dse-selection
Done
// Historically, "Simple Overrides" were exempt from this dialog. We are
// removing that exemption, but we grandfather in extensions installed
// before the policy change was enabled to prevent spamming existing users.nit: Lets add the bug number here, and add a description to the bug (basically this [section](https://docs.google.com/document/d/1ZeYWHCL-9tELnmBH33UBjoK9uINhpK8aySeipK-PSQ4/edit?tab=t.0#bookmark=id.h16zg5ufedlg) on the design doc)
Done
if (!base::FeatureList::IsEnabled(
extensions::features::kSearchEngineUnconditionalDialog)) {
PrefService* prefs = profile_->GetPrefs();
prefs->ClearPref(kSimpleOverrideBeginConfirmationTimestamp);
return false;
}nit: explain why we do this. My understanding is to handling if the feature is enabled, then disabled, and then re-enabled, the grandfathering timestamp will be reset to the time of re-enabling. Any extensions installed while the feature was disabled will be grandfathered
Done
// If the extension was installed before the enforcement logic began,
// do not show the dialog (grandfathered).
if (install_time < enforcement_time) {
return false;
}
return true;Good point. This can be simplified to:
```suggestion
// If the extension was installed after the enforcement logic began,
// show the dialog.
return install_time >= enforcement_time;
```
Done
// Tests that simple override extensions don't trigger the settings overridden
// dialog.
TEST_F(ExtensionSettingsOverriddenDialogUnitTest,
SimpleOverrideExtensionDoesntTriggerDialog) {
const extensions::Extension* extension =
AddExtension("alpha", extensions::mojom::ManifestLocation::kInternal,
/*include_extra_perms=*/false);
ExtensionSettingsOverriddenDialog controller(
CreateTestDialogParams(extension->id()), profile());
EXPECT_FALSE(controller.ShouldShow());
// The the extension should not be acknowledged. The latter is important to
// re-assess the extension in case it updates.
EXPECT_FALSE(IsExtensionAcknowledged(extension->id()));
}Lets leave this test while the feature is disabled. We can remove it when the feature rolls out.
Done
// Copyright 2026 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CHROME_BROWSER_UI_EXTENSIONS_FEATURES_H_
#define CHROME_BROWSER_UI_EXTENSIONS_FEATURES_H_
#include "base/feature_list.h"
#include "build/build_config.h"
#include "extensions/buildflags/buildflags.h"
namespace extensions::features {
#if BUILDFLAG(ENABLE_EXTENSIONS)
// When enabled, all search extensions will unconditionally get the search
// engine override dialog.
BASE_DECLARE_FEATURE(kSearchEngineUnconditionalDialog);
#endif // BUILDFLAG(ENABLE_EXTENSIONS)
} // namespace extensions::features
#endif // CHROME_BROWSER_UI_EXTENSIONS_FEATURES_H_nit: We could add it to extensions/common/extension_features.h instead of creating a new file
- Christopher Grant
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christopher Grant (Gerrit)
Christopher Grant added 1 comment![]( "Open in Gerrit")
BASE_DECLARE_FEATURE(kSearchEngineUnconditionalDialog);The rollout plan is now to roll out all parts of the extensions work together. Just confirming that we'll use separate flags, but bundle them in Finch and field trials, correct?
Related details
- Daniel Soromou
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou added 1 comment![]( "Open in Gerrit")
BASE_DECLARE_FEATURE(kSearchEngineUnconditionalDialog);The rollout plan is now to roll out all parts of the extensions work together. Just confirming that we'll use separate flags, but bundle them in Finch and field trials, correct?
Yes, I think for the rollout propose we can bundle them together in the same study. I am currently doing something similar with the infobar prioritization.
Related details
- Christopher Grant
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Emilia Paz voted and added 1 comment![]( "Open in Gerrit")
- Christopher Grant
- Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou added 1 comment![]( "Open in Gerrit")
BASE_DECLARE_FEATURE(kSearchEngineUnconditionalDialog);Daniel SoromouThe rollout plan is now to roll out all parts of the extensions work together. Just confirming that we'll use separate flags, but bundle them in Finch and field trials, correct?
Yes, I think for the rollout propose we can bundle them together in the same study. I am currently doing something similar with the infobar prioritization.
- Christopher Grant
- Darryl James
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |