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

Implement OS Version Detection for the User Agent #25372

Closed

Conversation

@MeFisto94
Copy link
Contributor

MeFisto94 commented Dec 23, 2019

User Agent now correctly reports Windows 7, 8.1, 10, ...


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #18194
  • These changes do not require tests, because that would involve testing servo across all windows versions, to be meaningful
@highfive
Copy link

highfive commented Dec 23, 2019

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @paulrouget (or someone else) soon.

@MeFisto94
Copy link
Contributor Author

MeFisto94 commented Dec 23, 2019

In order to test this changes, surf to "https://www.whatsmyua.info or "https://www.whatismybrowser.com/detect/what-is-my-user-agent (the latter shows another issue: analyseing the UA fails due to some CSRF problem).

I did only test this on Windows 10, but the crate is fairly simple and as such I don't expect any issues, apart from the following two:

  • The crate still uses 0.2.8 winapi, which makes test-tidy fail:
./Cargo.lock:1: duplicate versions for package `winapi`
The following packages depend on version 0.2.8 from 'crates.io':
The following packages depend on version 0.3.8 from 'https://github.com/servo/winapi-rs?branch=patch-1':

See stanislav-tkach/os_info#111 for tracking

  • The crate doesn't work on UWP, which doesn't really have a concept of a Windows Version either (stanislav-tkach/os_info#112), thus it has just been excluded in this PR.

Also note the usage of Box::leak, maybe you have a different vision. I also removed the const DESKTOP_UA_STRING: &'static str =

@paulrouget
Copy link
Contributor

paulrouget commented Dec 30, 2019

winapi duplicate versions is annoying. Do we have to use os_info? How does os_info::get().version().version() work?

@MeFisto94
Copy link
Contributor Author

MeFisto94 commented Jan 4, 2020

Someone already created an issue for winapi 0.30 on os_info. We could also submit a PR.
The code itself is really small, though and we probably don't need it for Linux and most of the other supported OS anyway. Only for Mac OS X maybe.

@atouchet
Copy link
Contributor

atouchet commented Jan 21, 2020

The latest version of os_info is now using winapi 0.3.

@jdm
jdm approved these changes Jan 21, 2020
Copy link
Member

jdm left a comment

This seems fine to me. Please update to the latest version of os_info and we can merge this.

@@ -21,6 +21,9 @@ use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::{RwLock, RwLockReadGuard};
use url::{self, Url};

#[cfg(all(target_os = "windows", not(target_vendor = "uwp")))]
extern crate os_info;

This comment has been minimized.

Copy link
@jdm

jdm Jan 21, 2020

Member

This statement should not be necessary in the 2018 edition of Rust, which components/config/Cargo.toml enables.

@jdm jdm assigned jdm and unassigned paulrouget Jan 21, 2020
@MeFisto94 MeFisto94 force-pushed the MeFisto94:useragent-windows-expose-version branch from 7c44064 to 957c49f Jan 23, 2020
@MeFisto94 MeFisto94 force-pushed the MeFisto94:useragent-windows-expose-version branch from 957c49f to a447136 Jan 23, 2020
Copy link
Member

jdm left a comment

Thinking about this a little harder, and how to avoid the leak and compilation error on non-windows, I'm becoming more hesitant about merging this. It only affects desktop windows builds, and only impacts windows versions that are already end-od-life. I am incluned to stick with the existing implementation and avoid the complexity without a compelling motivation otherwise.

{
match os_info::get().version().version() {
os_info::VersionType::Semantic(maj, min, _patch) => {
return Box::leak(

This comment has been minimized.

Copy link
@jdm

jdm Jan 23, 2020

Member

I just noticed that this will leak memory on every network request.

This comment has been minimized.

Copy link
@MeFisto94

MeFisto94 Jan 23, 2020

Author Contributor

While it doesn't change much about the reasoning, I think or better put assumed that this would happen only once on startup in https://github.com/MeFisto94/servo/blob/a4471368965d30c08042b73b5d475558a2e277f0/components/config/opts.rs#L567

This comment has been minimized.

Copy link
@jdm

jdm Jan 24, 2020

Member

Ah, right. My mistake.

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.

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