-
Notifications
You must be signed in to change notification settings - Fork 965
Remove DDG search toggle from private tabs #10413
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
Conversation
d9aa639
to
eae0835
Compare
There was a problem hiding this 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)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, @simonhong - fixed.
eae0835
to
4902fad
Compare
@petemill Fixed the storybook, sorry about that! |
Does your pull request take into consideration the following issue with Asking because you're putting work into the |
8ff3019
to
d00216c
Compare
There was a problem hiding this 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, fixed.
d00216c
to
715a69a
Compare
There was a problem hiding this 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.
…ivate-tabs Remove DDG search toggle from private tabs
Resolves brave/brave-browser#18486
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
brave.use_alternate_private_search_engine
preference tofalse
brave.use_alternate_private_search_engine
preference totrue