Summary: | [chromium] Allow screen space rects and occluding rects to be visualized for debugging. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | vollick | ||||||||
Component: | WebKit Misc. | Assignee: | vollick | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cc-bugs, danakj, enne, jamesr, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
vollick
2012-06-26 13:45:19 PDT
Created attachment 149600 [details]
Patch
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
Comment on attachment 149655 [details]
Patch
lgtm
Comment on attachment 149655 [details]
Patch
R=me. Sounds great!
Comment on attachment 149655 [details] Patch Clearing flags on attachment: 149655 Committed r121361: <http://trac.webkit.org/changeset/121361> All reviewed patches have been landed. Closing bug. |