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

Custom client for ddg. #1479

Merged
merged 2 commits into from Oct 24, 2019
Merged

Custom client for ddg. #1479

merged 2 commits into from Oct 24, 2019

Conversation

@iccub
Copy link
Contributor

iccub commented Sep 3, 2019

Ref: internal#648

Summary of Changes

This pull request fixes issue #1478

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

  • Verify iOS device is in the United States region

  • Launch Brave and use DDG as the default search engine (can go into private mode to make this easier)

  • Perform a search

  • Verify the URL contains t=brave in the URL bar (may need to long-press "copy" and paste somewhere else to see full URL)

  • Change iOS device region to New Zealand

  • Back in Brave perform another DDG search

  • verify URL contains t=braveed (noticed additional ed)

  • That's it. Cheers 🌮

Screenshots:

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue is assigned to a milestone (should happen at merge time).
@iccub iccub requested a review from jhreis Sep 3, 2019
@iccub iccub force-pushed the internal/648 branch from 28afeb6 to ff3652f Sep 4, 2019
@jhreis jhreis added the blocked label Sep 5, 2019
@jhreis
Copy link
Contributor

jhreis commented Sep 5, 2019

I think some of the behaviors here aren't quite right, but that is mostly due to product requirements not being clear. I'm blocking this for now, and bumping the ticket out of the current milestone.

@jhreis
Copy link
Contributor

jhreis commented Sep 5, 2019

Will leave more formal review once we identify the exact, expected behavior.

@iccub iccub force-pushed the internal/648 branch from ff3652f to 7f3dbfb Sep 9, 2019
@iccub
Copy link
Contributor Author

iccub commented Sep 26, 2019

This should be ready for review @jhreis

@iccub iccub added needs security review and removed blocked labels Sep 26, 2019
@@ -273,6 +272,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UIViewControllerRestorati
}

AdblockResourceDownloader.shared.startLoading()
Preferences.General.isFirstLaunch.value = false

This comment has been minimized.

Copy link
@jhreis

jhreis Sep 27, 2019

Contributor

I don't think we should move this, if we ran into some weird issue, it could end up duplicating all default favorites. I think that is the only real thing that uses the isFirstLaunch bool.

@iccub iccub force-pushed the internal/648 branch 2 times, most recently from 8bbc364 to fa4f693 Sep 30, 2019
iccub added 2 commits Sep 3, 2019
Ref: internal#648
@iccub iccub force-pushed the internal/648 branch from fa4f693 to 2a81201 Oct 22, 2019
@jhreis
jhreis approved these changes Oct 24, 2019
Copy link
Contributor

jhreis left a comment

Very nice! 😄

@jhreis jhreis merged commit af1a19f into development Oct 24, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jhreis jhreis deleted the internal/648 branch Oct 24, 2019
@jumde
Copy link
Collaborator

jumde commented Oct 24, 2019

lgtm

@iccub iccub mentioned this pull request Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.