CC: Fix missing swap-used-incomplete-tile updates

CC: Fix missing swap-used-incomplete-tile updates

When we early-out of a frame that has no damage, we didn't
correctly detect if the frame had incomplete tiles. This
patch only updates the flag if a swap actually occurs.

To prevent this kind of bug in the future, this patch
always sets the did-swap-use-incomplete-tile flag when it is
known, rather than resetting it to false in one place and
hoping it will always be set again correctly in all cases.

The test times-out without this patch.

BUG=284810
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221891

[epennerAtGoogle, 2013-09-05 00:44:40]

Ptal.

I could stash the has-incomplete-tiles flag from the last
frame, but this felt a lot more clean.

[epennerAtGoogle, 2013-09-05 00:48:22]

On 2013/09/05 00:44:40, epennerAtGoogle wrote:
> Ptal.
> 
> I could stash the has-incomplete-tiles flag from the last
> frame, but this felt a lot more clean.

Hmm.. I guess I could still call it "SetSwapUsedIncompleteTile" instead of
"SetHaveIncompleteTile". Let me know if I should change that.

[brianderson, 2013-09-05 00:49:58]

Nice. Looks like you address both the failed draw and the no damage cases.

Can you add a unit test that would fail without this patch?

Also, I would prefer to keep "SwapUsedIncompleteTile" somewhere in the function
name since it is clearer what has the incomplete tile.

Otherwise, lgtm.

[epennerAtGoogle, 2013-09-05 01:26:09]

On 2013/09/05 00:49:58, Brian Anderson wrote:
> Nice. Looks like you address both the failed draw and the no damage cases.
> 
> Can you add a unit test that would fail without this patch?
> 
> Also, I would prefer to keep "SwapUsedIncompleteTile" somewhere in the
function
> name since it is clearer what has the incomplete tile.
> 
> Otherwise, lgtm.

Just did the rename.

Regarding a test, I could do something like insure we always update the
tile-incomplete flag after a compositor swap, but that test seems a bit
contrived with the new code.

[brianderson, 2013-09-05 01:49:44]

Could you add a test to layer_tree_host_unittest.cc, where you do something
like:

1) Override BeginTest() and call PostSetNeedsCommitToMainThread();
2) Override PrepareToDrawOnThread and force frame_data.contains_incomplete_tile
to true to start off with.
3) Override SwapBuffersOnThread to monitor the
impl->active_tree()->source_frame_number() for the last swap expected (until
there is a new visible tile).
4) Trigger a draw that either fails or has no damage.
5) Verify that UpdateVisibleTiles continues to be called on the
LayerTreeHostImpl.  You'll probably have to modify LayerTreeHostImplForTesting
to monitor this state. ToT should fail this expectation or timeout before it
gets to 6.
6) After a couple calls to UpdateVisibleTiles, trigger
impl->DidInitializeVisibleTile and stop forcing
frame_data.contains_incomplete_tile to true.
7) Verify 6 triggers a draw.

[epennerAtGoogle, 2013-09-07 00:06:11]

Ptal. Added the test. Thanks brianderson@, for that excellent method to test
this.

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.h:416: void DidInitializeVisibleTile();
Oops. I'm fixing the test to not need this.

[brianderson, 2013-09-07 00:50:36]

