Bug 224595 – Add a mechanism to dump the PlatformCALayer subtree of a GraphicsLayer, for testing

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Add a mechanism to dump the PlatformCALayer subtree of a GraphicsLayer, for testing

[Tim Horton]

Created attachment 426083 [details]
Patch

[Tim Horton]

Chances are high that there are iOS rebaselines needed, I haven't run the tests on iOS yet.

[Tim Horton]

Created attachment 426092 [details]
Patch

[Sam Weinig]

Yay!!

[Simon Fraser (smfr)]

Comment on attachment 426092 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426092&action=review

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:4040
> +    UNUSED_PARAM(flags);

hmm?

> Source/WebCore/platform/graphics/ca/PlatformCALayer.h:124
> +    virtual PlatformCALayerList sublayersForLogging() = 0;

const?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2276
> +    frameView.updateLayoutAndStyleIfNeededRecursive();

I feel like there were tests where we explicitly didn't want to update layout here.

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 426092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426092&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:4040
> > +    UNUSED_PARAM(flags);
> 
> hmm?

Whoops.

> > Source/WebCore/platform/graphics/ca/PlatformCALayer.h:124
> > +    virtual PlatformCALayerList sublayersForLogging() = 0;
> 
> const?

Sure.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:2276
> > +    frameView.updateLayoutAndStyleIfNeededRecursive();
> 
> I feel like there were tests where we explicitly didn't want to update
> layout here.

The internals caller did it before, so I don't know how that can be.

[Tim Horton]

(In reply to Tim Horton from comment #6)
> (In reply to Simon Fraser (smfr) from comment #5)
> > Comment on attachment 426092 [details]
> > Patch
> > 
> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:2276
> > > +    frameView.updateLayoutAndStyleIfNeededRecursive();
> > 
> > I feel like there were tests where we explicitly didn't want to update
> > layout here.
> 
> The internals caller did it before, so I don't know how that can be.

Frame did it too :) so many layouts

[Tim Horton]

Created attachment 426131 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 426131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426131&action=review

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3967
> +        ts << indent << "(bounds " << layer->bounds().width() << " " << layer->bounds().height() << ")\n";

size rather than bounds?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3972
> +        if (!flags.contains(PlatformLayerTreeAsTextFlags::IgnoreChildren)) {

Double negative is a bit hard to read. Maybe the flag should be IncludeChildren?

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 426131 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426131&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3967
> > +        ts << indent << "(bounds " << layer->bounds().width() << " " << layer->bounds().height() << ")\n";
> 
> size rather than bounds?

I'm going to leave this be because it matches GraphicsLayer.

> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3972
> > +        if (!flags.contains(PlatformLayerTreeAsTextFlags::IgnoreChildren)) {
> 
> Double negative is a bit hard to read. Maybe the flag should be
> IncludeChildren?

This too, mostly because I feel like the double negative is worth not having a flag set by default.

[EWS]

Committed r276085 (236597@main): <https://commits.webkit.org/236597@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426131 [details].

[Ryan Haddad]

rdar://76729543