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

Hide DDG newtab of tor window in qwant region(ger/fr) and qwant is set #657

Merged
merged 2 commits into from
Oct 17, 2018
Merged

Hide DDG newtab of tor window in qwant region(ger/fr) and qwant is set #657

merged 2 commits into from
Oct 17, 2018

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Oct 17, 2018 •
edited by bsclifton
Loading

Hide DDG newtab of tor window in qwant region(ger/fr) if qwant is set to search provider.
Otherwise, DDG newetab is used in tor window newtab.

Close brave/brave-browser#1632

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. Set region to france
  2. Launch new browser instance with clean profile
  3. Open tor window
  4. Check search provider is qwant and qwant newtab is used
  5. Set region to non fr/ger
  6. Launch new browser instance with clean profile
  7. Open tor window
  8. Check search provider is ddg and ddg newtab is used

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

@simonhong
Show Qwant newtab in tor window when qwant is used in german/france
38e2720 
Otherwise, DDG newetab is used in tor window newtab.
@simonhong simonhong self-assigned this Oct 17, 2018
@simonhong simonhong changed the title WIP: Show Qwant newtab in tor window when qwant is used in german/france WIP: Show Qwant newtab in tor window in qwant region(ger/fr) Oct 17, 2018
@simonhong simonhong changed the title WIP: Show Qwant newtab in tor window in qwant region(ger/fr) WIP: Show Qwant newtab of tor window in qwant region(ger/fr) Oct 17, 2018
@simonhong
Copy link
Member Author

simonhong commented Oct 17, 2018 •
edited
Loading

Marked as WIP because after changing search provider and then reloading newtab doesn't reflect new search provider. But, when relaunch new search provider configuration is reflected in tor newtab.

After preference is changed, valid isQwant is passed but newtab page doesn't refreshed with new value. I think this is different issue with this.
I'll remove WIP and open separate issue for this.

@simonhong simonhong changed the title WIP: Show Qwant newtab of tor window in qwant region(ger/fr) WIP: Hide DDG newtab of tor window in qwant region(ger/fr) Oct 17, 2018
@simonhong simonhong changed the title WIP: Hide DDG newtab of tor window in qwant region(ger/fr) Hide DDG newtab of tor window in qwant region(ger/fr) Oct 17, 2018
@simonhong
Copy link
Member Author

I think this is ready to review!

@simonhong simonhong changed the title Hide DDG newtab of tor window in qwant region(ger/fr) Hide DDG newtab of tor window in qwant region(ger/fr) and qwant is set Oct 17, 2018
mkarolin
mkarolin previously approved these changes Oct 17, 2018
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

lgtm

bsclifton
bsclifton reviewed Oct 17, 2018
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

This works great for Tor window, but private window is still showing DDG

bsclifton
bsclifton reviewed Oct 17, 2018
View reviewed changes
browser/ui/webui/brave_new_tab_ui.cc Outdated
TemplateURLPrepopulateData::PREPOPULATED_ENGINE_ID_QWANT;
bool qwant_used =
profile->GetPrefs()->GetInteger(kAlternativeSearchEngineProviderInTor) ==
TemplateURLPrepopulateData::PREPOPULATED_ENGINE_ID_QWANT;
Copy link
Member

Choose a reason for hiding this comment

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

qwant_used only returns true for Tor tabs with this logic; this should return true also for incognito windows

@bsclifton
Update logic so that IsQwantUsedInQwantRegion also returns true in …
00185b3 
…an Incognito window (already worked as expected in a Tor window)

Auditors: @simonhong
@bsclifton bsclifton mentioned this pull request Oct 17, 2018
Closed
bsclifton
bsclifton approved these changes Oct 17, 2018
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Had to make a modification, but this works great 😄 There is one open issue, which I captured in brave/brave-browser#1666. We can fix this at a later time (this PR is much better than the prior JavaScript check that was done). All tests pass 🎉

mkarolin
mkarolin approved these changes Oct 17, 2018
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

👍

@bsclifton bsclifton merged commit 0bd21c1 into master Oct 17, 2018
@bsclifton bsclifton deleted the qwant_in_tor_window_new_tab branch October 17, 2018 19:07
bsclifton added a commit that referenced this pull request Oct 17, 2018
@bsclifton
Merge pull request #657 from brave/qwant_in_tor_window_new_tab
2e582a9 
Hide DDG newtab of tor window in qwant region(ger/fr) and qwant is set
bsclifton added a commit that referenced this pull request Oct 17, 2018
@bsclifton
Merge pull request #657 from brave/qwant_in_tor_window_new_tab
1b6d353 
Hide DDG newtab of tor window in qwant region(ger/fr) and qwant is set
bsclifton added a commit that referenced this pull request Oct 17, 2018
@bsclifton
Merge pull request #657 from brave/qwant_in_tor_window_new_tab
76d2c1d 
Hide DDG newtab of tor window in qwant region(ger/fr) and qwant is set
@bsclifton
Copy link
Member

master 0bd21c1
0.57.x 2e582a9
0.56.x 1b6d353
0.55.x 76d2c1d

@bbondy bbondy removed the 0.55.x label Jan 14, 2019
@bbondy bbondy added this to the 0.55.x - Release milestone Jan 14, 2019
@btlechowski btlechowski mentioned this pull request Jun 10, 2020
Merged
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkarolin mkarolin mkarolin approved these changes

@bsclifton bsclifton bsclifton approved these changes

@cezaraugusto cezaraugusto Awaiting requested review from cezaraugusto

Assignees

@simonhong simonhong

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

Successfully merging this pull request may close these issues.

Hide DDG option from Private/Tor new tab if default search is overridden
4 participants
@simonhong @bsclifton @mkarolin @bbondy