Repository navigation
[py] Align driver.script handlers with script ADR - #18270
AutomatedTester wants to merge 4 commits into
Conversation
Bring the Python driver.script event handlers in line with the script and logging ADR (#17776): - add_*_handler methods return a Registration, an int subclass, so existing code that stores the id keeps working. - execute() takes window_handle=; context_id= still works but is deprecated. - One set of handler names: add_console_message_handler and add_javascript_error_handler now deliver ConsoleMessage and ScriptError, which name the window handle the event came from. add_console_handler, add_error_handler and their removers are deprecated in favour of them. - Payload fields outside the ADR shape (source, line and column numbers, stack_trace, type_, and text/level on errors) still read as before but raise a DeprecationWarning. ScriptError.stacktrace is a StackTrace string that still answers ["callFrames"] with a warning.
|
/agentic_describe |
|
/agentic_review |
PR Summary by QodoAlign Python driver.script handlers with the script ADR
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1.
|
Review on #18270 pointed out that switching the handler payloads to plain classes broke code that checks isinstance(entry, ConsoleLogEntry) or uses dataclass helpers, and that a str stack trace broke dict use. - ConsoleMessage and ScriptError are dataclasses again, with window_handle (and type on ConsoleMessage) added. - add_console_message_handler and add_javascript_error_handler deliver subclasses of both the ADR payload and the generated ConsoleLogEntry / JavascriptLogEntry, so the old fields, isinstance checks and dataclass helpers keep working. - StackTrace is the protocol mapping (a dict subclass); str() gives the formatted frames and .frames lists them.
|
/agentic_review |
|
Thanks. Going through the five findings: 1, 2 and 5 (payload type, raw stack map, dataclass helpers): fixed in 85b17ea. 3 (conflicting window arguments): not changing this. 4 (iframe context id): real. Generated by Claude Code |
|
Code review by qodo was updated up to the latest commit 85b17ea |
A log entry names the browsing context of the document that logged it, which for an iframe is the frame's context, not a window handle. ConsoleMessage.window_handle and ScriptError.window_handle now resolve that context to its top-level window by walking parents with browsingContext.getTree on first sight and caching the answer (context ids are never reused). A new frame field carries the iframe's context when the event did not come from the window's own document. Lookups run on the event callback's thread, never on the websocket reader thread, so they cannot block command replies.
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit afe6d16 |
Each event callback runs on its own thread, so a burst of log entries from a context not yet cached could each send their own browsingContext.getTree. Serialise cache misses so the first lookup fills the cache and the rest read it.
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 9061956 |
Requested by David · project thread
🔗 Related Issues
Implements part of the script and logging ADR #17776 (still Proposed) for Python. Follows #18264.
💥 What does this PR do?
Fixes three gaps between
driver.scriptand the ADR:add_console_message_handler,add_javascript_error_handlerandadd_dom_mutation_handlerreturn aRegistration. It's anintsubclass, the same trickPinnedScript(str)uses, so code that stores or compares the id keeps working. The removers take it back.window_handle=instead ofcontext_id=(decision 8).execute()takeswindow_handle=.context_id=still works but raises aDeprecationWarning, and passing both raisesTypeError.add_console_message_handlerandadd_javascript_error_handler. They now deliver one shaped payload each,ConsoleMessageandScriptError, and each payload names thewindow_handleit came from.add_console_handler,add_error_handlerand their removers are deprecated in favour of the ADR names.🔧 Implementation Notes
The two old name sets delivered different payloads. This PR merges them without breaking anyone:
ConsoleMessageandScriptErrorstay dataclasses and gainwindow_handle(plustypeonConsoleMessage). The fields the ADR doesn't name are documented as deprecated:source,line_number,column_numberandstack_trace.add_console_message_handlerandadd_javascript_error_handlerdeliver subclasses of both the ADR payload and the generatedConsoleLogEntry/JavascriptLogEntry.isinstancechecks, dataclass helpers and the old field values (type_, the{realm, context}sourcedict,text/levelon errors) all keep working.ScriptError.stacktraceis aStackTrace. It's adictsubclass holding the protocol stack trace, so existing["callFrames"]andjson.dumpscode still works.str()gives the formatted frames and.frameslists them.common.bidi.scriptmodule is unchanged.driver.scriptpasses its own payload builders toLogHandlerRegistry.add_handler(payload=...).🤖 AI assistance
💡 Additional Considerations
browsingContext.getTreethe first time it sees a context, and caches the answer because context ids are never reused.framecarries the iframe's id.window_handleisNoneonly if the context is gone before the lookup. Lookups run on the event callback's thread, never on the websocket reader thread.Registration) and with the BiDiscript,log,quitandintegrationbrowser tests on Chromium 1243, by overlaying these files on the 4.52 wheel. That run passed: 114 tests, including a new iframe console test. Bazel was not run, and Firefox was not tested.pindon't takewindow_handle=yet (decision 8).DomMutationdoesn't carrywindow_handleyet.ConsoleMessage.argsare still protocol remote values.add_event_handlerand sandbox (decisions 4 and 9) aren't implemented.execute()signature to accept aWindowforcontext_id. Whichever PR lands second needs to move that ontowindow_handle.🔄 Types of changes
Generated by Claude Code