Bug 84812

Summary: [chromium] Prevent CCLayerImpl::willDraw/didDraw mismatches
Product: WebKit Reporter: Adrienne Walker <enne>
Component: New BugsAssignee: Adrienne Walker <enne>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, enne, eric.carlson, feature-media-reviews, jamesr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 84805    
Attachments:
Description Flags
Patch
none
Patch
none
Patch jamesr: review+

Adrienne Walker
Reported 2012-04-24 18:05:26 PDT
[chromium] Prevent CCLayerImpl::willDraw/didDraw mismatches
Attachments
Patch (27.66 KB, patch)
2012-04-24 18:11 PDT, Adrienne Walker 		no flags
DetailsFormatted DiffDiff
Patch (4.01 KB, patch)
2012-04-24 18:20 PDT, Adrienne Walker 		no flags
DetailsFormatted DiffDiff
Patch (27.49 KB, patch)
2012-04-25 09:14 PDT, Adrienne Walker 		jamesr: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Adrienne Walker
Comment 1 2012-04-24 18:11:40 PDT
Created attachment 138707 [details] Patch
Adrienne Walker
Comment 2 2012-04-24 18:20:17 PDT
Created attachment 138709 [details] Patch
Adrienne Walker
Comment 3 2012-04-24 18:20:41 PDT
(Ack. webkit-patch upload failed. Ignore that patch.)
Adrienne Walker
Comment 4 2012-04-25 09:14:47 PDT
Created attachment 138822 [details] Patch
Adrienne Walker
Comment 5 2012-04-25 09:16:52 PDT
I'm not totally happy with "didDrawAllLayers" as a function name here and will happily take suggestions, since it might be confusing that you call it even if you don't call drawLayers.
Dana Jansens
Comment 6 2012-04-25 09:30:42 PDT
Comment on attachment 138822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138822&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:103 > virtual bool prepareToDraw(FrameData&); > virtual void drawLayers(const FrameData&); > + // Must be called if and only if prepareToDraw was called. > + void didDrawAllLayers(const FrameData&); The naming here is getting funny.. or do you think this is more correct/descriptive? We have: 1. draw 2. drawLayers 3. drawAllLayers
James Robinson
Comment 7 2012-04-25 18:00:36 PDT
Comment on attachment 138822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138822&action=review R=me. > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:354 > + bool m_betweenWillDrawAndDidDraw; guard in #ifndef NDEBUG? i want to make it clear what bits of state are "real" and what are for asserts - i think it'd be pretty weird if we started basing "real" logic on this
Adrienne Walker
Comment 8 2012-04-25 18:47:21 PDT
Committed r115278: <http://trac.webkit.org/changeset/115278>
Adrienne Walker
Comment 9 2012-04-25 18:56:41 PDT
(In reply to comment #8) > Committed r115278: <http://trac.webkit.org/changeset/115278> ^ Guarded things was #ifndef NDEBUG. danakj: I don't know that the name difference is that confusing, even if it's inconsistent since there's only one type of thing we draw. Sorry for leaving the bikeshed painted an ugly color, but I wanted to land this because there's a video crash fix that depends on it.
Note You need to log in before you can comment on or make changes to this bug.