(Reland) cc:Use impl-side painting in LTH context lost test

(Reland) cc:Use impl-side painting in LTH context lost test

Current LTH Context Lost tests do not test for
impl-side painting, this updates the tests to use
ContentLayer or PictureLayer based on the settings.

Also, SetRequiresHighResToDraw when we recover from lost
context/ new output surface is used, to avoid checkerboards.

BUG=401492, 415155
Committed: https://crrev.com/9d1e420684922f97fc7628060fd4bedb5e86248c
Cr-Commit-Position: refs/heads/master@{#296236}

[sohanjg, 2014-09-20 09:56:17]

sohan.jyoti@samsung.com changed reviewers:
+ danakj@chromium.org

[sohanjg, 2014-09-20 09:56:18]

Can you please take a look, thanks.

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest_context.cc:490: 
We had a check here in the prev patch for, 
EXPECT_FALSE(picture_impl->HighResTiling()->TileAt(0, 0)->IsReadyToDraw())

But, EvictTexturesForTesting will post a MANAGE_TILES, and is not guaranteed to
ReleaseResource, before we check IsReadyToDraw() here?

[danakj, 2014-09-22 14:57:34]

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest_context.cc:490: 
On 2014/09/20 09:56:18, sohanjg wrote:
> We had a check here in the prev patch for, 
> EXPECT_FALSE(picture_impl->HighResTiling()->TileAt(0, 0)->IsReadyToDraw())
> 
> But, EvictTexturesForTesting will post a MANAGE_TILES, and is not guaranteed
to
> ReleaseResource, before we check IsReadyToDraw() here?

Ah ok, it was a bad check I guess, sorry.

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest_context.cc:496: if (num_commits_ > 1)
I don't see anything setting this variable anymore. Can you ++num_commits before
testing it?

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest_context.cc:503: EndTest();
Can you move this into the num_commits_ > 1 early out instead? We should come
here once, evict stuff, then recover and get here again.

[sohanjg, 2014-09-22 15:12:13]

can you please take a look, thanks.

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest_context.cc:490: 
On 2014/09/22 14:57:33, danakj wrote:
> On 2014/09/20 09:56:18, sohanjg wrote:
> > We had a check here in the prev patch for, 
> > EXPECT_FALSE(picture_impl->HighResTiling()->TileAt(0, 0)->IsReadyToDraw())
> > 
> > But, EvictTexturesForTesting will post a MANAGE_TILES, and is not guaranteed
> to
> > ReleaseResource, before we check IsReadyToDraw() here?
> 
> Ah ok, it was a bad check I guess, sorry.

np :)

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest_context.cc:496: if (num_commits_ > 1)
On 2014/09/22 14:57:33, danakj wrote:
> I don't see anything setting this variable anymore. Can you ++num_commits
before
> testing it?

Done.

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest_context.cc:503: EndTest();
On 2014/09/22 14:57:33, danakj wrote:
> Can you move this into the num_commits_ > 1 early out instead? We should come
> here once, evict stuff, then recover and get here again.

Done.
You mean EndTest here, as well in early-out, right ?

[danakj, 2014-09-22 15:15:33]

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest_context.cc:503: EndTest();
On 2014/09/22 15:12:13, sohanjg wrote:
> On 2014/09/22 14:57:33, danakj wrote:
> > Can you move this into the num_commits_ > 1 early out instead? We should
come
> > here once, evict stuff, then recover and get here again.
> 
> Done.
> You mean EndTest here, as well in early-out, right ?

No, just in the early out. We want to end after we know we've done all the
things we want to test.

[sohanjg_, 2014-09-22 17:12:27]

sohanjg@chromium.org changed reviewers:
+ sohanjg@chromium.org

[sohanjg_, 2014-09-22 17:12:28]

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/591473003/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest_context.cc:503: EndTest();
On 2014/09/22 15:15:33, danakj wrote:
> On 2014/09/22 15:12:13, sohanjg wrote:
> > On 2014/09/22 14:57:33, danakj wrote:
> > > Can you move this into the num_commits_ > 1 early out instead? We should
> come
> > > here once, evict stuff, then recover and get here again.
> > 
> > Done.
> > You mean EndTest here, as well in early-out, right ?
> 
> No, just in the early out. We want to end after we know we've done all the
> things we want to test.

Hmm..ok..it seems like few of the test goes to timeout, if we end test only on
early out.

[sohanjg, 2014-09-23 06:15:52]

please take a look, thanks.

https://codereview.chromium.org/591473003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/591473003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:512: LoseContext();
We were missing this lose context for LoseBeforeEvict tests, but we cant lose
context in DidCommitAndDrawFrame, as it runs on Main thread ?

[danakj, 2014-09-23 14:55:48]

https://codereview.chromium.org/591473003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/591473003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:512: LoseContext();
On 2014/09/23 06:15:52, sohanjg wrote:
> We were missing this lose context for LoseBeforeEvict tests, but we cant lose
> context in DidCommitAndDrawFrame, as it runs on Main thread ?

How about when you LoseContext() set a variable lost_context_ = true; Then in
DrawLayersOnThread, if lost_context_ is true, EndTest(). Will that work
reliably?

[sohanjg, 2014-09-23 15:18:41]

please take a look, thanks.

https://codereview.chromium.org/591473003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/591473003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:512: LoseContext();
On 2014/09/23 14:55:48, danakj wrote:
> On 2014/09/23 06:15:52, sohanjg wrote:
> > We were missing this lose context for LoseBeforeEvict tests, but we cant
lose
> > context in DidCommitAndDrawFrame, as it runs on Main thread ?
> 
> How about when you LoseContext() set a variable lost_context_ = true; Then in
> DrawLayersOnThread, if lost_context_ is true, EndTest(). Will that work
> reliably?

Done.
It works fine!

[danakj, 2014-09-23 15:20:19]

LGTM

[sohanjg, 2014-09-23 17:01:52]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-09-23 17:59:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/591473003/80001

[commit-bot: I haz the power, 2014-09-23 21:02:43]

Committed patchset #5 (id:80001) as 21c0b3bdede6921b3088c60d4c196ffc97266091

[commit-bot: I haz the power, 2014-09-23 21:03:34]

Patchset 5 (id:??) landed as
https://crrev.com/9d1e420684922f97fc7628060fd4bedb5e86248c
Cr-Commit-Position: refs/heads/master@{#296236}