Bug 88482 – [Chromium] Compositor should avoid drawing quads when cached textures are available and contents unchanged

• Component: New Bugs
• Status: RESOLVED FIXED

[Chromium] RenderSurface for opacity with descendentDrawsContent is slow when large

Created attachment 146178 [details]
Patch

This is a preliminary patch

Created attachment 146410 [details]
Patch

Created attachment 146612 [details]
Patch

Created attachment 146648 [details]
Patch

[Dana Jansens]

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

Some preliminary comments. I see you uploaded another patch already, so I thought I'd publish these as I'm distracted with my machine for a bit.

> 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.

I guess this is a long way of saying don't call this for layers that own surfaces that contribute to the target surface.

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

Since this is only called from damage tracker, can it be done there? Maybe file-static method if you like? Can the name be better so we don't need the verbose comment below?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:317
> +    // affect the pixels on its target surface, but would require redrawing

"on its target surface" -> "on its own surface" ?

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

"redrawing the contents of the layer's surface in its target" ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:231
> +        renderSurface->setContentsChanged(!renderSurface->damageTracker()->currentDamageRect().isEmpty());

I'm not sure why we need this bool on the RenderSurface. It can already see the rect is empty as it looks up here.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:422
> +void CCLayerTreeHostImpl::removeRedundantPasses(CCRenderPassList& passes)

I think this could have a better name. "Redundant" doesn't explain what this function is doing really - why are they redundant?

Thanks for comments; some responses below.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:278
> > +bool CCLayerImpl::layerTargetSurfaceDirty() const
> 
> Since this is only called from damage tracker, can it be done there? Maybe file-static method if you like? Can the name be better so we don't need the verbose comment below?

It sure could, but this answers a pretty generic question using only class's own data, so seemed natural to be on the layer. The question is how generic that question is - maybe a different name will make this better? layerTargetSurfaceRequiresRedrawing/ContentsChanged/PixelsChanged?

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:317
> > +    // affect the pixels on its target surface, but would require redrawing
> 
> "on its target surface" -> "on its own surface" ?

Sounds good, also need to add something about layers that don't have surfaces.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:318
> > +    // of the target surface of this layer's target surface
> 
> "redrawing the contents of the layer's surface in its target" ?

Same here.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:231
> > +        renderSurface->setContentsChanged(!renderSurface->damageTracker()->currentDamageRect().isEmpty());
> 
> I'm not sure why we need this bool on the RenderSurface. It can already see the rect is empty as it looks up here.

Yeah I really did want to put this in, but couldn't see how the surface can look at this rect without talking to damage tracker directly. Is there a way? 

If not then this flag can go after we fix the quad creation and relationship between these classes and occlusion tracker, so the flag can be set on the quad externally.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:422
> > +void CCLayerTreeHostImpl::removeRedundantPasses(CCRenderPassList& passes)
> 
> I think this could have a better name. "Redundant" doesn't explain what this function is doing really - why are they redundant?

removePassesWithCachedTextures?

[Dana Jansens]

Can you describe the state of the current patch you have up? Does it already handle the surface owned by non-drawsContent layer case, and work correctly for the iconify animation etc? ie. is this done from your point of view and ready for a more thorough review yet, or where is it at right now? Thanks!

(In reply to comment #7)
> Thanks for comments; some responses below.
> 
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:278
> > > +bool CCLayerImpl::layerTargetSurfaceDirty() const
> > 
> > Since this is only called from damage tracker, can it be done there? Maybe file-static method if you like? Can the name be better so we don't need the verbose comment below?
> 
> It sure could, but this answers a pretty generic question using only class's own data, so seemed natural to be on the layer. The question is how generic that question is - maybe a different name will make this better? layerTargetSurfaceRequiresRedrawing/ContentsChanged/PixelsChanged?

I think the pattern we've been using for layers is that they are dumb as possible, and we have other classes that act on them instead. So I think if we can just have simple getter/setter on the class, and do more clever things elsewhere (closer to where the computed result it used) that would be desirable. Others should correct me if I'm wrong here. :)

> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:317
> > > +    // affect the pixels on its target surface, but would require redrawing
> > 
> > "on its target surface" -> "on its own surface" ?
> 
> Sounds good, also need to add something about layers that don't have surfaces.

Add something to the comment? That sounds good.

> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:231
> > > +        renderSurface->setContentsChanged(!renderSurface->damageTracker()->currentDamageRect().isEmpty());
> > 
> > I'm not sure why we need this bool on the RenderSurface. It can already see the rect is empty as it looks up here.
> 
> Yeah I really did want to put this in, but couldn't see how the surface can look at this rect without talking to damage tracker directly. Is there a way? 
> 
> If not then this flag can go after we fix the quad creation and relationship between these classes and occlusion tracker, so the flag can be set on the quad externally.

I'm not sure I follow, can't we replace places that say "renderSurface->contentsChanged()" with "!renderSurface->damageTracker()->currentDamageRect().isEmpty()"? Or at worst, put a contentsChanged() method on CCRenderSurface that queries the damageTracker rect. I don't get why we need a new bool in the class.

> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:422
> > > +void CCLayerTreeHostImpl::removeRedundantPasses(CCRenderPassList& passes)
> > 
> > I think this could have a better name. "Redundant" doesn't explain what this function is doing really - why are they redundant?
> 
> removePassesWithCachedTextures?

Yes :)

