Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add options for enable/disable autocomplete in omnibox #4033

Merged
merged 4 commits into from
Nov 29, 2019
Merged

Add options for enable/disable autocomplete in omnibox #4033

merged 4 commits into from
Nov 29, 2019

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Nov 20, 2019 •
edited
Loading

When disabled, OmniboxController::StartAutocomplete() does early
return instead of requesting to AutocompleteController.

This option is disabled by default.

fix brave/brave-browser#843
image

Submitter Checklist:

Test Plan:

yarn test brave_browser_tests --filter=OmniboxAutocompleteTest*

Manual test steps

  1. Launch browser and load settings page
  2. Check disable autocomplete option is turned off by default
  3. Typing in omnibox and autocomplete works
  4. Toggle autocomplete option
  5. Typing in omnibox and autocomplete doesn't work.(popup is not shown and inline autocomplete doesn't work)

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@simonhong simonhong added this to the 1.3.x - Nightly milestone Nov 20, 2019
@simonhong simonhong self-assigned this Nov 20, 2019
@simonhong simonhong force-pushed the disable_autocomplete branch 2 times, most recently from 3bbc8ac to f823555 Compare November 20, 2019 06:49
@simonhong simonhong marked this pull request as ready for review November 20, 2019 06:55
@simonhong
Copy link
Member Author

@rebron Please check the sentence and position of this option.

@simonhong simonhong added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Nov 20, 2019
@simonhong
Copy link
Member Author

Looks like CI was disconnected during the test build for Win. Other platforms are finished.
Re-run only for Win.

@simonhong simonhong removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Nov 21, 2019
@bsclifton bsclifton mentioned this pull request Nov 21, 2019
Merged
32 tasks
@bsclifton
Copy link
Member

Took me a while to find the setting (under Appearance). Is there a better place for this?
Screen Shot 2019-11-21 at 2 40 09 PM

cc: @rebron
(also see #4044 (comment))

@simonhong
Copy link
Member Author

I thought user could see this like hide omnibox popup. So I put it in appearance :)
@rebron needs your opinion!

bsclifton
bsclifton approved these changes Nov 21, 2019
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM! ++ (comment left for @rebron about location of this setting)

Verified it works good locally and CI passed on all platforms

@rebron
Copy link
Collaborator

rebron commented Nov 21, 2019 •
edited
Loading

@simonhong
Let's put this setting underneath Use wide address bar
with setting title Show autocomplete in address bar
default on

cc: @petemill

@simonhong
Copy link
Member Author

@rebron Changed the sentence and position. see the newly captured image in description.

@simonhong
Copy link
Member Author

Friendly ping to owner review @bridiver

bridiver
bridiver reviewed Nov 25, 2019
View reviewed changes
patches/components-omnibox-browser-omnibox_controller.h.patch Outdated
~OmniboxController() override;

// The |current_url| field of input is only set for mobile ports.
+ virtual
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add virtual using a chromium_src override

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I didn't know c++ allowsvirtual keyword to appear both before and after the return type. 👍
Done. Thanks.

bridiver
bridiver reviewed Nov 25, 2019
View reviewed changes
chromium_src/components/omnibox/browser/omnibox_client.h Outdated
#define BRAVE_CHROMIUM_SRC_COMPONENTS_OMNIBOX_BROWSER_OMNIBOX_CLIENT_H_

#define BRAVE_OMNIBOX_CLIENT_H \
virtual bool IsAutocompleteEnabled() const;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no point in making this a virtual method. We know the omnibox client is BraveOmniboxClient so just cast it in BraveOmniboxController

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bridiver I think this causes layer violation because BraveOmniboxController is in component layer but BraveOmniboxClient is in chrome layer. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then make an abstract BraveOmniboxClient with IsAutocompleteEnabled in components and have BraveOmniboxClientImpl in the chrome layer

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then you can cast to BraveOmniboxClient without a layering violation

Copy link
Member Author

@simonhong simonhong Nov 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bridiver Thanks for tips!
Updated and this PR doesn't have any new patch files :) PTAL.

bridiver
bridiver requested changes Nov 25, 2019
View reviewed changes
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

simonhong and others added 4 commits November 28, 2019 16:24
@simonhong
Add options for enable/disable autocomplete in omnibox
780d202 
When disabled, OmniboxController::StartAutocomplete() does early
return instead of requesting to AutocompleteController.
@bsclifton @simonhong
Updated constant and pref name (since autocomplete is only in address…
a630cfb 
… bar)

kAutocompleteEnabledInAddressBar => kAutocompleteEnabled
brave.autocomplete_enabled_in_address_bar  => brave.autocomplete_enabled
@simonhong
Use override technique for omnibox_controller.h
07f8c7d
@simonhong
Subclassing OmniboxClient instead of patchig for adding method
4e1b29f 
BraveOmniboxClient has IsAutocompleteEnabled() method.
@simonhong simonhong merged commit 8715169 into master Nov 29, 2019
@simonhong simonhong deleted the disable_autocomplete branch November 29, 2019 23:16
@bsclifton bsclifton mentioned this pull request Dec 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsclifton bsclifton bsclifton approved these changes

@bridiver bridiver bridiver approved these changes

@rebron rebron Awaiting requested review from rebron

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
1.3.x - Release
Development

Successfully merging this pull request may close these issues.

Allow users to disable autocomplete
4 participants
@simonhong @bsclifton @rebron @bridiver