Stop showing mobile store banners for current users #14942
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
janbrasna
commented
Jul 31, 2024
janbrasna
force-pushed
the
fix/mobile-banner-nag
branch
from
7291ae9
to
a7d0c71
Compare
Co-authored-by: Ingi-Hong <ingihong@bu.edu>
janbrasna
force-pushed
the
fix/mobile-banner-nag
branch
from
a7d0c71
to
9f11e01
Compare
alexgibson
added
P3
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One-line summary
Shows mobile store banners only to nonFX users.
(This picks up and supersedes #13331, updating it to current codebase.)
Significant changes and points to review
This was unable to be fixed for a while when FxiOS and Focus stopped identifying themselves beyond generic UIWebView, but the identifiers are back for some time now so they can be told apart from the system engine again.
(At least on phones, or unless asking for "request desktop version" when the UA mimics either desktop Safari or desktop Linux FX, on iPads that's most of the time and even manually requesting mobile site doesn't change it reliably. But that's not issue with this code or changeset just consuming Mozilla.Client and its limitations — the issues are with opinionated choices in the mobile products, as linked e.g. from #6875 (comment))
Firefox mobile and Focus can't be told apart as they both report alike (FxiOS and Firefox/Mobile on 🤖 respectively), so there is no way to e.g. offer the banner for the second app to user of the first etc., there's no app ID logic involved as it would be with native smartbanners — so it's all or nothing only, with Focus being even more fuzzy about its uastring showing up as various versions of Safari instead;)
This does not add any new logic. Only what
Mozilla.Client.isMobile && !Mozilla.Client.isFirefoxalready sniff, know and provide. Better than nothing, but far from perfect when the browsers pretend they're something else completely.Issue / Bugzilla link
Resolves #10571
Testing
/firefox)/firefox/focus)/)/)/firefox/focus)Other platforms can be spoofed with just uastrings, I've used:
Mozilla/5.0 (Android 4.4; Mobile; rv:70.0) Gecko/70.0 Firefox/70.0(gecko)Mozilla/5.0 (Linux; Android 4.3; Nexus 7 Build/JSS15Q) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/127.0.0.0 Safari/537.36(chromium tablet)Mozilla/5.0 (Linux; Android 8.1.0; Pixel Build/OPM4.171019.021.D1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.109 Mobile Safari/537.36 EdgA/42.0.0.2057(edge mobile)Opera/12.02 (Android 4.1; Linux; Opera Mobi/ADR-1111101157; U; en-US) Presto/2.9.201 Version/12.02(opera mobi)Mozilla/5.0 (PlayBook; U; RIM Tablet OS 2.1.0; en-US) AppleWebKit/536.2+ (KHTML, like Gecko) Version/7.2.1.0 Safari/536.2+(bb tablet, not treated asisMobile)