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

Send W3C capabilities in the new session request. #4974

Merged
merged 2 commits into from
Nov 3, 2017

Conversation

juangj
Copy link
Contributor

@juangj juangj commented Oct 27, 2017

If the Selenium standalone server only finds legacy capabilities in the request,
then it will assume that the client doesn't speak W3C, and will return a legacy
response. Sending a request that includes the W3C keys induces it to send a W3C
response.

@juangj juangj requested a review from jleyba October 27, 2017 20:13
@juangj
Copy link
Contributor Author

juangj commented Oct 27, 2017

This probably also applies to Grid, but I'm really not that familiar with Grid so I'm not positive.

Other language bindings do fancier things like filtering non-W3C capability names out of the W3C capabilities blob (here it is in Python). Not sure how necessary you feel that is.

Also kinda undecided how to treat required capabilities, but those should go away at some point anyway, so maybe you were already planning an API change to eliminate them.

If the Selenium standalone server only finds legacy capabilities in the request,
then it will assume that the client doesn't speak W3C, and will return a legacy
response. Sending a request that includes the W3C keys induces it to send a W3C
response.
@p0deje p0deje added the C-nodejs label Nov 1, 2017
Copy link
Contributor

@jleyba jleyba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the tests pass with this change? Last time I tried there were a lot of failures still.

@@ -380,7 +380,7 @@ function normalizeProxyConfiguration(config) {
}
} else if ('pac' === config.proxyType) {
if (config.proxyAutoconfigUrl && !config.pacUrl) {
config.pacUrl = config.proxyAutoconfigUrl;
config.proxyAutoconfigUrl = config.proxyAutoconfigUrl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it looks like the entire normalizeProxyConfiguration function isn't needed anymore

@juangj
Copy link
Contributor Author

juangj commented Nov 1, 2017

npm test passed (or, well, it didn't fail any more than it already did on my machine). And this seemed to work for many tests, but it doesn't work for tests that set non-W3C capabilities.

I do have some code (probably living in the wrong file right now) that will filter non-W3C capability names like some of the other language bindings do. But if you're planning to completely ditch legacy protocol support anyway, then maybe there's no need for any of this -- once all the capability names are changed, you could just switch completely to W3C capabilities instead of sending this hybrid.

(I'm going to backport some version of this to our internal 3.6.0 branch in order to continue to support Chrome users, though.)

@juangj
Copy link
Contributor Author

juangj commented Nov 1, 2017

I added that commit that drops invalid capabilities, if you're curious.

The normalizeProxyConfiguration thing should probably just go in a separate commit.

@jleyba jleyba merged commit e46eb57 into SeleniumHQ:master Nov 3, 2017
@juangj juangj deleted the ff-w3c-caps branch November 3, 2017 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants