Use stub GL draw/clear calls for browser tests that do not need pixels.

Use stub GL draw/clear calls for browser tests that do not need pixels.

Instead of using the "TestCompositor" use real GL contexts that just
stub out these expensive functions.

Previously browser tests that did not disable the test compositor
would fall into legacy software mode in the renderer, causing the
renderer to produce frames in a way we don't ship. Now all tests
will use a compositor in both the browser and the renderers.

This allows us to remove the concept the "test compositor" from
browser tests (and from unit tests if we want).

R=piman@chromium.org, sky
BUG=270918
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246107

[no sievers, 2013-12-20 23:27:58]

Looks good. I would maybe rename it from 'Stub'(Bindings etc.) to 'NullDraw' or
something. Otherwise stub sounds a bit vague and suggests that all functions
might be stubbed out.

https://codereview.chromium.org/120313002/diff/1/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://codereview.chromium.org/120313002/diff/1/ui/gl/gl_surface.cc#newcode74
ui/gl/gl_surface.cc:74: InitializeStubGLBindings();
The only caveat with that is that it would potentially not work with a function
specified as a stub in the generator script which is part of an extension. In
that case we would lazily call InitializeExtensionBindings() from the first
MakeCurrent(), which will override them again.

[danakj, 2013-12-20 23:29:37]

https://codereview.chromium.org/120313002/diff/1/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://codereview.chromium.org/120313002/diff/1/ui/gl/gl_surface.cc#newcode74
ui/gl/gl_surface.cc:74: InitializeStubGLBindings();
On 2013/12/20 23:27:58, sievers wrote:
> The only caveat with that is that it would potentially not work with a
function
> specified as a stub in the generator script which is part of an extension. In
> that case we would lazily call InitializeExtensionBindings() from the first
> MakeCurrent(), which will override them again.

Ya, I noticed for the debug bindings, the generate_bindings script sets up a
bool and when it sets up extension bindings, it will check that bool and set up
debug bindings implicitly for the extensions at that time.

Since we don't need that now, we can ignore it, and use the same mechanism if we
wanted it, I think?

[no sievers, 2013-12-20 23:32:12]

On 2013/12/20 23:29:37, danakj wrote:
> https://codereview.chromium.org/120313002/diff/1/ui/gl/gl_surface.cc
> File ui/gl/gl_surface.cc (right):
> 
> https://codereview.chromium.org/120313002/diff/1/ui/gl/gl_surface.cc#newcode74
> ui/gl/gl_surface.cc:74: InitializeStubGLBindings();
> On 2013/12/20 23:27:58, sievers wrote:
> > The only caveat with that is that it would potentially not work with a
> function
> > specified as a stub in the generator script which is part of an extension.
In
> > that case we would lazily call InitializeExtensionBindings() from the first
> > MakeCurrent(), which will override them again.
> 
> Ya, I noticed for the debug bindings, the generate_bindings script sets up a
> bool and when it sets up extension bindings, it will check that bool and set
up
> debug bindings implicitly for the extensions at that time.
> 
> Since we don't need that now, we can ignore it, and use the same mechanism if
we
> wanted it, I think?

Sounds good. Also this is only an optimization I assume to speed up unit test
run time?
So I guess it wouldn't even really be broken functionally in a specific way.

[danakj, 2013-12-20 23:34:32]

On Fri, Dec 20, 2013 at 6:32 PM, <sievers@chromium.org> wrote:

> On 2013/12/20 23:29:37, danakj wrote:
>
>> https://codereview.chromium.org/120313002/diff/1/ui/gl/gl_surface.cc
>> File ui/gl/gl_surface.cc (right):
>>
>
>  https://codereview.chromium.org/120313002/diff/1/ui/gl/gl_
>> surface.cc#newcode74
>> ui/gl/gl_surface.cc:74: InitializeStubGLBindings();
>> On 2013/12/20 23:27:58, sievers wrote:
>> > The only caveat with that is that it would potentially not work with a
>> function
>> > specified as a stub in the generator script which is part of an
>> extension.
>>
> In
>
>> > that case we would lazily call InitializeExtensionBindings() from the
>> first
>> > MakeCurrent(), which will override them again.
>>
>
>  Ya, I noticed for the debug bindings, the generate_bindings script sets
>> up a
>> bool and when it sets up extension bindings, it will check that bool and
>> set
>>
> up
>
>> debug bindings implicitly for the extensions at that time.
>>
>
>  Since we don't need that now, we can ignore it, and use the same
>> mechanism if
>>
> we
>
>> wanted it, I think?
>>
>
> Sounds good. Also this is only an optimization I assume to speed up unit
> test
> run time?
> So I guess it wouldn't even really be broken functionally in a specific
> way.
>

Right, exactly

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-01-08 05:24:41]

I think you accidentally uploaded a couple of other changes together with this
one (the snapshot stuff and the OwnedMailbox one).

Other than that, it looks promising :)
Can we remove UseRealGLBindings altogether at this point?

[danakj, 2014-01-08 08:32:26]

On Jan 8, 2014 12:24 AM, <piman@chromium.org> wrote:
>
> I think you accidentally uploaded a couple of other changes together with
this
> one (the snapshot stuff and the OwnedMailbox one).

Ya not accident. I'll remove once I get to green bots.

> Other than that, it looks promising :)

Thanks :) Getting there..

> Can we remove UseRealGLBindings altogether at this point?

Yup! I'm still using to debug as I go but I'll def remove it. I'll rename
the other to be about pixel results after as well.

>
> https://codereview.chromium.org/120313002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-01-20 16:26:56]

piman: please review
sky: please review chrome/*

[sky, 2014-01-21 16:01:47]

LGTM

[danakj, 2014-01-21 18:35:01]

All pre-reqs have now landed AFAICT. I'll wait for piman@ review then land this
today.

[piman, 2014-01-21 19:20:51]

lgtm

[commit-bot: I haz the power, 2014-01-21 19:24:30]

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

[commit-bot: I haz the power, 2014-01-21 20:59:30]

Change committed as 246107