[chromium] Remove incorrect early exit in CCDamageTracker

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

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.

[Jonathan Backer]

Created attachment 123778 [details]
Patch

[Nat Duca]

Comment on attachment 123778 [details]
Patch

Unit test?

[Jonathan Backer]

(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.

[Shawn Singh]

(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.

[Shawn Singh]

Created attachment 123997 [details]
not for review, just for reference

Jonathan requested me to upload this, just for reference and temporary testing

[Shawn Singh]

Created attachment 125561 [details]
Patch

[Shawn Singh]

(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.

[Shawn Singh]

Jamesr - when you have a chance, could you please review this?  Thanks in advance.

[James Robinson]

Comment on attachment 125561 [details]
Patch

OK, R=me

[WebKit Review Bot]

Comment on attachment 125561 [details]
Patch

Clearing flags on attachment: 125561

Committed r107130: <http://trac.webkit.org/changeset/107130>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.