Clean up compositor initialization/destruction.

Clean up compositor initialization/destruction.

Moved from https://codereview.chromium.org/21052007/

Currently the ContextFactory is outliving the compositor thread in 
tests, which is bad since the contexts in there can be bound to the 
compositor thread. Then we end up reusing those contexts and bad 
things happen. 

This enforces ordering for initializing and destroying the 
compositor as follows: 

Initialize ContextFactory 
..Compositor::Initialize 
....Create Compositor instances 
....Delete Compositor instances 
..Compositor::Terminate 

The Compositor::Terminate call also destroys the ContextFactory. 

The ContextFactory can be initialized in one of two ways: 
1. ImageTransportFactory::Initialize will set up the 
ContextFactory. This is used by things that invoke all of 
content/browser/. 
2. Compositor::InitializeContextFactoryForTests() must be 
explicitly called by any unit tests that create a compositor. 

Since some tests want a real GL context and some don't, this 
does away with the misnomer of initializing the Compositor once 
for the entire test suite, and then re-initializing somewhere 
in the middle of the test suite. Instead, each test must 
Initialize/Terminate the compositor with the ContextFactory 
type of its choice. 

Incidently, this test enables 20 tests on aura or win_aura that 
were previously being skipped. 

TBR=piman@chromium.org
BUG=258625
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216780

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

[piman, 2013-08-07 01:03:16]

lgtm

[danakj, 2013-08-07 14:08:53]

Committed patchset #8 manually as r216179 (presubmit successful).

[commit-bot: I haz the power, 2013-08-07 14:42:51]

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

[commit-bot: I haz the power, 2013-08-07 17:02:10]

Failed to trigger a try job on linux_chromeos
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-08-07 17:02:47]

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

[danakj, 2013-08-07 21:40:06]

This has passed everything except win_rel EndToEnd which was being flaky. I'll
dcommit again.

[danakj, 2013-08-07 21:48:24]

Committed patchset #10 manually as r216278 (presubmit successful).

[miu, 2013-08-07 22:02:12]

On 2013/08/07 21:48:24, danakj wrote:
> Committed patchset #10 manually as r216278 (presubmit successful).

sgtm.  I'm sheriffing, so I'll monitor it.  Thanks for being patient with that
flaky EndToEnd test.  https://code.google.com/p/chromium/issues/detail?id=269659

[danakj, 2013-08-08 00:26:00]

I've matched things up so when real GL contexts (ie not faked out test contexts)
is enabled because we're running on a ChromeOS VM/device, that we also use real
GL bindings instead of OSMesa, so that the tests are not slowed down a bunch by
suddenly using OSMesa on device.

I've also made "--enable-gpu" use real GL contexts, as this is the flag used by
perf bots to run the test on a real GPU.

[danakj, 2013-08-09 00:47:58]

I've made android always use real GL bindings as OSmesa does not exist there,
and verified this passes the android_fyi_dbg bot as requested.

[danakj, 2013-08-09 17:01:44]

I got 6 clean runs of linux_chromeos while not using OSMesa in the TabDrag
tests. But on WinAura, then GLSurface init fails to find EGL. I'm not sure why
we would ever GLSurface init if we are using test contexts..

[danakj, 2013-08-09 18:58:29]

I've gone far enough down this rabbit hole to I think understand what's going
on. It's messy but maybe it can be less messy after this CL with time.

The GPU process is not used by the browser compositor when we use test contexts,
but it is still launched, and it is used by the Renderer compositor. This is a
super total waste of CPU cycles.

Switching tests to using OSMesa with test compositor has no effect in the
browser, but changes the timing with the renderer compositor starting up/running
etc. Since the bots don't have real GL, the renderer compositor just fails to
create an OutputSurface, and I think falls back into legacy software path.

This legacy path is, as the name suggests, going to go away. So this means I
guess we need to make browser tests not need it to run. We should make the
renderer compositor use test contexts which are super fast, when the browser is
using test contexts. This is blocked on the fact that the code uses
WGC3DCommandBufferImpl::SetLatencyInfo(). This same function is used in the
BrowserCompositorOutputSurface and prevents us from using test contexts there
for unit tests.

[danakj, 2013-08-09 18:59:14]

So, I believe my way forward is to land this with "real GL bindings that just
fail to work" for the tab dragging browser tests - as they currently do on TOT.

We can then have a better baseline to figure out how to make the renderer use
test contexts.

[commit-bot: I haz the power, 2013-08-09 19:01:26]

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

[commit-bot: I haz the power, 2013-08-09 21:11:54]

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

[commit-bot: I haz the power, 2013-08-09 23:48:59]

Change committed as 216780

[f(malita), 2013-08-11 21:45:56]

On 2013/08/09 23:48:59, I haz the power (commit-bot) wrote:
> Change committed as 216780

There's some suspicion that this might have broken
NetInternalsTest.netInternalsTimelineViewScrollbar. PTAL:

http://build.chromium.org/p/chromium.win/builders/Win%20Aura%20Tests%20(2)
http://build.chromium.org/p/chromium.win/builders/Win%20Aura%20Tests%20%283%2...

