Parameterize the composited RenderWidgetHostViewBrowserTests.

Parameterize the composited RenderWidgetHostViewBrowserTests.

Run these tests with both hardware and software compositing. When
software compositing is enabled for a browser test, it also sets
appropriate command line flags to ensure the renderer is software
composited and the ui compositor is single-threaded.

R=piman@chromium.org
BUG=327508
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242080

[danakj, 2013-12-19 20:50:21]

sky@ could you please review as piman is OOO, or suggest another owner?

[sky, 2013-12-19 21:35:32]

https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:227:
compositing_mode_ = GL_COMPOSITING;
Wouldn't this be better placed around INSTANTIATE_TEST_CASE_P as otherwise
you're going to run tests twice with GL_COMPOSITING?

https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:290:
CompositingMode compositing_mode_;
const and private and DISALLOW_...?

https://codereview.chromium.org/116893004/diff/40001/content/public/test/brow...
File content/public/test/browser_test_base.h (right):

https://codereview.chromium.org/116893004/diff/40001/content/public/test/brow...
content/public/test/browser_test_base.h:123: void UseRealGLContexts();
Can we have one setter here? Maybe SetCompositingMode or whatever you prefer
that takes an enum?

[danakj, 2013-12-19 21:40:53]

https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:227:
compositing_mode_ = GL_COMPOSITING;
On 2013/12/19 21:35:32, sky wrote:
> Wouldn't this be better placed around INSTANTIATE_TEST_CASE_P as otherwise
> you're going to run tests twice with GL_COMPOSITING?

Hm! Never heard of such a thing, I'll go look it up.

I had resigned to running twice since it's just GTK which is going away anyway,
but it's good to know about this.

https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:290:
CompositingMode compositing_mode_;
On 2013/12/19 21:35:32, sky wrote:
> const and private and DISALLOW_...?

Sure, leaky encapsulation in test fixtures has become a bit of habit.

https://codereview.chromium.org/116893004/diff/40001/content/public/test/brow...
File content/public/test/browser_test_base.h (right):

https://codereview.chromium.org/116893004/diff/40001/content/public/test/brow...
content/public/test/browser_test_base.h:123: void UseRealGLContexts();
On 2013/12/19 21:35:32, sky wrote:
> Can we have one setter here? Maybe SetCompositingMode or whatever you prefer
> that takes an enum?

Hm, this is a good idea, thanks. I'm actually going to be working on removing
the useRealGLContexts thing entirely next, but that still leaves 3 states that
could be an enum.

[danakj, 2013-12-19 21:45:30]

https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:227:
compositing_mode_ = GL_COMPOSITING;
On 2013/12/19 21:40:54, danakj wrote:
> On 2013/12/19 21:35:32, sky wrote:
> > Wouldn't this be better placed around INSTANTIATE_TEST_CASE_P as otherwise
> > you're going to run tests twice with GL_COMPOSITING?
> 
> Hm! Never heard of such a thing, I'll go look it up.
> 
> I had resigned to running twice since it's just GTK which is going away
anyway,
> but it's good to know about this.

Oh, I see what you mean, the INSTANTIATE thing at the bottom. I can change the
values passed to it, good thought.

[sky, 2013-12-19 21:45:59]

On Thu, Dec 19, 2013 at 1:45 PM,  <danakj@chromium.org> wrote:
>
>
https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
> File
> content/browser/renderer_host/render_widget_host_view_browsertest.cc
> (right):
>
>
https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_browsertest.cc:227:
> compositing_mode_ = GL_COMPOSITING;
> On 2013/12/19 21:40:54, danakj wrote:
>>
>> On 2013/12/19 21:35:32, sky wrote:
>> > Wouldn't this be better placed around INSTANTIATE_TEST_CASE_P as
>
> otherwise
>>
>> > you're going to run tests twice with GL_COMPOSITING?
>
>
>> Hm! Never heard of such a thing, I'll go look it up.
>
>
>> I had resigned to running twice since it's just GTK which is going
>
> away anyway,
>>
>> but it's good to know about this.
>
>
> Oh, I see what you mean, the INSTANTIATE thing at the bottom. I can
> change the values passed to it, good thought.

Exactly. I'm suggesting you move the ifdef down there.

  -Scott

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

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

[danakj, 2013-12-19 22:21:05]

Addressed feedback except for the enum. The enum is tricky.. here's the states
we have:

1. Use stubbed out test context that does not do real GL.

This is the default, unless a test turns it off because they want pixel output
or the context causes the test to fail somehow so they request a real GL
context.
- This is always off on OS_CHROMEOS.
- This can be turned off by a command line argument.
- This will go away entirely in the future.

Let's call this COMPOSITING_MODE_GL_WITH_STUBS.

2. Use hardware GL drivers instead of OSMesa (if 1 is false).

This is off by default, unless a test fails with OSMesa because of driver issues
or timing weirdness (flake I think).
- This is always on for OS_MACOSX.
- This is always on for OS_ANDROID.
- This is always on for OS_CHROMEOS when running on VM bots.
- This can be turned on by 2 different command line flags.

Let's call this COMPOSITING_MODE_GL_WITH_OSMESA and
COMPOSITING_MODE_GL_WITH_HARDWARE_GPU.

The settings here that are OS-specific will only kick in if setting 1 is false.
What this means is that on mac/android/chromeos vms,
COMPOSITING_MODE_GL_WITH_OSMESA is not a valid state.

The problem here is that some tests want to turn off 1, so they need to set the
mode to COMOPSITING_MODE_GL_WITH_HARDWARE_GPU, but now each caller of
SetCompositingMode needs to have an #ifdef MAC/etc. This isn't going to work out
nicely at all.

We could move these three bools to be an enum internally inside the
BrowserTestBase, but I'd prefer to punt on this and keep working on removing
COMPOSITING_MODE_GL_WITH_STUBS instead.

WDYT?

https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:227:
compositing_mode_ = GL_COMPOSITING;
On 2013/12/19 21:45:31, danakj wrote:
> On 2013/12/19 21:40:54, danakj wrote:
> > On 2013/12/19 21:35:32, sky wrote:
> > > Wouldn't this be better placed around INSTANTIATE_TEST_CASE_P as otherwise
> > > you're going to run tests twice with GL_COMPOSITING?
> > 
> > Hm! Never heard of such a thing, I'll go look it up.
> > 
> > I had resigned to running twice since it's just GTK which is going away
> anyway,
> > but it's good to know about this.
> 
> Oh, I see what you mean, the INSTANTIATE thing at the bottom. I can change the
> values passed to it, good thought.

Done.

https://codereview.chromium.org/116893004/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:290:
CompositingMode compositing_mode_;
On 2013/12/19 21:40:54, danakj wrote:
> On 2013/12/19 21:35:32, sky wrote:
> > const and private and DISALLOW_...?
> 
> Sure, leaky encapsulation in test fixtures has become a bit of habit.

Done.

[sky, 2013-12-19 22:27:50]

Ok, LGTM

[commit-bot: I haz the power, 2013-12-19 22:55:25]

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

[commit-bot: I haz the power, 2013-12-19 23:49:18]

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

[commit-bot: I haz the power, 2013-12-20 00:03:16]

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

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

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

[commit-bot: I haz the power, 2013-12-20 09:47:46]

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

[commit-bot: I haz the power, 2013-12-20 12:52:16]

Change committed as 242080