Apply 5 patches from Codex Phase A Gate round 2 review against spec
v19.1 (FROZEN at openpencil-docs commit 526791f):
- BLOCK 1: `SharedSkiaContext::new(provider) -> Result<Self>` single-arg
per spec §3.3. Provider owns surface configuration; constructor queries
GL viewport / sample count / stencil bits via glow after make_current
returns (option C — no trait change, no caller-side `SurfaceConfig`).
`dpi` field on `SurfaceConfig` was dead and is dropped.
- BLOCK 2(a): `glow()` returns `Option<&Arc<glow::Context>>` (borrow,
not clone) per spec §3.3. Hot-path callers clone explicitly.
- BLOCK 2(b): mobile `on_pause` drops `glow_handle` alongside surface
per spec §3.4 — backing GL context is invalid once activity backgrounds.
- CONCERN 1: `default_framebuffer_id` is now a required trait method
(no default body); explicit overrides on `GlutinProvider` (0),
`EglPbufferProvider` (0), `EaglProvider` (unimplemented! Step 1f),
`AndroidEglProvider` (0). Forces Step 1f mobile impls to specify the
non-zero CAEAGLLayer-backed FBO rather than silently inheriting 0.
- CONCERN 2: new `tests/resize_smoke.rs` with two raster-backed tests —
grow 400×300→800×600→400×300 paints through `NativeBackend` without
panic; resize span emits on grow / shrink / 0×0 clamp paths.
- NIT: stale "Spec mini-patch pending" comments rewritten to reflect
v19.1 frozen state.
cargo build / test / clippy / fmt all green on macOS local.
Applies Codex Phase A Gate round 1 review (3 BLOCK + 2 CONCERN + 1 NIT)
against the Task 2 SharedSkiaContext + NativeBackend implementation.
BLOCK 1 — `ProviderError::from_msg` `pub(crate)` blocked the Linux EGL
pbuffer test helper from constructing typed provider errors. Promoted
to `pub` so out-of-tree provider impls (test pbuffer, future Step 1f
mobile providers) can produce diagnostically-identical errors.
BLOCK 2 — Linux GPU smoke + chrome-stub-composition tests silently
returned `Ok(())` on EGL pbuffer setup failure, turning acceptance #3 /
#4 into false positives on hosted CI without GPU. Now gated by
`STEP1A_REQUIRE_GPU=1`: real-GPU runners panic on setup failure;
dev / hostless runs surface an explicit `INCONCLUSIVE` marker before
returning. Mirrors the macOS `catch_unwind` skip path in the same file.
BLOCK 3 — `tests/memory_loop.rs` was running 100 cycles against
`SharedSkiaContext::inert_for_test()` (every Option<> field None), so
the RSS budget proved nothing about real allocation lifecycle. Renamed
constructor to `inert_for_lifecycle_test()` (clearer intent) and split
the test into:
- Phase 0 warmup (100 inert + 100 raster) so Skia's lazy
glyph/path/binding caches are populated before measurement;
- Phase 1 lifecycle idempotence (100 inert);
- Phase 2 real-resource cycle: raster surface on macOS / Windows
(winit::EventLoop main-thread-only on macOS; Win Actions runner
has no GPU per spec §8.1), full EGL pbuffer + GL surface on Linux
when `STEP1A_REQUIRE_GPU=1`, raster fallback otherwise.
Budget kept at 5 % per acceptance #6 with a 1.5 MB absolute floor to
absorb macOS sysinfo's coarse RSS sampling jitter on small baselines.
CONCERN 1 — `GlContextProvider` had three non-spec methods (`resize`,
`size`, `default_framebuffer_id`). Audit:
- `resize`: actually used by `SharedSkiaContext::resize` (window /
pbuffer resize → Skia FBO rewrap). KEPT, spec mini-patch
documented in comment, escalation needed for spec v19 → v19.1.
- `default_framebuffer_id`: used by `SharedSkiaContext::new` /
`resize` for the FBO id Skia wraps; iOS EAGL provider (Step 1f)
will need non-zero values. KEPT, same escalation path.
- `size`: unused anywhere. DELETED (YAGNI), along with the unused
`size: (u32, u32)` field on `GlutinProvider` and the iOS / Android
stub impls.
CONCERN 2 — `glow_handle: Option<Arc<glow::Context>>` deviates from
spec v19 lines 120-125 + 191 (`Arc<glow::Context>`). Real lifecycle
needs the handle droppable: teardown releases the loaded function
table, `inert_for_lifecycle_test` has no GL backing, Step 1f Android
`on_pause` must drop alongside the EGL context. KEPT as Option<Arc>,
spec mini-patch documented for v19.1 escalation.
NIT 1 — Removed Task 1 link-check helper `placeholder()`. Task 2's
full re-export chain (`SharedSkiaContext`, `NativeBackend`, …)
already proves shell-core ↔ shell-native linkage; placeholder is
YAGNI now.
Verification (macOS local):
- cargo build -p openpencil-shell-native: clean
- cargo test -p openpencil-shell-native: 12/12 pass (8 binaries)
- cargo clippy -p openpencil-shell-native --tests --all-targets
-- -D warnings: clean
- cargo fmt -p openpencil-shell-native -- --check: clean
- memory_loop stress 8 consecutive runs: 8/8 pass
Drives the three-OS CI matrix verification of the skia-safe + glutin +
glow + winit dep stack per Step 1a spec §7.
- examples/p0_probe.rs: stencil_visibility + readback chain runner (must
own a real OS main thread because winit on macOS rejects
EventLoop::new() from cargo test worker threads).
- tests/p0_probe.rs: subprocess-invoke wrapper, gated
#[ignore = "P0_PROBE_GATE"] so default cargo test stays untouched.
- Cargo.toml: add transient [target.'cfg(not(target_arch = "wasm32"))'.
dev-dependencies] block (skia-safe 0.97 + glutin 0.32.3 + glutin-winit
0.5.0 + glow 0.17.0 + raw-window-handle 0.6.2 + scopeguard 1.2.0 +
winit defaults). Pinned to versions resolved in /tmp/skia-glow-probe.
- .github/workflows/rust-check.yml: install Linux GL prereqs (xvfb,
mesa, libxkbcommon, libwayland) and add a P0-probe-gate step running
cargo test --ignored on each OS (Linux through xvfb-run; Windows
early-returns per spec §8.2 WINDOWS_GPU_DEFERRED_NO_RUNNER).
All three artefacts are TRANSIENT — reverted in a follow-up cleanup
commit after CI is green and the loader-compat notes commit lands.
Task 1 owns the permanent integration.
Apply 5 patches from Codex Phase A Gate round 2 review against spec
v19.1 (FROZEN at openpencil-docs commit 526791f):
- BLOCK 1: `SharedSkiaContext::new(provider) -> Result<Self>` single-arg
per spec §3.3. Provider owns surface configuration; constructor queries
GL viewport / sample count / stencil bits via glow after make_current
returns (option C — no trait change, no caller-side `SurfaceConfig`).
`dpi` field on `SurfaceConfig` was dead and is dropped.
- BLOCK 2(a): `glow()` returns `Option<&Arc<glow::Context>>` (borrow,
not clone) per spec §3.3. Hot-path callers clone explicitly.
- BLOCK 2(b): mobile `on_pause` drops `glow_handle` alongside surface
per spec §3.4 — backing GL context is invalid once activity backgrounds.
- CONCERN 1: `default_framebuffer_id` is now a required trait method
(no default body); explicit overrides on `GlutinProvider` (0),
`EglPbufferProvider` (0), `EaglProvider` (unimplemented! Step 1f),
`AndroidEglProvider` (0). Forces Step 1f mobile impls to specify the
non-zero CAEAGLLayer-backed FBO rather than silently inheriting 0.
- CONCERN 2: new `tests/resize_smoke.rs` with two raster-backed tests —
grow 400×300→800×600→400×300 paints through `NativeBackend` without
panic; resize span emits on grow / shrink / 0×0 clamp paths.
- NIT: stale "Spec mini-patch pending" comments rewritten to reflect
v19.1 frozen state.
cargo build / test / clippy / fmt all green on macOS local.
Applies Codex Phase A Gate round 1 review (3 BLOCK + 2 CONCERN + 1 NIT)
against the Task 2 SharedSkiaContext + NativeBackend implementation.
BLOCK 1 — `ProviderError::from_msg` `pub(crate)` blocked the Linux EGL
pbuffer test helper from constructing typed provider errors. Promoted
to `pub` so out-of-tree provider impls (test pbuffer, future Step 1f
mobile providers) can produce diagnostically-identical errors.
BLOCK 2 — Linux GPU smoke + chrome-stub-composition tests silently
returned `Ok(())` on EGL pbuffer setup failure, turning acceptance #3 /
#4 into false positives on hosted CI without GPU. Now gated by
`STEP1A_REQUIRE_GPU=1`: real-GPU runners panic on setup failure;
dev / hostless runs surface an explicit `INCONCLUSIVE` marker before
returning. Mirrors the macOS `catch_unwind` skip path in the same file.
BLOCK 3 — `tests/memory_loop.rs` was running 100 cycles against
`SharedSkiaContext::inert_for_test()` (every Option<> field None), so
the RSS budget proved nothing about real allocation lifecycle. Renamed
constructor to `inert_for_lifecycle_test()` (clearer intent) and split
the test into:
- Phase 0 warmup (100 inert + 100 raster) so Skia's lazy
glyph/path/binding caches are populated before measurement;
- Phase 1 lifecycle idempotence (100 inert);
- Phase 2 real-resource cycle: raster surface on macOS / Windows
(winit::EventLoop main-thread-only on macOS; Win Actions runner
has no GPU per spec §8.1), full EGL pbuffer + GL surface on Linux
when `STEP1A_REQUIRE_GPU=1`, raster fallback otherwise.
Budget kept at 5 % per acceptance #6 with a 1.5 MB absolute floor to
absorb macOS sysinfo's coarse RSS sampling jitter on small baselines.
CONCERN 1 — `GlContextProvider` had three non-spec methods (`resize`,
`size`, `default_framebuffer_id`). Audit:
- `resize`: actually used by `SharedSkiaContext::resize` (window /
pbuffer resize → Skia FBO rewrap). KEPT, spec mini-patch
documented in comment, escalation needed for spec v19 → v19.1.
- `default_framebuffer_id`: used by `SharedSkiaContext::new` /
`resize` for the FBO id Skia wraps; iOS EAGL provider (Step 1f)
will need non-zero values. KEPT, same escalation path.
- `size`: unused anywhere. DELETED (YAGNI), along with the unused
`size: (u32, u32)` field on `GlutinProvider` and the iOS / Android
stub impls.
CONCERN 2 — `glow_handle: Option<Arc<glow::Context>>` deviates from
spec v19 lines 120-125 + 191 (`Arc<glow::Context>`). Real lifecycle
needs the handle droppable: teardown releases the loaded function
table, `inert_for_lifecycle_test` has no GL backing, Step 1f Android
`on_pause` must drop alongside the EGL context. KEPT as Option<Arc>,
spec mini-patch documented for v19.1 escalation.
NIT 1 — Removed Task 1 link-check helper `placeholder()`. Task 2's
full re-export chain (`SharedSkiaContext`, `NativeBackend`, …)
already proves shell-core ↔ shell-native linkage; placeholder is
YAGNI now.
Verification (macOS local):
- cargo build -p openpencil-shell-native: clean
- cargo test -p openpencil-shell-native: 12/12 pass (8 binaries)
- cargo clippy -p openpencil-shell-native --tests --all-targets
-- -D warnings: clean
- cargo fmt -p openpencil-shell-native -- --check: clean
- memory_loop stress 8 consecutive runs: 8/8 pass
Kayshen-X