[SEH] Show confirmation for extension-controlled default search [chromium/src : main]
Justin Donnelly (Gerrit)
Justin Donnelly added 9 comments![]( "Open in Gerrit")
default_search_extension_controlled_controller_ =This should be reset() in `TearDownPreBrowserWindowDestruction()`, I think?
#include "components/tabs/public/tab_interface.h"Did you mean to add this? I might have missed something, but I don't see any of the symbols defined there used here.
bool ChromeOmniboxClient::IsDefaultSearchIsExtensionControlled(The name `IsDefaultSearchIsExtensionControlled` shouldn't be used for a method that has potential side effects. I suggest `ShowConfirmationDialogIfDefaultSearchExtensionControlled`.
CHECK(browser_);This should probably be `if (!browser_) return false;`
CHECK(web_contents);Same here, early exit rather than CHECK.
? trueRemove `? true : false`, this is redundant.
base::TimeTicks match_selection_timestamp,Same here, please restore the default value.
GURL alternate_nav_url,Unless I'm missing something, you shouldn't change this.
bool check_dse_extension) {This parameter feels clunky and possibly unecessary. Couldn't we just unconditionally call `IsDefaultSearchIsExtensionControlled` and then rely on it to noop if the user has acknowledged the dialog already?
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. |
Justin Donnelly (Gerrit)
Justin Donnelly added 1 comment![]( "Open in Gerrit")
- 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. |
Emilia Paz (Gerrit)
Emilia Paz voted and added 3 comments![]( "Open in Gerrit")
Votes added by Emilia Paz
| Code-Review | +1 |
3 comments
Thanks Daniel! Extensions lgtm for adding a handle result callback. Didn't look at the other files
see: go/chrome-dse-selection.prefer public links if document should be public
// Returns true if the extension with the given |id| has already beennit: while we are here we can update this to use backticks
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 10 comments![]( "Open in Gerrit")
see: go/chrome-dse-selection.prefer public links if document should be public
Done
This should be reset() in `TearDownPreBrowserWindowDestruction()`, I think?
Done
// Returns true if the extension with the given |id| has already beennit: while we are here we can update this to use backticks
Done
Did you mean to add this? I might have missed something, but I don't see any of the symbols defined there used here.
No, The controller was initially a tab feature, but I moved it to a browser windows feature. Removed.
bool ChromeOmniboxClient::IsDefaultSearchIsExtensionControlled(The name `IsDefaultSearchIsExtensionControlled` shouldn't be used for a method that has potential side effects. I suggest `ShowConfirmationDialogIfDefaultSearchExtensionControlled`.
Done
CHECK(browser_);This should probably be `if (!browser_) return false;`
When `ShowConfirmationDialogIfDefaultSearchExtensionControlled` is called, I would expect `browser_` should not be null. That is why I use `CHECK(browser_);` since it's an invariant. Is there anything that I am missing?
Remove `? true : false`, this is redundant.
Done
Same here, please restore the default value.
Done
Unless I'm missing something, you shouldn't change this.
Done
This parameter feels clunky and possibly unecessary. Couldn't we just unconditionally call `IsDefaultSearchIsExtensionControlled` and then rely on it to noop if the user has acknowledged the dialog already?
- Emilia Paz
- Justin Donnelly
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")
// // Copyright 2026 The Chromium AuthorsWIP
Related details
- Emilia Paz
- Justin Donnelly
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. |
Justin Donnelly (Gerrit)
Justin Donnelly added 8 comments![]( "Open in Gerrit")
see: go/chrome-dse-selection.Daniel Soromouprefer public links if document should be public
Done
Public links are preferred but an internal go/ link is better than no link at all.
In this case, I don't think we can make this content public at this time, so please restore the go/ link. At least that way Googlers can find it and external contributors can ask a Googler they collaborate with about it if they're interested.
#endif // BUILDFLAG(ENABLE_EXTENSIONS) && (BUILDFLAG(IS_WIN) ||Super nit-picky nit: Since this is so close to the `#if` directive, I think this would be more readable without the comment. Similar to lines 848 and 851, above.
#endif // BUILDFLAG(ENABLE_EXTENSIONS) && (BUILDFLAG(IS_WIN) ||Nit: Same here, remove comment to match the style of the blocks immediately above.
CHECK(browser_);Daniel SoromouThis should probably be `if (!browser_) return false;`
When `ShowConfirmationDialogIfDefaultSearchExtensionControlled` is called, I would expect `browser_` should not be null. That is why I use `CHECK(browser_);` since it's an invariant. Is there anything that I am missing?
You're correct that we should expect it to be non-null here. Unfortunately, there are often cases where the browser object isn't available in tests. I think that's a risk here, since `OnAutocompleteAccept` has an `if (browser_)` block.
CHECK(web_contents);Same here, early exit rather than CHECK.
For this one, note the multiple instances of `if (web_contents)` in this file after using `location_bar_->GetWebContents()`.
if (!proceed) {Nit: Reverse the if and else branches here so the shorter, more straightforward path comes first (and matches the order of the description above).
controller_->client()->FocusWebContents();Do you know why this is necessary? I would have assumed that simply calling `OpenMatch`, which you always do above, would result in focus being handled appropriately. I'd prefer to not take this extra step unless necessary and we understand why it's necessary.
// // Copyright 2026 The Chromium AuthorsWIP
- 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. |
Emilia Paz (Gerrit)
Emilia Paz added 6 comments![]( "Open in Gerrit")
extensions slgtm -- test failure seems to be when we are creating the controller
see: go/chrome-dse-selection.Daniel Soromouprefer public links if document should be public
Justin DonnellyDone
Public links are preferred but an internal go/ link is better than no link at all.
In this case, I don't think we can make this content public at this time, so please restore the go/ link. At least that way Googlers can find it and external contributors can ask a Googler they collaborate with about it if they're interested.
+1, sorry should have been more clear with my comment
default_search_extension_controlled_controller_ =
GetUserDataFactory()
.CreateInstance<DefaultSearchExtensionControlledController>(
*browser_, *browser_view, *browser_->GetProfile());
should this be only created if feature is enabled?
This may be the reason test is crashing
#endif // BUILDFLAG(ENABLE_EXTENSIONS) && (BUILDFLAG(IS_WIN) ||Super nit-picky nit: Since this is so close to the `#if` directive, I think this would be more readable without the comment. Similar to lines 848 and 851, above.
Should this be only if enabled for the feature? This may be what's causing the test failure
static bool HasShownFor(Profile* profile, const extensions::ExtensionId& id);
// Returns true if the extension with the given `id` has already been
// acknowledged.
static bool HasAcknowledgedExtension(
Profile* profile,
const extensions::ExtensionId& id,
const std::string& extension_acknowledged_preference_name);nit: move this below the other static method (line 72)
// Returns true if the extension with the given `id` has already been shown.optional-nit: been shown where? Maybe rephrase to `has already had a settings overriden dialog shown for.`
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 12 comments![]( "Open in Gerrit")
see: go/chrome-dse-selection.Daniel Soromouprefer public links if document should be public
Justin DonnellyDone
Emilia PazPublic links are preferred but an internal go/ link is better than no link at all.
In this case, I don't think we can make this content public at this time, so please restore the go/ link. At least that way Googlers can find it and external contributors can ask a Googler they collaborate with about it if they're interested.
+1, sorry should have been more clear with my comment
For now, we do not want to make this doc public. I have added the private link
default_search_extension_controlled_controller_ =
GetUserDataFactory()
.CreateInstance<DefaultSearchExtensionControlledController>(
*browser_, *browser_view, *browser_->GetProfile());
should this be only created if feature is enabled?
This may be the reason test is crashing
Done
#endif // BUILDFLAG(ENABLE_EXTENSIONS) && (BUILDFLAG(IS_WIN) ||Emilia PazSuper nit-picky nit: Since this is so close to the `#if` directive, I think this would be more readable without the comment. Similar to lines 848 and 851, above.
Should this be only if enabled for the feature? This may be what's causing the test failure
Done
#endif // BUILDFLAG(ENABLE_EXTENSIONS) && (BUILDFLAG(IS_WIN) ||Nit: Same here, remove comment to match the style of the blocks immediately above.
Done
static bool HasShownFor(Profile* profile, const extensions::ExtensionId& id);
// Returns true if the extension with the given `id` has already been
// acknowledged.
static bool HasAcknowledgedExtension(
Profile* profile,
const extensions::ExtensionId& id,
const std::string& extension_acknowledged_preference_name);nit: move this below the other static method (line 72)
Done
// Returns true if the extension with the given `id` has already been shown.optional-nit: been shown where? Maybe rephrase to `has already had a settings overriden dialog shown for.`
Done
CHECK(browser_);Daniel SoromouThis should probably be `if (!browser_) return false;`
Justin DonnellyWhen `ShowConfirmationDialogIfDefaultSearchExtensionControlled` is called, I would expect `browser_` should not be null. That is why I use `CHECK(browser_);` since it's an invariant. Is there anything that I am missing?
You're correct that we should expect it to be non-null here. Unfortunately, there are often cases where the browser object isn't available in tests. I think that's a risk here, since `OnAutocompleteAccept` has an `if (browser_)` block.
Ok perfect. I have added a early return for now, I will revisit it in a follow up change.
CHECK(web_contents);Justin DonnellySame here, early exit rather than CHECK.
For this one, note the multiple instances of `if (web_contents)` in this file after using `location_bar_->GetWebContents()`.
Ok perfect. I have added a early return for now, I will revisit it in a follow up change.
Nit: Reverse the if and else branches here so the shorter, more straightforward path comes first (and matches the order of the description above).
Done
Do you know why this is necessary? I would have assumed that simply calling `OpenMatch`, which you always do above, would result in focus being handled appropriately. I'd prefer to not take this extra step unless necessary and we understand why it's necessary.
We need it because `ClassifyString` focus the omnibox. I move it below `ClassifyString` method.
// // Copyright 2026 The Chromium AuthorsJustin DonnellyWIP
Ping me when this is done and I'll take another look.
Done
// Copyright 2026 The Chromium AuthorsDaniel SoromouWIP
Done
Related details
- Emilia Paz
- Justin Donnelly
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Emilia Paz
| Code-Review | +1 |
- Daniel Soromou
- Justin Donnelly
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Donnelly (Gerrit)
Justin Donnelly voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Donnelly
| Code-Review | +1 |
1 comment
#include "chrome/browser/ui/search_engines/default_search_extension_controlled_controller.h"This should probably be guarded with a platform check, I think?
`#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)`
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 1 comment![]( "Open in Gerrit")
#include "chrome/browser/ui/search_engines/default_search_extension_controlled_controller.h"This should probably be guarded with a platform check, I think?
`#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)`
- Darryl James
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
1 comment
Related details
- Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan voted Code-Review+1![]( "Open in Gerrit")
- Daniel Soromou
- Darryl James
- Emilia Paz
- Justin Donnelly
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou voted Commit-Queue+2![]( "Open in Gerrit")
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[SEH] Show confirmation for extension-controlled default search
This change introduces `DefaultSearchExtensionControlledController` to
manage the explicit choice dialog for extension-controlled default
search engines (DSE).
When a user initiates a search from the Omnibox using an extension-
controlled DSE, this controller determines if confirmation is required
based on whether the extension has been previously acknowledged or is
force-installed.
BYPASS_LARGE_CHANGE_WARNING: The controller itself is long because of several simple conditional statement.
- M chrome/browser/ui/browser_window/internal/BUILD.gn
- M chrome/browser/ui/browser_window/internal/browser_window_features.cc
- M chrome/browser/ui/browser_window/public/browser_window_features.h
- M chrome/browser/ui/extensions/extension_settings_overridden_dialog.cc
- M chrome/browser/ui/extensions/extension_settings_overridden_dialog.h
- M chrome/browser/ui/extensions/settings_overridden_dialog_interactive_uitest.cc
- M chrome/browser/ui/omnibox/BUILD.gn
- M chrome/browser/ui/omnibox/chrome_omnibox_client.cc
- M chrome/browser/ui/omnibox/chrome_omnibox_client.h
- M chrome/browser/ui/omnibox/omnibox_edit_model.cc
- M chrome/browser/ui/omnibox/omnibox_edit_model.h
- M chrome/browser/ui/search_engines/BUILD.gn
- A chrome/browser/ui/search_engines/default_search_extension_controlled_controller.cc
- A chrome/browser/ui/search_engines/default_search_extension_controlled_controller.h
- A chrome/browser/ui/search_engines/default_search_extension_controlled_controller_unittest.cc
- M chrome/test/base/ui_test_utils.cc
- M chrome/test/base/ui_test_utils.h
- M components/omnibox/browser/omnibox_client.cc
- M components/omnibox/browser/omnibox_client.h
Code-Review: +1 by Muhammad Salmaan
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |