-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: user agent parsing #1607
feat: user agent parsing #1607
Conversation
46a7112
to
e4757ad
Compare
…0 if unparsable, updated related tests
20a23d1
to
c2ff37c
Compare
matches!(&self.device_family, DeviceFamily::Mobile) | ||
&& matches!(&self.os_family, OsFamily::Android) |
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.
Let's add a test for Platform::Other
-- going by the spreadsheet, I'd expect "Mozilla/5.0 (Linux; Android 9; SM-A920F) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4216.0 Mobile Safari/537.36" (Android Chrome I believe) to come back as it.
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.
I made an implementation change, since the parsing logic was still getting the right OS and device family. I adjusted it to be that if the browser is not Firefox, return the Default implementation of DeviceInfo
with Other as everything and FirefoxVersion
of 0, making it easy to screen out, added test too
} | ||
} | ||
|
||
pub fn get_device_info(user_agent: &str) -> DeviceInfo { |
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.
Let's add a note in the docs for this fn that it handles a mix regular user-agents and the sync specific ones. I guess the latter are mostly just the ios "Firefox-iOS-FxA/24" one. Desktop also has its sync specific "FxSync/<...>.desktop" thing but it doesn't end up requiring special handling
…y, device family and possible non-firefox user agent to return an empty device info
Description
Addition of User Agent parsing, consisting of:
Platform (Firefox Desktop, Firefox iOS, Fenix, Other)
Device family (Desktop, Mobile, Tablet, Other)
OS family (Mac, Windows, Linux, iOS, Android, other)
Note: User agent emission from iOS clients is non-standard. Two observed variants occur:
Firefox-iOS-FxA/24
orFirefox-iOS-Sync/115.0b32242 (iPhone; iPhone OS 17.7) (Firefox)
See the shared spreadsheet in SYNC-4413 for details.
As a result of Woothee being unable to parse this non-standard user agent, custom logic was added to fill in the gaps to so that the association logic can work to fill out all the matching enum values.
Subsequent PR will pull this into metarequest and apply the parsing.
Testing
Added unit tests within
user_agent
to verify new user_agent strings.Issue(s)
Closes SYNC-4414.