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.
Created attachment 133138 [details] Patch
(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.
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?
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.
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.
(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.
Created attachment 133312 [details] Addressed reviewers comments This patch opts for option A
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.
Committed r111752: <http://trac.webkit.org/changeset/111752>