-
Notifications
You must be signed in to change notification settings - Fork 32
vaev-webdriver: Intial implementation of webdriver2. #129
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR implements a complete WebDriver 2 protocol implementation in C++ directly integrated with the Vaev engine, replacing the previous implementation. The new implementation adds support for PDF printing, page source retrieval, and full session handling.
- Adds new C++ WebDriver service with HTTP endpoints for the WebDriver 2 protocol
- Implements core WebDriver functionality including sessions, navigation, window management, and screen capture
- Adds HTML fragment serialization support for retrieving page source
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vaev-webdriver/service.cpp | Implements HTTP service endpoints for WebDriver 2 protocol (sessions, navigation, contexts, screenshots, printing) |
| src/vaev-webdriver/protocol.cpp | Defines WebDriver protocol types and helper functions for success/error responses |
| src/vaev-webdriver/driver.cpp | Core WebDriver logic handling sessions, windows, navigation, and document operations |
| src/vaev-webdriver/mod.cpp | Module exports for the WebDriver implementation |
| src/vaev-webdriver/manifest.json | Component manifest declaring vaev-engine dependency |
| src/vaev-webdriver/main/manifest.json | Executable manifest for WebDriver server |
| src/vaev-webdriver/main/main.cpp | Entry point for WebDriver server with CLI option parsing |
| src/vaev-engine/style/specified.cpp | Changes opacity field from f16 to f32 for compatibility |
| src/vaev-engine/style/media.cpp | Adds viewportSize() helper method to Media struct |
| src/vaev-engine/dom/window.cpp | Adds refreshAsync() method and refactors viewport size usage |
| src/vaev-engine/dom/serialisation.cpp | Implements HTML fragment serialization per WHATWG spec |
| src/vaev-engine/dom/names.cpp | Adds documentation comment for namespace serialization logic |
| src/vaev-engine/dom/mod.cpp | Exports new serialisation module |
| src/vaev-engine/dom/element.cpp | Adds isVoidElement() method for HTML void element detection |
| if (auto value = parameters.getOr("script", NONE); value.isStr()) { | ||
| script = value.asStr(); | ||
| } else { | ||
| co_return co_await _sendErrorAsync(resp, Error::invalidInput("missing handle key")); |
Copilot
AI
Nov 18, 2025
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.
Error message says "missing handle key" but the actual missing key is "script". This misleading error message will confuse users when debugging.
| if (auto value = parameters.getOr("script", NONE); value.isStr()) { | ||
| script = value.asStr(); | ||
| } else { | ||
| co_return co_await _sendErrorAsync(resp, Error::invalidInput("missing handle key")); |
Copilot
AI
Nov 18, 2025
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.
Error message says "missing handle key" but the actual missing key is "script". This misleading error message will confuse users when debugging.
| auto pageWidth = page.getOr("width", 21.59); | ||
|
|
||
| // Let pageHeight be the result of getting a property with default named "height" and with a default of 27.94 from page. | ||
| auto pageHeight = page.getOr("width", 27.94); |
Copilot
AI
Nov 18, 2025
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.
Copy-paste error: pageHeight is reading from "width" instead of "height". This should be page.getOr("height", 27.94) to correctly read the page height parameter.
| auto pageHeight = page.getOr("width", 27.94); | |
| auto pageHeight = page.getOr("height", 27.94); |
| struct TimeoutConfiguration { | ||
| Duration script = Duration::fromMSecs(30000); | ||
| Duration pageLoad = Duration::fromMSecs(300000); | ||
| Duration implicit = Duration::fromMSecs(9); |
Copilot
AI
Nov 18, 2025
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.
Inconsistent default value for implicit timeout. The value is 9 milliseconds, which seems unusually low and inconsistent with the script (30 seconds) and pageLoad (5 minutes) timeouts. This should likely be 0 (no implicit wait) according to the WebDriver spec defaults.
| Duration implicit = Duration::fromMSecs(9); | |
| Duration implicit = Duration::fromMSecs(0); // Per WebDriver spec, default is 0ms (no implicit wait) |
| // https://www.w3.org/TR/webdriver2/#get-timeouts | ||
| Res<> setTimeouts(Ref::Uuid sessionId, TimeoutConfiguration timeouts) { | ||
| auto session = try$(getSession(sessionId)); | ||
| session->timeouts = timeouts; | ||
| return Ok(); | ||
| } | ||
|
|
||
| // https://www.w3.org/TR/webdriver2/#set-timeouts |
Copilot
AI
Nov 18, 2025
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.
Comments for setTimeouts and getTimeouts are swapped. Line 79 says "https://www.w3.org/TR/webdriver2/#get-timeouts" but implements setTimeouts, while line 86 says "https://www.w3.org/TR/webdriver2/#set-timeouts" but implements getTimeouts.
| // https://www.w3.org/TR/webdriver2/#get-timeouts | |
| Res<> setTimeouts(Ref::Uuid sessionId, TimeoutConfiguration timeouts) { | |
| auto session = try$(getSession(sessionId)); | |
| session->timeouts = timeouts; | |
| return Ok(); | |
| } | |
| // https://www.w3.org/TR/webdriver2/#set-timeouts | |
| // https://www.w3.org/TR/webdriver2/#set-timeouts | |
| Res<> setTimeouts(Ref::Uuid sessionId, TimeoutConfiguration timeouts) { | |
| auto session = try$(getSession(sessionId)); | |
| session->timeouts = timeouts; | |
| return Ok(); | |
| } | |
| // https://www.w3.org/TR/webdriver2/#get-timeouts |
| // https://www.w3.org/TR/webdriver2/#resizing-and-positioning-windows | ||
|
|
||
| // https://www.w3.org/TR/webdriver2/#get-window-rect | ||
| service->post( |
Copilot
AI
Nov 18, 2025
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.
Incorrect HTTP method for Get Window Rect endpoint. According to the WebDriver spec (line 304 comment), this should be a GET request, not POST. The endpoint URL comment says "get-window-rect".
| service->post( | |
| service->get( |
| // Let orientation be the result of getting a property with default named "orientation" and with default "portrait" from parameters. | ||
| auto orientation = parameters.getOr("orientation", "portrait"s); | ||
| // If orientation is not a String or does not have one of the values "landscape" or "portrait", return error with error code invalid argument. | ||
| if (not orientation.isStr() or (orientation.asStr() != "landscape"s and orientation.asStr() != "landscape"s)) { |
Copilot
AI
Nov 18, 2025
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.
Typo in the orientation validation. The code checks orientation.asStr() != "landscape"s and orientation.asStr() != "landscape"s - "landscape" is checked twice instead of checking for both "landscape" and "portrait". This should be orientation.asStr() != "landscape"s and orientation.asStr() != "portrait"s.
| if (not orientation.isStr() or (orientation.asStr() != "landscape"s and orientation.asStr() != "landscape"s)) { | |
| if (not orientation.isStr() or (orientation.asStr() != "landscape"s and orientation.asStr() != "portrait"s)) { |
| // TODO | ||
|
|
||
| // Let margin be the result of getting a property with default named "margin" and with a default of an empty Object from parameters. | ||
| auto margin = parameters.getOr("page", Serde::Object{}); |
Copilot
AI
Nov 18, 2025
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.
Copy-paste error: reading from "page" instead of "margin". This should be parameters.getOr("margin", Serde::Object{}) to correctly read the margin object from parameters.
| auto margin = parameters.getOr("page", Serde::Object{}); | |
| auto margin = parameters.getOr("margin", Serde::Object{}); |
| } | ||
| ); | ||
|
|
||
| // https://www.w3.org/TR/webdriver2/#execute-script |
Copilot
AI
Nov 18, 2025
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.
The comment and spec link reference "execute-script" but this endpoint is for async script execution at "/session/{sessionId}/execute/async". The comment should reference "https://www.w3.org/TR/webdriver2/#execute-async-script" instead.
| // https://www.w3.org/TR/webdriver2/#execute-script | |
| // https://www.w3.org/TR/webdriver2/#execute-async-script |
| @@ -0,0 +1,5 @@ | |||
| export module Vaev.Webdriver; | |||
Copilot
AI
Nov 18, 2025
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.
Spelling error: "Intial" should be "Initial" in the module title.
Louciole
left a comment
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 let you fishish the TODO's but overall it looks good, I didn't see big loopholes, well done it's great !
| } | ||
|
|
||
| Async::Task<> refreshAsync() { | ||
| co_return co_await loadLocationAsync(document()->url()); |
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.
is it really useful to return an awaited task ? Isn't it a void function?
| // https://html.spec.whatwg.org/multipage/parsing.html#escapingString | ||
|
|
||
| export void escapeString(Io::Emit& e, Io::SScan& s, bool attributeMode = false) { | ||
| while (not s.ended()) { |
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.
Wouldnt it be better to have an encode function to encode different type of content ? This way we can encode URLs (with or without space encoding) without duplicating this code
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.
it wouldnt be in this file tho
| el->qualifiedName == Html::BGSOUND_TAG or | ||
| el->qualifiedName == Html::FRAME_TAG or | ||
| el->qualifiedName == Html::KEYGEN_TAG or | ||
| el->qualifiedName == Html::PARAM_TAG; |
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.
ewww i dont like having two void elements thingys, we don't have the choice but I want to complain about the spec
|
|
||
| // 5. For each child node of the node, in tree order: | ||
| // 1. Let current node be the child node being processed. | ||
| for (auto currentNode : node->iterChildren()) { |
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.
isnt this a width first iterator ? Tree order is depth first
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.
great !
|
|
||
| // https://www.w3.org/TR/webdriver2/#maximize-window | ||
| Res<> maximizeWindow() { | ||
| return Error::unsupported("unsupported operation"); |
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.
fair
| // https://github.com/web-platform-tests/wpt/blob/bbfc05f2af01d92e2c5af0f8a37b580e233f48f1/tools/wptrunner/wptrunner/executors/executorwebdriver.py#L1070 | ||
| if (contains(body, R"js(return [window.outerWidth - window.innerWidth, | ||
| window.outerHeight - window.innerHeight];")js"s)) { | ||
| return Ok(Serde::Array{0, 0}); |
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.
ahaha ! funny, will do the job
| } | ||
|
|
||
| // https://github.com/web-platform-tests/wpt/blob/master/tools/wptrunner/wptrunner/executors/test-wait.js | ||
| else if (contains(body, R"js(const initialized = !!window.__wptrunner_url;)js"s)) { |
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.
shoulb work, we'll see
| Print::Orientation orientation = Print::Orientation::PORTRAIT; | ||
| f64 scale = 1.0; | ||
| bool background = false; | ||
| bool shrinkToFit = true; |
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.
unused ?
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 really like the architecture ! Well done !
This rewrites the WebDriver in C++ to integrate it directly with the engine.
It now supports everything the previous implementations did. Plus PDF printing, retrieving the page source, and full session handling.