Repository navigation
Conversation
… start only at the position the page set
busyWindowProof excused a hold that starts before the block and ends at a
change inside it, as WebKit holds a just-started compositor animation (a
sheet's release) before presenting it. It excused any such hold, even an
animation that was already moving and froze at the block's start: a hold
of 35 to 50 ms from just before the block, then motion, passed. The excuse
now needs `setBeforeBlock`, the value the page itself last set before the
block, and applies only to a hold at that value (within a pixel, the
coarsest cast's half block); without it, the hold is judged as a stall.
The harness's own rAF control ("a rAF one freezes") required at most one
frame of motion after the block began. A loaded runner presents more of
the frames the page drew before the block (two in 1 to 5 runs of 24, pinned
to two cores), so the control failed with nothing wrong. It now checks the
freeze from the page's own writes: no position set while the block ran,
the screen stalled for most of it, and the proof failing it against the
control as before. The hold self-test passes its hold position as the
page's.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: narrowing the hold excuse to setBeforeBlock works and fails closed, but it leaves the excuse's other two rules untested. is not excused when it starts inside the block in busy.control.test.ts still passes no setBeforeBlock. Since this change, that test passes whatever the start rule and the two-frame rule in busy.ts say, and removing both rules keeps every harness unit test green. Passing the last value before the judged part as setBeforeBlock in that test restores its coverage, for example frames.filter(f => f.t < BLOCK.from + 20).at(-1)!.box!.y.
[Automated review]
| // cast holds color in 2 by 2 pixel blocks, so a position reads up to a pixel | ||
| // off. | ||
| const heldFromBeforeBlock = | ||
| setBeforeBlock != null && |
There was a problem hiding this comment.
With this gate, is not excused when it starts inside the block in busy.control.test.ts (which passes no setBeforeBlock) can no longer reach the start rule and the two-frame rule below. Removing both rules leaves every harness unit test passing. Passing frames.filter(f => f.t < BLOCK.from + 20).at(-1)!.box!.y as setBeforeBlock in that test restores the coverage.
| // block still reach the screen as it starts (one or two, more on a loaded | ||
| // runner), and then the compositor sends nothing while nothing changes, | ||
| // so the bound's verdict fails it as the control's does. | ||
| expect(setDuringBlock).toBe(0); |
There was a problem hiding this comment.
Non-blocking: this assertion can't fail while blockMainThread without during spins inside a single page.evaluate, because no rAF callback runs during that loop. The stall and failure-message checks below are what prove the freeze.
… a freeze check that cannot fail With the excuse gated on `setBeforeBlock`, the case for a hold that starts inside the block passed no position and so no longer reached the excuse's start rule or its two-frame rule: removing both left every unit green. It now passes the last position before the judged part, and a new case holds from the frame presented as the block starts, at the position the page set, which only the start rule refuses. Removing the start rule fails one case; removing both rules fails two. The rAF control's count of positions the page set during the block could not fail: the block is one loop in one task, so no frame callback runs in it. The 600 ms stall and the verdict's failures carry the freeze.
|
Thanks, both addressed in f069d11.
|
There was a problem hiding this comment.
Changes requested: the start rule is now pinned and the description matches the head, but the two-frame rule still has no test. Deleting heldFrames >= 2 in busy.ts alone leaves every harness unit passing; judges a gap at the edge of the judged part from the screencast alone in busy.test.ts caught that on the base, and passing setBeforeBlock: frames.filter(f => f.t < BLOCK.from).at(-1)!.box!.y in that case restores the coverage.
[Automated review]
| holdStart > 0 && | ||
| prior[holdStart].t < window.from && | ||
| heldFrames >= 2; | ||
| heldFrames >= 2 && |
There was a problem hiding this comment.
Deleting this line alone leaves every harness unit passing. judges a gap at the edge of the judged part from the screencast alone in busy.test.ts pinned it on the base, but it passes no setBeforeBlock, so since line 640 it never reaches this rule. Passing setBeforeBlock: frames.filter(f => f.t < BLOCK.from).at(-1)!.box!.y in that case restores the coverage.
A case where the position the page set shows on one frame before the block, and the marker is then not found until 150 ms in: only the two-frame rule refuses to excuse it, so removing that rule now fails it.
|
Right, and I was wrong that it could not change a verdict. Fixed in 81a2fae.
|
There was a problem hiding this comment.
No further changes needed: the hold excuse now requires setBeforeBlock, each of its three conditions (the 1 px match, the start rule and the two-frame rule) fails its own unit case when removed, and the changed self-tests pass in Chromium and WebKit. Open pull requests whose proofs rely on the hold excuse will need to pass setBeforeBlock with the release position when they rebase.
[Automated review]
Maintainer impact
Two things in the interaction harness's busy-main-thread proof.
busyWindowProofexcuses a hold that starts before the block and ends at a change inside it: WebKit holds a just-started compositor animation, such as a sheet's release, before presenting it.Intended invariant
Change
busyWindowProoftakessetBeforeBlock: the tracked value the page last set before the block, in the tracked box's coordinates, such as a drag's last position at its release.setBeforeBlock.Evidence
Callers in open pull requests
Only a proof whose animation starts at a release just before its block, and is held at the block's start, used the hold excuse. If one of these now fails with a stall at the block's start, pass
setBeforeBlock(the release position in the tracked box's coordinates) when rebasing.Specs that call
busyWindowProofin open pull requests, with the first pull request that adds each:Scope
Testing
internal/interaction-harness: its unit tests, andharness.interaction.browser.spec.tsin Chromium (playwright.interaction.config.ts).Local gates at 81a2fae:
pnpm lint(0 errors);