cc: Put context-loss tests in layer_tree_host_unittest_context.cc

cc: Put context-loss tests in layer_tree_host_unittest_context.cc

These tests cover all of the cases covered by the context loss layout test and
more. It also provides full coverage both in single-thread and multi-thread modes.

I've moved tests from LayerTreeHostTests as usual, but also tests from
LayerTreeHostImplTests, and implemented them using the real commit flow,
instantiating layers by making the main thread versions of the layers, etc. This
makes for a much more realistic test, and doesn't rely on as much internal
compositor knowledge.

While digging around, I found that the layersFreeTextures test was not testing
anything. Then I also noticed that most of the layers in the test are not
actually responsible to free their resources when they disappear. So I made the
test do its job, and made the test just check layers that own resources - namely
IOSurface and Video layers.

Tests:
LayerTreeHostContextTestLostContextSucceeds.runSingleThread
LayerTreeHostContextTestLostContextSucceeds.runMultiThread
LayerTreeHostContextTestLostContextSucceedsWithContent.NoSurface_SingleThread
LayerTreeHostContextTestLostContextSucceedsWithContent.NoSurface_MultiThread
LayerTreeHostContextTestLostContextSucceedsWithContent.WithSurface_SingleThread
LayerTreeHostContextTestLostContextSucceedsWithContent.WithSurface_MultiThread
LayerTreeHostContextTestLostContextFails.RepeatLoss100_SingleThread
LayerTreeHostContextTestLostContextFails.RepeatLoss100_MultiThread
LayerTreeHostContextTestLostContextFails.FailRecreate100_SingleThread
LayerTreeHostContextTestLostContextFails.FailRecreate100_MultiThread
LayerTreeHostContextTestFinishAllRenderingAfterLoss.runSingleThread
LayerTreeHostContextTestFinishAllRenderingAfterLoss.runMultiThread
LayerTreeHostContextTestLostContextAndEvictTextures.LoseAfterEvict_SingleThread
LayerTreeHostContextTestLostContextAndEvictTextures.LoseAfterEvict_MultiThread
LayerTreeHostContextTestLostContextAndEvictTextures.LoseBeforeEvict_SingleThread
LayerTreeHostContextTestLostContextAndEvictTextures.LoseBeforeEvict_MultiThread
LayerTreeHostContextTestLostContextWhileUpdatingResources.runSingleThread
LayerTreeHostContextTestLostContextWhileUpdatingResources.runMultiThread
LayerTreeHostContextTestLayersNotified.runSingleThread
LayerTreeHostContextTestLayersNotified.runMultiThread
LayerTreeHostContextTestDontUseLostResources.runSingleThread
LayerTreeHostContextTestDontUseLostResources.runMultiThread

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

[danakj, 2012-12-21 05:36:54]

My intention is to land this and then remove the context lost layout tests, and
the LayerTreeHost::loseOutputSurface() mechanism etc.

[enne (OOO), 2013-01-02 20:30:39]

https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame.h
File cc/test/fake_video_frame.h (right):

https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame.h...
cc/test/fake_video_frame.h:9: #include
"third_party/WebKit/Source/Platform/chromium/public/WebVideoFrame.h"
Hmmmmmm.  Because of this include, should fake video frame go in compositor
bindings? Also, move the include to the .cc file.

https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame.h...
cc/test/fake_video_frame.h:18: virtual Format format() const;
OVERRIDE? Also, maybe add a // WebVideoFrame interface comment like we do
elsewhere in cc?

https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame_p...
File cc/test/fake_video_frame_provider.h (right):

https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame_p...
cc/test/fake_video_frame_provider.h:10: #include
"third_party/WebKit/Source/Platform/chromium/public/WebVideoFrameProvider.h"
Same question here about whether this should be in compositor bindings.

https://codereview.chromium.org/11662003/diff/8001/cc/tiled_layer.h
File cc/tiled_layer.h (right):

https://codereview.chromium.org/11662003/diff/8001/cc/tiled_layer.h#newcode69
cc/tiled_layer.h:69: const PrioritizedResource* resourceAtForTesting(int, int)
const;
I wish there were some better way to do all of these.  Maybe
s/private/protected/ below would let FakeContentLayer expose these functions,
rather than putting them on TiledLayer?

[danakj, 2013-01-02 20:45:00]

https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame.h
File cc/test/fake_video_frame.h (right):

https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame.h...
cc/test/fake_video_frame.h:9: #include
"third_party/WebKit/Source/Platform/chromium/public/WebVideoFrame.h"
On 2013/01/02 20:30:39, enne wrote:
> Hmmmmmm.  Because of this include, should fake video frame go in compositor
> bindings? Also, move the include to the .cc file.

There was some migration path for getting rid of WebVideoFrame entirely, which
has currently ground to a halt I guess. I think this should stay here, and
eventually WebVideoFrame becomes cc::VideoFrame, or media::VideoFrame, or
something.

https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame.h...
cc/test/fake_video_frame.h:18: virtual Format format() const;
On 2013/01/02 20:30:39, enne wrote:
> OVERRIDE? Also, maybe add a // WebVideoFrame interface comment like we do
> elsewhere in cc?

Comment added. OVERRIDE across the WebKit boundary seems to be more trouble than
good. See FakeWebGraphicsContext3D for a counter-example.

https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame_p...
File cc/test/fake_video_frame_provider.h (right):

https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame_p...
cc/test/fake_video_frame_provider.h:10: #include
"third_party/WebKit/Source/Platform/chromium/public/WebVideoFrameProvider.h"
On 2013/01/02 20:30:39, enne wrote:
> Same question here about whether this should be in compositor bindings.

Same answer, I think the Web* thing is supposed to just go away in the future.

https://codereview.chromium.org/11662003/diff/8001/cc/tiled_layer.h
File cc/tiled_layer.h (right):

https://codereview.chromium.org/11662003/diff/8001/cc/tiled_layer.h#newcode69
cc/tiled_layer.h:69: const PrioritizedResource* resourceAtForTesting(int, int)
const;
On 2013/01/02 20:30:39, enne wrote:
> I wish there were some better way to do all of these.  Maybe
> s/private/protected/ below would let FakeContentLayer expose these functions,
> rather than putting them on TiledLayer?

I kinda like them here because they are closer to the real code, so you can more
easily see the relationship to the real code, and maybe they have a better
chance of staying correct over time. With the ForTesting, it prevents anything
else from using it.

But, I'm okay with moving them entirely to the FakeContentLayer too.

[danakj, 2013-01-02 20:46:19]

https://codereview.chromium.org/11662003/diff/8001/cc/tiled_layer.h
File cc/tiled_layer.h (right):

https://codereview.chromium.org/11662003/diff/8001/cc/tiled_layer.h#newcode69
cc/tiled_layer.h:69: const PrioritizedResource* resourceAtForTesting(int, int)
const;
On 2013/01/02 20:45:00, danakj wrote:
> On 2013/01/02 20:30:39, enne wrote:
> > I wish there were some better way to do all of these.  Maybe
> > s/private/protected/ below would let FakeContentLayer expose these
functions,
> > rather than putting them on TiledLayer?
> 
> I kinda like them here because they are closer to the real code, so you can
more
> easily see the relationship to the real code, and maybe they have a better
> chance of staying correct over time. With the ForTesting, it prevents anything
> else from using it.
> 
> But, I'm okay with moving them entirely to the FakeContentLayer too.

I will also say, I like this a bit better than making m_tiler protected, because
that exposes it for non-testing code. Whereas this does not.

[jamesr, 2013-01-02 21:30:18]

