Skip to content

/ servo

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

Allow embedders to provide user agent string #25672

Closed
wants to merge 8 commits into from
Closed

Allow embedders to provide user agent string #25672

wants to merge 8 commits into from

Conversation

@mediremi
Copy link
Contributor

mediremi commented Feb 1, 2020 •
edited

Allow embedders to provide user agent string instead of setting it in servo/components/config/opts.rs.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #25646
  • There are tests for these changes OR
  • These changes do not require tests because ___
@highfive
Copy link

highfive commented Feb 1, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/pipeline.rs, components/constellation/constellation.rs, components/script/script_thread.rs
  • @cbrewster: components/constellation/pipeline.rs, components/constellation/constellation.rs
  • @KiChjang: components/script/script_thread.rs, components/script_traits/lib.rs
@highfive
Copy link

highfive commented Feb 1, 2020

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@bors-servo
Copy link
Contributor

bors-servo commented Feb 1, 2020

☔ The latest upstream changes (presumably #25669) made this pull request unmergeable. Please resolve the merge conflicts.
@atouchet
Copy link
Contributor

atouchet commented Feb 1, 2020

#25669 updated Servo's user agent strings. Could you please match those changes in this PR?
mediremi added 7 commits Feb 1, 2020
@mediremi
Make default_user_agent_string a method on the EmbedderMethods trait …
88f48d4 
…so that embedder can decide what user agent to report in network requests
@mediremi
Fix typo in comment
4ba38af
@mediremi
Fix passing of user_agent to create_constellation
fe744f5
@mediremi
Add default implementation of EmbedderMethods::default_user_agent_string
6329649
@mediremi
Fix constellation
6e43434
@mediremi
Fix formatting
8a03db7
@mediremi
Remove -u option
910745e
@mediremi mediremi force-pushed the mediremi:allow-embedders-to-set-ua branch from 0225338 to 249199c Feb 1, 2020
@mediremi
Update user agent strings to those from #25669
Loading status checks…
4405d3a
@mediremi mediremi force-pushed the mediremi:allow-embedders-to-set-ua branch from 249199c to 4405d3a Feb 1, 2020
@paulrouget
paulrouget reviewed Feb 7, 2020
View changes
Copy link
Contributor

paulrouget left a comment

So the UA is defined by (sorted by priority):

  • the -u option
  • the embedder implementation
  • the servo implementation

Maybe in component/servo check if the option is set (it’s also fine, even maybe better, if it happens in ports/glutin), if not, call the embedder callback. And maybe the default UA function should not live in windowing.rs but in component/servo.

It would look like this:

in components/servo,

  • get UA from -u option (maybe via ports/glutin/main2.rs (see the arg parsing code there), not opts.rs). If None,
  • get UA from embedder (windowing) function, If None,
  • get the default UA
components/compositing/windowing.rs Outdated
@@ -182,6 +182,9 @@ pub trait EmbedderMethods {

/// Register services with a WebXR Registry.
fn register_webxr(&mut self, _: &mut webxr::MainThreadRegistry) {}

/// Returns the default user agent string
fn default_user_agent_string(&self) -> &'static str;

This comment has been minimized.

Sign in to view
Copy link
@paulrouget

paulrouget Feb 7, 2020

Contributor

Make that a verb for consistency.
components/compositing/windowing.rs
const UA_STRING: &'static str =
"Mozilla/5.0 (Windows NT 10.0; rv:72.0) Servo/1.0 Firefox/72.0";

#[cfg(all(target_os = "macos"))]

This comment has been minimized.

Sign in to view
Copy link
@paulrouget

paulrouget Feb 7, 2020

Contributor

I don’t think the all is necessary here.
components/compositing/windowing.rs
)))]
// Use OS X user agent as fallback
const UA_STRING: &'static str =
"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:72.0) Servo/1.0 Firefox/72.0";

This comment has been minimized.

Sign in to view
Copy link
@paulrouget

paulrouget Feb 7, 2020

Contributor

That’s fine because it’s what we’ve been doing until now. But maybe we should just not have a fallback. Once we support a new platform, we would let it fail at compile time.
components/config/opts.rs
Some(ref ua) if ua == "desktop" => default_user_agent_string(UserAgent::Desktop).into(),
Some(ua) => ua.into(),
None => default_user_agent_string(DEFAULT_USER_AGENT).into(),
};

This comment has been minimized.

Sign in to view
Copy link
@paulrouget

paulrouget Feb 7, 2020

Contributor

This still needs to be possible.
@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

☔ The latest upstream changes (presumably #25822) made this pull request unmergeable. Please resolve the merge conflicts.
This was referenced Apr 3, 2020
Closed
Merged
@atouchet
Copy link
Contributor

atouchet commented Apr 9, 2020

This was finished in #26103.
@atouchet atouchet closed this Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulrouget paulrouget

Assignees

@paulrouget paulrouget

Labels
S-needs-code-changes S-needs-rebase
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Allow embedders to provide user agent string
6 participants
@mediremi @highfive @bors-servo @atouchet @paulrouget @jdm
You can’t perform that action at this time.