Thanks for the test.

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:695: if
(!settings_.impl_side_painting) {
Is there a macro that does not instantiate the MainThread version of this test?

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:739: break;
Are cases 4-6 needed?

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:752: // above). This is to insure we
still get UpdateVisibleTiles calls after
nit: ensure

[epenner, 2013-09-07 00:57:15]

Ptal.

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:695: if
(!settings_.impl_side_painting) {
On 2013/09/07 00:50:37, Brian Anderson wrote:
> Is there a macro that does not instantiate the MainThread version of this
test?

Doesn't look like it, and the code to do it is rather ugly. But I can add a
macro or copy that code in here if desired.

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:739: break;
On 2013/09/07 00:50:37, Brian Anderson wrote:
> Are cases 4-6 needed?

Oops. Removed those. Done.

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:752: // above). This is to insure we
still get UpdateVisibleTiles calls after
On 2013/09/07 00:50:37, Brian Anderson wrote:
> nit: ensure

Done.

[danakj, 2013-09-07 00:59:05]

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:695: if
(!settings_.impl_side_painting) {
On 2013/09/07 00:57:15, epenner wrote:
> On 2013/09/07 00:50:37, Brian Anderson wrote:
> > Is there a macro that does not instantiate the MainThread version of this
> test?
> 
> Doesn't look like it, and the code to do it is rather ugly. But I can add a
> macro or copy that code in here if desired.

Just override InitializeSettings and turn impl painting on for this test always.

[danakj, 2013-09-07 01:00:15]

https://codereview.chromium.org/23686011/diff/26001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/23686011/diff/26001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:703: root_ =
FakeContentLayer::Create(&client_);
If you're testing impl paint only, do you want FakePictureLayer?

https://codereview.chromium.org/23686011/diff/26001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:783: scoped_refptr<FakeContentLayer>
root_;
This can be a local in SetupTree, it's not used elsewhere.

[epenner, 2013-09-07 01:00:51]

On 2013/09/07 00:57:15, epenner wrote:
> Ptal.
> 
>
https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
> File cc/trees/layer_tree_host_unittest_damage.cc (right):
> 
>
https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest_damage.cc:695: if
> (!settings_.impl_side_painting) {
> On 2013/09/07 00:50:37, Brian Anderson wrote:
> > Is there a macro that does not instantiate the MainThread version of this
> test?
> 
> Doesn't look like it, and the code to do it is rather ugly. But I can add a
> macro or copy that code in here if desired.
> 
>
https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest_damage.cc:739: break;
> On 2013/09/07 00:50:37, Brian Anderson wrote:
> > Are cases 4-6 needed?
> 
> Oops. Removed those. Done.
> 
>
https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest_damage.cc:752: // above). This is to insure
we
> still get UpdateVisibleTiles calls after
> On 2013/09/07 00:50:37, Brian Anderson wrote:
> > nit: ensure
> 
> Done.

Anyone know if I can get to the thread proxy from these tests? I need to trigger
DidInitializeVisibleTile (or the same function in ThreadProxy) but don't want to
make it public on LayerTreeHostImpl.

[brianderson, 2013-09-07 01:08:13]

On 2013/09/07 01:00:51, epenner wrote:
> On 2013/09/07 00:57:15, epenner wrote:
> > Ptal.
> > 
> >
>
https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
> > File cc/trees/layer_tree_host_unittest_damage.cc (right):
> > 
> >
>
https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
> > cc/trees/layer_tree_host_unittest_damage.cc:695: if
> > (!settings_.impl_side_painting) {
> > On 2013/09/07 00:50:37, Brian Anderson wrote:
> > > Is there a macro that does not instantiate the MainThread version of this
> > test?
> > 
> > Doesn't look like it, and the code to do it is rather ugly. But I can add a
> > macro or copy that code in here if desired.
> > 
> >
>
https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
> > cc/trees/layer_tree_host_unittest_damage.cc:739: break;
> > On 2013/09/07 00:50:37, Brian Anderson wrote:
> > > Are cases 4-6 needed?
> > 
> > Oops. Removed those. Done.
> > 
> >
>
https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
> > cc/trees/layer_tree_host_unittest_damage.cc:752: // above). This is to
insure
> we
> > still get UpdateVisibleTiles calls after
> > On 2013/09/07 00:50:37, Brian Anderson wrote:
> > > nit: ensure
> > 
> > Done.
> 
> Anyone know if I can get to the thread proxy from these tests? I need to
trigger
> DidInitializeVisibleTile (or the same function in ThreadProxy) but don't want
to
> make it public on LayerTreeHostImpl.

This has been somewhat of a thorn when writing LTHI tests. The TestHooks methods
should be taking in a LayerTreeHostImplForTesting (rather than a
LayerTreeHostImpl )that we can add whatever we want to. To work around that in
the past, we've added *ForTesting methods to LTHI - but that isn't an ideal
solution...

[epenner, 2013-09-07 01:52:22]

Okay I made a "ForTesting" method.

Fixed remaining feedback also. Ptal.

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.h:416: void DidInitializeVisibleTile();
On 2013/09/07 00:06:11, epennerAtGoogle wrote:
> Oops. I'm fixing the test to not need this.

Done.

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/23686011/diff/19001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:695: if
(!settings_.impl_side_painting) {
That seemed less intuitive if it fails. But not very much so sounds good.

Done.

[epenner, 2013-09-07 01:55:18]

https://codereview.chromium.org/23686011/diff/26001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/23686011/diff/26001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:703: root_ =
FakeContentLayer::Create(&client_);
On 2013/09/07 01:00:15, danakj wrote:
> If you're testing impl paint only, do you want FakePictureLayer?

Done.

https://codereview.chromium.org/23686011/diff/26001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:783: scoped_refptr<FakeContentLayer>
root_;
On 2013/09/07 01:00:15, danakj wrote:
> This can be a local in SetupTree, it's not used elsewhere.

Done.

[brianderson, 2013-09-07 02:00:03]

lgtm, although you'll likely need to rebase on top of the readback/forced draw
patch that just landed.

[epennerAtGoogle, 2013-09-07 02:01:41]

On 2013/09/07 02:00:03, Brian Anderson wrote:
> lgtm, although you'll likely need to rebase on top of the readback/forced draw
> patch that just landed.

Thanks for the heads up.

[epenner, 2013-09-07 02:24:32]

On 2013/09/07 02:01:41, epennerAtGoogle wrote:
> On 2013/09/07 02:00:03, Brian Anderson wrote:
> > lgtm, although you'll likely need to rebase on top of the readback/forced
draw
> > patch that just landed.

I noticed you used the did_swap flag in the scheduler, so I kept it there. It
seems like 'swap' means slightly different things sometimes, but keep in mind
that that flag does not imply the real SwapBuffers was called.

[commit-bot: I haz the power, 2013-09-07 02:26:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/23686011/44001

[commit-bot: I haz the power, 2013-09-07 02:35:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/23686011/61001

[commit-bot: I haz the power, 2013-09-07 04:32:16]

Change committed as 221891