On 2013/01/02 20:45:00, danakj wrote:
> https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame.h
> File cc/test/fake_video_frame.h (right):
> 
>
https://codereview.chromium.org/11662003/diff/8001/cc/test/fake_video_frame.h...
> cc/test/fake_video_frame.h:9: #include
> "third_party/WebKit/Source/Platform/chromium/public/WebVideoFrame.h"
> On 2013/01/02 20:30:39, enne wrote:
> > Hmmmmmm.  Because of this include, should fake video frame go in compositor
> > bindings? Also, move the include to the .cc file.
> 
> There was some migration path for getting rid of WebVideoFrame entirely, which
> has currently ground to a halt I guess. I think this should stay here, and
> eventually WebVideoFrame becomes cc::VideoFrame, or media::VideoFrame, or
> something.

It's not ground to a halt so much as lacking an owner.  The end state is to make
all of cc:: depend on media::VideoFrame, move creation of the video layer to the
chromium side, and get rid of WebVideoLayer* completely.  The tricky bit is
getting the shutdown synchronization correct and finding time to migrate it all.

I think having this fake in cc/test/ is OK for now.

[danakj, 2013-01-03 16:06:44]

Giving the scrollbar theme geometry a thumb caused an extra texture in the
atomic commit tests. So I made that optional in the scrollbar theme geometry,
unit tests should be happy now.

[danakj, 2013-01-03 16:07:55]

On Wed, Jan 2, 2013 at 4:30 PM, <jamesr@chromium.org> wrote:

>  There was some migration path for getting rid of WebVideoFrame entirely,
>> which
>> has currently ground to a halt I guess. I think this should stay here, and
>> eventually WebVideoFrame becomes cc::VideoFrame, or media::VideoFrame, or
>> something.
>>
>
> It's not ground to a halt so much as lacking an owner.  The end state is
> to make
>

I guess I will take this on for ubercomp. I've started working on a CL for
it.

[jamesr, 2013-01-03 21:30:08]

I think the naming here is odd - unit tests should always end with _unittest.cc

I really don't like adding a new set of macros to set up tests with multiple
parameters.  Just write the code out by hand - there are only 2 users of this
new macro that I can see.

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_content_laye...
File cc/test/fake_content_layer_impl.h (right):

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_content_laye...
cc/test/fake_content_layer_impl.h:24: return lost_output_surface_count_; }
put the } on its own line

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_video_frame.cc
File cc/test/fake_video_frame.cc (right):

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_video_frame....
cc/test/fake_video_frame.cc:7: using WebKit::WebRect;
don't really need these using statements since there's only one reference to
WebRect/WebSize/WebVideoFrame in the file

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_video_frame_...
File cc/test/fake_video_frame_provider.h (right):

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_video_frame_...
cc/test/fake_video_frame_provider.h:20: 
extra newline

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_web_scrollba...
File cc/test/fake_web_scrollbar_theme_geometry.cc (right):

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_web_scrollba...
cc/test/fake_web_scrollbar_theme_geometry.cc:94: if (!m_hasThumb) {
this is 4-space indented, should be 2 since this is a chromium-ish file

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_web_scrollba...
cc/test/fake_web_scrollbar_theme_geometry.cc:95: thumb = WebRect(0, 0, 0, 0);
ditto

[danakj, 2013-01-03 21:49:10]

On 2013/01/03 21:30:08, jamesr wrote:
> I think the naming here is odd - unit tests should always end with
_unittest.cc

The naming is to match the other layer_tree_host test files. We have
layer_tree_host_unittest_animation/scroll/occlusion..and now context. These
files are tests that instantiate and poke at a LayerTreeHost, and they inherit
from ThreadedTest. There's some precident for doing the suffix (i also
considered other naming conventions when starting down this path):
./chrome/browser/geolocation/wifi_data_provider_unittest_chromeos.cc
./chrome/browser/extensions/api/messaging/native_message_process_host_unittest_posix.cc
./ui/views/focus/focus_manager_unittest_win.cc
etc..

> I really don't like adding a new set of macros to set up tests with multiple
> parameters.  Just write the code out by hand - there are only 2 users of this
> new macro that I can see.

