[webkit-reviews] review denied: [Bug 79927] [chromium] Add clipping to scissor rect to CCOcclusionTracker : [Attachment 129986] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 79927: [chromium] Add clipping to scissor rect to CCOcclusionTracker
https://bugs.webkit.org/show_bug.cgi?id=79927

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

------- Additional Comments from Adrienne Walker <enne at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=129986&action=review


I think this could be simplified a little bit more.

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:43
> +template<typename RenderSurfaceType>
> +class CCOcclusionTrackerDamageClientBase {
> +public:
> +    virtual FloatRect damageRect(const RenderSurfaceType*) const = 0;
> +};

I feel like this is introducing yet another abstraction mechanism.  We're
already using templates to deal with CCRenderSurface and RenderSurfaceChromium
having the same interface but being different classes.	Is it possible to stick
with that approach and just add RenderSurfaceType::damageRect() that returns
the real damage rect for CCRenderSurface and the full content rect for
RenderSurfaceChromium?

And, going further with this simplification, maybe there could just be a
RenderSurfaceType::scissorRect() that includes both damage and content.  That'd
eliminate the need to test damage and content rects separately, yeah?

This approach would still allow a future change to move the for-each-layer
logic here, right?

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:101
> +    // Allow tests to override this.
> +    virtual IntRect layerScissorRectInTargetSurface(const LayerType*) const;


Is it possible to just set the content rect directly on surfaces (or let it be
calculated)?