[Webkit-unassigned] [Bug 88482] [Chromium] RenderSurface for opacity with descendentDrawsContent is slow when large

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=88482





--- Comment #10 from Shawn Singh <shawnsingh at chromium.org>  2012-06-12 11:10:01 PST ---
(From update of attachment 146648)

Very awesome, so far =)   In general I like the way the logic is partitioned into clean helper functions, etc.
I have a lot of nitpicky wording comments, but I do feel strongly that clear wording is paramount to clear code.


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

> Source/WebCore/ChangeLog:3
> +        [Chromium] RenderSurface for opacity with descendentDrawsContent is slow when large

I wonder if its better to re-name this bug (including on the bugzilla page) to identify the more generic problem so that its clearer for people who see this out of context.

> Source/WebCore/ChangeLog:11
> +        CCLayerImpl to differentiated between content changes and property

(1) nit: "differentiated" --> "differentiate"

(2) wording issue:   again, we have to be very explicit and clear about which objects are being affected.  Otherwise this is very misleading.  "content changes" on the layer are already being tracked by m_updateRect.  I think you mean here "to differentiate whether the layer's property changed or the surface's property changed."  --> yes, if the layer's property changed, that means the surface's content changes, but for clarity I don't think we should use the word "content" here, _especially_ not if we don't clarify we're talking about surface content.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:614
> +    CCRenderSurface* drawingSurface = quad->renderSurface();

I think we should call this accessor "targetRenderSurface()" instead.

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:249
> +    // This method assumes that the layer either does not own a surface at all, or owns
> +    // our target surface. In all other cases, extendDamageForRenderSurface must be called instead.

This comment is technically not wrong, but I don't feel its the right thing to say.  It makes distinctions that don't generalize quite correctly -- in particular: "in all other cases" sounds like extendDamageForRenderSurface is a fall-back option, which is not true.  Furthermore, the wording makes it sound like if a layer's targetRenderSurface == layer's own surface, that it should ALWAYS enter this function, and that's absolutely not true.  In that case, there are two ways that layer gets traversed, (1) when the layer itself contributes to the targetRenderSurface that it owns, and (2) when the layer's surface needs to contribute to the ancestor targetRenderSurface.

So this comment really needs to change.  we could say "This method is called when we want to consider how a layer contributes to its targetRenderSurface, even if that layer owns the targetRenderSurface itself.  To consider how a layer's targetSurface contributes to the ancestorSurface, extendDamageForRenderSurface() must be called instead"

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:261
> +    if (layerIsNew || layer->layerTargetSurfaceDirty()) {

I think Dana mentioned that it might be worth moving layerTargetSurfaceDirty() logic to the damage tracker as a static helper function.   I agree with that, but for more reasons - in my opinion this functionality is not meaningful outside of damage tracking, and it could be error-prone/confusing/mis-used if we make it available as an accessor on layers.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:278
> +bool CCLayerImpl::layerTargetSurfaceDirty() const

I feel like the name of this is wrong.  talking about a "dirty bit" on the surface is a different concept.  Instead, this function is determining if this layer needs to be redrawn onto its target surface.  So we should re-name it to "layerNeedsToBeRedrawnToTargetSurface()"

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:284
> +    // However, if the layer DOES own a surface then the SurfacePropertyChanged
> +    // affects the target surface of its owned surface, and therefore our
> +    // owned surface is not affected. This is checked here.

"affects" --> ambiguous in my opinion.  it makes it sound like that flag actually changes state on the objects, which of course it doesn't -->  how about this:  "if the layer DOES own a surface, then the SurfacePropertyChanged flag should not be used here, because that flag represents whether the layer's surface has changed."

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:302
> +    // Run up the tree of layers until we find a surface-owning layer.
> +    // Ask everyone except for the surface-owning layer.
> +    CCLayerImpl* current = this->m_parent;
> +    while (current && !current->m_renderSurface) {
> +        if (current->m_layerSurfacePropertyChanged)
> +            return true;
> +        current = current->m_parent;
> +    }

I'm half-understanding this, but can you please explain why this code is needed?

A comment explaining why its needed would be helpful, too.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:249
> +    // Uses m_layerXxxxChanged flags to answer the question whether this layer's

I think this comment risks getting outdated/inconsistent if things change soon.   Especially if we do rename it as I proposed above, then this comment might not be as necessary.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:318
> +    // of the target surface of this layer's target surface

"but would require redrawing the targetSurface onto its ancestor targetSurface"
Also, the comment should probably also clarify what happens with this flag if the layer does not own a renderSurface.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.