feat: make the port of the JS client the only implementation (#3280)

Author: pavelfeldman

.claude/skills/playwright-roll/SKILL.md

@@ -154,29 +154,28 @@ A few rules of thumb that catch most "actually a PORT" cases:
 
 #### PORT
 
-Implement the change in `playwright/_impl/<module>.py`. Use the upstream JS implementation as a reference: `$PW_SRC_DIR/packages/playwright-core/src/client/<module>.ts`. Translate idioms:
+Implement the change in `playwright/_impl/<module>.py`, the line-by-line port of `$PW_SRC_DIR/packages/playwright-core/src/client/<module>.ts` (snake_case: `browserContext.ts` is `browser_context.py`). The client change that implements the new API is usually in the same upstream commit as the docs change, or close to it: port it line by line with the rules of [`transform.md`](../../../transform.md), and walk the whole client range as section 14 of that file says (`git -C "$PW_SRC_DIR" log <prev>..<rev> -- packages/playwright-core/src/client packages/isomorphic packages/protocol`). A few reminders:
 
 | Upstream JS | Python |
 |---|---|
-| `async foo(): Promise<X>` | `async def foo(self) -> X:` |
 | `foo(): X` (sync getter, no args, no body) | `@property def foo(self) -> X:` (the generator makes argument-less sync members properties — see `isPythonProperty` in `scripts/codegen/generate_api.js` — and reads `self._impl_obj.foo` without calling it. mypy does not catch a plain method here; the test does.) |
-| `await this._channel.foo({ a, b }, this._timeout(options))` | `await self._channel.foo(a=a, b=b, timeout=self._timeout(timeout))`. The generated channel method takes every protocol parameter, with no defaults; an absent option is `None`. The implementation method's own parameters use the Python (snake_case) names, with no defaults. |
+| `await this._channel.foo({ a, b }, this._timeout(options))` | `await self._channel.foo(a=a, b=b, timeout=self._timeout(timeout))`. The generated channel method takes every protocol parameter, with no defaults; an absent option is `None`. It sends the message at once and returns an awaitable (`transform.md`, 1.5). The implementation method's own parameters use the Python (snake_case) names, with no defaults. |
 | `(await this._channel.foo()).value` | `(await self._channel.foo())["value"]`. The result dict has every top-level field; a field that the driver left out is `None`. |
 | `this._channel.on('foo', ({ a }) => ...)` | `self._channel.onFoo(lambda event: ...)`; `event["a"]` is `None` when the driver left the optional field out. |
-| `try { ... } catch (e) { if (isTargetClosedError(e)) return; throw e; }` | `try: ...; except Exception as e: if is_target_closed_error(e): return; raise` (import from `playwright._impl._errors`) |
+| `try { ... } catch (e) { if (isTargetClosedError(e)) return; throw e; }` | `try: ...; except Exception as e: if is_target_closed_error(e): return; raise e` (import from `playwright._impl.errors`) |
 | Inline `[Object]` return like `{endpoint: string}` | A `TypedDict` in `playwright/_impl/_api_structures.py`, which the generator writes. It names the struct from the docs: `` "Name" <[Object]> ``, or an `* alias-python:` line under it. See `RemoteAddr`, `BrowserBindResult`, `DebuggerPausedDetails`. |
-| `binary` event/return field | The Python channel layer hands you a base64 string. Decode with `base64.b64decode(value)` before exposing as `bytes`. See `Screencast._dispatch_frame`. |
+| `binary` event/return field | The channel layer hands you a base64 string. Decode with `base64.b64decode(value)` before exposing as `bytes`. See the frame event in `screencast.py`. |
 
 When implementing a new ChannelOwner subclass (one constructed by the protocol with `(parent, type, guid, initializer)`):
-1. Register it in `playwright/_impl/_object_factory.py:create_remote_object` — otherwise the guid resolves to `DummyObject` and downstream code breaks mysteriously.
+1. Register it in `_object_factories()` in `playwright/_impl/connection.py` (the port of the factory table of `connection.ts`); the connection raises for a type it has no factory for.
 2. Add it to `playwright/_impl/_api_classes.py` (the import and `__all__`). The generator emits every Python class in the docs, and the generated API imports each implementation from that file. If the class is missing there, `_generated.py` fails to import.
 
 When implementing a non-ChannelOwner wrapper class (a plain class that holds a Page/Context reference, like `Screencast`, `Clock`):
 - Add it to `playwright/_impl/_api_classes.py`, as for a ChannelOwner subclass.
-- Set `self._loop = parent._loop` and `self._dispatcher_fiber = parent._dispatcher_fiber` in `__init__`. The generated `AsyncBase`/`SyncBase` wrappers read these; missing them gives `AttributeError: 'X' object has no attribute '_loop'` at first use.
+- Set `self._loop = parent._loop` and `self._dispatcher_fiber = parent._dispatcher_fiber` in `__init__`, with the `# Python:` comment that the other wrappers have. The generated `AsyncBase`/`SyncBase` wrappers read these; missing them gives `AttributeError: 'X' object has no attribute '_loop'` at first use. A user callback that the new code calls and awaits runs through `run_in_greenlet()` under the sync API (`transform.md`, section 13).
 
 When the docs add a new struct (an inline `[Object]` with named fields):
-- The generator writes its TypedDict into `_api_structures.py`. Never edit that file by hand; internal types go in `_internal_structures.py`.
+- The generator writes its TypedDict into `_api_structures.py`. Never edit that file by hand; internal types go in `internal_structures.py`.
 - It needs a Python name in the docs: a name before the type (`` - `size` "ScreenSize" <[Object]> ``), or an `* alias-python:` line under the type. The generator fails with "No Python name for the struct at …" until one exists. Nested structs need names too.
 - Re-export it from both `playwright/async_api/__init__.py` and `playwright/sync_api/__init__.py`: assignment line plus an entry in `__all__`. Same pattern as `ViewportSize`, `RemoteAddr`.
 
@@ -227,7 +226,7 @@ The script, for `sync` and then `async`:
 1. runs `node scripts/codegen/generate_api.js --flavor <flavor>` (reads the docs from `$PW_SRC_DIR`) straight into `playwright/<flavor>_api/_generated.py`,
 2. invokes `pre-commit run --files` on the generated file.
 
-It then runs `python scripts/codegen/generate_protocol.py`, which reads `packages/protocol/spec/*.yml` from `$PW_SRC_DIR` and writes the command parameter tables to `playwright/_impl/_protocol.py`, the message types to `playwright/_impl/_protocol_types.py` and the typed channels to `playwright/_impl/_protocol_channels.py`.
+It then runs `python scripts/codegen/generate_protocol.py`, which reads `packages/protocol/spec/*.yml` from `$PW_SRC_DIR` and writes the command parameter tables to `playwright/_impl/_protocol.py`, the message types to `playwright/_impl/protocol_types.py`, the typed channels to `playwright/_impl/protocol_channels.py` and the validator to `playwright/_impl/protocol/validator.py`.
 
 Then run `mypy playwright` and work through the errors in `_generated.py` — each one is a call into `_impl/` that the implementation does not support yet.
 
@@ -260,7 +259,6 @@ Failure modes and fixes:
 | `pre-commit` keeps reformatting `_generated.py` on each run | First run after regen always reformats once; rerun until idle. | `pre-commit run --all-files` to settle. |
 | A kwarg in `_impl/` that the docs don't mention (e.g. legacy name from a doc rename) | Nothing fails: the generator simply never passes it. | If the param is positional with no default, just rename it in `_impl/`. Check `packages/protocol/spec/*.yml` and the server dispatcher — if the wire field renamed too, also update the channel-send dict key. Only use `extraOptions` for genuinely hidden internal kwargs (`default_browser_type`). |
 | `AttributeError: 'X' object has no attribute '_loop'` (or `_dispatcher_fiber`) at first use of a new wrapper class | The non-ChannelOwner wrapper isn't initializing the fields the generated `AsyncBase`/`SyncBase` reads. | In the wrapper's `__init__`, set `self._loop = parent._loop` and `self._dispatcher_fiber = parent._dispatcher_fiber`. |
-| `'NoneType' object has no attribute 'get'` after `send_return_as_dict` | Method's protocol response carries no fields and `send_return_as_dict` returned `None`. | `(result or {}).get(...)`. |
 | A test fails at teardown with `Protocol violations: X.y: params.z: unknown` (or `missing`, or `expected …`) | The client sends a message that the protocol spec does not allow. The driver would drop an unknown parameter without an error. | Fix the send in `_impl/`: rename the key after a wire rename, or remove a client-side option before the send. |
 | Frame/buffer payload arrives as a `str` instead of `bytes` | Protocol `binary` fields cross the wire as base64. | `base64.b64decode(value)` in the impl before exposing. |
 
@@ -279,7 +277,7 @@ python scripts/check_test_registry.py --report
 - **Changed titles**: the checker names the expected name; rename the Python test and port the changed body.
 - **Removed identifiers**: delete the Python test (the checker reports it as not in the registry).
 - **Not ported**: a test that can not exist in Python goes in `tests/upstream_not_ported.txt`, with the reason. The checker fails on a rule that no longer matches a test.
-- Use `from playwright.async_api import …`, **not** `from playwright._impl._page import Page` (the impl class doesn't have the public wrappers like `expect_console_message`).
+- Use `from playwright.async_api import …`, **not** `from playwright._impl.page import Page` (the impl class doesn't have the public wrappers like `expect_console_message`).
 - For event-info objects, `await message_info.value` (it's an `async` property).
 - Don't write tests that hang the page (e.g. `page.evaluate(... fetch slow ...)` followed by `page.close()` from a fixture) — the request task gets a `TargetClosedError`. Use `page.on("event", handler)` to capture state at event time instead.
 - `playwright install chromium` (no `--with-deps`) is sufficient for the test suite under sandbox.
@@ -296,7 +294,7 @@ Check that `$PW_SRC_DIR` is still at the `rev=` commit, with no local changes: t
 
 ```sh
 pre-commit run --all-files
-mypy playwright                 # 2 pre-existing errors in _json_pipe.py and _artifact.py are unrelated
+mypy playwright
 pytest --browser chromium tests/async/<files-you-touched> tests/sync/<files-you-touched>
 ```
 
@@ -321,7 +319,7 @@ All keys use docs names: `Class.member` or `Class.member.arg`, with the upstream
 - **A cluster of rules on the same class is a smell.** If you're about to add five entries for `Foo.*`, you're almost certainly looking at a class that needs to be implemented. Implement the whole thing once and the entries disappear.
 - **Watch for revert pairs in the same range.** 1.59 added and reverted `Browser.isRemote` (#39613 / #39620) inside the same release. Walking chronologically lets you skip the add when you see the revert later.
 - **Watch for rename-revert pairs.** 1.59 had `Locator.normalize` → `Locator.toCode` (#39648) → `Locator.normalize` (#39754). Final state wins; only port the last.
-- **Doc renames almost always include a wire-protocol rename.** Whenever you see `### param: X.y.oldName` → `### param: X.y.newName` in a doc commit, also `git -C "$PW_SRC_DIR" show <sha> -- packages/protocol/spec/` and the corresponding `*Dispatcher.ts` file. If the wire field changed too, the regenerated `_protocol_channels.py` renames the argument of the channel method, and mypy reports each call in `_impl/` that passes the old name.
+- **Doc renames almost always include a wire-protocol rename.** Whenever you see `### param: X.y.oldName` → `### param: X.y.newName` in a doc commit, also `git -C "$PW_SRC_DIR" show <sha> -- packages/protocol/spec/` and the corresponding `*Dispatcher.ts` file. If the wire field changed too, the regenerated `protocol_channels.py` renames the argument of the channel method, and mypy reports each call in `_impl/` that passes the old name.
 - **TypedDicts beat `Dict[str, X]` for any structured return.** When the docs describe a return as `[Object]` with named fields, name the struct for the generator, which writes its `TypedDict` into `_api_structures.py`, and re-export it from both public `__init__.py` files. Zero runtime cost (it's still a `dict`), and the docstrings expand its fields from the docs.
 - **Positional renames are free.** A param with no default before any `*` separator is positional-or-keyword in Python, but realistic call sites pass it positionally. Renaming such a param doesn't break callers.
 - **The "Backport changes" GitHub issue can be misleading.** In the 1.59 roll its checkboxes were all marked `[x]` with annotations like "✅ IMPLEMENTED", but several of those features had not actually been merged into the Python port. Trust the `docs/src/api/` walk over the issue.

.github/workflows/ci.yml

@@ -39,7 +39,7 @@ jobs:
 
   build:
     name: Build
-    timeout-minutes: 45
+    timeout-minutes: 90
     strategy:
       fail-fast: false
       matrix:
@@ -114,52 +114,20 @@ jobs:
       run: pytest tests/test_installation.py --browser=${{ matrix.browser }}
     - name: Test Sync API
       if: matrix.os != 'ubuntu-latest'
-      run: pytest tests/sync --browser=${{ matrix.browser }} --timeout 90
+      run: pytest tests/sync --browser=${{ matrix.browser }} --timeout 90 --timeout-method thread -n auto
     - name: Test Sync API
       if: matrix.os == 'ubuntu-latest'
-      run: xvfb-run pytest tests/sync --browser=${{ matrix.browser }} --timeout 90
+      run: xvfb-run pytest tests/sync --browser=${{ matrix.browser }} --timeout 90 --timeout-method thread -n auto
     - name: Test Async API
       if: matrix.os != 'ubuntu-latest'
-      run: pytest tests/async --browser=${{ matrix.browser }} --timeout 90
+      run: pytest tests/async --browser=${{ matrix.browser }} --timeout 90 --timeout-method thread -n auto
     - name: Test Async API
       if: matrix.os == 'ubuntu-latest'
-      run: xvfb-run pytest tests/async --browser=${{ matrix.browser }} --timeout 90
-
-  build-v2:
-    # playwright/_impl_v2, the line-by-line port of the JS client, behind PW_IMPL=v2.
-    # It runs all of tests/sync and tests/async: the older tests, the ports of the
-    # upstream tests and the Python-only tests of v2.
-    name: Build (v2)
-    timeout-minutes: 45
-    strategy:
-      fail-fast: false
-      matrix:
-        browser: [chromium, firefox, webkit]
-    runs-on: ubuntu-22.04
-    env:
-      PW_IMPL: v2
-    steps:
-    - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
-    - name: Set up Python
-      uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0
-      with:
-        python-version: '3.12'
-    - name: Install dependencies & browsers
-      run: |
-        python -m pip install --upgrade pip
-        pip install -r local-requirements.txt
-        pip install -r requirements.txt
-        pip install -e .
-        python -m build --wheel
-        python -m playwright install --with-deps ${{ matrix.browser }}
-    - name: Test Sync API
-      run: xvfb-run pytest tests/sync --browser=${{ matrix.browser }} --timeout 90 --timeout-method thread -n auto
-    - name: Test Async API
       run: xvfb-run pytest tests/async --browser=${{ matrix.browser }} --timeout 90 --timeout-method thread -n auto
 
   test-stable:
     name: Stable
-    timeout-minutes: 45
+    timeout-minutes: 90
     strategy:
       fail-fast: false
       matrix:
@@ -189,16 +157,16 @@ jobs:
       run: pytest tests/common --browser=chromium --browser-channel=${{ matrix.browser-channel }} --timeout 90
     - name: Test Sync API
       if: matrix.os != 'ubuntu-latest'
-      run: pytest tests/sync --browser=chromium --browser-channel=${{ matrix.browser-channel }} --timeout 90
+      run: pytest tests/sync --browser=chromium --browser-channel=${{ matrix.browser-channel }} --timeout 90 --timeout-method thread -n auto
     - name: Test Sync API
       if: matrix.os == 'ubuntu-latest'
-      run: xvfb-run pytest tests/sync --browser=chromium --browser-channel=${{ matrix.browser-channel }} --timeout 90
+      run: xvfb-run pytest tests/sync --browser=chromium --browser-channel=${{ matrix.browser-channel }} --timeout 90 --timeout-method thread -n auto
     - name: Test Async API
       if: matrix.os != 'ubuntu-latest'
-      run: pytest tests/async --browser=chromium --browser-channel=${{ matrix.browser-channel }} --timeout 90
+      run: pytest tests/async --browser=chromium --browser-channel=${{ matrix.browser-channel }} --timeout 90 --timeout-method thread -n auto
     - name: Test Async API
       if: matrix.os == 'ubuntu-latest'
-      run: xvfb-run pytest tests/async --browser=chromium --browser-channel=${{ matrix.browser-channel }} --timeout 90
+      run: xvfb-run pytest tests/async --browser=chromium --browser-channel=${{ matrix.browser-channel }} --timeout 90 --timeout-method thread -n auto
 
   test_examples:
     name: Examples