Repository navigation
Conversation
|
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🔎 Preview ·
|
![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.
There was a problem hiding this comment.
Fixed: that case now passes the last position before the judged part as setBeforeBlock, so it reaches both rules. At 0d8298e, deleting the start rule fails is not excused when its first held frame is the one at the block start, and deleting the two-frame rule fails is not excused when the position it holds shows on one frame only.
| // 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.
|
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.
There was a problem hiding this comment.
Fixed: is not excused when the position it holds shows on one frame only passes the position the page set, so it reaches this rule. At 0d8298e, deleting the rule fails that case.
|
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]
81a2fae to
b74486e
Compare
There was a problem hiding this comment.
Changes requested: narrowing the hold excuse to setBeforeBlock works and fails closed, but its 1 px match is too tight for WebKit.
- In
busy.ts, a hold must sit within 1 px ofsetBeforeBlock, but WebKit's cast reads positions on a 2 px grid: the hold self-test's animation held at 100.5 px reads 1.5 px off in WebKit and fails as a stall in 3 of 3 runs, so match within one cast step (castStep) and add a half-pixel or odd-offset hold that pins it.
At 99, 100 and 101 px WebKit reads at most 1 px off and passes, Chromium passes every offset, and with the match under 2 px the 100.5 px hold passes in WebKit while a hold with no position still fails.
[Automated review]
| prior[holdStart].t < window.from && | ||
| heldFrames >= 2; | ||
| heldFrames >= 2 && | ||
| Math.abs(lead[lead.length - 1] - setBeforeBlock) <= 1; |
There was a problem hiding this comment.
WebKit's cast puts positions on a 2 px grid, so a held value can read more than 1 px off. This hold self-test's animation, held at 100.5 px with setBeforeBlock set to the rest position plus 100.5, reads 142 against 140.5 in WebKit and fails as a stall in 3 of 3 runs (99 and 101 px read 1 px off and pass). Matching within one cast step (castStep(tracked, axis), 1 px when no frame carries a step) passes it, while a hold with no position still fails.
There was a problem hiding this comment.
Fixed in 0d8298e: the hold matches setBeforeBlock within one cast step (castStep(tracked, axis), 1 px when no frame carries a step). The hold self-test now holds at 100.5 px. It passes 3 of 3 in WebKit with CI's settings, and fails 3 of 3 with the 1 px match. Two unit cases pin both sides: a 2 px cast that reads the hold 1.5 px off is excused, and the same offset on a stepless cast is not.
busyWindowProof excused any hold that starts before the block and ends at a change inside it, as WebKit holds a just-started compositor animation, such as a sheet's release, before presenting it. An animation that was already moving and froze at the block's start passed too: a 35 to 50 ms freeze from just before the block, then motion. The proof now takes setBeforeBlock, the tracked value the page last set before the block, in the tracked box's coordinates. The hold excuse applies only to a hold at that value, within one cast step: WebKit's cast reads positions on a 2 px grid. Without it, the hold is judged as a stall. The at-rest start rule is unchanged. The hold self-test holds at a half pixel and passes that position as the page's, and the README describes setBeforeBlock.
b74486e to
0d8298e
Compare
There was a problem hiding this comment.
No further changes needed: the hold now matches setBeforeBlock within one cast step, so WebKit's 2 px cast reading the self-test's 100.5 px hold 1.5 px off is excused, while a hold with no position or at another position still fails.
- Non-blocking: no unit case refuses a hold at another position on frames that carry a step, the branch every real cast takes, so replacing
castStep(tracked, axis)withInfinitykeeps every unit passing; giving the 6 px-off casestep: {x: 2, y: 2}would pin it.
Open pull requests whose WebKit proofs rely on the hold excuse need to pass setBeforeBlock with the release position when they rebase.
[Automated review]
| it('is not excused when it holds a position the page did not set', () => { | ||
| const proof = busyWindowProof(sheet(), BLOCK, { | ||
| axis: 'y', | ||
| control, | ||
| setBeforeBlock: released + 6, | ||
| }); | ||
| expect(proof.verdict.pass, proof.summary).toBe(false); | ||
| expect(proof.onScreenFromMs).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Non-blocking: this case runs on frames with no step, so it takes the 1 px fallback. Every real cast of the default marker carries a step and takes the castStep branch in busy.ts, where no case refuses a hold at another position: replacing castStep(tracked, axis) with Infinity keeps every unit passing. A stepped copy of this case pins it:
const grid = sheet().map(f => ({...f, step: {x: 2, y: 2}}));
const proof = busyWindowProof(grid, BLOCK, {axis: 'y', control, setBeforeBlock: released + 6});
expect(proof.verdict.pass, proof.summary).toBe(false);
cixzhang
added
the
ci:run
Maintainer impact
The interaction harness's busy-main-thread proof excused too much at the block's start.
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
A hold at the block's start is excused only when it holds the position the page itself last set before the block, the one a late hand-off holds.
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.castStep): WebKit's cast reads positions on a 2 px grid, Chromium's on 1 px. A cast whose frames carry no step is matched within a pixel.setBeforeBlock.Evidence
main'sbusy.ts:mainloses the excuse. A local probe, not part of the change, logged every hold the old rule excused and this one does not. Overmain's callers ofbusyWindowProofit logged none: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 and WebKit (playwright.interaction.config.ts).Local gates at 0d8298e:
--cpus=4,CI=1, 1 worker): 95 passed, 13 Chromium-only skipped;pnpm review:pre: no findings.At the same change without the cast-step match, on the previous
main(62e00cc7da): the self-test's rAF control, unchanged, pinned to two cores with 4 workers, passes 24 of 24.Rebuilt on
mainf03257e as one commit. The earlier version also relaxed the harness self-test's rAF control ("a rAF one freezes"), which failed 5 runs in 24 on a loaded runner then. On thismainthe unchanged check passes 24 of 24 at that profile, so it stays as it is.