[webkit-reviews] review granted: [Bug 231358] REGRESSION(r272201): Safari showed red distortion on webview after using Web Inspector, returning from another app : [Attachment 443198] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 3 11:34:12 PDT 2021


Devin Rousso <drousso at apple.com> has granted Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 231358: REGRESSION(r272201): Safari showed red distortion on webview after
using Web Inspector, returning from another app
https://bugs.webkit.org/show_bug.cgi?id=231358

Attachment 443198: Patch

https://bugs.webkit.org/attachment.cgi?id=443198&action=review




--- Comment #9 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 443198
  --> https://bugs.webkit.org/attachment.cgi?id=443198
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.backgroundC
olor = "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)


More information about the webkit-reviews mailing list