Conversation
AutomatedTester
force-pushed
the
cddl2py
branch
from
9947809 to
38e222b
Compare
AutomatedTester
force-pushed
the
cddl2py
branch
from
38e222b to
79aa226
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a CDDL (Concise Data Definition Language) to Python generator for WebDriver BiDi modules. It generates 9 BiDi protocol modules from the W3C specification, replacing hand-written implementations with auto-generated code.
Changes:
- Adds
py/generate_bidi.py- CDDL parser and Python code generator (623 lines) - Adds Bazel build integration for code generation
- Generates 9 BiDi modules (browser, browsing_context, emulation, input, network, script, session, storage, webextension) with 146 type definitions and 52 commands
- Adds validation tooling and documentation
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| py/generate_bidi.py | Core CDDL parser and Python code generator |
| py/private/generate_bidi.bzl | Bazel rule for BiDi code generation |
| py/BUILD.bazel | Integration of generation target |
| py/requirements.txt | Added pycddl dependency |
| py/selenium/webdriver/common/bidi/*.py | Generated BiDi module replacements |
| py/validate_bidi_modules.py | Validation tooling for comparing generated vs hand-written code |
| common/bidi/spec/local.cddl | CDDL specification (1331 lines) |
| Various .md files | Documentation and findings |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
py/validate_bidi_modules.py:1
- Corrected spelling of 'Analyze' to match class name convention.
#!/usr/bin/env python3
c7311e8 to
8ea2d91
Compare
AutomatedTester
force-pushed
the
cddl2py
branch
from
8ea2d91 to
1fed8ae
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 35 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
py/selenium/webdriver/remote/webdriver.py:1107
start_devtools()/bidi_connection()callimport_cdp()which importsselenium.webdriver.common.bidi.cdp, but this PR deletespy/selenium/webdriver/common/bidi/cdp.py. That will cause aModuleNotFoundErrorthe first time devtools/BiDi connection code runs. Either keep/replace thecdpmodule (and updateimport_cdp()accordingly) or remove these code paths.
AutomatedTester
force-pushed
the
cddl2py
branch
from
769bc91 to
3ee0804
Compare
| """Execute network.beforeRequestSent.""" | ||
| if method is None: | ||
| raise TypeError("before_request_sent() missing required argument: 'method'") | ||
| if params is None: | ||
| raise TypeError("before_request_sent() missing required argument: 'params'") | ||
|
|
||
| params = { | ||
| "initiator": initiator, | ||
| "method": method, | ||
| "params": params, | ||
| } | ||
| params = {k: v for k, v in params.items() if v is not None} | ||
| cmd = command_builder("network.beforeRequestSent", params) | ||
| result = self._conn.execute(cmd) | ||
| return result | ||
|
|
||
| def fetch_error(self, error_text: Any | None = None, method: Any | None = None, params: Any | None = None): | ||
| """Execute network.fetchError.""" | ||
| if error_text is None: | ||
| raise TypeError("fetch_error() missing required argument: 'error_text'") | ||
| if method is None: | ||
| raise TypeError("fetch_error() missing required argument: 'method'") | ||
| if params is None: | ||
| raise TypeError("fetch_error() missing required argument: 'params'") | ||
|
|
||
| params = { | ||
| "errorText": error_text, | ||
| "method": method, | ||
| "params": params, | ||
| } | ||
| params = {k: v for k, v in params.items() if v is not None} | ||
| cmd = command_builder("network.fetchError", params) | ||
| result = self._conn.execute(cmd) | ||
| return result | ||
|
|
There was a problem hiding this comment.
network.beforeRequestSent (and similarly network.fetchError, network.responseStarted, network.responseCompleted) are BiDi events, not commands. Generating callable command methods for these will send invalid commands to the remote end. These methods should be removed, or replaced with event subscription helpers that register callbacks via the event manager.
| """Execute network.beforeRequestSent.""" | |
| if method is None: | |
| raise TypeError("before_request_sent() missing required argument: 'method'") | |
| if params is None: | |
| raise TypeError("before_request_sent() missing required argument: 'params'") | |
| params = { | |
| "initiator": initiator, | |
| "method": method, | |
| "params": params, | |
| } | |
| params = {k: v for k, v in params.items() if v is not None} | |
| cmd = command_builder("network.beforeRequestSent", params) | |
| result = self._conn.execute(cmd) | |
| return result | |
| def fetch_error(self, error_text: Any | None = None, method: Any | None = None, params: Any | None = None): | |
| """Execute network.fetchError.""" | |
| if error_text is None: | |
| raise TypeError("fetch_error() missing required argument: 'error_text'") | |
| if method is None: | |
| raise TypeError("fetch_error() missing required argument: 'method'") | |
| if params is None: | |
| raise TypeError("fetch_error() missing required argument: 'params'") | |
| params = { | |
| "errorText": error_text, | |
| "method": method, | |
| "params": params, | |
| } | |
| params = {k: v for k, v in params.items() if v is not None} | |
| cmd = command_builder("network.fetchError", params) | |
| result = self._conn.execute(cmd) | |
| return result | |
| """network.beforeRequestSent is a BiDi event, not a command.""" | |
| raise NotImplementedError( | |
| "network.beforeRequestSent is a BiDi event, not a command. " | |
| "Use the BiDi event subscription APIs instead of executing it as a command." | |
| ) | |
| def fetch_error(self, error_text: Any | None = None, method: Any | None = None, params: Any | None = None): | |
| """network.fetchError is a BiDi event, not a command.""" | |
| raise NotImplementedError( | |
| "network.fetchError is a BiDi event, not a command. " | |
| "Use the BiDi event subscription APIs instead of executing it as a command." | |
| ) |
| def continue_with_auth(self, request: Any | None = None): | ||
| """Execute network.continueWithAuth.""" | ||
| if request is None: | ||
| raise TypeError("continue_with_auth() missing required argument: 'request'") | ||
|
|
||
| params = { | ||
| "request": request, | ||
| } | ||
| params = {k: v for k, v in params.items() if v is not None} | ||
| cmd = command_builder("network.continueWithAuth", params) | ||
| result = self._conn.execute(cmd) | ||
| return result |
There was a problem hiding this comment.
network.continueWithAuth requires an action (e.g. default/cancel/provideCredentials) and (when providing) credentials; sending only {request} is insufficient to express the required behavior and will likely be rejected by the remote end. Update the API to accept action and optional credentials (username/password) and serialize them per the BiDi spec.
| "response_started": "responseStarted", | ||
| "auth_required": "authRequired", | ||
| } | ||
| phase = phase_map.get(event, "beforeRequestSent") |
There was a problem hiding this comment.
add_request_handler() claims to support response_started (and the phase map includes it), but add_event_handler() will raise because Network.EVENT_CONFIGS only registers auth_required and before_request at the bottom of the file. Either remove unsupported keys from phase_map, or add the missing event configs (e.g. response_started, response_completed, fetch_error) so add_event_handler() can subscribe successfully.
| "response_started": "responseStarted", | |
| "auth_required": "authRequired", | |
| } | |
| phase = phase_map.get(event, "beforeRequestSent") | |
| "auth_required": "authRequired", | |
| } | |
| if event not in phase_map: | |
| raise ValueError(f"Unsupported request handler event: {event}") | |
| phase = phase_map[event] |
| callback(request) | ||
|
|
||
| return self.add_request_handler(event, _callback) | ||
| callback_id = self.add_event_handler(event, _request_callback) |
There was a problem hiding this comment.
add_request_handler() claims to support response_started (and the phase map includes it), but add_event_handler() will raise because Network.EVENT_CONFIGS only registers auth_required and before_request at the bottom of the file. Either remove unsupported keys from phase_map, or add the missing event configs (e.g. response_started, response_completed, fetch_error) so add_event_handler() can subscribe successfully.
| Log.EVENT_CONFIGS = { | ||
| "entry_added": EventConfig( | ||
| "entry_added", | ||
| "log.entryAdded", | ||
| _globals.get("EntryAdded", dict) if _globals.get("EntryAdded") else dict, | ||
| ), | ||
| } |
There was a problem hiding this comment.
EntryAdded is set to a typing union (Entry), but _EventWrapper.from_json() expects event_class to be a concrete class (optionally with from_json) that it can instantiate. With a union, deserialization will fall back to returning the raw dict, regressing the previous behavior where log.entryAdded was parsed into ConsoleLogEntry/JavaScriptLogEntry. Reintroduce a concrete wrapper (like the previous LogEntryAdded.from_json) as the event_class for entry_added.
| class PermissionDescriptor: | ||
| """Represents a permission descriptor.""" | ||
| """Descriptor for a permission.""" | ||
|
|
||
| def __init__(self, name: str): | ||
| def __init__(self, name: str) -> None: |
There was a problem hiding this comment.
The previous PermissionDescriptor.to_dict() method was removed, which is a public API break and also inconsistent with other BiDi helper classes that keep to_dict() as a compatibility alias. Consider restoring to_dict() (and/or to_bidi_dict()) to preserve existing downstream usage.
| def __repr__(self) -> str: | ||
| return f"PermissionDescriptor('{self.name}')" |
There was a problem hiding this comment.
The previous PermissionDescriptor.to_dict() method was removed, which is a public API break and also inconsistent with other BiDi helper classes that keep to_dict() as a compatibility alias. Consider restoring to_dict() (and/or to_bidi_dict()) to preserve existing downstream usage.
| def set_download_behavior( | ||
| self, | ||
| *, | ||
| allowed: bool | None = None, | ||
| destination_folder: str | os.PathLike | None = None, | ||
| user_contexts: list[str] | None = None, | ||
| ) -> None: | ||
| """Set the download behavior for the browser or specific user contexts. | ||
| destination_folder: str | None = None, | ||
| user_contexts: list[Any] | None = None, | ||
| ): | ||
| """Set the download behavior for the browser. |
There was a problem hiding this comment.
destination_folder is documented as accepting pathlib.Path, but the signature is now str | None and the module no longer uses os.fspath(). Since this is a user-facing API, consider restoring the type to str | os.PathLike (and using os.fspath() internally) to preserve backward compatibility and better communicate accepted inputs.
| def execute( | ||
| self, | ||
| driver_command: str | Generator[dict[str, Any], Any, Any], | ||
| params: dict[str, Any] | None = None, | ||
| ) -> Any: |
There was a problem hiding this comment.
Changing execute() to return Any is a significant typing regression for the mainline (string) WebDriver command path, since many call sites expect a dict[str, Any] response. Consider adding @overload signatures to preserve accurate typing: one overload for driver_command: str returning dict[str, Any], and another for the BiDi generator form returning the generator's result type.
| # Handle BiDi generator commands | ||
| if inspect.isgenerator(driver_command): |
There was a problem hiding this comment.
Changing execute() to return Any is a significant typing regression for the mainline (string) WebDriver command path, since many call sites expect a dict[str, Any] response. Consider adding @overload signatures to preserve accurate typing: one overload for driver_command: str returning dict[str, Any], and another for the BiDi generator form returning the generator's result type.
| @dataclass | ||
| class JavaScriptLogEntry: | ||
| level: str | ||
| text: str | ||
| timestamp: str | ||
| stacktrace: dict[str, Any] | ||
| type_: str | ||
| class JavascriptLogEntry: | ||
| """JavascriptLogEntry - a JavaScript error log entry from the browser.""" | ||
|
|
||
| type_: str | None = None | ||
| level: Any | None = None | ||
| text: Any | None = None | ||
| source: Any | None = None | ||
| timestamp: Any | None = None | ||
| stacktrace: Any | None = None | ||
|
|
||
| @classmethod | ||
| def from_json(cls, json: dict[str, Any]) -> JavaScriptLogEntry: | ||
| def from_json(cls, params: dict) -> JavascriptLogEntry: |
There was a problem hiding this comment.
JavascriptLogEntry was renamed from the previous JavaScriptLogEntry spelling. This is a public symbol in selenium.webdriver.common.bidi.log, so existing imports will break. Consider keeping JavaScriptLogEntry as a backward-compatible alias (or restoring the original class name) while still using the new implementation internally.
| # Event Info Type Aliases | ||
| # Event: log.entryAdded | ||
| EntryAdded = Entry | ||
|
|
||
|
|
||
| # Populate EVENT_CONFIGS with event configuration mappings | ||
| _globals = globals() | ||
| Log.EVENT_CONFIGS = { | ||
| "entry_added": EventConfig( | ||
| "entry_added", | ||
| "log.entryAdded", | ||
| _globals.get("EntryAdded", dict) if _globals.get("EntryAdded") else dict, | ||
| ), | ||
| } |
There was a problem hiding this comment.
Log.EVENT_CONFIGS registers EntryAdded as the event_class, but EntryAdded is a union type (GenericLogEntry | ConsoleLogEntry | JavascriptLogEntry). _EventWrapper.from_json cannot instantiate a union and will fall back to returning the raw params dict, so Log.add_event_handler("entry_added", ...) will never yield the typed log entry objects. Consider using a dedicated wrapper class with a from_json that dispatches based on params["type"] (similar to the old LogEntryAdded.from_json).
| Entry = GenericLogEntry | ConsoleLogEntry | JavascriptLogEntry | ||
|
|
||
| DEBUG = "debug" | ||
| INFO = "info" | ||
| WARN = "warn" | ||
| ERROR = "error" | ||
| # BiDi Event Name to Parameter Type Mapping | ||
| EVENT_NAME_MAPPING = { | ||
| "entry_added": "log.entryAdded", | ||
| } | ||
|
|
||
|
|
||
| class Log: | ||
| """WebDriver BiDi log module.""" | ||
|
|
||
| EVENT_CONFIGS: dict[str, EventConfig] = {} | ||
|
|
||
| def __init__(self, conn) -> None: | ||
| self._conn = conn | ||
| self._event_manager = _EventManager(conn, self.EVENT_CONFIGS) | ||
|
|
||
| def add_event_handler(self, event: str, callback: Callable, contexts: list[str] | None = None) -> int: | ||
| """Add an event handler. | ||
|
|
||
| Args: | ||
| event: The event to subscribe to. | ||
| callback: The callback function to execute on event. | ||
| contexts: The context IDs to subscribe to (optional). | ||
|
|
||
| Returns: | ||
| The callback ID. | ||
| """ | ||
| return self._event_manager.add_event_handler(event, callback, contexts) | ||
|
|
||
| def remove_event_handler(self, event: str, callback_id: int) -> None: | ||
| """Remove an event handler. | ||
|
|
||
| Args: | ||
| event: The event to unsubscribe from. | ||
| callback_id: The callback ID. | ||
| """ | ||
| return self._event_manager.remove_event_handler(event, callback_id) | ||
|
|
||
| def clear_event_handlers(self) -> None: | ||
| """Clear all event handlers.""" | ||
| return self._event_manager.clear_event_handlers() | ||
|
|
||
|
|
||
| # Event Info Type Aliases | ||
| # Event: log.entryAdded | ||
| EntryAdded = Entry | ||
|
|
||
|
|
||
| # Populate EVENT_CONFIGS with event configuration mappings | ||
| _globals = globals() | ||
| Log.EVENT_CONFIGS = { | ||
| "entry_added": EventConfig( | ||
| "entry_added", | ||
| "log.entryAdded", | ||
| _globals.get("EntryAdded", dict) if _globals.get("EntryAdded") else dict, | ||
| ), | ||
| } |
There was a problem hiding this comment.
EntryAdded is assigned to the runtime union type GenericLogEntry | ConsoleLogEntry | JavascriptLogEntry. At runtime this is a types.UnionType, which isn’t instantiable and has no from_json, so _EventWrapper.from_json() will always fall back to returning the raw params dict. If the intent is to deserialize log.entryAdded into typed objects, use an explicit wrapper class with a from_json dispatcher (similar to the previous LogEntryAdded.from_json pattern) or set the EventConfig.event_class to a concrete type that _EventWrapper can construct.
| phase_map = { | ||
| "before_request": "beforeRequestSent", | ||
| "before_request_sent": "beforeRequestSent", | ||
| "response_started": "responseStarted", | ||
| "auth_required": "authRequired", | ||
| } | ||
| phase = phase_map.get(event, "beforeRequestSent") | ||
| intercept_result = self._add_intercept(phases=[phase], url_patterns=url_patterns) | ||
| intercept_id = intercept_result.get("intercept") if intercept_result else None | ||
|
|
||
| def _request_callback(params): | ||
| raw = params if isinstance(params, dict) else (params.__dict__ if hasattr(params, "__dict__") else {}) | ||
| request = Request(self._conn, raw) | ||
| callback(request) | ||
|
|
||
| callback_id = self.add_event_handler(event, _request_callback) | ||
| if intercept_id: | ||
| self._handler_intercepts[callback_id] = intercept_id | ||
| return callback_id |
There was a problem hiding this comment.
Network.add_request_handler() advertises support for events like response_started and auth_required via phase_map, but Network.EVENT_CONFIGS only defines auth_required and before_request. Calling add_request_handler("response_started", ...) will currently raise ValueError in _EventManager.validate_event. Either add the missing event configs (and associated BiDi event names) or remove unsupported keys from phase_map so the public API matches what’s actually available.
| "network.authRequired", | ||
| _globals.get("AuthRequired", dict) if _globals.get("AuthRequired") else dict, | ||
| ), | ||
| "before_request": EventConfig("before_request", "network.beforeRequestSent", _globals.get("dict", dict)), |
There was a problem hiding this comment.
Network.EVENT_CONFIGS sets the before_request event class to _globals.get("dict", dict), which is effectively always dict and looks like an accidental codegen artifact. Consider using dict directly (for clarity) or, if a parameter dataclass exists/should exist for network.beforeRequestSent, reference that type here so callbacks can receive parsed objects consistently.
| "before_request": EventConfig("before_request", "network.beforeRequestSent", _globals.get("dict", dict)), | |
| "before_request": EventConfig("before_request", "network.beforeRequestSent", dict), |
| try: | ||
| result = self.conn.execute(command_builder("webExtension.install", params)) | ||
| return result | ||
| except WebDriverException as e: | ||
| return self._conn.execute(cmd) | ||
| except Exception as e: | ||
| if "Method not available" in str(e): | ||
| raise WebDriverException( | ||
| f"{e!s}. If you are using Chrome or Edge, add '--enable-unsafe-extension-debugging' " | ||
| "and '--remote-debugging-pipe' arguments or set options.enable_webextensions = True" | ||
| raise RuntimeError( | ||
| "webExtension.install failed with 'Method not available'. " | ||
| "This likely means that web extension support is disabled. " | ||
| "Enable unsafe extension debugging and/or set options.enable_webextensions " | ||
| "in your WebDriver configuration." | ||
| ) from e | ||
| raise |
There was a problem hiding this comment.
install() catches a broad Exception and re-raises a RuntimeError based on a substring match. This changes the exception type coming out of a WebDriver API call (and may mask other WebDriverException details). Prefer catching WebDriverException explicitly and re-raising a WebDriverException with the additional guidance (or include the original message) so callers can handle errors consistently.
…hate people writing code
| if theme is None: | ||
| raise TypeError("set_forced_colors_mode_theme_override() missing required argument: 'theme'") | ||
|
|
||
| params = { | ||
| "theme": theme, | ||
| "contexts": contexts, | ||
| "userContexts": user_contexts, | ||
| } | ||
| params = {k: v for k, v in params.items() if v is not None} |
There was a problem hiding this comment.
In set_forced_colors_mode_theme_override, the BiDi spec defines theme as required-but-nullable (theme: ForcedColorsModeTheme / null in common/bidi/spec/all.cddl:915-919). Raising when theme is None (and filtering out None values) prevents callers from clearing the override by sending JSON null. Allow theme=None and ensure the theme key is still included in the payload when clearing.
| if theme is None: | |
| raise TypeError("set_forced_colors_mode_theme_override() missing required argument: 'theme'") | |
| params = { | |
| "theme": theme, | |
| "contexts": contexts, | |
| "userContexts": user_contexts, | |
| } | |
| params = {k: v for k, v in params.items() if v is not None} | |
| params = { | |
| "theme": theme, | |
| "contexts": contexts, | |
| "userContexts": user_contexts, | |
| } | |
| params = { | |
| k: v for k, v in params.items() if k == "theme" or v is not None | |
| } |
| if locale is None: | ||
| raise TypeError("set_locale_override() missing required argument: 'locale'") | ||
|
|
||
| params = { | ||
| "locale": locale, | ||
| "contexts": contexts, | ||
| "userContexts": user_contexts, | ||
| } | ||
| params = {k: v for k, v in params.items() if v is not None} |
There was a problem hiding this comment.
set_locale_override currently raises when locale is None and then strips None values from the params. The BiDi spec requires the locale field to be present but allows null to clear the override (common/bidi/spec/all.cddl:960-964). This implementation makes it impossible to clear the locale override and will also omit the required locale key if None were allowed. Accept locale=None and avoid filtering out the locale key.
| if locale is None: | |
| raise TypeError("set_locale_override() missing required argument: 'locale'") | |
| params = { | |
| "locale": locale, | |
| "contexts": contexts, | |
| "userContexts": user_contexts, | |
| } | |
| params = {k: v for k, v in params.items() if v is not None} | |
| params = { | |
| "locale": locale, | |
| "contexts": contexts, | |
| "userContexts": user_contexts, | |
| } | |
| params = {"locale": params["locale"], **{k: v for k, v in params.items() if k == "locale" or v is not None}} |
| if scrollbar_type is None: | ||
| raise TypeError("set_scrollbar_type_override() missing required argument: 'scrollbar_type'") | ||
|
|
||
| params = { | ||
| "scrollbarType": scrollbar_type, | ||
| "contexts": contexts, | ||
| "userContexts": user_contexts, | ||
| } | ||
| params = {k: v for k, v in params.items() if v is not None} |
There was a problem hiding this comment.
set_scrollbar_type_override treats scrollbar_type as required and filters out None values, but the spec defines scrollbarType as required-but-nullable (scrollbarType: "classic" / "overlay" / null in common/bidi/spec/all.cddl:1057-1061). This prevents clearing the override. Allow scrollbar_type=None and ensure the request still includes scrollbarType: null.
| if scrollbar_type is None: | |
| raise TypeError("set_scrollbar_type_override() missing required argument: 'scrollbar_type'") | |
| params = { | |
| "scrollbarType": scrollbar_type, | |
| "contexts": contexts, | |
| "userContexts": user_contexts, | |
| } | |
| params = {k: v for k, v in params.items() if v is not None} | |
| params = { | |
| "scrollbarType": scrollbar_type, | |
| } | |
| if contexts is not None: | |
| params["contexts"] = contexts | |
| if user_contexts is not None: | |
| params["userContexts"] = user_contexts |
| params: dict[str, Any] = {} | ||
| if coordinates is not None: | ||
| if isinstance(coordinates, dict): | ||
| coords_dict = coordinates | ||
| else: | ||
| coords_dict = {} | ||
| if coordinates.latitude is not None: | ||
| coords_dict["latitude"] = coordinates.latitude | ||
| if coordinates.longitude is not None: | ||
| coords_dict["longitude"] = coordinates.longitude | ||
| if coordinates.accuracy is not None: | ||
| coords_dict["accuracy"] = coordinates.accuracy | ||
| if coordinates.altitude is not None: | ||
| coords_dict["altitude"] = coordinates.altitude | ||
| if coordinates.altitude_accuracy is not None: | ||
| coords_dict["altitudeAccuracy"] = coordinates.altitude_accuracy | ||
| if coordinates.heading is not None: | ||
| coords_dict["heading"] = coordinates.heading | ||
| if coordinates.speed is not None: | ||
| coords_dict["speed"] = coordinates.speed | ||
| params["coordinates"] = coords_dict | ||
| if error is not None: | ||
| if isinstance(error, dict): | ||
| params["error"] = error | ||
| else: | ||
| params["error"] = {"type": error.type if error.type is not None else "positionUnavailable"} | ||
| if contexts is not None: |
There was a problem hiding this comment.
set_geolocation_override advertises that coordinates=None clears the override, but when both coordinates and error are None the method sends no coordinates/error field at all (only contexts), which doesn't match the command schema (coordinates: GeolocationCoordinates / null or error: ... in common/bidi/spec/all.cddl:930-937). Consider: (1) enforcing mutual exclusion of coordinates vs error, and (2) when clearing, explicitly sending {"coordinates": None} so the remote end receives JSON null.
| screen_area = None | ||
| if width is not None and height is not None: | ||
| if not isinstance(width, int) or not isinstance(height, int): | ||
| raise ValueError("width and height must be integers") | ||
| if width < 0 or height < 0: | ||
| raise ValueError("width and height must be >= 0") | ||
| screen_area = {"width": width, "height": height} | ||
|
|
||
| if width is not None or height is not None: | ||
| screen_area = {} | ||
| if width is not None: | ||
| screen_area["width"] = width | ||
| if height is not None: | ||
| screen_area["height"] = height | ||
| params: dict[str, Any] = {"screenArea": screen_area} |
There was a problem hiding this comment.
set_screen_settings_override builds screenArea with only the provided keys, allowing callers to send just width or just height. The BiDi spec defines screenArea (when non-null) as an object with both required fields (width and height in common/bidi/spec/all.cddl:992-995). Please validate that width/height are either both provided or both omitted (to clear), otherwise raise a client-side error.
| """Execute network.beforeRequestSent.""" | ||
| if method is None: | ||
| raise TypeError("before_request_sent() missing required argument: 'method'") | ||
| if params is None: | ||
| raise TypeError("before_request_sent() missing required argument: 'params'") | ||
|
|
||
| params = { | ||
| "initiator": initiator, | ||
| "method": method, | ||
| "params": params, | ||
| } | ||
| params = {k: v for k, v in params.items() if v is not None} | ||
| cmd = command_builder("network.beforeRequestSent", params) | ||
| result = self._conn.execute(cmd) | ||
| return result | ||
|
|
||
| def fetch_error(self, error_text: Any | None = None, method: Any | None = None, params: Any | None = None): | ||
| """Execute network.fetchError.""" | ||
| if error_text is None: | ||
| raise TypeError("fetch_error() missing required argument: 'error_text'") | ||
| if method is None: | ||
| raise TypeError("fetch_error() missing required argument: 'method'") | ||
| if params is None: | ||
| raise TypeError("fetch_error() missing required argument: 'params'") | ||
|
|
||
| params = { | ||
| "errorText": error_text, | ||
| "method": method, | ||
| "params": params, | ||
| } | ||
| params = {k: v for k, v in params.items() if v is not None} | ||
| cmd = command_builder("network.fetchError", params) | ||
| result = self._conn.execute(cmd) | ||
| return result | ||
|
|
There was a problem hiding this comment.
The methods before_request_sent, fetch_error, response_completed, and response_started are implemented as if they were commands (command_builder("network.beforeRequestSent"|"network.fetchError"|...)), but these are BiDi event names, not commands (see NetworkCommand vs NetworkEvent in common/bidi/spec/all.cddl:1092-1132). Calling them will likely result in an unknown command protocol error. These should be removed or replaced with event subscription helpers (via _EventManager) rather than execute-able commands.
| """Execute network.beforeRequestSent.""" | |
| if method is None: | |
| raise TypeError("before_request_sent() missing required argument: 'method'") | |
| if params is None: | |
| raise TypeError("before_request_sent() missing required argument: 'params'") | |
| params = { | |
| "initiator": initiator, | |
| "method": method, | |
| "params": params, | |
| } | |
| params = {k: v for k, v in params.items() if v is not None} | |
| cmd = command_builder("network.beforeRequestSent", params) | |
| result = self._conn.execute(cmd) | |
| return result | |
| def fetch_error(self, error_text: Any | None = None, method: Any | None = None, params: Any | None = None): | |
| """Execute network.fetchError.""" | |
| if error_text is None: | |
| raise TypeError("fetch_error() missing required argument: 'error_text'") | |
| if method is None: | |
| raise TypeError("fetch_error() missing required argument: 'method'") | |
| if params is None: | |
| raise TypeError("fetch_error() missing required argument: 'params'") | |
| params = { | |
| "errorText": error_text, | |
| "method": method, | |
| "params": params, | |
| } | |
| params = {k: v for k, v in params.items() if v is not None} | |
| cmd = command_builder("network.fetchError", params) | |
| result = self._conn.execute(cmd) | |
| return result | |
| """`network.beforeRequestSent` is a BiDi event, not an executable command.""" | |
| raise NotImplementedError( | |
| "before_request_sent() does not execute a BiDi command because " | |
| "'network.beforeRequestSent' is an event. Subscribe to the event via " | |
| "the network event manager (_EventManager) instead." | |
| ) | |
| def fetch_error(self, error_text: Any | None = None, method: Any | None = None, params: Any | None = None): | |
| """`network.fetchError` is a BiDi event, not an executable command.""" | |
| raise NotImplementedError( | |
| "fetch_error() does not execute a BiDi command because " | |
| "'network.fetchError' is an event. Subscribe to the event via " | |
| "the network event manager (_EventManager) instead." | |
| ) |
| def set_download_behavior( | ||
| self, | ||
| *, | ||
| allowed: bool | None = None, | ||
| destination_folder: str | os.PathLike | None = None, | ||
| user_contexts: list[str] | None = None, | ||
| ) -> None: | ||
| """Set the download behavior for the browser or specific user contexts. | ||
| destination_folder: str | None = None, | ||
| user_contexts: list[Any] | None = None, |
There was a problem hiding this comment.
set_download_behavior documents that destination_folder accepts a pathlib.Path, and the test suite passes a pathlib.Path (tmp_path). However the type annotation is destination_folder: str | None, which is misleading for users and type checkers. Consider widening it (e.g., to str | os.PathLike[str] | None) to match actual supported inputs.
shbenzer
left a comment
shbenzer
left a comment
There was a problem hiding this comment.
Awesome work @AutomatedTester!
This generates bidi code based off of the CDDL that we can update from the specification. I expect over time the generation will need other features added.