[webkit-reviews] review granted: [Bug 87877] [chromium] Move drawing code for RenderSurfaces into LayerRendererChromium : [Attachment 144933] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 87877: [chromium] Move drawing code for RenderSurfaces into
LayerRendererChromium
https://bugs.webkit.org/show_bug.cgi?id=87877

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144933&action=review


R=me, few nits here and there.	Nice work.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:69
> +    bool isReplica;
> +
>      OwnPtr<CCSharedQuadState> sharedQuadState =
surface->createSharedQuadState();
> -    if (layer->hasDebugBorders()) {
> -	   Color color(debugSurfaceBorderColorRed,
debugSurfaceBorderColorGreen, debugSurfaceBorderColorBlue,
debugSurfaceBorderAlpha);
> -	  
quadCuller.appendSurface(CCDebugBorderDrawQuad::create(sharedQuadState.get(),
surface->contentRect(), color, debugSurfaceBorderWidth));
> -    }
> -    bool isReplica = false;
> -   
quadCuller.appendSurface(CCRenderSurfaceDrawQuad::create(sharedQuadState.get(),
surface->contentRect(), layer, surfaceDamageRect(), isReplica));
> +    isReplica = false;

I'd prefer initializing this bool on the same line as we declare it, just to be
a bit clearer what's going on

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:112
> +    IntSize requiredSize(m_contentRect.size());
> +    return m_contentsTexture && m_contentsTexture->isReserved() &&
m_contentsTexture->isValid(requiredSize, GraphicsContext3D::RGBA);

why is a temp IntSize needed here? can you just pass m_contentRect.size() to
isValid() ?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:142
> +    IntSize requiredSize(m_contentRect.size());
> +    return m_backgroundTexture && m_backgroundTexture->isReserved() &&
m_backgroundTexture->isValid(requiredSize, GraphicsContext3D::RGBA);

same Q here

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:264
> +	   int red = !forReplica ? debugSurfaceBorderColorRed :
debugReplicaBorderColorRed;

the ! ? : makes my brain hurt - why not the other way 'round i.e. "forReplica ?
debugReplicaBorderColorRed : ..." ?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceDrawQuad.cpp:46
> +    m_filters.assign(filters);
> +    m_backgroundFilters.assign(backgroundFilters);

can you put these in the initializer list like other members, or does that not
work with WebFilterOperations? i.e. just have

, m_filters(filters)
, m_backgroundFilters(backgroundFilters)

?

If that doesn't work we should add the appropriate c'tor to WebFilterOperations