Okay.. My goal here was to minimize the number of lines of biolerplate required
to write new LayerTreeHost-based tests, as it obscures the test code, and is
generally annoying. Maybe we can find another way to do this without writing a
bunch of extra classes.. While there's only 2 users of this now, I have been
intending to proliferate their use especially with various device-scale-factors
(1.5, 2, etc) and page-scale-factors.  Without these macros, you have to make
another class for every different combination of device-scale and page-scale for
tests such as that.

[jamesr, 2013-01-03 21:52:13]

On 2013/01/03 21:49:10, danakj wrote:
> Okay.. My goal here was to minimize the number of lines of biolerplate
required
> to write new LayerTreeHost-based tests, as it obscures the test code, and is
> generally annoying. 

80+ lines of dense macro code is even more obscuring and annoying.

> Maybe we can find another way to do this without writing a
> bunch of extra classes..

Yes, that sounds like a better approach.

[danakj, 2013-01-03 21:52:20]

Comments addressed except the macros/filename, pending your reply.

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_content_laye...
File cc/test/fake_content_layer_impl.h (right):

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_content_laye...
cc/test/fake_content_layer_impl.h:24: return lost_output_surface_count_; }
On 2013/01/03 21:30:08, jamesr wrote:
> put the } on its own line

Done.

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_video_frame.cc
File cc/test/fake_video_frame.cc (right):

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_video_frame....
cc/test/fake_video_frame.cc:7: using WebKit::WebRect;
On 2013/01/03 21:30:08, jamesr wrote:
> don't really need these using statements since there's only one reference to
> WebRect/WebSize/WebVideoFrame in the file

Done.

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_video_frame_...
File cc/test/fake_video_frame_provider.h (right):

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_video_frame_...
cc/test/fake_video_frame_provider.h:20: 
On 2013/01/03 21:30:08, jamesr wrote:
> extra newline

Done.

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_web_scrollba...
File cc/test/fake_web_scrollbar_theme_geometry.cc (right):

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_web_scrollba...
cc/test/fake_web_scrollbar_theme_geometry.cc:94: if (!m_hasThumb) {
On 2013/01/03 21:30:08, jamesr wrote:
> this is 4-space indented, should be 2 since this is a chromium-ish file

Done.

https://codereview.chromium.org/11662003/diff/20001/cc/test/fake_web_scrollba...
cc/test/fake_web_scrollbar_theme_geometry.cc:95: thumb = WebRect(0, 0, 0, 0);
On 2013/01/03 21:30:08, jamesr wrote:
> ditto

Done.

[danakj, 2013-01-03 22:30:38]

Removed the macros. PTAL

[jamesr, 2013-01-03 22:59:07]

https://codereview.chromium.org/11662003/diff/22003/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/11662003/diff/22003/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest_context.cc:187: template<bool kUseSurface>
Why the template?  Code inside a TEST_F body is a subclass of the fixture so you
should be able to just do:

TEST_F(LayerTreeHostContextTestLostContextSucceedsWithContent, NoSurface) {
  use_surface_ = false;
  runTest(false);
}

and the same thing with

  runTest(true)

if you want to run it in threaded mode as well.  I get that you're trying to cut
down on boilerplate but I think this is optimizing for line count instead of
understandability.  Sometimes a few more lines of straightforward code is
clearer than a clever thing that shaves a few LOC

https://codereview.chromium.org/11662003/diff/22003/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/11662003/diff/22003/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest_scroll.cc:459:
LayerTreeHostScrollTestScrollChild_DeviceScaleFactor1)
this is 5 lines to set up and run a threaded test.  I think this would be a lot
clearer:

TEST_F(LayerTreeHostScrollTestScrollChild, DeviceScaleFactor1) {
  device_scale_ = 15;
  runTest(true);
}

and is 1 line shorter to boot.

[danakj, 2013-01-03 23:13:32]

OK, this is without templates. PTAL

[jamesr, 2013-01-03 23:28:33]

lgtm

[jamesr, 2013-01-03 23:28:34]

lgtm

lgtm

[commit-bot: I haz the power, 2013-01-03 23:28:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11662003/20003

[commit-bot: I haz the power, 2013-01-04 02:08:14]

Change committed as 175080