(In reply to comment #8)
> Can you describe the state of the current patch you have up? Does it already handle the surface owned by non-drawsContent layer case, and work correctly for the iconify animation etc? ie. is this done from your point of view and ready for a more thorough review yet, or where is it at right now? Thanks!

The patch has everything that is needed, however when applying it to the current branch, unit tests break. Bisecting gives cc04e867743286c3faa1ca805460523b883bebb0 as the first commit that caused this, so there will probably be a new patch.

[Shawn Singh]

Comment on attachment 146648 [details]
Patch


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.

[Shawn Singh]

> 
> 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()"
> 

Actually I think the name I'm proposing is also not ideal.  "needsToBeRedrawn" could be mistaken with logic in CCLayerTreeHostCommon.

how about "layerChangesWillAffectTargetSurface" or something like that.

[Dana Jansens]

(In reply to comment #11)
> > 
> > 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()"
> > 
> 
> Actually I think the name I'm proposing is also not ideal.  "needsToBeRedrawn" could be mistaken with logic in CCLayerTreeHostCommon.
> 
> how about "layerChangesWillAffectTargetSurface" or something like that.

"layerChangesWillAffectTargetSurfaceContents" :)

[Shawn Singh]

(In reply to comment #12)
> (In reply to comment #11)
> > > 
> > > 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()"
> > > 
> > 
> > Actually I think the name I'm proposing is also not ideal.  "needsToBeRedrawn" could be mistaken with logic in CCLayerTreeHostCommon.
> > 
> > how about "layerChangesWillAffectTargetSurface" or something like that.
> 
> "layerChangesWillAffectTargetSurfaceContents" :)

Sure =)

(In reply to comment #8)
> Can you describe the state of the current patch you have up? Does it already handle the surface owned by non-drawsContent layer case, and work correctly for the iconify animation etc? ie. is this done from your point of view and ready for a more thorough review yet, or where is it at right now? Thanks!
> 
> (In reply to comment #7)
> > Thanks for comments; some responses below.
> > 
> > > 
> > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:278
> > > > +bool CCLayerImpl::layerTargetSurfaceDirty() const
> > > 
> > > Since this is only called from damage tracker, can it be done there? Maybe file-static method if you like? Can the name be better so we don't need the verbose comment below?
> > 
> > It sure could, but this answers a pretty generic question using only class's own data, so seemed natural to be on the layer. The question is how generic that question is - maybe a different name will make this better? layerTargetSurfaceRequiresRedrawing/ContentsChanged/PixelsChanged?
> 
> I think the pattern we've been using for layers is that they are dumb as possible, and we have other classes that act on them instead. So I think if we can just have simple getter/setter on the class, and do more clever things elsewhere (closer to where the computed result it used) that would be desirable. Others should correct me if I'm wrong here. :)

If this is the policy I will of course move this. Though I do have to wonder why we have such a constraint. To me, object = state + behaviour, and to artificially limit it to state only needs some reason behind it. Also to properly implement this policy, layer should be a struct without any methods.

I also have to point out that right now we have some "sacred knowledge" around the layer, things like - "you shouldn't call setVisibleLayerRect manually, it will be set by other code". This is also an artifact of having layer-related logic outside the layer. If this method shouldn't be called, then the layer is the one to compute it and return it via a getter method, with setter being either private or none at all. However, since the relevant algorithm is outside we have to expose the setter and warn people verbally not to call it.

Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:231
> > > > +        renderSurface->setContentsChanged(!renderSurface->damageTracker()->currentDamageRect().isEmpty());
> > > 
> > > I'm not sure why we need this bool on the RenderSurface. It can already see the rect is empty as it looks up here.
> > 
> > Yeah I really did want to put this in, but couldn't see how the surface can look at this rect without talking to damage tracker directly. Is there a way? 
> > 
> > If not then this flag can go after we fix the quad creation and relationship between these classes and occlusion tracker, so the flag can be set on the quad externally.
> 
> I'm not sure I follow, can't we replace places that say "renderSurface->contentsChanged()" with "!renderSurface->damageTracker()->currentDamageRect().isEmpty()"? Or at worst, put a contentsChanged() method on CCRenderSurface that queries the damageTracker rect. I don't get why we need a new bool in the class.

Because both these options will require renderSurface to talk with damage tracker (talk with X == call methods on X). This is the circular dependency we are trying to get rid of - right now render surface only creates it, and we can fix it pretty easily.

I agree the bool should go away, but I see it done differently and in a new CL. We need to first remove the RS talking to quad culler directly, and then insert setting the bool on the draw quad outside the RS.

(In reply to comment #10)
> 
> > 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.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.

Well... the answer to this is "for layers that do not own a surface this flag acts as layerPropertyChanged". But this is exactly the documentation of a function which you & Dana asked me to move to DamageTracker because it doesn't belong in the layer.

So my question is - how come the algorithm doesn't belong in the layer, but its description does?

For this patch I already move the function, but we really, really need to consider this strategy.

[Shawn Singh]

(In reply to comment #15)
> (In reply to comment #10)
> > 
> > > 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.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.
> 
> Well... the answer to this is "for layers that do not own a surface this flag acts as layerPropertyChanged". But this is exactly the documentation of a function which you & Dana asked me to move to DamageTracker because it doesn't belong in the layer.
> 
> So my question is - how come the algorithm doesn't belong in the layer, but its description does?
> 
> For this patch I already move the function, but we really, really need to consider this strategy.

I'm a little confused... I don't think Dana and I were asking to move this m_layerSurfacePropertyChanged flag to the damage tracker?   so this comment and the explanation of what it represents would still remain here?

We were proposing to move the layerChangesWillAffectTargetSurfaceContents() computation.  I think its reasonable to see it both ways - you see it as a minor translation between existing layer properties which fits on the layer itself, but personally I see it as code that answers a question that only the damage tracker should be asking.

As for reconsidering the design strategy of this code, we should include a larger group of teammates in that discussion.  I suspect any ideal solution will be quite disruptive, and it might be necessary to incrementally evolve towards a better design than to fix it all at once -- which is what we have been doing =)

Created attachment 147153 [details]
Patch

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #10)
> > > 
> > > > 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.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.
> > 
> > Well... the answer to this is "for layers that do not own a surface this flag acts as layerPropertyChanged". But this is exactly the documentation of a function which you & Dana asked me to move to DamageTracker because it doesn't belong in the layer.
> > 
> > So my question is - how come the algorithm doesn't belong in the layer, but its description does?
> > 
> > For this patch I already move the function, but we really, really need to consider this strategy.
> 
> I'm a little confused... I don't think Dana and I were asking to move this m_layerSurfacePropertyChanged flag to the damage tracker?   so this comment and the explanation of what it represents would still remain here?
> 
> We were proposing to move the layerChangesWillAffectTargetSurfaceContents() computation.  I think its reasonable to see it both ways - you see it as a minor translation between existing layer properties which fits on the layer itself, but personally I see it as code that answers a question that only the damage tracker should be asking.
> 
> As for reconsidering the design strategy of this code, we should include a larger group of teammates in that discussion.  I suspect any ideal solution will be quite disruptive, and it might be necessary to incrementally evolve towards a better design than to fix it all at once -- which is what we have been doing =)

My point here was that I was adding a documentation to a field, but ended up with a description of an algorithm that is said to belong somewhere else. How can we say the algorithm belongs somewhere else if it follows naturally from the plain description of a field it operates on?

[Dana Jansens]

Comment on attachment 147153 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:294
> +    // If this layer's surface property hasn't changed, we want to see if
> +    // some layer above us has changed this property. This is done for the
> +    // case when such parent layer does not draw content, and therefore will
> +    // not be traversed by the damage tracker. We need to make sure that
> +    // property change on such layer will be caught by its descendants.
> +    CCLayerImpl* current = this->m_parent;
> +    while (current && !current->m_renderSurface) {
> +        if (current->m_layerSurfacePropertyChanged)
> +            return true;
> +        current = current->m_parent;
> +    }

As per offline I'm not sure about this being in CCLayerImpl since it depends on state from calcDrawEtc. But we do have lots of other getters like this, all the drawFoo() ones so maybe my feeling is misplaced.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:92
> +    if (layerRenderer) {

Can layerRenderer be null now? This method is only called from LayerRendererChromium so this seems weird. I assume this is for testing, but can you do the tests in a way that don't call this with null instead?

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

FYI @vollick a place to change to m_contentsSize if you rebase on top of this.

Comment on attachment 147153 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:92
>> +    if (layerRenderer) {
> 
> Can layerRenderer be null now? This method is only called from LayerRendererChromium so this seems weird. I assume this is for testing, but can you do the tests in a way that don't call this with null instead?

I think it makes sense in the contract of this method. When you are asking to prepare texture, you signal to the surface whether it should create it if non-existent or not. When reserving a texture that you just checked that exists (like we do now in CCLayerTreeHost::removePassesWithCachedTextures), passing, and therefore getting access to, an instance of LayerRendererChromium is redundant.

In fact I'd prefer to get rid of CCRenderSurface -> LayerRendererChromium dependency entirely. First, because it's circular, and second because surface should not have a knowledge of the way it's being used/drawn.

[Dana Jansens]

Comment on attachment 147153 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:92
>>> +    if (layerRenderer) {
>> 
>> Can layerRenderer be null now? This method is only called from LayerRendererChromium so this seems weird. I assume this is for testing, but can you do the tests in a way that don't call this with null instead?
> 
> I think it makes sense in the contract of this method. When you are asking to prepare texture, you signal to the surface whether it should create it if non-existent or not. When reserving a texture that you just checked that exists (like we do now in CCLayerTreeHost::removePassesWithCachedTextures), passing, and therefore getting access to, an instance of LayerRendererChromium is redundant.
> 
> In fact I'd prefer to get rid of CCRenderSurface -> LayerRendererChromium dependency entirely. First, because it's circular, and second because surface should not have a knowledge of the way it's being used/drawn.

I'm working on that last part.

[Dana Jansens]

Comment on attachment 147153 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:399
> +    // The pass was alreday removed by another quad - probably the original, and we are the replica

nit: "already" and end the comment with a period.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:409
> +    while (quadListIterator != quadList.backToFrontEnd()) {

for loop? even if the variable is initialized the line above, it lets you put the increment up here

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:411
> +        if (currentQuad->material() == CCDrawQuad::RenderPass) {

Use != and continue to nest less.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:426
> +    ssize_t passIndex = passes.size() - 1;
> +    while (passIndex >= 0) {

for loop?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:430
> +        while (quadListIterator != quadList.backToFrontEnd()) {

for loop?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:431
> +            CCDrawQuad* currentQuad = (*quadListIterator).get();

quadListIterator->get()?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:432
> +            if (currentQuad->material() == CCDrawQuad::RenderPass) {

Use != and continue to nest less.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:435
> +                if (!renderPassQuad->contentsChanged() && targetSurface->hasCachedContentsTexture()) {

Same here, can use continue instead of nesting.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:445
> +                        // We are changing the vector in the middle of reverse iteration.
> +                        // We are guaranteed that any data from iterator to the end will not change.
> +                        // Capture the iterator position from the end, and restore it after the change.
> +                        int positionFromEnd = passes.size() - passIndex;
> +                        removeRenderPassesRecursive(passes, passIndex, renderPassQuad->renderPass());
> +                        passIndex = passes.size() - positionFromEnd;
> +                        ASSERT(passIndex >= 0);

Can we simplify this? Perhaps make removeRenderPassesRecursive() return the number of passes removed (including recusion) and then move iterator by that amount?

>>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:92
>>>> +    if (layerRenderer) {
>>> 
>>> Can layerRenderer be null now? This method is only called from LayerRendererChromium so this seems weird. I assume this is for testing, but can you do the tests in a way that don't call this with null instead?
>> 
>> I think it makes sense in the contract of this method. When you are asking to prepare texture, you signal to the surface whether it should create it if non-existent or not. When reserving a texture that you just checked that exists (like we do now in CCLayerTreeHost::removePassesWithCachedTextures), passing, and therefore getting access to, an instance of LayerRendererChromium is redundant.
>> 
>> In fact I'd prefer to get rid of CCRenderSurface -> LayerRendererChromium dependency entirely. First, because it's circular, and second because surface should not have a knowledge of the way it's being used/drawn.
> 
> I'm working on that last part.

Oh I see, you're using this to reserve, I missed that, thanks. It might make sense to make a reserve() method instead and have prepareContentsTexture() call that? Then again I'm (re)moving this code entirely soon.

[Adrienne Walker]

Comment on attachment 147153 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:294
>> +    }
> 
> As per offline I'm not sure about this being in CCLayerImpl since it depends on state from calcDrawEtc. But we do have lots of other getters like this, all the drawFoo() ones so maybe my feeling is misplaced.

I don't really like the potential O(n^2) walk through layers.

Could calcDraw do the correct surface property changed propagation (or conversion to recursive layer property changed) as it determines what gets a surface and what doesn't? Or, is there some other way to maybe more explicitly propagate these flags?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:47
>      const CCRenderPass* renderPass() const { return m_renderPass; }
> +    CCRenderSurface* targetRenderSurface() const { return m_targetRenderSurface; }
>      bool isReplica() const { return m_isReplica; }
>      unsigned maskTextureId() const { return m_maskTextureId; }
> +    bool contentsChanged() const { return m_contentsChanged; }

I think this is the wrong direction to be going in for CCRenderPassDrawQuad; we should have fewer pointers in quads and not more.  In the future, I'd like to change the CCRenderPass pointer to just be an integer index into the render pass list for the frame.

I don't think either of these members should be added.  If you need the target render surface, get it from the pass.  If you need to know if contents have changed, you can get that from either the pass or the surface.  The quad doesn't need to know if its contents have changed or not in order to draw, that's really a concern of a higher level (CCLayerTreeHostImpl as the owner of the list of the passes).

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:183
> +    // FIXME: When the relationships between CCDamageTracker/CCRenderPass/CCRenderSurface/CCRenderPasDrawQuad
> +    // are fixed, this flag may go away. Right now needed to transfer the information from CCDamageTracker
> +    // to CCRenderPassDrawQuad without creating new class dependencies.

Can you explain what the eventual goal is here, possibly with a diagram of who talks to who?

If you remove m_contentsChanged from the quad, then it seems to me that there's no extra dependency, and maybe no need for this extra member?

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2398
> +TestCase removeRenderPassesCases[] =

Neat!

[Dana Jansens]

Comment on attachment 147153 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:335
> +    removePassesWithCachedTextures(passes);

Is it possible to make this call in drawLayers()?

Specifically, I don't think we are going to know what surfaces have cached textures until we have given them all to the LRC and let it figure out what it can afford for the frame. And I don't think that a call to LRC belongs in here, right now I am planning to include this in LRC::beginDrawingFrame().

(In reply to comment #24)
> (From update of attachment 147153 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147153&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:335
> > +    removePassesWithCachedTextures(passes);
> 
> Is it possible to make this call in drawLayers()?
> 
> Specifically, I don't think we are going to know what surfaces have cached textures until we have given them all to the LRC and let it figure out what it can afford for the frame. And I don't think that a call to LRC belongs in here, right now I am planning to include this in LRC::beginDrawingFrame().

Just realized that putting this in drawLayers will constitute a side-effect. Callers passing in a frame do not expect this frame to change.

Maybe a more appropriate place will be an explicit call in some proxy before drawLayers is called?

Comment on attachment 147153 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:47
>> +    bool contentsChanged() const { return m_contentsChanged; }
> 
> I think this is the wrong direction to be going in for CCRenderPassDrawQuad; we should have fewer pointers in quads and not more.  In the future, I'd like to change the CCRenderPass pointer to just be an integer index into the render pass list for the frame.
> 
> I don't think either of these members should be added.  If you need the target render surface, get it from the pass.  If you need to know if contents have changed, you can get that from either the pass or the surface.  The quad doesn't need to know if its contents have changed or not in order to draw, that's really a concern of a higher level (CCLayerTreeHostImpl as the owner of the list of the passes).

I put in a pointer to renderSurface precisely because of this reason - so I don't have to assume that m_renderPass points to a valid render pass object. Once the ubercomp change is done, the LRC will have another way of getting the render surface and the m_targetRenderSurface can go. If we try to do some hack now, we will have to undo much more stuff with the ubercomp change, IMO.

For the contentsChanged, I would claim that this field is part of the information required in order to efficiently draw a render pass list, therefore it belongs in the quad. This is part of the information based on which the render pass list will be reduced. However, before transmitting the render pass to the other side, we do not have enough knowledge to use this information (i.e. whether or not cached textures exist), so we use this flag to preserve what we have and let the compositor make use of it.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:183
>> +    // to CCRenderPassDrawQuad without creating new class dependencies.
> 
> Can you explain what the eventual goal is here, possibly with a diagram of who talks to who?
> 
> If you remove m_contentsChanged from the quad, then it seems to me that there's no extra dependency, and maybe no need for this extra member?

I don't think we can remove it from the quad (see separate comment), as we won't transmit the entire layer structure in the ubercomp. We need to transmit contentsChanged flag *somewhere* so the render passes can be deleted properly.

For the relationship change, I was just referring to the ubercomp and the change Dana is doing, making CCRenderPassDrawQuad more independent. I'm not really sure at this point whether the flag will actually go away, but I suspect it may.

The reason it is currently there, is that damage tracker is the one who knows whether or not this flag should be set, by having an empty currentDamageRect. At that time, CCRenderPassDrawQuad does not exist yet, so cannot set from damage tracker. At the same time, we cannot query CCDamageTracker from CCRenderSurface because it will create a dependency (it actually exists today, but it's circular and we should get rid of it). And we obviously cannot and shouldn't talk to CCDamageTracker from CCRenderPassDrawQuad.

[Dana Jansens]

Comment on attachment 147153 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:335
>>> +    removePassesWithCachedTextures(passes);
>> 
>> Is it possible to make this call in drawLayers()?
>> 
>> Specifically, I don't think we are going to know what surfaces have cached textures until we have given them all to the LRC and let it figure out what it can afford for the frame. And I don't think that a call to LRC belongs in here, right now I am planning to include this in LRC::beginDrawingFrame().
> 
> Just realized that putting this in drawLayers will constitute a side-effect. Callers passing in a frame do not expect this frame to change.
> 
> Maybe a more appropriate place will be an explicit call in some proxy before drawLayers is called?

Ok just chatted with enne about this as well. Let's just leave it here, since CCLTHI owns the renderer so it can know when it should do this step here in the future.

Comment on attachment 147153 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:294
>>> +    }
>> 
>> As per offline I'm not sure about this being in CCLayerImpl since it depends on state from calcDrawEtc. But we do have lots of other getters like this, all the drawFoo() ones so maybe my feeling is misplaced.
> 
> I don't really like the potential O(n^2) walk through layers.
> 
> Could calcDraw do the correct surface property changed propagation (or conversion to recursive layer property changed) as it determines what gets a surface and what doesn't? Or, is there some other way to maybe more explicitly propagate these flags?

For this one how about we fall back on propagating it the same way layerPropertyChanged is - every time it is set? Except we have to stop at surface-owning layers for this to be correct.

[Dana Jansens]

Comment on attachment 147153 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:294
>>>> +    }
>>> 
>>> As per offline I'm not sure about this being in CCLayerImpl since it depends on state from calcDrawEtc. But we do have lots of other getters like this, all the drawFoo() ones so maybe my feeling is misplaced.
>> 
>> I don't really like the potential O(n^2) walk through layers.
>> 
>> Could calcDraw do the correct surface property changed propagation (or conversion to recursive layer property changed) as it determines what gets a surface and what doesn't? Or, is there some other way to maybe more explicitly propagate these flags?
> 
> For this one how about we fall back on propagating it the same way layerPropertyChanged is - every time it is set? Except we have to stop at surface-owning layers for this to be correct.

That last bit is why we can't. We don't know surfaces when we set these flags.

Comment on attachment 147153 [details]
Patch

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

>>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:294
>>>>> +    }
>>>> 
>>>> As per offline I'm not sure about this being in CCLayerImpl since it depends on state from calcDrawEtc. But we do have lots of other getters like this, all the drawFoo() ones so maybe my feeling is misplaced.
>>> 
>>> I don't really like the potential O(n^2) walk through layers.
>>> 
>>> Could calcDraw do the correct surface property changed propagation (or conversion to recursive layer property changed) as it determines what gets a surface and what doesn't? Or, is there some other way to maybe more explicitly propagate these flags?
>> 
>> For this one how about we fall back on propagating it the same way layerPropertyChanged is - every time it is set? Except we have to stop at surface-owning layers for this to be correct.
> 
> That last bit is why we can't. We don't know surfaces when we set these flags.

Oh yeah that's right, forgot about this.

As I was dumping layer trees debugging this, I found they are for most part pretty flat. Is O(n^2) a realistic possibility here?

I can come up with a separate walk that will propagate the flag, but it will be way more code, and will be much slower in most realistic cases, though technically O(n). I'd prefer not to modify calcDraw at this point as I'm not sure I can do this safely.

What do you think?

Comment on attachment 147153 [details]
Patch

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

>>>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:294
>>>>>> +    }
>>>>> 
>>>>> As per offline I'm not sure about this being in CCLayerImpl since it depends on state from calcDrawEtc. But we do have lots of other getters like this, all the drawFoo() ones so maybe my feeling is misplaced.
>>>> 
>>>> I don't really like the potential O(n^2) walk through layers.
>>>> 
>>>> Could calcDraw do the correct surface property changed propagation (or conversion to recursive layer property changed) as it determines what gets a surface and what doesn't? Or, is there some other way to maybe more explicitly propagate these flags?
>>> 
>>> For this one how about we fall back on propagating it the same way layerPropertyChanged is - every time it is set? Except we have to stop at surface-owning layers for this to be correct.
>> 
>> That last bit is why we can't. We don't know surfaces when we set these flags.
> 
> Oh yeah that's right, forgot about this.
> 
> As I was dumping layer trees debugging this, I found they are for most part pretty flat. Is O(n^2) a realistic possibility here?
> 
> I can come up with a separate walk that will propagate the flag, but it will be way more code, and will be much slower in most realistic cases, though technically O(n). I'd prefer not to modify calcDraw at this point as I'm not sure I can do this safely.
> 
> What do you think?

Here's one other possibility: have this getter set the flag for the entire path it just traversed in case of positive result. I.e. instead of just "return true", run up the tree again, this time setting this flag as a side-effect. This will avoid duplicating this work next time our ancestor is queried.

[Adrienne Walker]

(In reply to comment #26)
> (From update of attachment 147153 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147153&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:47
> >> +    bool contentsChanged() const { return m_contentsChanged; }
> > 
> > I think this is the wrong direction to be going in for CCRenderPassDrawQuad; we should have fewer pointers in quads and not more.  In the future, I'd like to change the CCRenderPass pointer to just be an integer index into the render pass list for the frame.
> > 
> > I don't think either of these members should be added.  If you need the target render surface, get it from the pass.  If you need to know if contents have changed, you can get that from either the pass or the surface.  The quad doesn't need to know if its contents have changed or not in order to draw, that's really a concern of a higher level (CCLayerTreeHostImpl as the owner of the list of the passes).
> 
> I put in a pointer to renderSurface precisely because of this reason - so I don't have to assume that m_renderPass points to a valid render pass object. Once the ubercomp change is done, the LRC will have another way of getting the render surface and the m_targetRenderSurface can go. If we try to do some hack now, we will have to undo much more stuff with the ubercomp change, IMO.

When does m_renderPass not point to a valid render pass?

I don't understand what the hack is here; I'm just asking that the iteration to remove passes in CCLayerTreeHostImpl use the surface pointer and damage tracker that it already has access to, rather than pushing more pointers into quads.

> For the contentsChanged, I would claim that this field is part of the information required in order to efficiently draw a render pass list, therefore it belongs in the quad. This is part of the information based on which the render pass list will be reduced. However, before transmitting the render pass to the other side, we do not have enough knowledge to use this information (i.e. whether or not cached textures exist), so we use this flag to preserve what we have and let the compositor make use of it.

Sorry, but I disagree.  That field is part of the information used to remove some other render pass entirely, but not to actually draw the quad representing that pass.

The damage is also a piece of information that we could use to set a partial scissor when beginning to draw a render pass, so it seems like it's most suited to live somewhere more like the pass or the surface, which it essentially does right now in the form of querying the damage tracker.  In an ubercomp world, I think we would pass the current damage rect with the pass and not with the quad.

> >> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:183
> >> +    // to CCRenderPassDrawQuad without creating new class dependencies.
> > 
> > Can you explain what the eventual goal is here, possibly with a diagram of who talks to who?
> > 
> > If you remove m_contentsChanged from the quad, then it seems to me that there's no extra dependency, and maybe no need for this extra member?
> 
> I don't think we can remove it from the quad (see separate comment), as we won't transmit the entire layer structure in the ubercomp. We need to transmit contentsChanged flag *somewhere* so the render passes can be deleted properly.
> 
> For the relationship change, I was just referring to the ubercomp and the change Dana is doing, making CCRenderPassDrawQuad more independent. I'm not really sure at this point whether the flag will actually go away, but I suspect it may.
> 
> The reason it is currently there, is that damage tracker is the one who knows whether or not this flag should be set, by having an empty currentDamageRect. At that time, CCRenderPassDrawQuad does not exist yet, so cannot set from damage tracker. At the same time, we cannot query CCDamageTracker from CCRenderSurface because it will create a dependency (it actually exists today, but it's circular and we should get rid of it). And we obviously cannot and shouldn't talk to CCDamageTracker from CCRenderPassDrawQuad.

I don't understand why you can't query CCDamageTracker from CCRenderSurface.  CCRenderSurface *owns* the damage tracker.  Offhand, I think the circular reference would be better broken by pushing the damage tracker up to CCRenderPass rather than pushing the results down.  That's problematic because CCRenderSurface is the persistent object from frame to frame, so it'd take more doing to make that work.

It seems like you're trying to do two things in this patch: preserve render passes and eliminate circular dependencies.  The former seems like it's more important and the latter seems more contentious.  Can we punt on the second for now?

[Adrienne Walker]

(In reply to comment #31)
> (From update of attachment 147153 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147153&action=review

> > As I was dumping layer trees debugging this, I found they are for most part pretty flat. Is O(n^2) a realistic possibility here?
> > 
> > I can come up with a separate walk that will propagate the flag, but it will be way more code, and will be much slower in most realistic cases, though technically O(n). I'd prefer not to modify calcDraw at this point as I'm not sure I can do this safely.
> > 
> > What do you think?
> 
> Here's one other possibility: have this getter set the flag for the entire path it just traversed in case of positive result. I.e. instead of just "return true", run up the tree again, this time setting this flag as a side-effect. This will avoid duplicating this work next time our ancestor is queried.

Re: the other possibility.  I would prefer not to have a getter be non-const, even if the result is the same regardless.

It's the negative result case I'm most worried about--not only is it far more likely that transform/opacity is not changed on a given frame, but it's the negative result that requires walking all the way up to the root.

Eh.  Maybe this is fine as-is and we can make it more efficient in the future if need be.

Comment on attachment 147153 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:47
>>>> +    bool contentsChanged() const { return m_contentsChanged; }
>>> 
>>> I think this is the wrong direction to be going in for CCRenderPassDrawQuad; we should have fewer pointers in quads and not more.  In the future, I'd like to change the CCRenderPass pointer to just be an integer index into the render pass list for the frame.
>>> 
>>> I don't think either of these members should be added.  If you need the target render surface, get it from the pass.  If you need to know if contents have changed, you can get that from either the pass or the surface.  The quad doesn't need to know if its contents have changed or not in order to draw, that's really a concern of a higher level (CCLayerTreeHostImpl as the owner of the list of the passes).
>> 
>> I put in a pointer to renderSurface precisely because of this reason - so I don't have to assume that m_renderPass points to a valid render pass object. Once the ubercomp change is done, the LRC will have another way of getting the render surface and the m_targetRenderSurface can go. If we try to do some hack now, we will have to undo much more stuff with the ubercomp change, IMO.
>> 
>> For the contentsChanged, I would claim that this field is part of the information required in order to efficiently draw a render pass list, therefore it belongs in the quad. This is part of the information based on which the render pass list will be reduced. However, before transmitting the render pass to the other side, we do not have enough knowledge to use this information (i.e. whether or not cached textures exist), so we use this flag to preserve what we have and let the compositor make use of it.
> 
> When does m_renderPass not point to a valid render pass?
> 
> I don't understand what the hack is here; I'm just asking that the iteration to remove passes in CCLayerTreeHostImpl use the surface pointer and damage tracker that it already has access to, rather than pushing more pointers into quads.

> When does m_renderPass not point to a valid render pass?

In two cases. First, when the render pass it points to is removed from the vector and deleted. The pointer becomes dangling, as the vector has OwnPtr's. And second, when we are going to convert renderPass pointer to render pass ID. My understanding was that this is the direction we're going.

> I don't understand what the hack is here

A couple of possibilities, I'd consider them as hacks. First is to cache the removed render passes in a separate vector so that they don't go out of scope. That caching will have to be removed when render pass pointer becomes and ID, that's why I said it's more changes. The second possibility is to introduce a flag in render pass that will cause the draw to be skipped. I'd consider this problematic because we shouldn't have issues with removing render passes - they never survive a single frame anyways. Given that the current direction is for the render pass pointer inside the quad to become and ID, I thought this solution was the closest to it.

It is a matter of opinion however so if you want any of these possibilities or have a different one in mind this can be done. Just wanted to point out that it's not as simple as replacing quad->surface() with quad->pass()->surface().

> Sorry, but I disagree.  That field is part of the information used to remove some other render pass entirely, but not to actually draw the quad representing that pass.
>
> The damage is also a piece of information that we could use to set a partial scissor when beginning to draw a render pass, so it seems like it's most suited to live somewhere more like the pass or the surface, which it essentially does right now in the form of querying the damage tracker.  In an ubercomp world, I think we would pass the current damage rect with the pass and not with the quad.

OK, I'll query the surface.

>>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:183
>>>> +    // to CCRenderPassDrawQuad without creating new class dependencies.
>>> 
>>> Can you explain what the eventual goal is here, possibly with a diagram of who talks to who?
>>> 
>>> If you remove m_contentsChanged from the quad, then it seems to me that there's no extra dependency, and maybe no need for this extra member?
>> 
>> I don't think we can remove it from the quad (see separate comment), as we won't transmit the entire layer structure in the ubercomp. We need to transmit contentsChanged flag *somewhere* so the render passes can be deleted properly.
>> 
>> For the relationship change, I was just referring to the ubercomp and the change Dana is doing, making CCRenderPassDrawQuad more independent. I'm not really sure at this point whether the flag will actually go away, but I suspect it may.
>> 
>> The reason it is currently there, is that damage tracker is the one who knows whether or not this flag should be set, by having an empty currentDamageRect. At that time, CCRenderPassDrawQuad does not exist yet, so cannot set from damage tracker. At the same time, we cannot query CCDamageTracker from CCRenderSurface because it will create a dependency (it actually exists today, but it's circular and we should get rid of it). And we obviously cannot and shouldn't talk to CCDamageTracker from CCRenderPassDrawQuad.
> 
> I don't understand why you can't query CCDamageTracker from CCRenderSurface.  CCRenderSurface *owns* the damage tracker.  Offhand, I think the circular reference would be better broken by pushing the damage tracker up to CCRenderPass rather than pushing the results down.  That's problematic because CCRenderSurface is the persistent object from frame to frame, so it'd take more doing to make that work.
> 
> It seems like you're trying to do two things in this patch: preserve render passes and eliminate circular dependencies.  The former seems like it's more important and the latter seems more contentious.  Can we punt on the second for now?

Right now the situation is actually not so bad. Render surface instantiates damage tracker, but that can be fixed pretty easily. I checked in the source, this is their only interaction right now.

So I was looking at adding communication between surface and tracker as degradation in terms of dependencies; what you suggesting is pretty easily done and if you're not convinced by the above I can do this.

[Adrienne Walker]

(In reply to comment #34)

> > I don't understand what the hack is here
> 
> A couple of possibilities, I'd consider them as hacks. First is to cache the removed render passes in a separate vector so that they don't go out of scope. That caching will have to be removed when render pass pointer becomes and ID, that's why I said it's more changes. The second possibility is to introduce a flag in render pass that will cause the draw to be skipped. I'd consider this problematic because we shouldn't have issues with removing render passes - they never survive a single frame anyways. Given that the current direction is for the render pass pointer inside the quad to become and ID, I thought this solution was the closest to it.
> 
> It is a matter of opinion however so if you want any of these possibilities or have a different one in mind this can be done. Just wanted to point out that it's not as simple as replacing quad->surface() with quad->pass()->surface().

Oh, I see now.

Even with ids, you're still going to need to keep the render passes alive to be referred to, I suspect.  In the short term, I prefer the caching solution you mention--you could just add a skippedPasses member to FrameData and move passes from renderPasses to skippedPasses to keep them alive.  I think that's safer than having a pointer on a quad that may or may not be valid at some given time.

Created attachment 147685 [details]
Patch

Implemented latest comments; enne can you please take a look.

[Adrienne Walker]

Comment on attachment 147685 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:267
> -bool CCLayerTreeHostImpl::calculateRenderPasses(CCRenderPassList& passes, CCLayerList& renderSurfaceLayerList, CCLayerList& willDrawLayers)
> +bool CCLayerTreeHostImpl::calculateRenderPasses(CCRenderPassList& passes, CCLayerList& renderSurfaceLayerList, CCLayerList& willDrawLayers, CCRenderPassList& skippedPasses)

Can this function just take FrameData& instead of all of parameters separately?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:386
> +static ssize_t indexOfRenderPass(const CCRenderPassList& passes, const CCRenderPass* pass, size_t searchUntil)

Did you mean to use searchUntil?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:397
> +    ssize_t removeIndex = indexOfRenderPass(passes, firstToRemove, bottomPass);

Maybe just use passes.find(firstToRemove) rather than roll your own?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:445
> +            // Reserve the texture immediately. We do not need to pass in layer renderer
> +            // since the texture already exists, just needs to be reserved.
> +            if (!targetSurface->prepareContentsTexture(0))

If you need a static function for testing, then I'd rather you passed in the layer renderer to the function and forwarded that along to prepareContentsTexture, rather than making CCRenderSurface::prepareContentsTexture robust to a null LRC.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:61
> +    void appendQuad(PassOwnPtr<CCDrawQuad>);

I know you're adding this for testing, but can you not add this as a public function? The reason the other appendQuads functions don't take quads is so that the render pass can own the lifetime of the shared quad state.  With this function, it's not clear that the caller has to manage the lifetime of the shared quad state on the quad.

Maybe change the private modifier below to protected and make a CCTestRenderPass that exposes what you need?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.cpp:31
> +#include "cc/CCRenderPass.h"
> +

Not needed anymore?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:30
> +#include "cc/CCRenderSurface.h"

Not needed anymore?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:184
> +    // FIXME: When the relationships between CCDamageTracker/CCRenderPass/CCRenderSurface/CCRenderPasDrawQuad
> +    // are fixed, this flag may go away. Right now needed to transfer the information from CCDamageTracker
> +    // to CCRenderPassDrawQuad without creating new class dependencies.
> +    bool m_contentsChanged;
> +

Unused?

> Source/WebKit/chromium/tests/CCLayerImplTest.cpp:130
> +    // Changing these properties only affects how render's surface is drawn

s/'s//

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2171
> +    mutable OwnPtr<CCRenderPass> renderPassPtr;

C++11's move semantics can't get here soon enough.

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2379
> +    EXPECT_STREQ(expectedResult, actualResult);

Can you pipe a string about which test case failed, e.g. "EXPECT_STREQ(expectedResult, actualResult) << testName;" so that when this fails there's more indication in the log about which case is failing?

[Shawn Singh]

Comment on attachment 147685 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:242
> +static bool layerTargetSurfaceRequiresRedrawing(CCLayerImpl* layer)

I'm not sure about our past discussion, but looking at this version of the patch now, I would prefer to re-name this.

If this returns false, the name implies that the targetSurface does not require redrawing, which is not necessarily what we mean here.  I think what this function is doing is telling whether the layer needs to be redrawn to its targetSurface, no?

so shouldn't this be called "layerNeedsToRedrawOntoItsTargetSurface"   (or something similar but saying the exact same thing) ??

Comment on attachment 147685 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:445
>> +            if (!targetSurface->prepareContentsTexture(0))
> 
> If you need a static function for testing, then I'd rather you passed in the layer renderer to the function and forwarded that along to prepareContentsTexture, rather than making CCRenderSurface::prepareContentsTexture robust to a null LRC.

Please see comments #20 and #22 (last few lines). Will you agree to wait for Dana's change and separate the functions then?

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:61
>> +    void appendQuad(PassOwnPtr<CCDrawQuad>);
> 
> I know you're adding this for testing, but can you not add this as a public function? The reason the other appendQuads functions don't take quads is so that the render pass can own the lifetime of the shared quad state.  With this function, it's not clear that the caller has to manage the lifetime of the shared quad state on the quad.
> 
> Maybe change the private modifier below to protected and make a CCTestRenderPass that exposes what you need?

Just wondering, can shared quad state be ref-counted between the quads that use it? Not for this patch of course.

Created attachment 147819 [details]
Patch

[Adrienne Walker]

(In reply to comment #40)
> (From update of attachment 147685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147685&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:445
> >> +            if (!targetSurface->prepareContentsTexture(0))
> > 
> > If you need a static function for testing, then I'd rather you passed in the layer renderer to the function and forwarded that along to prepareContentsTexture, rather than making CCRenderSurface::prepareContentsTexture robust to a null LRC.
> 
> Please see comments #20 and #22 (last few lines). Will you agree to wait for Dana's change and separate the functions then?

Ok, but put a FIXME in prepareContentsTexture to separate out creation and reservation.

> >> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:61
> >> +    void appendQuad(PassOwnPtr<CCDrawQuad>);
> > 
> > I know you're adding this for testing, but can you not add this as a public function? The reason the other appendQuads functions don't take quads is so that the render pass can own the lifetime of the shared quad state.  With this function, it's not clear that the caller has to manage the lifetime of the shared quad state on the quad.
> > 
> > Maybe change the private modifier below to protected and make a CCTestRenderPass that exposes what you need?
> 
> Just wondering, can shared quad state be ref-counted between the quads that use it? Not for this patch of course.

Maybe? I think I'd want to see how the quad serialization shaped up first.

Created attachment 147856 [details]
Patch

[Adrienne Walker]

Comment on attachment 147856 [details]
Patch

Thanks for all the changes.  R=me.

[WebKit Review Bot]

Comment on attachment 147856 [details]
Patch

Attachment 147856 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12970035

New failing tests:
platform/chromium/compositing/layout-width-change.html
platform/chromium/compositing/render-surface-alpha-blending.html
platform/chromium/compositing/tiny-layer-rotated.html
platform/chromium/compositing/huge-layer-rotated.html
fast/loader/loadInProgress.html
platform/chromium/compositing/3d-corners.html
platform/chromium/compositing/video-frame-size-change.html
platform/chromium/compositing/perpendicular-layer-sorting.html
fast/loader/unload-form-post-about-blank.html
platform/chromium/compositing/child-layer-3d-sorting.html
platform/chromium/compositing/accelerated-drawing/svg-filters.html
platform/chromium/compositing/lost-compositor-context-permanently.html
platform/chromium/compositing/filters/background-filter-blur-outsets.html
platform/chromium/compositing/lost-compositor-context-with-video.html
platform/chromium/compositing/accelerated-drawing/alpha.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
platform/chromium/compositing/plugins/webplugin-alpha.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/backface-visibility-transformed.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/img-layer-grow.html
platform/chromium/compositing/filters/background-filter-blur-off-axis.html
platform/chromium/compositing/filters/background-filter-blur.html
platform/chromium/compositing/lost-compositor-context-twice.html

[WebKit Review Bot]

Created attachment 147898 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 147965 [details]
Patch

[Dana Jansens]

Comment on attachment 147965 [details]
Patch

Looks like a rebase? Clearing the R flag. Put a CQ? on here if you want me to commit please :)

Created attachment 147973 [details]
Patch

[WebKit Review Bot]

Comment on attachment 147973 [details]
Patch

Clearing flags on attachment: 147973

Committed r120539: <http://trac.webkit.org/changeset/120539>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 89295

[Adam Barth]

FAILED: clang++ -MMD -MF obj/third_party/WebKit/Source/WebCore/platform/graphics/chromium/webcore_platform.ContentLayerChromium.o.d -DCHROMIUM_BUILD -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_TASK_MANAGER=1 -DENABLE_WEB_INTENTS=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PROTECTOR_SERVICE=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_PROMO_RESOURCE_SERVICE=1 -DENABLE_AUTOMATION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DWEBKIT_IMPLEMENTATION=1 '-DWEBCORE_NAVIGATOR_VENDOR="Google Inc."' '-DWEBCORE_NAVIGATOR_PLATFORM="MacIntel"' -DWebCascadeList=ChromiumWebCoreObjCWebCascadeList -DWebScrollbarPrefsObserver=ChromiumWebCoreObjCWebScrollbarPrefsObserver -DWebCoreRenderThemeNotificationObserver=ChromiumWebCoreObjCWebCoreRenderThemeNotificationObserver -DWebFontCache=ChromiumWebCoreObjCWebFontCache -DWebScrollAnimationHelperDelegate=ChromiumWebCoreObjCWebScrollAnimationHelperDelegate -DWebScrollbarPainterControllerDelegate=ChromiumWebCoreObjCWebScrollbarPainterControllerDelegate -DWebScrollbarPainterDelegate=ChromiumWebCoreObjCWebScrollbarPainterDelegate -DWebScrollbarPartAnimation=ChromiumWebCoreObjCWebScrollbarPartAnimation -DENABLE_3D_PLUGIN=1 -DENABLE_BATTERY_STATUS=0 -DENABLE_BLOB=1 -DENABLE_BLOB_SLICE=1 -DENABLE_CHANNEL_MESSAGING=1 -DENABLE_CSS3_FLEXBOX=1 -DENABLE_CSS_BOX_DECORATION_BREAK=1 -DENABLE_CSS_EXCLUSIONS=1 -DENABLE_CSS_FILTERS=1 -DENABLE_CSS_IMAGE_SET=1 -DENABLE_CSS_IMAGE_RESOLUTION=0 -DENABLE_CSS_REGIONS=1 -DENABLE_CSS_SHADERS=1 -DENABLE_CSS_VARIABLES=0 -DENABLE_CUSTOM_SCHEME_HANDLER=0 -DENABLE_DATALIST=1 -DENABLE_DASHBOARD_SUPPORT=0 -DENABLE_DATA_TRANSFER_ITEMS=1 -DENABLE_DETAILS=1 -DENABLE_DEVICE_ORIENTATION=1 -DENABLE_DIRECTORY_UPLOAD=1 -DENABLE_DOWNLOAD_ATTRIBUTE=1 -DENABLE_ENCRYPTED_MEDIA=1 -DENABLE_FILE_SYSTEM=1 -DENABLE_FILTERS=1 -DENABLE_FULLSCREEN_API=1 -DENABLE_GAMEPAD=1 -DENABLE_GEOLOCATION=1 -DENABLE_GESTURE_EVENTS=1 -DENABLE_ICONDATABASE=0 -DENABLE_IFRAME_SEAMLESS=1 -DENABLE_INDEXED_DATABASE=1 -DENABLE_INPUT_TYPE_DATE=1 -DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_LEGACY_CSS_VENDOR_PREFIXES=0 -DENABLE_LEGACY_WEBKIT_BLOB_BUILDER=1 -DENABLE_LINK_PREFETCH=1 -DENABLE_LINK_PRERENDER=1 -DENABLE_MEDIA_SOURCE=1 -DENABLE_MEDIA_STATISTICS=1 -DENABLE_METER_TAG=1 -DENABLE_MHTML=1 -DENABLE_MICRODATA=0 -DENABLE_MUTATION_OBSERVERS=1 -DENABLE_PAGE_VISIBILITY_API=1 -DENABLE_POINTER_LOCK=1 -DENABLE_PROGRESS_TAG=1 -DENABLE_QUOTA=1 -DENABLE_REGISTER_PROTOCOL_HANDLER=1 -DENABLE_REQUEST_ANIMATION_FRAME=1 -DENABLE_RUBY=1 -DENABLE_SANDBOX=1 -DENABLE_SCRIPTED_SPEECH=1 -DENABLE_SHADOW_DOM=1 -DENABLE_SMOOTH_SCROLLING=1 -DENABLE_SQL_DATABASE=1 -DENABLE_STYLE_SCOPED=1 -DENABLE_SVG=1 -DENABLE_SVG_FONTS=1 -DENABLE_TOUCH_ADJUSTMENT=1 -DENABLE_TOUCH_EVENTS=1 -DENABLE_TOUCH_ICON_LOADING=0 -DENABLE_V8_SCRIPT_DEBUG_SERVER=1 -DENABLE_VIDEO=1 -DENABLE_VIDEO_TRACK=1 -DENABLE_VIEWPORT=1 -DENABLE_WEBGL=1 -DENABLE_WEB_SOCKETS=1 -DENABLE_WEB_TIMING=1 -DENABLE_WORKERS=1 -DENABLE_XHR_RESPONSE_BLOB=1 -DENABLE_XSLT=1 -DWTF_USE_LEVELDB=1 -DWTF_USE_BUILTIN_UTF8_CODEC=1 -DWTF_USE_OPENTYPE_SANITIZER=1 -DWTF_USE_RTL_SCROLLBAR=1 -DWTF_USE_SKIA_TEXT=1 -DWTF_USE_WEBP=1 -DWTF_USE_WEBKIT_IMAGE_DECODERS=1 -DENABLE_CALENDAR_PICKER=1 -DENABLE_FONT_BOOSTING=0 -DENABLE_FULLSCREEN_MEDIA_CONTROLS=1 -DENABLE_INPUT_SPEECH=1 -DENABLE_JAVASCRIPT_I18N_API=1 -DENABLE_LEGACY_NOTIFICATIONS=1 -DENABLE_MEDIA_CAPTURE=0 -DENABLE_MEDIA_STREAM=1 -DENABLE_NOTIFICATIONS=1 -DENABLE_ORIENTATION_EVENTS=0 -DENABLE_OVERFLOW_SCROLLING=0 -DENABLE_PAGE_POPUP=1 -DENABLE_SHARED_WORKERS=1 -DENABLE_WEB_AUDIO=1 -DENABLE_INPUT_TYPE_COLOR=1 -DWTF_USE_ACCELERATED_COMPOSITING=1 -DENABLE_3D_RENDERING=1 -DENABLE_ACCELERATED_2D_CANVAS=1 -DENABLE_RUBBER_BANDING=1 -DWTF_USE_SKIA_ON_MAC_CHROMIUM=1 -DBUILDING_CHROMIUM__=1 -DUSE_SYSTEM_MALLOC=1 -DWTF_USE_NEW_THEME=1 -DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -DSK_BUILD_NO_IMAGE_ENCODE '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DGR_AGGRESSIVE_SHADER_OPTS=1 -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG -DLIBXML_STATIC -DLIBXSLT_STATIC -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../../third_party/icu/public/common -I../../third_party/icu/public/i18n -I../../third_party/apple_webkit -I../../third_party/khronos -I../.. -Iobj/third_party/WebKit/Source/WebCore/WebCore.gyp/webcore_platform.gen -I../../third_party/WebKit/Source/WebCore -I../../third_party/WebKit/Source -I../../third_party/WebKit/Source/WebCore/Modules/battery -I../../third_party/WebKit/Source/WebCore/Modules/filesystem -I../../third_party/WebKit/Source/WebCore/Modules/filesystem/chromium -I../../third_party/WebKit/Source/WebCore/Modules/gamepad -I../../third_party/WebKit/Source/WebCore/Modules/geolocation -I../../third_party/WebKit/Source/WebCore/Modules/intents -I../../third_party/WebKit/Source/WebCore/Modules/indexeddb -I../../third_party/WebKit/Source/WebCore/Modules/mediastream -I../../third_party/WebKit/Source/WebCore/Modules/quota -I../../third_party/WebKit/Source/WebCore/Modules/speech -I../../third_party/WebKit/Source/WebCore/Modules/webaudio -I../../third_party/WebKit/Source/WebCore/Modules/webdatabase -I../../third_party/WebKit/Source/WebCore/Modules/webdatabase/chromium -I../../third_party/WebKit/Source/WebCore/Modules/websockets -I../../third_party/WebKit/Source/WebCore/accessibility -I../../third_party/WebKit/Source/WebCore/accessibility/chromium -I../../third_party/WebKit/Source/WebCore/bindings -I../../third_party/WebKit/Source/WebCore/bindings/generic -I../../third_party/WebKit/Source/WebCore/bindings/v8 -I../../third_party/WebKit/Source/WebCore/bindings/v8/custom -I../../third_party/WebKit/Source/WebCore/bindings/v8/specialization -I../../third_party/WebKit/Source/WebCore/bridge -I../../third_party/WebKit/Source/WebCore/bridge/jni -I../../third_party/WebKit/Source/WebCore/bridge/jni/v8 -I../../third_party/WebKit/Source/WebCore/css -I../../third_party/WebKit/Source/WebCore/dom -I../../third_party/WebKit/Source/WebCore/dom/default -I../../third_party/WebKit/Source/WebCore/editing -I../../third_party/WebKit/Source/WebCore/fileapi -I../../third_party/WebKit/Source/WebCore/history -I../../third_party/WebKit/Source/WebCore/html -I../../third_party/WebKit/Source/WebCore/html/canvas -I../../third_party/WebKit/Source/WebCore/html/parser -I../../third_party/WebKit/Source/WebCore/html/shadow -I../../third_party/WebKit/Source/WebCore/html/track -I../../third_party/WebKit/Source/WebCore/inspector -I../../third_party/WebKit/Source/WebCore/loader -I../../third_party/WebKit/Source/WebCore/loader/appcache -I../../third_party/WebKit/Source/WebCore/loader/archive -I../../third_party/WebKit/Source/WebCore/loader/archive/cf -I../../third_party/WebKit/Source/WebCore/loader/archive/mhtml -I../../third_party/WebKit/Source/WebCore/loader/cache -I../../third_party/WebKit/Source/WebCore/loader/icon -I../../third_party/WebKit/Source/WebCore/mathml -I../../third_party/WebKit/Source/WebCore/notifications -I../../third_party/WebKit/Source/WebCore/page -I../../third_party/WebKit/Source/WebCore/page/animation -I../../third_party/WebKit/Source/WebCore/page/chromium -I../../third_party/WebKit/Source/WebCore/page/scrolling -I../../third_party/WebKit/Source/WebCore/platform -I../../third_party/WebKit/Source/WebCore/platform/animation -I../../third_party/WebKit/Source/WebCore/platform/audio -I../../third_party/WebKit/Source/WebCore/platform/audio/chromium -I../../third_party/WebKit/Source/WebCore/platform/chromium -I../../third_party/WebKit/Source/WebCore/platform/chromium/support -I../../third_party/WebKit/Source/WebCore/platform/graphics -I../../third_party/WebKit/Source/WebCore/platform/graphics/chromium -I../../third_party/WebKit/Source/WebCore/platform/graphics/filters -I../../third_party/WebKit/Source/WebCore/platform/graphics/filters/arm -I../../third_party/WebKit/Source/WebCore/platform/graphics/gpu -I../../third_party/WebKit/Source/WebCore/platform/graphics/opentype -I../../third_party/WebKit/Source/WebCore/platform/graphics/skia -I../../third_party/WebKit/Source/WebCore/platform/graphics/transforms -I../../third_party/WebKit/Source/WebCore/platform/image-decoders -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/bmp -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/gif -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/ico -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/jpeg -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/png -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/skia -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/webp -I../../third_party/WebKit/Source/WebCore/platform/image-encoders/skia -I../../third_party/WebKit/Source/WebCore/platform/leveldb -I../../third_party/WebKit/Source/WebCore/platform/mediastream -I../../third_party/WebKit/Source/WebCore/platform/mediastream/chromium -I../../third_party/WebKit/Source/WebCore/platform/mock -I../../third_party/WebKit/Source/WebCore/platform/network -I../../third_party/WebKit/Source/WebCore/platform/network/chromium -I../../third_party/WebKit/Source/WebCore/platform/sql -I../../third_party/WebKit/Source/WebCore/platform/text -I../../third_party/WebKit/Source/WebCore/platform/text/transcoder -I../../third_party/WebKit/Source/WebCore/plugins -I../../third_party/WebKit/Source/WebCore/plugins/chromium -I../../third_party/WebKit/Source/WebCore/rendering -I../../third_party/WebKit/Source/WebCore/rendering/style -I../../third_party/WebKit/Source/WebCore/rendering/svg -I../../third_party/WebKit/Source/WebCore/storage -I../../third_party/WebKit/Source/WebCore/storage/chromium -I../../third_party/WebKit/Source/WebCore/svg -I../../third_party/WebKit/Source/WebCore/svg/animation -I../../third_party/WebKit/Source/WebCore/svg/graphics -I../../third_party/WebKit/Source/WebCore/svg/graphics/filters -I../../third_party/WebKit/Source/WebCore/svg/properties -I../../third_party/WebKit/Source/ThirdParty/glu -I../../third_party/WebKit/Source/WebCore/workers -I../../third_party/WebKit/Source/WebCore/xml -I../../third_party/WebKit/Source/WebCore/xml/parser -I../../third_party/WebKit/Source/WebCore/platform/audio/mac -I../../third_party/WebKit/Source/WebCore/platform/cocoa -I../../third_party/WebKit/Source/WebCore/platform/graphics/cg -I../../third_party/WebKit/Source/WebCore/platform/graphics/cocoa -I../../third_party/WebKit/Source/WebCore/platform/graphics/mac -I../../third_party/WebKit/Source/WebCore/platform/mac -I../../third_party/WebKit/Source/WebCore/platform/text/mac -I../../third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz -I../../third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/ng -I../../gpu -I../../third_party/angle/include/GLSLANG -Igen/webkit -Igen/webkit/bindings -I../../third_party/WebKit/Source/WTF -I../../third_party/WebKit/Source/JavaScriptCore -I../../third_party/WebKit/Source/Platform/chromium -Igen/webcore_headers -I../../skia/config -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu -I../../third_party/skia/include/gpu/gl -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../skia/ext -I../../third_party/skia/include/utils/mac -I../../third_party/iccjpeg -I../../third_party/libwebp -I../../third_party/libpng -I../../third_party/libxml/mac/include -I../../third_party/libxml/src/include -I../../third_party/libxslt -I../../third_party/npapi -I../../third_party/npapi/bindings -I../../third_party/ots/include -I../../third_party/qcms/src -I../../third_party/sqlite -I../../third_party/zlib -I../../v8/include -I../../third_party/libjpeg_turbo -I../../third_party/leveldatabase/src/include -I../../third_party/leveldatabase/src -I../../third_party/harfbuzz-ng/src -isysroot /Developer/SDKs/MacOSX10.5.sdk -O3 -fvisibility=hidden -Werror -Wnewline-eof -mmacosx-version-min=10.5 -arch i386 -Wglobal-constructors -Wall -Wendif-labels -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene -Wno-char-subscripts -Wno-unused-function -Wno-unnamed-type-template-args -Wno-c++11-extensions -Wno-covered-switch-default -Wexit-time-destructors -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics -fno-strict-aliasing -Xclang -load -Xclang /Volumes/data/b/build/slave/webkit-mac-latest-rel/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang skip-virtuals-in-implementations -fcolor-diagnostics -include ../../third_party/WebKit/Source/WebCore/WebCorePrefix.h -c ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp -o obj/third_party/WebKit/Source/WebCore/platform/graphics/chromium/webcore_platform.ContentLayerChromium.o
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:35:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:37:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:31:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/LayerChromium.h:38:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/GraphicsContext.h:31:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/DashArray.h:29:
In file included from ../../third_party/WebKit/Source/WTF/wtf/Vector.h:30:
In file included from ../../third_party/WebKit/Source/WTF/wtf/VectorTraits.h:24:
In file included from ../../third_party/WebKit/Source/WTF/wtf/OwnPtr.h:26:
../../third_party/WebKit/Source/WTF/wtf/OwnPtrCommon.h:56:13:error: delete called on 'WebCore::CCRenderSurface' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
            delete ptr;
            ^
../../third_party/WebKit/Source/WTF/wtf/OwnPtr.h:100:9: note: in instantiation of function template specialization 'WTF::deleteOwnedPtr<WebCore::CCRenderSurface>' requested here
        deleteOwnedPtr(ptr);
        ^
../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:166:49: note: in instantiation of member function 'WTF::OwnPtr<WebCore::CCRenderSurface>::clear' requested here
    void clearRenderSurface() { m_renderSurface.clear(); }
                                                ^
1 error generated.
FAILED: clang++ -MMD -MF obj/third_party/WebKit/Source/WebCore/platform/graphics/chromium/webcore_platform.GraphicsLayerChromium.o.d -DCHROMIUM_BUILD -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_TASK_MANAGER=1 -DENABLE_WEB_INTENTS=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PROTECTOR_SERVICE=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_PROMO_RESOURCE_SERVICE=1 -DENABLE_AUTOMATION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DWEBKIT_IMPLEMENTATION=1 '-DWEBCORE_NAVIGATOR_VENDOR="Google Inc."' '-DWEBCORE_NAVIGATOR_PLATFORM="MacIntel"' -DWebCascadeList=ChromiumWebCoreObjCWebCascadeList -DWebScrollbarPrefsObserver=ChromiumWebCoreObjCWebScrollbarPrefsObserver -DWebCoreRenderThemeNotificationObserver=ChromiumWebCoreObjCWebCoreRenderThemeNotificationObserver -DWebFontCache=ChromiumWebCoreObjCWebFontCache -DWebScrollAnimationHelperDelegate=ChromiumWebCoreObjCWebScrollAnimationHelperDelegate -DWebScrollbarPainterControllerDelegate=ChromiumWebCoreObjCWebScrollbarPainterControllerDelegate -DWebScrollbarPainterDelegate=ChromiumWebCoreObjCWebScrollbarPainterDelegate -DWebScrollbarPartAnimation=ChromiumWebCoreObjCWebScrollbarPartAnimation -DENABLE_3D_PLUGIN=1 -DENABLE_BATTERY_STATUS=0 -DENABLE_BLOB=1 -DENABLE_BLOB_SLICE=1 -DENABLE_CHANNEL_MESSAGING=1 -DENABLE_CSS3_FLEXBOX=1 -DENABLE_CSS_BOX_DECORATION_BREAK=1 -DENABLE_CSS_EXCLUSIONS=1 -DENABLE_CSS_FILTERS=1 -DENABLE_CSS_IMAGE_SET=1 -DENABLE_CSS_IMAGE_RESOLUTION=0 -DENABLE_CSS_REGIONS=1 -DENABLE_CSS_SHADERS=1 -DENABLE_CSS_VARIABLES=0 -DENABLE_CUSTOM_SCHEME_HANDLER=0 -DENABLE_DATALIST=1 -DENABLE_DASHBOARD_SUPPORT=0 -DENABLE_DATA_TRANSFER_ITEMS=1 -DENABLE_DETAILS=1 -DENABLE_DEVICE_ORIENTATION=1 -DENABLE_DIRECTORY_UPLOAD=1 -DENABLE_DOWNLOAD_ATTRIBUTE=1 -DENABLE_ENCRYPTED_MEDIA=1 -DENABLE_FILE_SYSTEM=1 -DENABLE_FILTERS=1 -DENABLE_FULLSCREEN_API=1 -DENABLE_GAMEPAD=1 -DENABLE_GEOLOCATION=1 -DENABLE_GESTURE_EVENTS=1 -DENABLE_ICONDATABASE=0 -DENABLE_IFRAME_SEAMLESS=1 -DENABLE_INDEXED_DATABASE=1 -DENABLE_INPUT_TYPE_DATE=1 -DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_LEGACY_CSS_VENDOR_PREFIXES=0 -DENABLE_LEGACY_WEBKIT_BLOB_BUILDER=1 -DENABLE_LINK_PREFETCH=1 -DENABLE_LINK_PRERENDER=1 -DENABLE_MEDIA_SOURCE=1 -DENABLE_MEDIA_STATISTICS=1 -DENABLE_METER_TAG=1 -DENABLE_MHTML=1 -DENABLE_MICRODATA=0 -DENABLE_MUTATION_OBSERVERS=1 -DENABLE_PAGE_VISIBILITY_API=1 -DENABLE_POINTER_LOCK=1 -DENABLE_PROGRESS_TAG=1 -DENABLE_QUOTA=1 -DENABLE_REGISTER_PROTOCOL_HANDLER=1 -DENABLE_REQUEST_ANIMATION_FRAME=1 -DENABLE_RUBY=1 -DENABLE_SANDBOX=1 -DENABLE_SCRIPTED_SPEECH=1 -DENABLE_SHADOW_DOM=1 -DENABLE_SMOOTH_SCROLLING=1 -DENABLE_SQL_DATABASE=1 -DENABLE_STYLE_SCOPED=1 -DENABLE_SVG=1 -DENABLE_SVG_FONTS=1 -DENABLE_TOUCH_ADJUSTMENT=1 -DENABLE_TOUCH_EVENTS=1 -DENABLE_TOUCH_ICON_LOADING=0 -DENABLE_V8_SCRIPT_DEBUG_SERVER=1 -DENABLE_VIDEO=1 -DENABLE_VIDEO_TRACK=1 -DENABLE_VIEWPORT=1 -DENABLE_WEBGL=1 -DENABLE_WEB_SOCKETS=1 -DENABLE_WEB_TIMING=1 -DENABLE_WORKERS=1 -DENABLE_XHR_RESPONSE_BLOB=1 -DENABLE_XSLT=1 -DWTF_USE_LEVELDB=1 -DWTF_USE_BUILTIN_UTF8_CODEC=1 -DWTF_USE_OPENTYPE_SANITIZER=1 -DWTF_USE_RTL_SCROLLBAR=1 -DWTF_USE_SKIA_TEXT=1 -DWTF_USE_WEBP=1 -DWTF_USE_WEBKIT_IMAGE_DECODERS=1 -DENABLE_CALENDAR_PICKER=1 -DENABLE_FONT_BOOSTING=0 -DENABLE_FULLSCREEN_MEDIA_CONTROLS=1 -DENABLE_INPUT_SPEECH=1 -DENABLE_JAVASCRIPT_I18N_API=1 -DENABLE_LEGACY_NOTIFICATIONS=1 -DENABLE_MEDIA_CAPTURE=0 -DENABLE_MEDIA_STREAM=1 -DENABLE_NOTIFICATIONS=1 -DENABLE_ORIENTATION_EVENTS=0 -DENABLE_OVERFLOW_SCROLLING=0 -DENABLE_PAGE_POPUP=1 -DENABLE_SHARED_WORKERS=1 -DENABLE_WEB_AUDIO=1 -DENABLE_INPUT_TYPE_COLOR=1 -DWTF_USE_ACCELERATED_COMPOSITING=1 -DENABLE_3D_RENDERING=1 -DENABLE_ACCELERATED_2D_CANVAS=1 -DENABLE_RUBBER_BANDING=1 -DWTF_USE_SKIA_ON_MAC_CHROMIUM=1 -DBUILDING_CHROMIUM__=1 -DUSE_SYSTEM_MALLOC=1 -DWTF_USE_NEW_THEME=1 -DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -DSK_BUILD_NO_IMAGE_ENCODE '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DGR_AGGRESSIVE_SHADER_OPTS=1 -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG -DLIBXML_STATIC -DLIBXSLT_STATIC -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../../third_party/icu/public/common -I../../third_party/icu/public/i18n -I../../third_party/apple_webkit -I../../third_party/khronos -I../.. -Iobj/third_party/WebKit/Source/WebCore/WebCore.gyp/webcore_platform.gen -I../../third_party/WebKit/Source/WebCore -I../../third_party/WebKit/Source -I../../third_party/WebKit/Source/WebCore/Modules/battery -I../../third_party/WebKit/Source/WebCore/Modules/filesystem -I../../third_party/WebKit/Source/WebCore/Modules/filesystem/chromium -I../../third_party/WebKit/Source/WebCore/Modules/gamepad -I../../third_party/WebKit/Source/WebCore/Modules/geolocation -I../../third_party/WebKit/Source/WebCore/Modules/intents -I../../third_party/WebKit/Source/WebCore/Modules/indexeddb -I../../third_party/WebKit/Source/WebCore/Modules/mediastream -I../../third_party/WebKit/Source/WebCore/Modules/quota -I../../third_party/WebKit/Source/WebCore/Modules/speech -I../../third_party/WebKit/Source/WebCore/Modules/webaudio -I../../third_party/WebKit/Source/WebCore/Modules/webdatabase -I../../third_party/WebKit/Source/WebCore/Modules/webdatabase/chromium -I../../third_party/WebKit/Source/WebCore/Modules/websockets -I../../third_party/WebKit/Source/WebCore/accessibility -I../../third_party/WebKit/Source/WebCore/accessibility/chromium -I../../third_party/WebKit/Source/WebCore/bindings -I../../third_party/WebKit/Source/WebCore/bindings/generic -I../../third_party/WebKit/Source/WebCore/bindings/v8 -I../../third_party/WebKit/Source/WebCore/bindings/v8/custom -I../../third_party/WebKit/Source/WebCore/bindings/v8/specialization -I../../third_party/WebKit/Source/WebCore/bridge -I../../third_party/WebKit/Source/WebCore/bridge/jni -I../../third_party/WebKit/Source/WebCore/bridge/jni/v8 -I../../third_party/WebKit/Source/WebCore/css -I../../third_party/WebKit/Source/WebCore/dom -I../../third_party/WebKit/Source/WebCore/dom/default -I../../third_party/WebKit/Source/WebCore/editing -I../../third_party/WebKit/Source/WebCore/fileapi -I../../third_party/WebKit/Source/WebCore/history -I../../third_party/WebKit/Source/WebCore/html -I../../third_party/WebKit/Source/WebCore/html/canvas -I../../third_party/WebKit/Source/WebCore/html/parser -I../../third_party/WebKit/Source/WebCore/html/shadow -I../../third_party/WebKit/Source/WebCore/html/track -I../../third_party/WebKit/Source/WebCore/inspector -I../../third_party/WebKit/Source/WebCore/loader -I../../third_party/WebKit/Source/WebCore/loader/appcache -I../../third_party/WebKit/Source/WebCore/loader/archive -I../../third_party/WebKit/Source/WebCore/loader/archive/cf -I../../third_party/WebKit/Source/WebCore/loader/archive/mhtml -I../../third_party/WebKit/Source/WebCore/loader/cache -I../../third_party/WebKit/Source/WebCore/loader/icon -I../../third_party/WebKit/Source/WebCore/mathml -I../../third_party/WebKit/Source/WebCore/notifications -I../../third_party/WebKit/Source/WebCore/page -I../../third_party/WebKit/Source/WebCore/page/animation -I../../third_party/WebKit/Source/WebCore/page/chromium -I../../third_party/WebKit/Source/WebCore/page/scrolling -I../../third_party/WebKit/Source/WebCore/platform -I../../third_party/WebKit/Source/WebCore/platform/animation -I../../third_party/WebKit/Source/WebCore/platform/audio -I../../third_party/WebKit/Source/WebCore/platform/audio/chromium -I../../third_party/WebKit/Source/WebCore/platform/chromium -I../../third_party/WebKit/Source/WebCore/platform/chromium/support -I../../third_party/WebKit/Source/WebCore/platform/graphics -I../../third_party/WebKit/Source/WebCore/platform/graphics/chromium -I../../third_party/WebKit/Source/WebCore/platform/graphics/filters -I../../third_party/WebKit/Source/WebCore/platform/graphics/filters/arm -I../../third_party/WebKit/Source/WebCore/platform/graphics/gpu -I../../third_party/WebKit/Source/WebCore/platform/graphics/opentype -I../../third_party/WebKit/Source/WebCore/platform/graphics/skia -I../../third_party/WebKit/Source/WebCore/platform/graphics/transforms -I../../third_party/WebKit/Source/WebCore/platform/image-decoders -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/bmp -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/gif -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/ico -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/jpeg -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/png -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/skia -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/webp -I../../third_party/WebKit/Source/WebCore/platform/image-encoders/skia -I../../third_party/WebKit/Source/WebCore/platform/leveldb -I../../third_party/WebKit/Source/WebCore/platform/mediastream -I../../third_party/WebKit/Source/WebCore/platform/mediastream/chromium -I../../third_party/WebKit/Source/WebCore/platform/mock -I../../third_party/WebKit/Source/WebCore/platform/network -I../../third_party/WebKit/Source/WebCore/platform/network/chromium -I../../third_party/WebKit/Source/WebCore/platform/sql -I../../third_party/WebKit/Source/WebCore/platform/text -I../../third_party/WebKit/Source/WebCore/platform/text/transcoder -I../../third_party/WebKit/Source/WebCore/plugins -I../../third_party/WebKit/Source/WebCore/plugins/chromium -I../../third_party/WebKit/Source/WebCore/rendering -I../../third_party/WebKit/Source/WebCore/rendering/style -I../../third_party/WebKit/Source/WebCore/rendering/svg -I../../third_party/WebKit/Source/WebCore/storage -I../../third_party/WebKit/Source/WebCore/storage/chromium -I../../third_party/WebKit/Source/WebCore/svg -I../../third_party/WebKit/Source/WebCore/svg/animation -I../../third_party/WebKit/Source/WebCore/svg/graphics -I../../third_party/WebKit/Source/WebCore/svg/graphics/filters -I../../third_party/WebKit/Source/WebCore/svg/properties -I../../third_party/WebKit/Source/ThirdParty/glu -I../../third_party/WebKit/Source/WebCore/workers -I../../third_party/WebKit/Source/WebCore/xml -I../../third_party/WebKit/Source/WebCore/xml/parser -I../../third_party/WebKit/Source/WebCore/platform/audio/mac -I../../third_party/WebKit/Source/WebCore/platform/cocoa -I../../third_party/WebKit/Source/WebCore/platform/graphics/cg -I../../third_party/WebKit/Source/WebCore/platform/graphics/cocoa -I../../third_party/WebKit/Source/WebCore/platform/graphics/mac -I../../third_party/WebKit/Source/WebCore/platform/mac -I../../third_party/WebKit/Source/WebCore/platform/text/mac -I../../third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz -I../../third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/ng -I../../gpu -I../../third_party/angle/include/GLSLANG -Igen/webkit -Igen/webkit/bindings -I../../third_party/WebKit/Source/WTF -I../../third_party/WebKit/Source/JavaScriptCore -I../../third_party/WebKit/Source/Platform/chromium -Igen/webcore_headers -I../../skia/config -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu -I../../third_party/skia/include/gpu/gl -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../skia/ext -I../../third_party/skia/include/utils/mac -I../../third_party/iccjpeg -I../../third_party/libwebp -I../../third_party/libpng -I../../third_party/libxml/mac/include -I../../third_party/libxml/src/include -I../../third_party/libxslt -I../../third_party/npapi -I../../third_party/npapi/bindings -I../../third_party/ots/include -I../../third_party/qcms/src -I../../third_party/sqlite -I../../third_party/zlib -I../../v8/include -I../../third_party/libjpeg_turbo -I../../third_party/leveldatabase/src/include -I../../third_party/leveldatabase/src -I../../third_party/harfbuzz-ng/src -isysroot /Developer/SDKs/MacOSX10.5.sdk -O3 -fvisibility=hidden -Werror -Wnewline-eof -mmacosx-version-min=10.5 -arch i386 -Wglobal-constructors -Wall -Wendif-labels -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene -Wno-char-subscripts -Wno-unused-function -Wno-unnamed-type-template-args -Wno-c++11-extensions -Wno-covered-switch-default -Wexit-time-destructors -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics -fno-strict-aliasing -Xclang -load -Xclang /Volumes/data/b/build/slave/webkit-mac-latest-rel/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang skip-virtuals-in-implementations -fcolor-diagnostics -include ../../third_party/WebKit/Source/WebCore/WebCorePrefix.h -c ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp -o obj/third_party/WebKit/Source/WebCore/platform/graphics/chromium/webcore_platform.GraphicsLayerChromium.o
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:46:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:36:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:37:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:31:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/LayerChromium.h:38:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/GraphicsContext.h:31:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/DashArray.h:29:
In file included from ../../third_party/WebKit/Source/WTF/wtf/Vector.h:30:
In file included from ../../third_party/WebKit/Source/WTF/wtf/VectorTraits.h:24:
In file included from ../../third_party/WebKit/Source/WTF/wtf/OwnPtr.h:26:
../../third_party/WebKit/Source/WTF/wtf/OwnPtrCommon.h:56:13:error: delete called on 'WebCore::CCRenderSurface' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
            delete ptr;
            ^
../../third_party/WebKit/Source/WTF/wtf/OwnPtr.h:100:9: note: in instantiation of function template specialization 'WTF::deleteOwnedPtr<WebCore::CCRenderSurface>' requested here
        deleteOwnedPtr(ptr);
        ^
../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:166:49: note: in instantiation of member function 'WTF::OwnPtr<WebCore::CCRenderSurface>::clear' requested here
    void clearRenderSurface() { m_renderSurface.clear(); }
                                                ^
1 error generated.
FAILED: clang++ -MMD -MF obj/third_party/WebKit/Source/WebCore/platform/graphics/chromium/webcore_platform.OpaqueRectTrackingContentLayerDelegate.o.d -DCHROMIUM_BUILD -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_TASK_MANAGER=1 -DENABLE_WEB_INTENTS=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PROTECTOR_SERVICE=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_PROMO_RESOURCE_SERVICE=1 -DENABLE_AUTOMATION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DWEBKIT_IMPLEMENTATION=1 '-DWEBCORE_NAVIGATOR_VENDOR="Google Inc."' '-DWEBCORE_NAVIGATOR_PLATFORM="MacIntel"' -DWebCascadeList=ChromiumWebCoreObjCWebCascadeList -DWebScrollbarPrefsObserver=ChromiumWebCoreObjCWebScrollbarPrefsObserver -DWebCoreRenderThemeNotificationObserver=ChromiumWebCoreObjCWebCoreRenderThemeNotificationObserver -DWebFontCache=ChromiumWebCoreObjCWebFontCache -DWebScrollAnimationHelperDelegate=ChromiumWebCoreObjCWebScrollAnimationHelperDelegate -DWebScrollbarPainterControllerDelegate=ChromiumWebCoreObjCWebScrollbarPainterControllerDelegate -DWebScrollbarPainterDelegate=ChromiumWebCoreObjCWebScrollbarPainterDelegate -DWebScrollbarPartAnimation=ChromiumWebCoreObjCWebScrollbarPartAnimation -DENABLE_3D_PLUGIN=1 -DENABLE_BATTERY_STATUS=0 -DENABLE_BLOB=1 -DENABLE_BLOB_SLICE=1 -DENABLE_CHANNEL_MESSAGING=1 -DENABLE_CSS3_FLEXBOX=1 -DENABLE_CSS_BOX_DECORATION_BREAK=1 -DENABLE_CSS_EXCLUSIONS=1 -DENABLE_CSS_FILTERS=1 -DENABLE_CSS_IMAGE_SET=1 -DENABLE_CSS_IMAGE_RESOLUTION=0 -DENABLE_CSS_REGIONS=1 -DENABLE_CSS_SHADERS=1 -DENABLE_CSS_VARIABLES=0 -DENABLE_CUSTOM_SCHEME_HANDLER=0 -DENABLE_DATALIST=1 -DENABLE_DASHBOARD_SUPPORT=0 -DENABLE_DATA_TRANSFER_ITEMS=1 -DENABLE_DETAILS=1 -DENABLE_DEVICE_ORIENTATION=1 -DENABLE_DIRECTORY_UPLOAD=1 -DENABLE_DOWNLOAD_ATTRIBUTE=1 -DENABLE_ENCRYPTED_MEDIA=1 -DENABLE_FILE_SYSTEM=1 -DENABLE_FILTERS=1 -DENABLE_FULLSCREEN_API=1 -DENABLE_GAMEPAD=1 -DENABLE_GEOLOCATION=1 -DENABLE_GESTURE_EVENTS=1 -DENABLE_ICONDATABASE=0 -DENABLE_IFRAME_SEAMLESS=1 -DENABLE_INDEXED_DATABASE=1 -DENABLE_INPUT_TYPE_DATE=1 -DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_LEGACY_CSS_VENDOR_PREFIXES=0 -DENABLE_LEGACY_WEBKIT_BLOB_BUILDER=1 -DENABLE_LINK_PREFETCH=1 -DENABLE_LINK_PRERENDER=1 -DENABLE_MEDIA_SOURCE=1 -DENABLE_MEDIA_STATISTICS=1 -DENABLE_METER_TAG=1 -DENABLE_MHTML=1 -DENABLE_MICRODATA=0 -DENABLE_MUTATION_OBSERVERS=1 -DENABLE_PAGE_VISIBILITY_API=1 -DENABLE_POINTER_LOCK=1 -DENABLE_PROGRESS_TAG=1 -DENABLE_QUOTA=1 -DENABLE_REGISTER_PROTOCOL_HANDLER=1 -DENABLE_REQUEST_ANIMATION_FRAME=1 -DENABLE_RUBY=1 -DENABLE_SANDBOX=1 -DENABLE_SCRIPTED_SPEECH=1 -DENABLE_SHADOW_DOM=1 -DENABLE_SMOOTH_SCROLLING=1 -DENABLE_SQL_DATABASE=1 -DENABLE_STYLE_SCOPED=1 -DENABLE_SVG=1 -DENABLE_SVG_FONTS=1 -DENABLE_TOUCH_ADJUSTMENT=1 -DENABLE_TOUCH_EVENTS=1 -DENABLE_TOUCH_ICON_LOADING=0 -DENABLE_V8_SCRIPT_DEBUG_SERVER=1 -DENABLE_VIDEO=1 -DENABLE_VIDEO_TRACK=1 -DENABLE_VIEWPORT=1 -DENABLE_WEBGL=1 -DENABLE_WEB_SOCKETS=1 -DENABLE_WEB_TIMING=1 -DENABLE_WORKERS=1 -DENABLE_XHR_RESPONSE_BLOB=1 -DENABLE_XSLT=1 -DWTF_USE_LEVELDB=1 -DWTF_USE_BUILTIN_UTF8_CODEC=1 -DWTF_USE_OPENTYPE_SANITIZER=1 -DWTF_USE_RTL_SCROLLBAR=1 -DWTF_USE_SKIA_TEXT=1 -DWTF_USE_WEBP=1 -DWTF_USE_WEBKIT_IMAGE_DECODERS=1 -DENABLE_CALENDAR_PICKER=1 -DENABLE_FONT_BOOSTING=0 -DENABLE_FULLSCREEN_MEDIA_CONTROLS=1 -DENABLE_INPUT_SPEECH=1 -DENABLE_JAVASCRIPT_I18N_API=1 -DENABLE_LEGACY_NOTIFICATIONS=1 -DENABLE_MEDIA_CAPTURE=0 -DENABLE_MEDIA_STREAM=1 -DENABLE_NOTIFICATIONS=1 -DENABLE_ORIENTATION_EVENTS=0 -DENABLE_OVERFLOW_SCROLLING=0 -DENABLE_PAGE_POPUP=1 -DENABLE_SHARED_WORKERS=1 -DENABLE_WEB_AUDIO=1 -DENABLE_INPUT_TYPE_COLOR=1 -DWTF_USE_ACCELERATED_COMPOSITING=1 -DENABLE_3D_RENDERING=1 -DENABLE_ACCELERATED_2D_CANVAS=1 -DENABLE_RUBBER_BANDING=1 -DWTF_USE_SKIA_ON_MAC_CHROMIUM=1 -DBUILDING_CHROMIUM__=1 -DUSE_SYSTEM_MALLOC=1 -DWTF_USE_NEW_THEME=1 -DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -DSK_BUILD_NO_IMAGE_ENCODE '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DGR_AGGRESSIVE_SHADER_OPTS=1 -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG -DLIBXML_STATIC -DLIBXSLT_STATIC -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../../third_party/icu/public/common -I../../third_party/icu/public/i18n -I../../third_party/apple_webkit -I../../third_party/khronos -I../.. -Iobj/third_party/WebKit/Source/WebCore/WebCore.gyp/webcore_platform.gen -I../../third_party/WebKit/Source/WebCore -I../../third_party/WebKit/Source -I../../third_party/WebKit/Source/WebCore/Modules/battery -I../../third_party/WebKit/Source/WebCore/Modules/filesystem -I../../third_party/WebKit/Source/WebCore/Modules/filesystem/chromium -I../../third_party/WebKit/Source/WebCore/Modules/gamepad -I../../third_party/WebKit/Source/WebCore/Modules/geolocation -I../../third_party/WebKit/Source/WebCore/Modules/intents -I../../third_party/WebKit/Source/WebCore/Modules/indexeddb -I../../third_party/WebKit/Source/WebCore/Modules/mediastream -I../../third_party/WebKit/Source/WebCore/Modules/quota -I../../third_party/WebKit/Source/WebCore/Modules/speech -I../../third_party/WebKit/Source/WebCore/Modules/webaudio -I../../third_party/WebKit/Source/WebCore/Modules/webdatabase -I../../third_party/WebKit/Source/WebCore/Modules/webdatabase/chromium -I../../third_party/WebKit/Source/WebCore/Modules/websockets -I../../third_party/WebKit/Source/WebCore/accessibility -I../../third_party/WebKit/Source/WebCore/accessibility/chromium -I../../third_party/WebKit/Source/WebCore/bindings -I../../third_party/WebKit/Source/WebCore/bindings/generic -I../../third_party/WebKit/Source/WebCore/bindings/v8 -I../../third_party/WebKit/Source/WebCore/bindings/v8/custom -I../../third_party/WebKit/Source/WebCore/bindings/v8/specialization -I../../third_party/WebKit/Source/WebCore/bridge -I../../third_party/WebKit/Source/WebCore/bridge/jni -I../../third_party/WebKit/Source/WebCore/bridge/jni/v8 -I../../third_party/WebKit/Source/WebCore/css -I../../third_party/WebKit/Source/WebCore/dom -I../../third_party/WebKit/Source/WebCore/dom/default -I../../third_party/WebKit/Source/WebCore/editing -I../../third_party/WebKit/Source/WebCore/fileapi -I../../third_party/WebKit/Source/WebCore/history -I../../third_party/WebKit/Source/WebCore/html -I../../third_party/WebKit/Source/WebCore/html/canvas -I../../third_party/WebKit/Source/WebCore/html/parser -I../../third_party/WebKit/Source/WebCore/html/shadow -I../../third_party/WebKit/Source/WebCore/html/track -I../../third_party/WebKit/Source/WebCore/inspector -I../../third_party/WebKit/Source/WebCore/loader -I../../third_party/WebKit/Source/WebCore/loader/appcache -I../../third_party/WebKit/Source/WebCore/loader/archive -I../../third_party/WebKit/Source/WebCore/loader/archive/cf -I../../third_party/WebKit/Source/WebCore/loader/archive/mhtml -I../../third_party/WebKit/Source/WebCore/loader/cache -I../../third_party/WebKit/Source/WebCore/loader/icon -I../../third_party/WebKit/Source/WebCore/mathml -I../../third_party/WebKit/Source/WebCore/notifications -I../../third_party/WebKit/Source/WebCore/page -I../../third_party/WebKit/Source/WebCore/page/animation -I../../third_party/WebKit/Source/WebCore/page/chromium -I../../third_party/WebKit/Source/WebCore/page/scrolling -I../../third_party/WebKit/Source/WebCore/platform -I../../third_party/WebKit/Source/WebCore/platform/animation -I../../third_party/WebKit/Source/WebCore/platform/audio -I../../third_party/WebKit/Source/WebCore/platform/audio/chromium -I../../third_party/WebKit/Source/WebCore/platform/chromium -I../../third_party/WebKit/Source/WebCore/platform/chromium/support -I../../third_party/WebKit/Source/WebCore/platform/graphics -I../../third_party/WebKit/Source/WebCore/platform/graphics/chromium -I../../third_party/WebKit/Source/WebCore/platform/graphics/filters -I../../third_party/WebKit/Source/WebCore/platform/graphics/filters/arm -I../../third_party/WebKit/Source/WebCore/platform/graphics/gpu -I../../third_party/WebKit/Source/WebCore/platform/graphics/opentype -I../../third_party/WebKit/Source/WebCore/platform/graphics/skia -I../../third_party/WebKit/Source/WebCore/platform/graphics/transforms -I../../third_party/WebKit/Source/WebCore/platform/image-decoders -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/bmp -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/gif -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/ico -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/jpeg -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/png -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/skia -I../../third_party/WebKit/Source/WebCore/platform/image-decoders/webp -I../../third_party/WebKit/Source/WebCore/platform/image-encoders/skia -I../../third_party/WebKit/Source/WebCore/platform/leveldb -I../../third_party/WebKit/Source/WebCore/platform/mediastream -I../../third_party/WebKit/Source/WebCore/platform/mediastream/chromium -I../../third_party/WebKit/Source/WebCore/platform/mock -I../../third_party/WebKit/Source/WebCore/platform/network -I../../third_party/WebKit/Source/WebCore/platform/network/chromium -I../../third_party/WebKit/Source/WebCore/platform/sql -I../../third_party/WebKit/Source/WebCore/platform/text -I../../third_party/WebKit/Source/WebCore/platform/text/transcoder -I../../third_party/WebKit/Source/WebCore/plugins -I../../third_party/WebKit/Source/WebCore/plugins/chromium -I../../third_party/WebKit/Source/WebCore/rendering -I../../third_party/WebKit/Source/WebCore/rendering/style -I../../third_party/WebKit/Source/WebCore/rendering/svg -I../../third_party/WebKit/Source/WebCore/storage -I../../third_party/WebKit/Source/WebCore/storage/chromium -I../../third_party/WebKit/Source/WebCore/svg -I../../third_party/WebKit/Source/WebCore/svg/animation -I../../third_party/WebKit/Source/WebCore/svg/graphics -I../../third_party/WebKit/Source/WebCore/svg/graphics/filters -I../../third_party/WebKit/Source/WebCore/svg/properties -I../../third_party/WebKit/Source/ThirdParty/glu -I../../third_party/WebKit/Source/WebCore/workers -I../../third_party/WebKit/Source/WebCore/xml -I../../third_party/WebKit/Source/WebCore/xml/parser -I../../third_party/WebKit/Source/WebCore/platform/audio/mac -I../../third_party/WebKit/Source/WebCore/platform/cocoa -I../../third_party/WebKit/Source/WebCore/platform/graphics/cg -I../../third_party/WebKit/Source/WebCore/platform/graphics/cocoa -I../../third_party/WebKit/Source/WebCore/platform/graphics/mac -I../../third_party/WebKit/Source/WebCore/platform/mac -I../../third_party/WebKit/Source/WebCore/platform/text/mac -I../../third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz -I../../third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/ng -I../../gpu -I../../third_party/angle/include/GLSLANG -Igen/webkit -Igen/webkit/bindings -I../../third_party/WebKit/Source/WTF -I../../third_party/WebKit/Source/JavaScriptCore -I../../third_party/WebKit/Source/Platform/chromium -Igen/webcore_headers -I../../skia/config -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu -I../../third_party/skia/include/gpu/gl -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../skia/ext -I../../third_party/skia/include/utils/mac -I../../third_party/iccjpeg -I../../third_party/libwebp -I../../third_party/libpng -I../../third_party/libxml/mac/include -I../../third_party/libxml/src/include -I../../third_party/libxslt -I../../third_party/npapi -I../../third_party/npapi/bindings -I../../third_party/ots/include -I../../third_party/qcms/src -I../../third_party/sqlite -I../../third_party/zlib -I../../v8/include -I../../third_party/libjpeg_turbo -I../../third_party/leveldatabase/src/include -I../../third_party/leveldatabase/src -I../../third_party/harfbuzz-ng/src -isysroot /Developer/SDKs/MacOSX10.5.sdk -O3 -fvisibility=hidden -Werror -Wnewline-eof -mmacosx-version-min=10.5 -arch i386 -Wglobal-constructors -Wall -Wendif-labels -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene -Wno-char-subscripts -Wno-unused-function -Wno-unnamed-type-template-args -Wno-c++11-extensions -Wno-covered-switch-default -Wexit-time-destructors -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics -fno-strict-aliasing -Xclang -load -Xclang /Volumes/data/b/build/slave/webkit-mac-latest-rel/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang skip-virtuals-in-implementations -fcolor-diagnostics -include ../../third_party/WebKit/Source/WebCore/WebCorePrefix.h -c ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/OpaqueRectTrackingContentLayerDelegate.cpp -o obj/third_party/WebKit/Source/WebCore/platform/graphics/chromium/webcore_platform.OpaqueRectTrackingContentLayerDelegate.o

Created attachment 147999 [details]
Patch

[WebKit Review Bot]

Comment on attachment 147999 [details]
Patch

Clearing flags on attachment: 147999

Committed r120553: <http://trac.webkit.org/changeset/120553>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.