andreastt in https://github.com/mozilla/webdriver-rust/issues/51: > In order to make imperative checks at runtime for support of > certain commands, such as the window size- and positioning > commands which may be unsupported on certain configurations, we > need to support HTTP HEAD requests for all commands.

This is actually a conformance issue.

Comment on attachment 8955840 [details] [diff] [review] Handle HEAD requests as GETs and omit response body Review of attachment 8955840 [details] [diff] [review]: ----------------------------------------------------------------- First of all, thanks for the patch! The purpose of allowing HEAD requests to geckodriver would be to check if a certain command is available by querying its endpoint. Whilst it is true that the HEAD method is meant to be identical to GET except for not returning a body, the fact that WebDriver maps a remote-control interface on top of HTTP means this isn’t exactly what is needed. Instead of processing the command on a request with the HEAD method, we could simply return 200 with an empty message-body if the command exists, and 404 otherwise. This would also apply to commands that are POST requests. I hope that makes sense.

Makes sense! In that case, WebDriverHttpApi is probably a better place to handle the HEAD method. That's where there is access to the method call and the code to make the call to handle the request. Depending on the former, we can handle the HEAD shortroute by checking for the presence of the request handler, but omit the call itself. If this is the correct approach, I can make a patch for the change as needed.

Comment on attachment 8956696 [details] Bug 1396824 - Handle head requests by sending 200 in event of WebDriverMessage existing https://reviewboard.mozilla.org/r/225676/#review231626 I don’t think this will work as expected because you will only end up here if you match a route in WebDriverHttpApi::routes, and this route list doesn’t mention any HEAD paths. You will probably want to modify WebDriverHttpApi::decode_request() to always return a 200 success if the route matches and the method is HEAD: https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/testing/webdriver/src/httpapi.rs#225-244

It definitely appears you're correct; I was mistaken earlier (I misinterpreted the valid headers returned from the error message as valid *ok* headers). It looks like the decode_request method does the mapping from HTTP call to WebDriverMessage (WebDriverCommand/Session pair), and doesn't provide an easy way to return out a generic 200, which happens one call up, in HttpHandler::handle. Perhaps a combination of both approaches, whereby we map the HEAD call to a GET, send it to decode_request, and if it successfully decodes the command, we return headers instead of sending it down the the HttpHandler.chan channel, where it's actually handled/executed. I'll give it a shot, and if it's the correct approach/it works, I'll push a patch to review.

I pushed a change with the fix that seems to address the problem at hand, but it does lack tests. Would a test case of checking the HEAD of one of the commands (say, "url") suffice, or would we want something more comprehensive?

Comment on attachment 8956828 [details] Bug 1396824 - Treat HEAD requests as GET requests when decoding and return 200 before executing https://reviewboard.mozilla.org/r/225788/#review232008 Your latest patch actually appears to work, but only for GET commands. We can file a follow-up bug that we need WPT tests for all commands for this. You don’t have to add them here. ::: testing/webdriver/src/server.rs:179 (Diff revision 1) > + let (head_req, equivalent_req_method) = match req.method { > + Method::Head => (true, Method::Get), > + method => (false, method) > + }; Can you find some way of getting rid of this? I don’t think it is needed. ::: testing/webdriver/src/server.rs:180 (Diff revision 1) > } > > debug!("-> {} {} {}", req.method, req.uri, body); > > + let (head_req, equivalent_req_method) = match req.method { > + Method::Head => (true, Method::Get), This will only make it work for GET commands. I.e. /session/{session id}/refresh will still return 404.

At a glance, it does appear that adding HEAD support for standard POST routes would require more substantial changes, so I did opt to solve GET routes only on this ticket. The updated patch should omit the block upfront. It does require a "req.method.clone()" because "api.decode_requests()" consumes. It's minor, but I'm still mostly unaware to what standards are in place for these projects.

Sorry it has taken my long to get back to you on this. I looked into this and have concluded any approach to fix this without changing the existing program structure too much is futile. I’m attaching my own attempt which is imperect in a lot of ways. I’ve not requested review for it because I think a better and more sustainable long-term approach would be to move geckodriver to use one of the higher-level web frameworks which come with built-in routing/route matching.

Comment on attachment 8975280 [details] [diff] [review] 0001-Bug-1396824-Support-HEAD-requests-in-geckodriver.patch Now that we use serde in geckodriver, we will consider using either a Rust web framework or a request router not written by us to correctly supply HEAD requests. This renders the attached patch obsolete.

And this cannot be done with hyper? Or why would we need a different framework?

(In reply to Henrik Skupin (:whimboo) from comment #15) > And this cannot be done with hyper? Or why would we need a > different framework? All frameworks I know of use hyper internally, but hyper itself does not to request routing. eijebong suggested we look at warp, which is written by the maintainer of hyper: https://seanmonstar.com/post/176530511587/warp AIUI it is a sort of opinionated “mini” web framework, which at its core provides request routing. An important requirement to note here is that we need a solution that can be supported in Rust stable. This excludes frameworks such as Rocket.

Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dbb1c8fa2aba Part 1: Replace the webdriver router by warp. r=ato https://hg.mozilla.org/integration/autoland/rev/4d31fcd3085d Part 2: Revendor dependencies. r=ato