-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Removes multiple calls to the WebSettings.getDefaultUserAgent api #20603
Conversation
Generated by 🚫 Danger |
Hey @irfano, @thomashorta 👋 |
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #20603 +/- ##
=======================================
Coverage 40.39% 40.40%
=======================================
Files 1474 1474
Lines 67920 67911 -9
Branches 11231 11230 -1
=======================================
- Hits 27438 27437 -1
+ Misses 38000 37992 -8
Partials 2482 2482 ☔ View full report in Codecov by Sentry. |
Quality Gate passedIssues Measures |
Sentry Issue: JETPACK-ANDROID-P8V |
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.
Nice work! Thank you for implementing this change. I conducted a smoke test on the app, and everything seems to be in order. ✅ It makes sense to move the userAgent to a single place and make it Singleton
in AppConfigModule
. The diff looks safe to me. Feel free to merge the PR if you believe a single approval is sufficient.
Thank you for reviewing @irfano 🙇
I'm not sure if this PR fixes the issue but I feel confident with the introduced changes and I'll proceed with merging. I'll monitor the |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Fixes #12259
Description
This PR replaces multiple calls to the
WebSettings.getDefaultUserAgent
(which is not recommended) with just one, hoping to prevent the ANR of #12259 and probably reduce the crash count in #20147The current implementation is making 27 calls to the
WebSettings.getDefaultUserAgent
function during the app startup and more later. One of those was in the AppInitialiser and the rest were calls to the AppConfigModule that uses the FluxC implementation.Since I wasn't able to reproduce the ANR my plan is to monitor how this fix would affect the occurrences of the issue. If the issue persist I would probably try a more drastic approach of changing the user agent used for the api calls and keep the full user agent using the
WebSettings.getDefaultUserAgent
only for the WebViews.Internal ref: pcdRpT-5Us-p2#comment-9676
To Test:
AppInitializer.userAgentString
is logged correctlyRegression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):