ui: Move software fallback decisions to GpuProcessTransportFactory.

ui: Move software fallback decisions to GpuProcessTransportFactory.

This moves software fallback decisions to GpuProcessTransportFactory
from the ui::Compositor, where it was already making decisions like
"can I fallback to software compositing on this OS". Now it will not
return an OutputSurface to ui::Compositor until it has successfully
bound/initialized one.

R=jbauman, piman@chromium.org
BUG=461396
Committed: https://crrev.com/13d970853c63df742f471b1b4c4a66e36a831c6d
Cr-Commit-Position: refs/heads/master@{#318135}

[danakj, 2015-02-25 01:32:21]

danakj@chromium.org changed reviewers:
+ sievers@chromium.org

[danakj, 2015-02-25 01:32:22]

+sievers

[piman, 2015-02-25 01:44:47]

lgtm

https://codereview.chromium.org/953803005/diff/1/content/browser/compositor/g...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/953803005/diff/1/content/browser/compositor/g...
content/browser/compositor/gpu_process_transport_factory.cc:131: static bool
CreateGpuOutputSurface() {
nit: ShouldCreateGpuOuputSurface? (this doesn't create anything).

[jbauman, 2015-02-25 01:48:04]

lgtm

[no sievers, 2015-02-25 01:51:01]

lgtm with 2 nits

https://codereview.chromium.org/953803005/diff/1/content/browser/compositor/g...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/953803005/diff/1/content/browser/compositor/g...
content/browser/compositor/gpu_process_transport_factory.cc:63: static const int
kNumRetriesBeforeSoftwareFallback = 4;
nit: use anonymous namespace, drop static

https://codereview.chromium.org/953803005/diff/1/content/browser/compositor/g...
content/browser/compositor/gpu_process_transport_factory.cc:131: static bool
CreateGpuOutputSurface() {
nit: ShouldCreateGpuOutputSurface() or ShouldUseGpuCompositing() or something?

Or inverse: bool ShouldUseSoftwareCompositing()?

https://codereview.chromium.org/953803005/diff/1/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/953803005/diff/1/ui/compositor/compositor.cc#...
ui/compositor/compositor.cc:313: NOTREACHED();
Although there is also |worker_context_provider_| being initialized in
OutputSurface::BindToClient() which could still fail.

Maybe we shouldn't make the assumption here about what else LTH(Impl)
initializes (or what GLRenderer does) that could fail and trigger this callback?

It would be a problem if it happens to create an infinite retry loop without
generating a crash report, I guess.

Should this maybe just also have a simple counter for successive attempts? (for
example reset in DidInitializeOutputSurface)

[enne (OOO), 2015-02-25 19:37:33]

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

[enne (OOO), 2015-02-25 19:37:33]

non-OWNER lgtm

https://codereview.chromium.org/953803005/diff/1/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/953803005/diff/1/ui/compositor/compositor.cc#...
ui/compositor/compositor.cc:313: NOTREACHED();
On 2015/02/25 at 01:51:00, sievers wrote:
> Although there is also |worker_context_provider_| being initialized in
OutputSurface::BindToClient() which could still fail.
> 
> Maybe we shouldn't make the assumption here about what else LTH(Impl)
initializes (or what GLRenderer does) that could fail and trigger this callback?
> 
> It would be a problem if it happens to create an infinite retry loop without
generating a crash report, I guess.
> 
> Should this maybe just also have a simple counter for successive attempts?
(for example reset in DidInitializeOutputSurface)

I think it'd be fine to document that this can only be called if the output
surface fails to bind.  That's definitely the case right now.

[no sievers, 2015-02-25 20:52:13]

https://codereview.chromium.org/953803005/diff/1/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/953803005/diff/1/ui/compositor/compositor.cc#...
ui/compositor/compositor.cc:313: NOTREACHED();
On 2015/02/25 19:37:33, enne wrote:
> On 2015/02/25 at 01:51:00, sievers wrote:
> > Although there is also |worker_context_provider_| being initialized in
> OutputSurface::BindToClient() which could still fail.
> > 
> > Maybe we shouldn't make the assumption here about what else LTH(Impl)
> initializes (or what GLRenderer does) that could fail and trigger this
callback?
> > 
> > It would be a problem if it happens to create an infinite retry loop without
> generating a crash report, I guess.
> > 
> > Should this maybe just also have a simple counter for successive attempts?
> (for example reset in DidInitializeOutputSurface)
> 
> I think it'd be fine to document that this can only be called if the output
> surface fails to bind.  That's definitely the case right now.

Sounds good then. I guess it is in the name :)

[danakj, 2015-02-25 21:25:39]

https://codereview.chromium.org/953803005/diff/1/content/browser/compositor/g...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/953803005/diff/1/content/browser/compositor/g...
content/browser/compositor/gpu_process_transport_factory.cc:131: static bool
CreateGpuOutputSurface() {
On 2015/02/25 01:51:00, sievers wrote:
> nit: ShouldCreateGpuOutputSurface() or ShouldUseGpuCompositing() or something?

+1

> Or inverse: bool ShouldUseSoftwareCompositing()?

I did that at first but had more !s in the code so I inverted it.

[danakj, 2015-02-25 21:28:23]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-02-25 21:28:23]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
sievers@chromium.org, jbauman@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/953803005/#ps20001 (title:
"fallback: winfixandshould")

[commit-bot: I haz the power, 2015-02-25 21:29:08]

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

[commit-bot: I haz the power, 2015-02-25 23:05:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-25 23:06:11]

Patchset 2 (id:??) landed as
https://crrev.com/13d970853c63df742f471b1b4c4a66e36a831c6d
Cr-Commit-Position: refs/heads/master@{#318135}