NetInternalsTest.netInternalsTimelineViewScrollbar: 
[5084:3900:0809/194513:3680765:ERROR:chrome_views_delegate.cc(158)] NOT
IMPLEMENTED
[5084:3900:0809/194513:3680765:ERROR:desktop_root_window_host_win.cc(711)] NOT
IMPLEMENTED
[3772:5396:0809/194514:ERROR:dxva_video_decode_accelerator.cc(401)]
Direct3DCreate9Ex failed, HRESULT: 0x8876086a
[3772:5396:0809/194514:ERROR:dxva_video_decode_accelerator.cc(394)] Failed to
initialize D3D device and manager
[5084:4404:0809/194514:3681919:ERROR:gpu_message_filter.cc(189)] Renderer 3
tried to access a surface for renderer 0
[5084:4404:0809/194514:3681919:ERROR:gpu_message_filter.cc(189)] Renderer 3
tried to access a surface for renderer 0
[5084:4404:0809/194514:3681919:ERROR:gpu_message_filter.cc(189)] Renderer 3
tried to access a surface for renderer 0
[5084:4404:0809/194514:3681935:ERROR:gpu_message_filter.cc(189)] Renderer 3
tried to access a surface for renderer 0
[5084:4404:0809/194516:3683027:ERROR:gpu_message_filter.cc(189)] Renderer 4
tried to access a surface for renderer 0
[5084:4404:0809/194516:3683027:ERROR:gpu_message_filter.cc(189)] Renderer 4
tried to access a surface for renderer 0
[5084:4404:0809/194516:3683027:ERROR:gpu_message_filter.cc(189)] Renderer 4
tried to access a surface for renderer 0
[5084:4404:0809/194516:3683027:ERROR:gpu_message_filter.cc(189)] Renderer 4
tried to access a surface for renderer 0
LEAK: 8 WebCoreNode
[5084:3900:0809/194518:3685320:INFO:CONSOLE(3414)] "Running TestCase
NetInternalsTest.netInternalsTimelineViewScrollbar", source:  (3414)
[5084:3900:0809/194518:3685804:ERROR:textfield.h(175)] NOT IMPLEMENTED
[5084:3900:0809/194518:3685820:ERROR:desktop_root_window_host_win.cc(711)] NOT
IMPLEMENTED
Killed (timed out).

[Isaac (away), 2013-08-12 05:53:40]

This was reverted in r216907 and it did indeed fix the broken aura test.

Interestingly, win7_aura tryjobs did not fail from this patch; only the main
waterfall bot went red. I wonder if there is a difference in configuration that
needs to be resolved.

[danakj, 2013-08-12 15:56:09]

On 2013/08/12 05:53:40, Isaac wrote:
> This was reverted in r216907 and it did indeed fix the broken aura test.
> 
> Interestingly, win7_aura tryjobs did not fail from this patch; only the main
> waterfall bot went red. I wonder if there is a difference in configuration
that
> needs to be resolved.

I'll turn on real GPU GL bindings for the test in question and file a bug about
it.

[danakj, 2013-08-12 16:57:16]

Patch set 19 turns on real GL bindings for Webrtc browser tests, which are perf
tests, pending them having the bot pass in appropriate command line flags in
crbug.com/271504

[danakj, 2013-08-12 17:13:56]

Patch set 20 turns on real GL bindings for NetInternalsTest tests on Win7 Aura.

[commit-bot: I haz the power, 2013-08-12 17:13:57]

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

[commit-bot: I haz the power, 2013-08-12 17:23:41]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-08-12 17:26:13]

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

[commit-bot: I haz the power, 2013-08-12 20:46:55]

List of reviewers changed. joi@chromium.org did a drive-by without LGTM'ing!

[commit-bot: I haz the power, 2013-08-12 20:48:49]

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

[commit-bot: I haz the power, 2013-08-12 20:58:06]

Change committed as 217069

[gab, 2013-08-15 19:40:46]

Hey danakj, just noticed something and wanted to make sure this was
known/intentional, see below.

Cheers,
Gab

https://chromiumcodereview.appspot.com/22293003/diff/185001/content/public/te...
File content/public/test/browser_test_base.cc (right):

https://chromiumcodereview.appspot.com/22293003/diff/185001/content/public/te...
content/public/test/browser_test_base.cc:105: MainFunctionParams
params(*command_line);
Looks like the |command_line| is used here to launch the test; yet you are
appending more switches to the |command_line| after this line; is this
ok/intentional?

[danakj, 2013-08-15 20:15:12]

https://chromiumcodereview.appspot.com/22293003/diff/185001/content/public/te...
File content/public/test/browser_test_base.cc (right):

https://chromiumcodereview.appspot.com/22293003/diff/185001/content/public/te...
content/public/test/browser_test_base.cc:105: MainFunctionParams
params(*command_line);
On 2013/08/15 19:40:46, gab wrote:
> Looks like the |command_line| is used here to launch the test; yet you are
> appending more switches to the |command_line| after this line; is this
> ok/intentional?

Hm, it seems to work fine in practice for these switches. But I should move them
up. Thanks.

[gab, 2013-08-15 20:18:46]

https://chromiumcodereview.appspot.com/22293003/diff/185001/content/public/te...
File content/public/test/browser_test_base.cc (right):

https://chromiumcodereview.appspot.com/22293003/diff/185001/content/public/te...
content/public/test/browser_test_base.cc:105: MainFunctionParams
params(*command_line);
On 2013/08/15 20:15:13, danakj wrote:
> On 2013/08/15 19:40:46, gab wrote:
> > Looks like the |command_line| is used here to launch the test; yet you are
> > appending more switches to the |command_line| after this line; is this
> > ok/intentional?
> 
> Hm, it seems to work fine in practice for these switches. But I should move
them
> up. Thanks.

Ah, just noticed that it seems MainFunctionParams stores the CommandLine by
reference (which would explain why it still works); still a weird flow to read.

I'd say we can just move the creation of MainFunctionParams to the bottom of
this method as it's not used before anyways...