From d64fe70a0ba85c30c256f71e10d0b81cc7c9db83 Mon Sep 17 00:00:00 2001 From: Tatu Aalto Date: Sat, 17 Oct 2026 20:46:47 +0300 Subject: [PATCH] refactor: resolve the video size of a context in one place _record_video already resolves recordVideo's size from the size given, the viewport or 1280x720. New Context, New Persistent Context and register_adopted_context worked it out again with _video_size for the context cache, each in its own way. They now cache the resolved size, and a context without a video caches none, since only an embedded video reads it. The video dict that Node returns and _embed_video reads is typed as ContextVideo, and _video_size and _record_video take RecordVideo and ViewportDimensions instead of mappings. Co-Authored-By: Claude Opus 5.5 --- Browser/keywords/playwright_state.py | 45 +++++++++++----------------- Browser/utils/data_types.py | 10 +++++++ utest/test_adopted_context.py | 13 ++++++++ utest/test_get_video_size.py | 4 +-- 4 files changed, 43 insertions(+), 29 deletions(-) diff --git a/Browser/keywords/playwright_state.py b/Browser/keywords/playwright_state.py index beef0cbce..11f26d1b0 100755 --- a/Browser/keywords/playwright_state.py +++ b/Browser/keywords/playwright_state.py @@ -14,7 +14,6 @@ import json import os -from collections.abc import Mapping from contextlib import suppress from copy import copy from datetime import datetime, timedelta, timezone @@ -27,7 +26,7 @@ from robot.utils import get_link_path from robot.utils.robottypes import is_truthy -from Browser.utils.data_types import BrowserInfo, TracingGroupMode +from Browser.utils.data_types import BrowserInfo, ContextVideo, TracingGroupMode from Browser.utils.misc import get_download_id from ..assertion_engine import assertion_formatter_used, with_assertion_polling @@ -849,11 +848,7 @@ def new_context( "No browser was open. New browser was automatically opened " "when this context is created." ) - self._register_context( - response.id, - trace_file, - self._video_size(params.get("recordVideo"), params.get("viewport")), - ) + self._register_context(response.id, trace_file, params.get("recordVideo")) return response.id @keyword() @@ -998,9 +993,7 @@ def new_persistent_context( logger.debug("Teardown: Closed new persistent context.") raise e video_path = self._cache_context( - response.id, - self._video_size(params.get("recordVideo"), params.get("viewport")), - json.loads(response.video), + response.id, params.get("recordVideo"), json.loads(response.video) ) return ( browser_id, @@ -1029,16 +1022,13 @@ def register_adopted_context( video_path: str | None = None, ) -> str: record_video = options["recordVideo"] - video = ( + video: ContextVideo | None = ( {"video_path": video_path, "contextUuid": context_id} if record_video is not None else None ) return self._register_context( - context_id, - options["tracing"] or "", - self._video_size(record_video, None), - video, + context_id, options["tracing"] or "", record_video, video ) def _resolve_trace_file(self, tracing: bool | Path | None) -> str: @@ -1142,25 +1132,26 @@ def _register_context( self, context_id: str, trace_file: str, - video_size: ViewportDimensions, - video: dict | None = None, + record_video: RecordVideo | None, + video: ContextVideo | None = None, ) -> str: self.add_context_and_keyword_call_stack_to_trace(trace_file, context_id) - return self._cache_context(context_id, video_size, video) + return self._cache_context(context_id, record_video, video) def _cache_context( self, context_id: str, - video_size: ViewportDimensions, - video: dict | None = None, + record_video: RecordVideo | None, + video: ContextVideo | None = None, ) -> str: - self.context_cache.add(context_id, video_size) + if record_video is not None: + self.context_cache.add(context_id, record_video["size"]) return self._embed_video(video) if video else "" def _record_video( self, record_video: RecordVideo | None, - viewport: Mapping[str, Any] | None, + viewport: ViewportDimensions | None, ) -> RecordVideo | None: if record_video is None: return None @@ -1179,11 +1170,11 @@ def _record_video( def _video_size( self, - record_video: Mapping[str, Any] | None, - viewport: Mapping[str, Any] | None, + record_video: RecordVideo, + viewport: ViewportDimensions | None, ) -> ViewportDimensions: - size = (record_video or {}).get("size") or {} - if size.get("width") and size.get("height"): + size = record_video.get("size") + if size and size.get("width") and size.get("height"): return {"width": int(size["width"]), "height": int(size["height"])} if viewport: return {"width": viewport["width"], "height": viewport["height"]} @@ -1278,7 +1269,7 @@ def _remove_failed_page(self, token: str): response = stub.RemoveFailedPage(Request().FailedPage(token=token)) logger.info(response.log) - def _embed_video(self, video: dict) -> str: + def _embed_video(self, video: ContextVideo) -> str: if not video.get("video_path"): logger.debug("Video is not enabled.") return "" diff --git a/Browser/utils/data_types.py b/Browser/utils/data_types.py index d3afa38c7..ac643e6a9 100644 --- a/Browser/utils/data_types.py +++ b/Browser/utils/data_types.py @@ -405,6 +405,16 @@ class AriaSnapshotReturnType(Enum): parsed = auto() +class ContextVideo(TypedDict): + """The video of a context as the Node side reports it. + + ``video_path`` is ``None`` when the context records no video. + """ + + video_path: str | None + contextUuid: str + + class KeywordCallStackEntry(TypedDict): """Information about the keyword call stack. diff --git a/utest/test_adopted_context.py b/utest/test_adopted_context.py index 7dc1706e4..e9d313460 100644 --- a/utest/test_adopted_context.py +++ b/utest/test_adopted_context.py @@ -420,3 +420,16 @@ def test_registered_context_caches_its_video_size( browser.register_adopted_context("context=1", options, "page.webm") assert browser._playwright_state.context_cache.get("context=1") == video_size + + +def test_context_without_video_caches_no_video_size(browser: Browser): + stub = browser._playwright.stub + stub.NewContext.return_value = MagicMock(id="context=1", contextOptions="{}") + stub.NewPersistentContext.return_value.id = "context=2" + + browser.new_context(viewport=VIEWPORT) + browser.new_persistent_context(viewport=VIEWPORT) + browser.register_adopted_context("context=3", browser.adopted_context_options()) + + cache = browser._playwright_state.context_cache + assert [cache.get(f"context={n}") for n in (1, 2, 3)] == [None, None, None] diff --git a/utest/test_get_video_size.py b/utest/test_get_video_size.py index 6432bd956..a93f5c19c 100644 --- a/utest/test_get_video_size.py +++ b/utest/test_get_video_size.py @@ -11,10 +11,10 @@ def state(): def test_get_video_size(state: PlaywrightState): results = [ - state._video_size(None, None), + state._video_size({}, None), state._video_size({"path": "/path/to"}, None), state._video_size({"size": {"width": 12, "height": 11}}, None), - state._video_size(None, {"width": 123, "height": 111}), + state._video_size({}, {"width": 123, "height": 111}), state._video_size( {"size": {"width": 12, "height": 11}}, {"width": 123, "height": 111} ),