Bug 76924 - [chromium] Remove incorrect early exit in CCDamageTracker
Summary: [chromium] Remove incorrect early exit in CCDamageTracker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-24 10:24 PST by Shawn Singh
Modified: 2012-02-08 13:42 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.25 KB, patch)
2012-01-24 11:37 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
not for review, just for reference (4.14 KB, patch)
2012-01-25 13:20 PST, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (11.08 KB, patch)
2012-02-05 21:13 PST, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-01-24 10:24:55 PST
In the damage tracker, the original logic did an early-exit if we knew the entire surface was damaged.   This is wrong, however, since an early exit means that the tracking state of all other layers will not be updated.

Jonathan and I already verified a simple change that fixes the problem, but we should still create a nice unit test to reproduce this.
Comment 1 Jonathan Backer 2012-01-24 11:37:54 PST
Created attachment 123778 [details]
Patch
Comment 2 Nat Duca 2012-01-24 12:10:16 PST
Comment on attachment 123778 [details]
Patch

Unit test?
Comment 3 Jonathan Backer 2012-01-24 12:35:36 PST
(In reply to comment #2)
> (From update of attachment 123778 [details])
> Unit test?

I hit the --no-review flag. I wasn't sure that Shawn had the cycles to work on this, so I just wanted to document the patch.

Turns out that he's got a different version (with a unit-test). I'm sure he'll upload shortly.
Comment 4 Shawn Singh 2012-01-24 19:43:14 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 123778 [details] [details])
> > Unit test?
> 
> I hit the --no-review flag. I wasn't sure that Shawn had the cycles to work on this, so I just wanted to document the patch.
> 
> Turns out that he's got a different version (with a unit-test). I'm sure he'll upload shortly.

I have a fix, and it does work.  But it actually ends up breaking other CCDamageTracker unit tests.   As far as I have debugged so far, I believe it is the result of a unrelated bug in CCLayerTreeHostCommon... setting an incorrect contentRect when there is a replicaLayer.

I would prefer to fully understand and fix that bug first, and then submit the patch for this.  I will be able to look into that issue starting early next week.
Comment 5 Shawn Singh 2012-01-25 13:20:50 PST
Created attachment 123997 [details]
not for review, just for reference

Jonathan requested me to upload this, just for reference and temporary testing
Comment 6 Shawn Singh 2012-02-05 21:13:15 PST
Created attachment 125561 [details]
Patch
Comment 7 Shawn Singh 2012-02-05 21:17:34 PST
(In reply to comment #6)
> Created an attachment (id=125561) [details]
> Patch

The patch just uploaded does three things:
(1) adds unit test that demonstrates that early exiting in CCDamageTracker is wrong,
(2) removes the early exit and cleans up the surrounding code,
(3) re-names several functions in CCDamageTracker to reflect that they do more than just "compute" something, they also update state held by the damage tracker.  Before renaming, this state tracking was more of a "side-effect"... renaming them makes it more clear that this is intended, and not an un-desired side effect.

The contentRect problem I found is actually un-related, and I placed a FIXME in this patch to reflect it.
Comment 8 Shawn Singh 2012-02-07 16:10:40 PST
Jamesr - when you have a chance, could you please review this?  Thanks in advance.
Comment 9 James Robinson 2012-02-08 12:05:20 PST
Comment on attachment 125561 [details]
Patch

OK, R=me
Comment 10 WebKit Review Bot 2012-02-08 13:42:04 PST
Comment on attachment 125561 [details]
Patch

Clearing flags on attachment: 125561

Committed r107130: <http://trac.webkit.org/changeset/107130>
Comment 11 WebKit Review Bot 2012-02-08 13:42:08 PST
All reviewed patches have been landed.  Closing bug.