Created attachment 440496 [details] Patch for EWS

<rdar://problem/81505208>

Created attachment 440498 [details] Patch for EWS

Created attachment 440499 [details] Patch for EWS

Created attachment 440538 [details] Layout Test and Plumbing v1.0

Right now the test still fails, even with the patch applied – If I add a breakpoint inside RepaintIndicatorLayerClient::notifyAnimationEnded in WebInspectorClient.cpp, I don’t hit the breakpoint until I do something like open a new tab, which hides the page (which solves the user facing issue since at that point we will clean everything up) - I’d expect to get the notifications 0.25s after the animation starts, though. My test checks if there are currently repaint rects, but I’m expecting the repaint rects to have been cleared out once their animation finishes. I think once notifyAnimationEnded fires when the animation actually ends, it should work.

Created attachment 443198 [details] Patch

Comment on attachment 443198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443198&action=review r=me > Source/WebCore/inspector/InspectorOverlay.h:209 > + unsigned paintRectCount() const { return m_paintRects.size(); } NIT: please move this next to `setShowPaintRects` and `showPaintRect since it's more related to those than grid overlays :) > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3090 > + animation.m_beginTime = *animation.m_beginTime + animationGroupBeginTime; Would `*animation.m_beginTime += animationGroupBeginTime;` also work? > LayoutTests/inspector/page/setShowPaintRects.html:14 > + let hasPaintRects = !!await InspectorTest.evaluateInPage(`window.internals.inspectorPaintRectCount()`); > + InspectorTest.expectEqual(hasPaintRects, expected, `Should ${expected ? "" : "not "}have paint rects displayed.`); NIT: I personally try to avoid using operators in conjunction with `await` as I find it not super easy to read. I think renaming this to `paintRectsCount` and then using it as `!!paintRectsCount` would be clearer. > LayoutTests/inspector/page/setShowPaintRects.html:23 > + PageAgent.setShowPaintRects(true); NIT: you could/should also `await` this just in case there's an error > LayoutTests/inspector/page/setShowPaintRects.html:26 > + InspectorTest.evaluateInPage(`document.getElementById("test").style.backgroundColor = "papayawhip"`); ditto (:23) > LayoutTests/inspector/page/setShowPaintRects.html:28 > + setTimeout(() => { Is there no better way to do this than just having a `setTimeout`? Perhaps we could/should have a `setInterval` loop in the page that constantly checks and `TestPage.dispatchEventToFrontend` when it notices a change so that we're at least way less likely to miss anything? > LayoutTests/inspector/page/setShowPaintRects.html:45 > + PageAgent.setShowPaintRects(false); ditto :(23) > LayoutTests/inspector/page/setShowPaintRects.html:48 > + InspectorTest.evaluateInPage(`document.getElementById("test").style.width = "rebeccapurple"`); ditto (:23)

(In reply to Devin Rousso from comment #9) > Comment on attachment 443198 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443198&action=review > > r=me > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3090 > > + animation.m_beginTime = *animation.m_beginTime + animationGroupBeginTime; > > Would `*animation.m_beginTime += animationGroupBeginTime;` also work? I wasn't sure that it would and didn't bother trying… but it does work! I'm uploading a new patch with this change just to make sure there's no funny stuff. I'll let Patrick Angle answer your other comments as this is the part he wrote.

Created attachment 443288 [details] Patch

Created attachment 443289 [details] Patch

Comment on attachment 443289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443289&action=review > LayoutTests/inspector/page/setShowPaintRects.html:6 > +<script src="resources/search-script.js"></script> Needed? (Maybe it is!) > LayoutTests/inspector/page/setShowPaintRects.html:48 > + InspectorTest.evaluateInPage(`document.getElementById("test").style.width = "rebeccapurple"`); Err, should this attempt to change the backgroundColor and not width to `rebeccapurple`? > LayoutTests/inspector/page/setShowPaintRects.html:69 > + width: 100px; > + height: 100px; Odd spacing.

Created attachment 443626 [details] Address test-related feedback

Created attachment 443713 [details] Patch for landing

Committed r285529 (244046@main): <https://commits.webkit.org/244046@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443713 [details].