[webkit-reviews] review requested: [Bug 90012] [chromium] Allow screen space rects and occluding rects to be visualized for debugging. : [Attachment 149653] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 90012: [chromium] Allow screen space rects and occluding rects to be
visualized for debugging.
https://bugs.webkit.org/show_bug.cgi?id=90012

Attachment 149653: Patch
https://bugs.webkit.org/attachment.cgi?id=149653&action=review

------- Additional Comments from vollick at chromium.org
(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. :)