ui: Allow individual test suites to opt-out of the TestCompositor.

ui: Allow individual test suites to opt-out of the TestCompositor.

This adds a temporary bool to GLSurface::InitializeOneOffForTests()
allowing a test suite to opt-in to NullDraw bindings. When a test
suite as opted in, the InitializeContextFactoryForTests() method
can see, and will not use the TestContextFactory, instead using the
InProcessContextFactory.

Once all test suites are optign into NullDraw bindings. The bool
and query method for HasInitializedNullDrawGLBindings() will be
removed.

R=kbr@chromium.org, sievers@chromium.org
BUG=270918
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252647

[danakj, 2014-02-21 17:04:31]

Daniel, can you please have a look? This is a temporary state to let me get to
https://codereview.chromium.org/145293007/ one test suite at a time until I can
remove TestContextFactory entirely.

[danakj, 2014-02-21 18:45:00]

oh, sievers isn't ui/gl/ owner surprisingly.

+kbr for ui/gl/

[no sievers, 2014-02-21 19:51:49]

https://codereview.chromium.org/175093004/diff/160002/ui/compositor/test/cont...
File ui/compositor/test/context_factories_for_test.cc (right):

https://codereview.chromium.org/175093004/diff/160002/ui/compositor/test/cont...
ui/compositor/test/context_factories_for_test.cc:18: static
gfx::DisableNullDrawGLBindings* g_disable_null_draw = NULL;
nit: Shouldn't these statics be in an anonymous namespace instead?

https://codereview.chromium.org/175093004/diff/160002/ui/compositor/test/cont...
ui/compositor/test/context_factories_for_test.cc:31: use_test_contexts = false;
Is that ok even if enable_pixel_output == true?

https://codereview.chromium.org/175093004/diff/160002/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/175093004/diff/160002/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:78: static void InitializeOneOffForTests(bool disable_drawing
= false);
I thought the default param was against the style guide.

[danakj, 2014-02-21 19:56:58]

https://codereview.chromium.org/175093004/diff/160002/ui/compositor/test/cont...
File ui/compositor/test/context_factories_for_test.cc (right):

https://codereview.chromium.org/175093004/diff/160002/ui/compositor/test/cont...
ui/compositor/test/context_factories_for_test.cc:18: static
gfx::DisableNullDrawGLBindings* g_disable_null_draw = NULL;
On 2014/02/21 19:51:49, sievers wrote:
> nit: Shouldn't these statics be in an anonymous namespace instead?

Sure!

https://codereview.chromium.org/175093004/diff/160002/ui/compositor/test/cont...
ui/compositor/test/context_factories_for_test.cc:31: use_test_contexts = false;
On 2014/02/21 19:51:49, sievers wrote:
> Is that ok even if enable_pixel_output == true?

Ya, the gfx::DisableNullDrawGLBindings down below is going to cause us to get
pixel output.

Eventually use_test_contexts will always be false, replacing it with the
nulldraw bindings.

https://codereview.chromium.org/175093004/diff/160002/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/175093004/diff/160002/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:78: static void InitializeOneOffForTests(bool disable_drawing
= false);
On 2014/02/21 19:51:49, sievers wrote:
> I thought the default param was against the style guide.

Ya kinda. But it's just for a bit, in test suites I'll
InitializeOneOffForTests(true) to opt them in. Once everyone is (true) I can
remove the argument.

The alternative is add another method InitializeOneOffForTestsWithNullContexts,
and then go back and delete the method. I was trying to keep the churn down for
a temporary hack for our tests, but it's going to hit all the same lines of code
either way. But makes more more churn here in gl_surface, as I would have to
split this method up and have both methods call a 3rd internal method which then
just calls a 4th internal method. Meh, defer to your judgement on it :)

[Ken Russell (switch to Gerrit), 2014-02-21 20:03:37]

lgtm

https://codereview.chromium.org/175093004/diff/160002/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/175093004/diff/160002/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:78: static void InitializeOneOffForTests(bool disable_drawing
= false);
On 2014/02/21 19:56:58, danakj wrote:
> On 2014/02/21 19:51:49, sievers wrote:
> > I thought the default param was against the style guide.
> 
> Ya kinda. But it's just for a bit, in test suites I'll
> InitializeOneOffForTests(true) to opt them in. Once everyone is (true) I can
> remove the argument.
> 
> The alternative is add another method
InitializeOneOffForTestsWithNullContexts,
> and then go back and delete the method. I was trying to keep the churn down
for
> a temporary hack for our tests, but it's going to hit all the same lines of
code
> either way. But makes more more churn here in gl_surface, as I would have to
> split this method up and have both methods call a 3rd internal method which
then
> just calls a 4th internal method. Meh, defer to your judgement on it :)

It's fine as a temporary workaround, but please add a TODO about removing it.

[no sievers, 2014-02-21 20:09:08]

lgtm2

[danakj, 2014-02-21 20:20:53]

https://codereview.chromium.org/175093004/diff/160002/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/175093004/diff/160002/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:78: static void InitializeOneOffForTests(bool disable_drawing
= false);
On 2014/02/21 20:03:37, Ken Russell wrote:
> On 2014/02/21 19:56:58, danakj wrote:
> > On 2014/02/21 19:51:49, sievers wrote:
> > > I thought the default param was against the style guide.
> > 
> > Ya kinda. But it's just for a bit, in test suites I'll
> > InitializeOneOffForTests(true) to opt them in. Once everyone is (true) I can
> > remove the argument.
> > 
> > The alternative is add another method
> InitializeOneOffForTestsWithNullContexts,
> > and then go back and delete the method. I was trying to keep the churn down
> for
> > a temporary hack for our tests, but it's going to hit all the same lines of
> code
> > either way. But makes more more churn here in gl_surface, as I would have to
> > split this method up and have both methods call a 3rd internal method which
> then
> > just calls a 4th internal method. Meh, defer to your judgement on it :)
> 
> It's fine as a temporary workaround, but please add a TODO about removing it.

Thanks, good plan. Done!

[danakj, 2014-02-21 20:21:34]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-02-21 20:21:45]

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

[danakj, 2014-02-21 21:19:09]

Committed patchset #6 manually as r252647 (presubmit successful).