Omnibox Command Palette #16947

fallaciousreasoning · 2023-02-01T02:18:44Z

Adds a prototype command palette in the Omnibox. Behind the Chromium brave://flags/#quick-commands flag. Note: This flag is set to expire in M111 so you might need to also enable the brave://flags/#temporary-unexpire-flags-m110 flag if you're running a recent version of Brave.

The crbug issue for this feature is here: https://bugs.chromium.org/p/chromium/issues/detail?id=1014639

commander-demo.webm

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 wiki
- npm run lint, npm run presubmit wiki, 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:

Enable the brave://flags#quick-commands flag (if not present enable the Temporarily Enable M110 flags)
Press Ctrl+Space
The Omnibox should focus, and a list of available commands should be visible
Type Create tab and select the Create new tab command. A new tab should open
Press Ctrl+Space
Type Move window and select Move tab to new window. The tab should move to a new window
Navigate to https://readr.nz
Press Ctrl+Space
Type Move window and select Move tabs to window...
Select the New tab (or whatever your original window was called).
The readr.nz tab should move back to the original window.

browser/ui/commander/commander_throttle.cc

sangwoo108

@fallaciousreasoning , this is a awesome work! I've been craving for a feature like this on omnibox. I'm still loading the context of codes here so might be wrong about some comments.

chromium_src/chrome/browser/ui/commander/fuzzy_finder.cc

chromium_src/chrome/browser/ui/views/commander_frontend_views.cc

chromium_src/chrome/browser/ui/commander/commander_frontend.h

browser/ui/commander/command_centre.h

components/commander/common/commander_url.cc

components/commander/common/constants.h

components/commander/common/commander_url.cc

components/commander/common/commander_model.h

components/omnibox/browser/brave_omnibox_edit_model.cc

fallaciousreasoning · 2023-02-03T02:27:11Z

Sorry ssangwoo, I've only got to about half your feedback so far!

components/omnibox/browser/brave_omnibox_edit_model.cc

components/commander/common/commander_url.cc

sangwoo108

Sorry ssangwoo, I've only got to about half your feedback so far!

No worries, I know I commented a lot 😭 I hope my new comments doesn't feel overwhelming.

browser/ui/commander/command_centre.cc

components/commander/common/commander_model.cc

components/commander/common/commander_url.cc

components/commander/common/commander_url.h

components/omnibox/browser/commander_provider.cc

fallaciousreasoning · 2023-02-06T23:57:39Z

Nah, thanks heaps for all your feedback @sangwoo108! You're my primary source of C++ tips & tricks 😆

browser/ui/commander/commander_service.cc

goodov · 2023-03-27T14:29:08Z

browser/ui/commander/commander_service.cc

+  if (!browser) {
+    return nullptr;
+  }
+  return browser->window()->GetLocationBar()->GetOmniboxView();


does window() always valid on MacOS?

The only comment I see on window is this one:

// |window()| will return NULL if called before |CreateBrowserWindow()| // is done.

but that doesn't mean it can't be. Maybe I'll add a CHECK?

browser/ui/commander/commander_service.cc

browser/ui/commander/commander_service_factory.cc

patches/chrome-browser-ui-commander-fuzzy_finder.cc.patch

goodov · 2023-03-30T06:41:33Z

chromium_src/components/omnibox/browser/autocomplete_controller.cc

