Bug 231358 – REGRESSION(r272201): Safari showed red distortion on webview after using Web Inspector, returning from another app

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

Safari showed red distortion on webview after using Web Inspector, returning from another app

[Antoine Quint]

Created attachment 440496 [details]
Patch for EWS

[Antoine Quint]

<rdar://problem/81505208>

[Antoine Quint]

Created attachment 440498 [details]
Patch for EWS

[Antoine Quint]

Created attachment 440499 [details]
Patch for EWS

[Patrick Angle]

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

[Patrick Angle]

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.

[Antoine Quint]

Created attachment 443198 [details]
Patch

[Devin Rousso]

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)

[Antoine Quint]

(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.

[Antoine Quint]

Created attachment 443288 [details]
Patch

[Antoine Quint]

Created attachment 443289 [details]
Patch

[Joseph Pecoraro]

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.

[Patrick Angle]

Created attachment 443626 [details]
Address test-related feedback

[Antoine Quint]

Created attachment 443713 [details]
Patch for landing

[EWS]

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].