[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