Bug 81831 – [chromium] Make CCDamageTracker robust to empty layer lists

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

Minor fixes that do not change any behavior.  In particular, we need to protect users against accidental crashes if layerList is empty.  On the other hand, it is completely unexpected if the layerList is empty so there should still be a assertion to trigger in debug mode.

Those crashes have been reported in http://code.google.com/p/chromium/issues/detail?id=119353 and it might be appropriate to wait for landing this until after we either (1) repro and solve or (2) give up on trying to reproduce the crashes.

[Shawn Singh]

Created attachment 133138 [details]
Patch

[Shawn Singh]

(In reply to comment #0)
> 
> Those crashes have been reported in http://code.google.com/p/chromium/issues/detail?id=119353 and it might be appropriate to wait for landing this until after we either (1) repro and solve or (2) give up on trying to reproduce the crashes.

FYI, we did give up on that bug (you can see the details on that bug), so this patch is ready for review/land.

[Adrienne Walker]

Comment on attachment 133138 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [chromium] CCDamageTracker code maintenence

Can you give this bug a better description of what and why you're fixing? Something like "Make CCDamageTracker robust to empty layer lists"?

> Source/WebCore/ChangeLog:11
> +        bug outside the damage tracker, but for robustness its appropriate

s/its/it's/

> Source/WebCore/ChangeLog:14
> +        In addition to this fix, performed some trivial maintenence on the

s/maintenence/maintenance/

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:59
> -void CCDamageTracker::updateDamageTrackingState(const Vector<CCLayerImpl*>& layerList, int targetSurfaceLayerID, CCLayerImpl* targetSurfaceMaskLayer)
> +void CCDamageTracker::updateDamageTrackingState(const Vector<CCLayerImpl*>& layerList, int targetSurfaceLayerID, bool targetSurfacePropertyChangedOnlyFromDescendant, const IntRect& targetSurfaceContentRect, CCLayerImpl* targetSurfaceMaskLayer)

This is a drive-by API question, but I'm curious if this function should just take the render surface layer, rather than five properties that can be gotten off of it? Is there some time that you call it with different values other than what's on the render surface layer?

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:808
> +    // FIXME: check the damage rect is empty in this case.
> +    FloatRect damageRect = targetSurface->damageTracker()->currentDamageRect();
> +    EXPECT_TRUE(damageRect.isEmpty());

Why is this a FIXME? It kind of looks like you're doing what it says already?

[Shawn Singh]

Thanks for the quick review =)  
I'll make all those fixes.  One point remains, though - I'll wait for you to give me the final decision of your preference.

> > Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:59
> > -void CCDamageTracker::updateDamageTrackingState(const Vector<CCLayerImpl*>& layerList, int targetSurfaceLayerID, CCLayerImpl* targetSurfaceMaskLayer)
> > +void CCDamageTracker::updateDamageTrackingState(const Vector<CCLayerImpl*>& layerList, int targetSurfaceLayerID, bool targetSurfacePropertyChangedOnlyFromDescendant, const IntRect& targetSurfaceContentRect, CCLayerImpl* targetSurfaceMaskLayer)
> 
> This is a drive-by API question, but I'm curious if this function should just take the render surface layer, rather than five properties that can be gotten off of it? Is there some time that you call it with different values other than what's on the render surface layer?

nduca was against it.  If I remember correctly, his thought process was that the damage tracker should not know about the surface that owns it, unless its really necessary.  Its a good point, and frankly I don't think the API on this class has stablized yet anyway, it will still evolve and improve a bit more with hindsight.  So, whichever option you prefer for this patch is OK with me =)

option A: keep the large number of args
option B: pass only the rendersurface itself, and add a maskLayer() accessor to CCRenderSurface.

> 
> > Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:808
> > +    // FIXME: check the damage rect is empty in this case.
> > +    FloatRect damageRect = targetSurface->damageTracker()->currentDamageRect();
> > +    EXPECT_TRUE(damageRect.isEmpty());
> 
> Why is this a FIXME? It kind of looks like you're doing what it says already?

oops =) I'll remove that comment.  A vestige of having to stash and switch contexts while I wrote this.

[Shawn Singh]

Comment on attachment 133138 [details]
Patch

removing r=? and cq=? since a new patch will be coming soon.

But, if you guys had a preference about the too-many-args issue, let me know =)  I'll submit a new patch tomorrow.

[Nat Duca]

(In reply to comment #5)
> (From update of attachment 133138 [details])
> removing r=? and cq=? since a new patch will be coming soon.
> 
> But, if you guys had a preference about the too-many-args issue, let me know =)  I'll submit a new patch tomorrow.

its getting long but I think we want to avoid direct coupling. That changes testability a bit.

[Shawn Singh]

Created attachment 133312 [details]
Addressed reviewers comments

This patch opts for option A

[Adrienne Walker]

Comment on attachment 133312 [details]
Addressed reviewers comments

Looks good! R=me.  Please land this ASAP if possible, due to http://code.google.com/p/chromium/issues/detail?id=119465.

[Shawn Singh]

Committed r111752: <http://trac.webkit.org/changeset/111752>