+                               AutocompleteController* controller) {
+#if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS)
+  if (commander::CommanderEnabled()) {
+    providers.push_back(new commander::CommanderProvider(


std::make_unique

I've changed this to base::MakeRefCounted but all the other providers are added via new ..... Should they be updated (in a future PR)? In the Chromium file they're all added/created via new too, so I almost feel this should be left as is?

Yeah, usually we should prefer make_foo over bare "new" if possible.

I almost forgot why, but this post reminded me why 😄

https://herbsutter.com/2013/05/29/gotw-89-solution-smart-pointers/

make_unique is more robust to exceptions

Potential leak, which can be caused by expression order, can be prevented

components/commander/common/commander_frontend_delegate.h

bridiver · 2023-07-31T22:55:44Z

components/omnibox/browser/brave_omnibox_edit_model.cc

+
+#include "base/strings/string_util.h"
+
+#if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS)


please replace all of these !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS) guards with a dedicated buildflag for this feature

bridiver · 2023-07-31T22:57:15Z

components/omnibox/browser/sources.gni

@@ -36,3 +38,15 @@ brave_components_omnibox_browser_deps = [
  "//components/prefs",
  "//url",
 ]
+
+if (!is_android && !is_ios) {


Same as above, please add a dedicated buildflag

bridiver · 2023-07-31T22:58:46Z

components/commander/browser/BUILD.gn

+# License, v. 2.0. If a copy of the MPL was not distributed with this file,
+# You can obtain one at https://mozilla.org/MPL/2.0/.
+
+assert(!is_android && !is_ios)


replace with dedicated buildflag. In general os guards should only be used directly when you are guarding os-specific code. In other cases you should use them indirectly to set the dedicated buildflag value

bridiver · 2023-07-31T23:01:06Z

components/commander/common/features.cc

+
+namespace commander {
+
+bool CommanderEnabled() {


IsCommanderEnabled, but this pattern is generally only used when there are more conditions than just a single buildflag so I would probably remove this

bridiver · 2023-07-31T23:05:21Z

components/commander/common/constants.cc

+
+#include "brave/components/commander/common/constants.h"
+
+#include "base/strings/string_piece.h"


this is unused

bridiver · 2023-07-31T23:08:23Z

components/omnibox/browser/commander_provider.cc

+#include <string>
+#include <utility>
+
+#include "base/functional/bind.h"


a lot of these includes don't appear to be used

bridiver · 2023-07-31T23:10:00Z

components/omnibox/browser/commander_provider.h

+#include "base/memory/weak_ptr.h"
+#include "base/scoped_observation.h"
+#include "brave/components/commander/browser/commander_frontend_delegate.h"
+#include "brave/components/commander/browser/commander_item_model.h"


Is this include used here? Also it seems like many of these could be forward declarations

github-actions bot assigned fallaciousreasoning Feb 1, 2023

github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Feb 1, 2023

fallaciousreasoning force-pushed the commands-search branch 2 times, most recently from f802607 to e81a566 Compare February 1, 2023 03:12

fallaciousreasoning marked this pull request as ready for review February 1, 2023 03:48

fallaciousreasoning requested review from a team as code owners February 1, 2023 03:48

fallaciousreasoning requested review from simonhong, sangwoo108 and petemill February 1, 2023 22:16

diracdeltas reviewed Feb 1, 2023

View reviewed changes

browser/ui/commander/commander_throttle.cc Outdated Show resolved Hide resolved

fallaciousreasoning force-pushed the commands-search branch from df482e4 to a3605a3 Compare February 2, 2023 02:10

sangwoo108 reviewed Feb 2, 2023

View reviewed changes

diracdeltas reviewed Feb 3, 2023

View reviewed changes

components/omnibox/browser/brave_omnibox_edit_model.cc Outdated Show resolved Hide resolved

diracdeltas reviewed Feb 3, 2023

View reviewed changes

components/commander/common/commander_url.cc Outdated Show resolved Hide resolved

fallaciousreasoning requested a review from sangwoo108 February 6, 2023 00:42

sangwoo108 reviewed Feb 6, 2023

View reviewed changes

fallaciousreasoning force-pushed the commands-search branch from 84cba7f to 27903c4 Compare February 15, 2023 01:41

github-actions bot reviewed Feb 15, 2023

View reviewed changes

browser/ui/commander/commander_service.cc Show resolved Hide resolved

github-actions bot added the needs-security-review label Feb 15, 2023

github-actions bot assigned fmarier and thypon Feb 15, 2023

thypon removed the needs-security-review label Feb 15, 2023

fmarier removed their assignment Feb 15, 2023

fallaciousreasoning force-pushed the commands-search branch from 27903c4 to b033c30 Compare February 16, 2023 23:22

fallaciousreasoning force-pushed the commands-search branch 2 times, most recently from 7ec1c06 to c5c5d25 Compare March 6, 2023 02:37

fallaciousreasoning force-pushed the commands-search branch from 34e6702 to 29a96ae Compare March 15, 2023 23:17

fallaciousreasoning force-pushed the commands-search branch from 29a96ae to e1dcee6 Compare March 26, 2023 23:59

goodov reviewed Mar 27, 2023

View reviewed changes

fallaciousreasoning force-pushed the commands-search branch from e1dcee6 to 5ff2672 Compare March 27, 2023 21:03

fallaciousreasoning requested a review from goodov March 27, 2023 21:03

goodov reviewed Mar 28, 2023

View reviewed changes

patches/chrome-browser-ui-commander-fuzzy_finder.cc.patch Show resolved Hide resolved

goodov approved these changes Mar 30, 2023

View reviewed changes

fallaciousreasoning added 9 commits March 31, 2023 08:47

Add Commander to Omnibox

7646dbf

Changes for PR

82dbc58

Invalidate WeakPtrs

4f4df3d

Use WeakPtrs properly

33bb3e8

CHECK window

d68e990

Use intermediate variable

bdbce4b

Use FromBrowserContext instead of static_cast

d3900fa

Fix unittest

fb82a1c

Fix bug where matches remained on backspace

549000b

fallaciousreasoning force-pushed the commands-search branch 2 times, most recently from e43bc83 to b03c25a Compare March 30, 2023 20:09

Move commander interface to commander/browser

3619b8c

fallaciousreasoning force-pushed the commands-search branch from b03c25a to 3619b8c Compare March 30, 2023 21:10

fallaciousreasoning merged commit 3c22732 into master Mar 31, 2023

fallaciousreasoning deleted the commands-search branch March 31, 2023 01:07

github-actions bot added this to the 1.52.x - Nightly milestone Mar 31, 2023

bridiver reviewed Jul 31, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omnibox Command Palette #16947

Omnibox Command Palette #16947

fallaciousreasoning commented Feb 1, 2023 •

edited

Loading

sangwoo108 left a comment •

edited

Loading

fallaciousreasoning commented Feb 3, 2023

sangwoo108 left a comment

fallaciousreasoning commented Feb 6, 2023

goodov Mar 27, 2023

fallaciousreasoning Mar 27, 2023 •

edited

Loading

goodov Mar 30, 2023

fallaciousreasoning Mar 30, 2023

sangwoo108 Mar 30, 2023

sangwoo108 Mar 30, 2023

bridiver Jul 31, 2023

bridiver Jul 31, 2023

bridiver Jul 31, 2023

bridiver Jul 31, 2023

bridiver Jul 31, 2023

bridiver Jul 31, 2023

bridiver Jul 31, 2023


		#include "base/strings/string_util.h"

		#if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS)


		#include "brave/components/commander/common/constants.h"

		#include "base/strings/string_piece.h"


		namespace commander {

		bool CommanderEnabled() {

Omnibox Command Palette #16947

Omnibox Command Palette #16947

Conversation

fallaciousreasoning commented Feb 1, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

sangwoo108 left a comment • edited Loading

Choose a reason for hiding this comment

fallaciousreasoning commented Feb 3, 2023

sangwoo108 left a comment

Choose a reason for hiding this comment

fallaciousreasoning commented Feb 6, 2023

Choose a reason for hiding this comment

fallaciousreasoning Mar 27, 2023 • edited Loading

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

Choose a reason for hiding this comment

fallaciousreasoning commented Feb 1, 2023 •

edited

Loading

sangwoo108 left a comment •

edited

Loading

fallaciousreasoning Mar 27, 2023 •

edited

Loading