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.

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

Remove DDG search toggle from private tabs #10413

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

emerick
Copy link
Contributor

@emerick emerick commented Oct 7, 2021

Resolves brave/brave-browser#18486

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

  • Clean profile
  • Set brave.use_alternate_private_search_engine preference to false
  • Launch browser
  • Open Private Tab
  • Verify that Private Tab shows new design and no DDG toggle
  • Clean profile
  • Set brave.use_alternate_private_search_engine preference to true
  • Launch browser
  • Open Private Tab
  • Verify that Private Tab shows new design and DDG toggle
  • Verify that toggle works as expected

@emerick emerick self-assigned this Oct 7, 2021
@emerick emerick force-pushed the remove-ddg-search-toggle-from-private-tabs branch from d9aa639 to eae0835 Compare October 7, 2021 20:40
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

Error in Storybook:

[2021-10-07T22:06:16.648Z] ERR! => Failed to build the preview
[2021-10-07T22:06:16.648Z] ERR! /home/ubuntu/workspace/pr-brave-browser-remove-ddg-search-toggle-from-private-tabs-linux/src/brave/components/brave_new_tab_ui/stories/privateNTP/privateWindow.tsx
[2021-10-07T22:06:16.648Z] ERR! [tsl] ERROR in /home/ubuntu/workspace/pr-brave-browser-remove-ddg-search-toggle-from-private-tabs-linux/src/brave/components/brave_new_tab_ui/stories/privateNTP/privateWindow.tsx(64,16)
[2021-10-07T22:06:16.648Z] ERR!       TS2551: Property 'showAlternativePrivateSearchEngine' does not exist on type 'PrivateTab'. Did you mean 'useAlternativePrivateSearchEngine'?

@@ -207,6 +210,9 @@ BraveNewTabMessageHandler* BraveNewTabMessageHandler::Create(

BraveNewTabMessageHandler::BraveNewTabMessageHandler(Profile* profile)
: profile_(profile), weak_ptr_factory_(this) {
if (brave::UseAlternativeSearchEngineProviderEnabled(profile)) {
Copy link
Member

Choose a reason for hiding this comment

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

We will show new toggle button again even if user turned off by using new toggle button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is that we'll only show the alternative search provider toggle button to the user if they've already enabled it in the past; if they never enabled the alternative search engine (which would be most users), we will no longer show the toggle that enables it on this page.

cc: @rebron to double-check my understanding of how this should work...

Copy link
Member

Choose a reason for hiding this comment

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

This will be called whenever new tab is created.
Instead, how about setting this in ctor of PrivateWindowSearchEngineProviderService?
PrivateWindowSearchEngineProviderService is only instantiated for private profile with non-qwant region.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, @simonhong - fixed.

@emerick emerick force-pushed the remove-ddg-search-toggle-from-private-tabs branch from eae0835 to 4902fad Compare October 8, 2021 02:26
@emerick
Copy link
Contributor Author

emerick commented Oct 8, 2021

@petemill Fixed the storybook, sorry about that!

@emerick emerick requested a review from petemill October 8, 2021 02:34
@Tonev
Copy link
Contributor

Tonev commented Oct 8, 2021

@emerick

Does your pull request take into consideration the following issue with DuckDuckGo in private windows?

brave/brave-browser#17556

Asking because you're putting work into the DuckDuckGo search functionality in private windows, but if the aforementioned issue remains unsolved, users who would like to keep using DuckDuckGo will still have to use Show autocomplete in address bar.

@emerick emerick force-pushed the remove-ddg-search-toggle-from-private-tabs branch 3 times, most recently from 8ff3019 to d00216c Compare October 8, 2021 14:26
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++ for c++ with one nit.
I'll past js fils review to @petemill

void ToggleUseAlternativeSearchEngineProvider(Profile* profile);

void SetShowAlternativeSearchEngineProviderToggle(Profile* profile, bool value);
Copy link
Member

Choose a reason for hiding this comment

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

nit: As we don't use false for value, I think we can remove value from parameter list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, fixed.

@emerick emerick force-pushed the remove-ddg-search-toggle-from-private-tabs branch from d00216c to 715a69a Compare October 8, 2021 14:51
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

Nit: it would be good to add a toggle to storybook so that it can see the search variation now that it's hidden by a state Boolean. Can definitely be a follow up.

@emerick emerick merged commit bcc97a5 into master Oct 8, 2021
@emerick emerick deleted the remove-ddg-search-toggle-from-private-tabs branch October 8, 2021 23:47
@emerick emerick added this to the 1.32.x - Nightly milestone Oct 8, 2021
emerick added a commit that referenced this pull request Oct 16, 2021
…ivate-tabs

Remove DDG search toggle from private tabs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refresh Private Window New Tab page
4 participants