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

Make user change search engine provider in tor window #635

Merged
merged 1 commit into from
Oct 15, 2018

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Oct 15, 2018

Initially, DDG is set to search engine provider.
When user changes in settings, that change will be persisted.

Fixes brave/brave-browser#1513

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  1. Launch browser with clean profile
  2. Open tor window
  3. Check search provider is DDG.
  4. Change search provider to Qwant
  5. Re-launch browser
  6. Open tor window
  7. Check search provider is Qwant

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

Initially, DDG is set to search engine provider.
When user changes in settings, that change will be persisted.
@simonhong simonhong self-assigned this Oct 15, 2018
if (profile->GetProfileType() == Profile::INCOGNITO_PROFILE) {
new PrivateWindowSearchEngineProviderController(profile);
return;
}

if (profile->IsTorProfile() &&
profile->GetProfileType() == Profile::GUEST_PROFILE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the significance of checking for GUEST_PROFILE here? Is checking for IsTorProfile not sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

yep, this is redundant unless you want to remind people tor profile is also a gust profile but that can be done in a comment. So it is better to remove this check.
IsTorProfile traits is established upon profile creation.

Copy link
Member Author

@simonhong simonhong Oct 15, 2018

Choose a reason for hiding this comment

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

This is mandatory checking if we want to use off the record profile of tor window.

Copy link
Member

Choose a reason for hiding this comment

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

If it is tor profile, it is already a off the record profile just like guest profile. And for now this is the only way we use tor profile.

Copy link
Member Author

@simonhong simonhong Oct 15, 2018

Choose a reason for hiding this comment

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

When I open tor window, I can see two profiles are created.
Both are IsTorProfile() is true but with different profile type.(one is REGULAR and the other is GUEST). I think GUEST (off the record) is created from regular one.
That causes this runs twice and I need to use guest one.
I'm using NOTIFICATION_PROFILE_CREATED to get here in ProfileCreationMonitor.

@mkarolin
Copy link
Collaborator

Tested on Windows and was able to change the search engine in Tor window's Settings to different engines. Consequent searches use the selected engine. The engine selection is preserved after the browser is restarted. The search engine selection setting in the Tor window is independent from the regular profile search engine selection and the guest window search engine selection.

@yrliou yrliou requested a review from darkdh October 15, 2018 16:41
@simonhong simonhong changed the title Make user change search engine provider change in tor window Make user change search engine provider in tor window Oct 15, 2018
@bbondy bbondy merged commit f1b96db into master Oct 15, 2018
bbondy added a commit that referenced this pull request Oct 15, 2018
Make user change search engine provider in tor window
bbondy added a commit that referenced this pull request Oct 15, 2018
Make user change search engine provider in tor window
@bbondy
Copy link
Member

bbondy commented Oct 15, 2018

master: f1b96db
0.56.x: 7c50373
0.55.x: 8285d00

@simonhong simonhong deleted the optional_search_provider_in_tor branch October 15, 2018 22:03
@bbondy bbondy added this to the 0.55.x - Release milestone Jan 14, 2019
@bbondy bbondy removed the 0.55.x label Jan 14, 2019
fmarier pushed a commit that referenced this pull request Oct 29, 2019
Modify build settings to allow sccache support on Windows
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.

allowing users to change default search engine for Tor Window
4 participants