Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #5599: Remove dependency on fetch_httpurlconnection #5716

Merged
merged 3 commits into from
Oct 4, 2019

Conversation

colintheshots
Copy link
Contributor

This PR removes an unused dependency to ensure it doesn't get used. Using httpurlconnection would foil the ability to funnel all traffic through the secure proxy tunnel.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@colintheshots colintheshots changed the title For fenix#5599: Remove dependency on fetch_httpurlconnection For #5599: Remove dependency on fetch_httpurlconnection Oct 1, 2019
@codecov-io
Copy link

codecov-io commented Oct 1, 2019

Codecov Report

Merging #5716 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5716      +/-   ##
============================================
+ Coverage     14.44%   14.45%   +<.01%     
+ Complexity      324      323       -1     
============================================
  Files           256      255       -1     
  Lines         10499    10516      +17     
  Branches       1530     1536       +6     
============================================
+ Hits           1517     1520       +3     
- Misses         8856     8873      +17     
+ Partials        126      123       -3
Impacted Files Coverage Δ Complexity Δ
...la/fenix/components/metrics/GleanMetricsService.kt 5.79% <0%> (-0.16%) 3 <0> (ø)
.../java/org/mozilla/fenix/search/SearchInteractor.kt 90% <0%> (-10%) 9% <0%> (ø)
...deletebrowsingdata/DeleteBrowsingDataController.kt 79.31% <0%> (-8.69%) 0% <0%> (ø)
.../java/org/mozilla/fenix/search/SearchController.kt 87.75% <0%> (-5.27%) 0% <0%> (ø)
...va/org/mozilla/fenix/search/SearchFragmentStore.kt 61.76% <0%> (-1.88%) 1% <0%> (ø)
...rc/main/java/org/mozilla/fenix/FenixApplication.kt 3.7% <0%> (-0.81%) 2% <0%> (-2%)
.../src/main/java/org/mozilla/fenix/utils/Settings.kt 72.25% <0%> (-0.53%) 15% <0%> (ø)
...org/mozilla/fenix/crashes/CrashReporterFragment.kt 0% <0%> (ø) 0% <0%> (ø) ⬇️
...ug/java/org/mozilla/fenix/DebugFenixApplication.kt 0% <0%> (ø) 0% <0%> (ø) ⬇️
...in/java/org/mozilla/fenix/search/SearchFragment.kt 0% <0%> (ø) 0% <0%> (ø) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 841b06b...7122435. Read the comment docs.

Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

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

@colintheshots Unfortunately, there's more :) #3976

Glean is using HttpURLConnectionClient by default, but we should be able to provide an alternative fetch client in its configuration here: https://github.com/mozilla-mobile/fenix/blob/master/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt#L440

@colintheshots colintheshots changed the title For #5599: Remove dependency on fetch_httpurlconnection For #5599: Remove dependency on fetch_httpurlconnection Oct 3, 2019
@colintheshots
Copy link
Contributor Author

@csadilek I think it's ready for re-review. Glean should now be using Gecko instead.

Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

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

Looks good! Needs one small change re: reusing our existing client see:

httpClient = lazy(LazyThreadSafetyMode.NONE) { components.core.client }

I will try to test this manually as well quickly.

Glean.initialize(context,
Configuration(channel = BuildConfig.BUILD_TYPE,
httpClient = ConceptFetchHttpUploader(
lazy { GeckoViewFetchClient(context) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to use our existing client instance here lazy { context.components.core.client } or is there a reason you created a new instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, no I didn't do that deliberately. Sure, I can fix that easily.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK cool, let's land once you get a chance to push up the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just waiting for master to become unbroken and then I'll land it.

Copy link
Contributor Author

@colintheshots colintheshots Oct 4, 2019

Choose a reason for hiding this comment

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

@csadilek It's been pushed now.

@csadilek
Copy link
Contributor

csadilek commented Oct 3, 2019

OK, tested and saw it successfully sending pings with GeckoViewFetch. Excited to finally have a single http client in the stack!

Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

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

👍

@csadilek csadilek changed the title For #5599: Remove dependency on fetch_httpurlconnection For #5599: Remove dependency on fetch_httpurlconnection Oct 4, 2019
@colintheshots colintheshots merged commit b7647a4 into mozilla-mobile:master Oct 4, 2019
@colintheshots colintheshots deleted the for5599 branch October 4, 2019 17:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants