WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
81831
[chromium] Make CCDamageTracker robust to empty layer lists
https://bugs.webkit.org/show_bug.cgi?id=81831
Summary
[chromium] Make CCDamageTracker robust to empty layer lists
Shawn Singh
Reported
2012-03-21 14:22:27 PDT
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.
Attachments
Patch
(10.48 KB, patch)
2012-03-21 16:18 PDT
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
Addressed reviewers comments
(10.44 KB, patch)
2012-03-22 12:02 PDT
,
Shawn Singh
enne
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Shawn Singh
Comment 1
2012-03-21 16:18:59 PDT
Created
attachment 133138
[details]
Patch
Shawn Singh
Comment 2
2012-03-21 16:22:38 PDT
(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 3
2012-03-21 20:42:31 PDT
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
Comment 4
2012-03-21 21:38:45 PDT
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 5
2012-03-22 00:46:52 PDT
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
Comment 6
2012-03-22 02:33:53 PDT
(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
Comment 7
2012-03-22 12:02:34 PDT
Created
attachment 133312
[details]
Addressed reviewers comments This patch opts for option A
Adrienne Walker
Comment 8
2012-03-22 13:34:06 PDT
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
Comment 9
2012-03-22 13:43:02 PDT
Committed
r111752
: <
http://trac.webkit.org/changeset/111752
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug