Bug 90012 – [chromium] Allow screen space rects and occluding rects to be visualized for debugging.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

It would be nice to beef up the debug rect visualization code so that we can see the rects used for occlusion, and the screen space rects for render surfaces and their replicas.

Created attachment 149600 [details]
Patch

[Dana Jansens]

Comment on attachment 149600 [details]
Patch

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

Occlusion makes for a really cool visualization :)

> Source/WebCore/platform/graphics/chromium/cc/CCDebugRectHistory.h:41
> +// There are currently five types of debug rects:

nit: six?

> Source/WebCore/platform/graphics/chromium/cc/CCDebugRectHistory.h:56
> +// - Screen space rects: this is the region the contents occupy in screen space.
> +//
> +// - Replica screen space rects: this is the region the replica's contents occupy in screen space.

nit: Comment upkeep :/ Does occlusion rects need to be here too then?

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:249
> +            // Screen space rects in purple.

nit: copypasta, update this one.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:298
> +        frame.occlusionTracker->startRecordingOccludingScreenSpaceRects();

Could we maybe stick the list of rects on the frame instead of the whole occlusion tracker? Maybe pass a reference to the frame's list in to the startRecording function and it can record directly into the frame? This way we avoid an extra variable and getter method on the tracker class.

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:367
> +        addOcclusionBehindLayer<LayerType>(m_stack.last().occlusionInScreen, layer, contentToScreenSpaceTransform<LayerType>(layer), opaqueContents, scissorInScreenRect, m_minimumTrackingSize, m_recordingOccludingScreenSpaceRects ? &m_occludingScreenSpaceRects : 0);

please use a temp var for this. the line is so long already, and its also easier to see whats going on in debugger.

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:-72
> -

prefer keep this whitespace. the below is not a getter for the above.

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:79
> +    const Region& occlusionInScreenSpace() const { return m_stack.last().occlusionInScreen; }
> +    void setOcclusionInScreenSpace(const Region& region) { m_stack.last().occlusionInScreen = region; }
> +
> +    const Region& occlusionInTargetSurface() const { return m_stack.last().occlusionInTarget; }
> +    void setOcclusionInTargetSurface(const Region& region) { m_stack.last().occlusionInTarget = region; }

Can these still live in the testing subclass? If making them here but protected is easier that's okay, but prefer not public.

> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:1290
> -        // But the opaque layer's occlusion is preserved on the parent. 
> +        // But the opaque layer's occlusion is preserved on the parent.

:p

Created attachment 149653 [details]
Patch

(In reply to comment #2)
> (From update of attachment 149600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149600&action=review
>
> Occlusion makes for a really cool visualization :)
>
> > Source/WebCore/platform/graphics/chromium/cc/CCDebugRectHistory.h:41
> > +// There are currently five types of debug rects:
>
> nit: six?
Fixed.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCDebugRectHistory.h:56
> > +// - Screen space rects: this is the region the contents occupy in screen space.
> > +//
> > +// - Replica screen space rects: this is the region the replica's contents occupy in screen space.
>
> nit: Comment upkeep :/ Does occlusion rects need to be here too then?
Fixed.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:249
> > +            // Screen space rects in purple.
>
> nit: copypasta, update this one.
Fixed.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:298
> > +        frame.occlusionTracker->startRecordingOccludingScreenSpaceRects();
>
> Could we maybe stick the list of rects on the frame instead of the whole occlusion tracker? Maybe pass a reference to the frame's list in to the startRecording function and it can record directly into the frame? This way we avoid an extra variable and getter method on the tracker class.
Sure can. This also saves me from having to forward declare CCOcclusionTracerImpl, so a whole bunch of the patch can disappear.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:367
> > +        addOcclusionBehindLayer<LayerType>(m_stack.last().occlusionInScreen, layer, contentToScreenSpaceTransform<LayerType>(layer), opaqueContents, scissorInScreenRect, m_minimumTrackingSize, m_recordingOccludingScreenSpaceRects ? &m_occludingScreenSpaceRects : 0);
>
> please use a temp var for this. the line is so long already, and its also easier to see whats going on in debugger.
Ternary is gone.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:-72
> > -
>
> prefer keep this whitespace. the below is not a getter for the above.
Put it back.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:79
> > +    const Region& occlusionInScreenSpace() const { return m_stack.last().occlusionInScreen; }
> > +    void setOcclusionInScreenSpace(const Region& region) { m_stack.last().occlusionInScreen = region; }
> > +
> > +    const Region& occlusionInTargetSurface() const { return m_stack.last().occlusionInTarget; }
> > +    void setOcclusionInTargetSurface(const Region& region) { m_stack.last().occlusionInTarget = region; }
>
> Can these still live in the testing subclass? If making them here but protected is easier that's okay, but prefer not public.
Yep. Done.
>
> > Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:1290
> > -        // But the opaque layer's occlusion is preserved on the parent.
> > +        // But the opaque layer's occlusion is preserved on the parent.
>
> :p
My whitespace is awesomer. :)

Created attachment 149655 [details]
Patch

[Dana Jansens]

Comment on attachment 149655 [details]
Patch

lgtm

[Adrienne Walker]

Comment on attachment 149655 [details]
Patch

R=me.  Sounds great!

[WebKit Review Bot]

Comment on attachment 149655 [details]
Patch

Clearing flags on attachment: 149655

Committed r121361: <http://trac.webkit.org/changeset/121361>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.