aura: Clean up compositor initialization/destruction.

Clean up compositor initialization/destruction.

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.

R=jbauman, piman
BUG=258625, 266565

[danakj, 2013-07-31 01:32:50]

Throwing this at bots, and I'll come back to it tomorrow to look through any
failures.. I expect compile fails still maybe, though it seems to be okay on
chromeos=1 linux at least.

Please have a look at the overall direction though and if you approve of that
type of thing.

https://codereview.chromium.org/21052007/diff/3001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/21052007/diff/3001/ui/compositor/compositor.c...
ui/compositor/compositor.cc:462: if
(command_line->HasSwitch(switches::kDisableTestCompositor))
I don't know if we really need to honor command line or IsRunningOnChromeOS here
anymore. Were those only for ImageTransportFactory::Initialize?

I'd kinda expect that in unit tests they should know what kind of contexts they
want, and we could just use the value passed into this method?

[piman, 2013-07-31 07:01:48]

LGTM on the approach. Thanks for the cleanup. You probably need some extra fixes
in some test suites to resolve the remaining failures.

https://codereview.chromium.org/21052007/diff/3001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/21052007/diff/3001/ui/compositor/compositor.c...
ui/compositor/compositor.cc:462: if
(command_line->HasSwitch(switches::kDisableTestCompositor))
On 2013/07/31 01:32:51, danakj wrote:
> I don't know if we really need to honor command line or IsRunningOnChromeOS
here
> anymore. Were those only for ImageTransportFactory::Initialize?
> 
> I'd kinda expect that in unit tests they should know what kind of contexts
they
> want, and we could just use the value passed into this method?

The "test compositor" is an optimization for the bots, so that they run faster.
--disable-test-compositor is so that you can see what's happening when you need
to debug things, and that's definitely useful for UI people.
I forgot what led to the IsRunningOnChromeOS thing below, but on Chrome OS we
have a real GPU or at least a fast software implementation on VMs, so the
optimization doesn't bring us much. I suspect also it breaks some
ChromeOS-specific tests (e.g. login UI) because it disables accelerated
compositing in the renderer.

https://codereview.chromium.org/21052007/diff/3001/ui/compositor/compositor.c...
ui/compositor/compositor.cc:476: // the gl_bindings.
nit: I guess we don't leak it any more...

[danakj, 2013-07-31 20:14:50]

I think I should have all (or most) of the tests working now, try jobs running.
Notable changes I had to make were:

https://codereview.chromium.org/21052007/diff/31001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/21052007/diff/31001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:215:
DoNotUseTestContexts();
This change makes these tests actually run instead of just earlying out from
SET_UP_SURFACE_OR_PASS_TEST failing.

Once they started running, they had a memory problem since they'd perform a
readback but not keep the VideoFrame alive that it was reading into.

Since this is a valid thing to do... make the GLHelper take a ref on the video
frame while it is using it!

https://codereview.chromium.org/21052007/diff/31001/content/common/gpu/client...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/21052007/diff/31001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:866:
base::Bind(&CallbackKeepingVideoFrameAlive,
This was causing an ASAN failure for the FrameSubscriber test once it started
actually running.

https://codereview.chromium.org/21052007/diff/24002/chrome/test/base/view_eve...
File chrome/test/base/view_event_test_base.cc (right):

https://codereview.chromium.org/21052007/diff/24002/chrome/test/base/view_eve...
chrome/test/base/view_event_test_base.cc:106: ash_test_helper_.reset(
Instead of doing all this ash setup here, use the AshTestHelper which does all
of these things already. It also sets up the compositor to use test contexts.

[danakj, 2013-07-31 20:36:03]

Also, interestingly on win7_aura:

LayerWithRealCompositorTest.DrawPixels: 
e:\b\build\slave\win7_aura\build\src\ui\compositor\layer_unittest.cc(819):
error: Value of: GetCompositor()->size().ToString()
Actual: "484x462"
Expected: gfx::Size(500, 500).ToString()
Which is: "500x500"
e:\b\build\slave\win7_aura\build\src\ui\compositor\layer_unittest.cc(822):
error: Value of: GetCompositor()->ReadPixels(&bitmap, gfx::Rect(0, 0, 500, 500))
Actual: false
Expected: true

The compositor seems to be smaller there, maybe due to window borders etc being
included in the requested host size? It seems like we should just use some
ui::Layers that aren't meant to fill the whole compositor window there.

[danakj, 2013-08-01 21:30:27]

PTAL, the tests failing in PS12 were all flaky failing for me locally, so I'm
disabling them in another CL (also inside this one for now, it'll rebase away).

[danakj, 2013-08-01 21:31:53]

+sky Please have a look for a more deterministic and orderly compositor
startup/shutdown. We use DCHECKs to ensure things are set up for tests and torn
down in an appropriate order, rather than lazy initialization and leaking.

[danakj, 2013-08-02 16:02:16]

piman/sky: review ping

I keep getting different flaky tests failing on the bots, but the patch should
be fully ready for review/landing otherwise.

[danakj, 2013-08-03 00:06:37]

Non-test comopsitor is super flaky with threaded compositor in these tests:
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

So went back to disabling the thread - but only when using real contexts, so
this isn't disabling all test coverage of the thread.

[sky, 2013-08-05 16:50:58]

LGTM

https://codereview.chromium.org/21052007/diff/295001/content/browser/aura/ima...
File content/browser/aura/image_transport_factory.cc (right):

https://codereview.chromium.org/21052007/diff/295001/content/browser/aura/ima...
content/browser/aura/image_transport_factory.cc:49: if (use_test_contexts) {
nit: no need for local variable here.

https://codereview.chromium.org/21052007/diff/295001/content/browser/media/me...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/21052007/diff/295001/content/browser/media/me...
content/browser/media/media_browsertest.cc:35: 
nit: only one newline.

https://codereview.chromium.org/21052007/diff/295001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/21052007/diff/295001/ui/compositor/compositor...
ui/compositor/compositor.h:105: virtual bool UsesTestContexts() = 0;
nit: IMO a better name for this is IsUsingTestContext().

[danakj, 2013-08-05 16:53:34]

https://codereview.chromium.org/21052007/diff/295001/content/browser/aura/ima...
File content/browser/aura/image_transport_factory.cc (right):

https://codereview.chromium.org/21052007/diff/295001/content/browser/aura/ima...
content/browser/aura/image_transport_factory.cc:49: if (use_test_contexts) {
On 2013/08/05 16:50:58, sky wrote:
> nit: no need for local variable here.

Done.

https://codereview.chromium.org/21052007/diff/295001/content/browser/media/me...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/21052007/diff/295001/content/browser/media/me...
content/browser/media/media_browsertest.cc:35: 
On 2013/08/05 16:50:58, sky wrote:
> nit: only one newline.

Done.

https://codereview.chromium.org/21052007/diff/295001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/21052007/diff/295001/ui/compositor/compositor...
ui/compositor/compositor.h:105: virtual bool UsesTestContexts() = 0;
On 2013/08/05 16:50:58, sky wrote:
> nit: IMO a better name for this is IsUsingTestContext().

That name sounds like it could change, whereas I was going for something to
describe the class for its entire lifetime. How about DoesUseTestContexts() if
you want a verb in front.

[sky, 2013-08-05 17:44:54]

I don't think any of the names imply the value can or can't change. A
comment is better for detailing whether the value can or can't change
during its lifetime.

On Mon, Aug 5, 2013 at 9:53 AM,  <danakj@chromium.org> wrote:
>
>
https://codereview.chromium.org/21052007/diff/295001/content/browser/aura/ima...
> File content/browser/aura/image_transport_factory.cc (right):
>
>
https://codereview.chromium.org/21052007/diff/295001/content/browser/aura/ima...
> content/browser/aura/image_transport_factory.cc:49: if
> (use_test_contexts) {
> On 2013/08/05 16:50:58, sky wrote:
>>
>> nit: no need for local variable here.
>
>
> Done.
>
>
>
https://codereview.chromium.org/21052007/diff/295001/content/browser/media/me...
> File content/browser/media/media_browsertest.cc (right):
>
>
https://codereview.chromium.org/21052007/diff/295001/content/browser/media/me...
> content/browser/media/media_browsertest.cc:35:
> On 2013/08/05 16:50:58, sky wrote:
>>
>> nit: only one newline.
>
>
> Done.
>
>
>
https://codereview.chromium.org/21052007/diff/295001/ui/compositor/compositor.h
> File ui/compositor/compositor.h (right):
>
>
https://codereview.chromium.org/21052007/diff/295001/ui/compositor/compositor...
> ui/compositor/compositor.h:105: virtual bool UsesTestContexts() = 0;
> On 2013/08/05 16:50:58, sky wrote:
>>
>> nit: IMO a better name for this is IsUsingTestContext().
>
>
> That name sounds like it could change, whereas I was going for something
> to describe the class for its entire lifetime. How about
> DoesUseTestContexts() if you want a verb in front.
>
> https://codereview.chromium.org/21052007/

[danakj, 2013-08-05 20:53:35]

On 2013/08/05 17:44:54, sky wrote:
> I don't think any of the names imply the value can or can't change. A
> comment is better for detailing whether the value can or can't change
> during its lifetime.

Hm, I think this pointed out the bad/lazy name that I had picked in general.
DoesCreateTestContexts() captures more correctly what the function was trying to
say. The factory doesn't really use contexts so much as create them. Hope that
sounds okay!

> On Mon, Aug 5, 2013 at 9:53 AM,  <mailto:danakj@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/21052007/diff/295001/content/browser/aura/ima...
> > File content/browser/aura/image_transport_factory.cc (right):
> >
> >
>
https://codereview.chromium.org/21052007/diff/295001/content/browser/aura/ima...
> > content/browser/aura/image_transport_factory.cc:49: if
> > (use_test_contexts) {
> > On 2013/08/05 16:50:58, sky wrote:
> >>
> >> nit: no need for local variable here.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/21052007/diff/295001/content/browser/media/me...
> > File content/browser/media/media_browsertest.cc (right):
> >
> >
>
https://codereview.chromium.org/21052007/diff/295001/content/browser/media/me...
> > content/browser/media/media_browsertest.cc:35:
> > On 2013/08/05 16:50:58, sky wrote:
> >>
> >> nit: only one newline.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/21052007/diff/295001/ui/compositor/compositor.h
> > File ui/compositor/compositor.h (right):
> >
> >
>
https://codereview.chromium.org/21052007/diff/295001/ui/compositor/compositor...
> > ui/compositor/compositor.h:105: virtual bool UsesTestContexts() = 0;
> > On 2013/08/05 16:50:58, sky wrote:
> >>
> >> nit: IMO a better name for this is IsUsingTestContext().
> >
> >
> > That name sounds like it could change, whereas I was going for something
> > to describe the class for its entire lifetime. How about
> > DoesUseTestContexts() if you want a verb in front.
> >
> > https://codereview.chromium.org/21052007/

[piman, 2013-08-05 21:30:20]

lgtm

[commit-bot: I haz the power, 2013-08-05 21:49:55]

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

[commit-bot: I haz the power, 2013-08-05 22:11:55]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[danakj, 2013-08-05 22:22:08]

+joi for components/
+rsleevi for net/

[Ryan Sleevi, 2013-08-05 22:23:01]

https://codereview.chromium.org/21052007/diff/321001/net/http/http_network_tr...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/21052007/diff/321001/net/http/http_network_tr...
net/http/http_network_transaction_unittest.cc:10798: 
Um... do you actually need this? :)

[danakj, 2013-08-05 22:33:30]

https://codereview.chromium.org/21052007/diff/321001/net/http/http_network_tr...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/21052007/diff/321001/net/http/http_network_tr...
net/http/http_network_transaction_unittest.cc:10798: 
On 2013/08/05 22:23:03, Ryan Sleevi wrote:
> Um... do you actually need this? :)

Oh, heh. :)

[danakj, 2013-08-06 21:05:06]

I can no longer upload to this CL (boo). So I'm moving to
https://codereview.chromium.org/22293003/ and closing this. I'll TBR from there
once I can get win_rel to pass if I don't have to do anything silly.

[Jói Sigurðsson, 2013-08-09 16:57:33]

joi.sigurdsson@gmail.com -> joi@chromium.org; also, I'm on vacation
until Monday, will look then if this is still live.

Cheers,
Jói


On Tue, Aug 6, 2013 at 9:05 PM,  <danakj@chromium.org> wrote:
> I can no longer upload to this CL (boo). So I'm moving to
> https://codereview.chromium.org/22293003/ and closing this. I'll TBR from
> there
> once I can get win_rel to pass if I don't have to do anything silly.
>
> https://codereview.chromium.org/21052007/