Bug 84812 - [chromium] Prevent CCLayerImpl::willDraw/didDraw mismatches
Summary: [chromium] Prevent CCLayerImpl::willDraw/didDraw mismatches
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks: 84805
  Show dependency tree / graph
 
Reported: 2012-04-24 18:05 PDT by Adrienne Walker
Modified: 2012-04-25 18:56 PDT (History)
6 users (show)

See Also:

Attachments
Patch (27.66 KB, patch)
2012-04-24 18:11 PDT, Adrienne Walker 		no flags Details | Formatted Diff | Diff
Patch (4.01 KB, patch)
2012-04-24 18:20 PDT, Adrienne Walker 		no flags Details | Formatted Diff | Diff
Patch (27.49 KB, patch)
2012-04-25 09:14 PDT, Adrienne Walker 		jamesr: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-04-24 18:05:26 PDT 
[chromium] Prevent CCLayerImpl::willDraw/didDraw mismatches
Comment 1 Adrienne Walker 2012-04-24 18:11:40 PDT 
Created attachment 138707 [details]
Patch
Comment 2 Adrienne Walker 2012-04-24 18:20:17 PDT 
Created attachment 138709 [details]
Patch
Comment 3 Adrienne Walker 2012-04-24 18:20:41 PDT 
(Ack.  webkit-patch upload failed.  Ignore that patch.)
Comment 4 Adrienne Walker 2012-04-25 09:14:47 PDT 
Created attachment 138822 [details]
Patch
Comment 5 Adrienne Walker 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.
Comment 6 Dana Jansens 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
Comment 7 James Robinson 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
Comment 8 Adrienne Walker 2012-04-25 18:47:21 PDT 
Committed r115278: <http://trac.webkit.org/changeset/115278>
Comment 9 Adrienne Walker 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.