Always enable FCM on Windows.

Always enable FCM on Windows.

This also re-enables loading the blacklist in tests (and adds a content_browsertest to make sure that the configuration we expect to be testing is indeed the one we are testing).

This is a stepping stone towards getting the waterfall closer to what we actually ship and closer to Aura; I will try to force threaded compositing next provided this goes well.

This CL also cleans up compositor_util.cc which was enforcing the blacklist twice.

Instead of keeping --skip-gpu-data-loading around to avoid regressing issue 190942, introduce a better global solution which involves initializing a fake gpu_info in GpuDataManagerImplPrivate::Initialize instead of querying real gpu_info (slow) when OSMesa is the GL being used (which it is in non-pure GPU tests).

Keeps --skip-gpu-data-loading around temporarily on Mac and Linux, see http://crbug.com/277242 and try failures on patch set 17 for details.

BUG=233830, 267038, 190942, 277242
TBR=jcivelli, piman

Originally Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219132
Reverted: https://src.chromium.org/viewvc/chrome?view=rev&revision=219159

[gab, 2013-08-05 16:38:19]

Short CL, please take a look.

Thanks!
Gab

[apatrick_chromium, 2013-08-05 19:06:22]

Is it possible to do this with the GPU blacklist? See
gpu/config/software_rendering_list_json.cc. I think you could maybe just add an
entry to blacklist FCM on versions of Windows prior to Vista.

[gab, 2013-08-06 15:10:38]

On 2013/08/05 19:06:22, apatrick_chromium wrote:
> Is it possible to do this with the GPU blacklist? See
> gpu/config/software_rendering_list_json.cc. I think you could maybe just add
an
> entry to blacklist FCM on versions of Windows prior to Vista.

Done, tweaked some, and added a test. PTAL.

Thanks!
Gab

[gab, 2013-08-06 15:11:21]

Some inline comments to guide your review.

https://codereview.chromium.org/22198004/diff/14001/content/browser/gpu/compo...
File content/browser/gpu/compositor_util.cc (left):

https://codereview.chromium.org/22198004/diff/14001/content/browser/gpu/compo...
content/browser/gpu/compositor_util.cc:39: bool
IsForceCompositingModeBlacklisted() {
Already enforced by CanDoAcceleratedCompositing()

https://codereview.chromium.org/22198004/diff/14001/content/browser/gpu/compo...
content/browser/gpu/compositor_util.cc:57: // Command line switches take
precedence over blacklist and field trials.
Moved CanDoAcceleratedCompositing() check down to respect this comment.

[wiltzius, 2013-08-06 16:58:04]

Yeah; Al as Gab says the idea is to move to using the blacklist as the single
control point, rather than also using the field trial. We can just get rid of
the field trial, it's been at 100% for many milestones now.

The only logic we should be changing is that Aura builds should always have FCM
and threaded compositing enabled.


On 2013/08/06 15:11:21, gab wrote:
> Some inline comments to guide your review.
> 
>
https://codereview.chromium.org/22198004/diff/14001/content/browser/gpu/compo...
> File content/browser/gpu/compositor_util.cc (left):
> 
>
https://codereview.chromium.org/22198004/diff/14001/content/browser/gpu/compo...
> content/browser/gpu/compositor_util.cc:39: bool
> IsForceCompositingModeBlacklisted() {
> Already enforced by CanDoAcceleratedCompositing()
> 
>
https://codereview.chromium.org/22198004/diff/14001/content/browser/gpu/compo...
> content/browser/gpu/compositor_util.cc:57: // Command line switches take
> precedence over blacklist and field trials.
> Moved CanDoAcceleratedCompositing() check down to respect this comment.

[wiltzius, 2013-08-06 16:58:14]

https://codereview.chromium.org/22198004/diff/26001/content/browser/gpu/compo...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/22198004/diff/26001/content/browser/gpu/compo...
content/browser/gpu/compositor_util.cc:101: // TODO(gab): Do the same thing for
Mac OS (which has been enabled at 100%
Be careful about the Mac code, since it's only enabled for 10.8 (not 10.6 and
10.7). ccameron@ is weighing the possibility of backporting to 10.6 and 10.7
now; so good to coordinate with him (or let him clean this up)

[wiltzius, 2013-08-06 17:01:15]

Lastly, Gab this doesn't actually remove the field trial code -- is the idea
you'll enable it manually then remove the trial code after it sticks?

On 2013/08/06 16:58:14, wiltzius wrote:
>
https://codereview.chromium.org/22198004/diff/26001/content/browser/gpu/compo...
> File content/browser/gpu/compositor_util.cc (right):
> 
>
https://codereview.chromium.org/22198004/diff/26001/content/browser/gpu/compo...
> content/browser/gpu/compositor_util.cc:101: // TODO(gab): Do the same thing
for
> Mac OS (which has been enabled at 100%
> Be careful about the Mac code, since it's only enabled for 10.8 (not 10.6 and
> 10.7). ccameron@ is weighing the possibility of backporting to 10.6 and 10.7
> now; so good to coordinate with him (or let him clean this up)

[gab, 2013-08-06 18:34:58]

On 2013/08/06 17:01:15, wiltzius wrote:
> Lastly, Gab this doesn't actually remove the field trial code -- is the idea
> you'll enable it manually then remove the trial code after it sticks?

Mac still uses the field trial code, I put a TODO to do the same thing there
(blacklist for < 10.8 and otherwise return true?).

I want to minimize this CL's impact to Windows for now and follow-up with a full
cleanup shortly after.

> 
> On 2013/08/06 16:58:14, wiltzius wrote:
> >
>
https://codereview.chromium.org/22198004/diff/26001/content/browser/gpu/compo...
> > File content/browser/gpu/compositor_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/22198004/diff/26001/content/browser/gpu/compo...
> > content/browser/gpu/compositor_util.cc:101: // TODO(gab): Do the same thing
> for
> > Mac OS (which has been enabled at 100%
> > Be careful about the Mac code, since it's only enabled for 10.8 (not 10.6
and
> > 10.7). ccameron@ is weighing the possibility of backporting to 10.6 and 10.7
> > now; so good to coordinate with him (or let him clean this up)

[gab, 2013-08-06 18:35:51]

Also added a test to make sure the bots actually run what we expect them to run
(so we never end up in the current state where we aren't even sure what the bots
are running....).

On 2013/08/06 18:34:58, gab wrote:
> On 2013/08/06 17:01:15, wiltzius wrote:
> > Lastly, Gab this doesn't actually remove the field trial code -- is the idea
> > you'll enable it manually then remove the trial code after it sticks?
> 
> Mac still uses the field trial code, I put a TODO to do the same thing there
> (blacklist for < 10.8 and otherwise return true?).
> 
> I want to minimize this CL's impact to Windows for now and follow-up with a
full
> cleanup shortly after.
> 
> > 
> > On 2013/08/06 16:58:14, wiltzius wrote:
> > >
> >
>
https://codereview.chromium.org/22198004/diff/26001/content/browser/gpu/compo...
> > > File content/browser/gpu/compositor_util.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/22198004/diff/26001/content/browser/gpu/compo...
> > > content/browser/gpu/compositor_util.cc:101: // TODO(gab): Do the same
thing
> > for
> > > Mac OS (which has been enabled at 100%
> > > Be careful about the Mac code, since it's only enabled for 10.8 (not 10.6
> and
> > > 10.7). ccameron@ is weighing the possibility of backporting to 10.6 and
10.7
> > > now; so good to coordinate with him (or let him clean this up)

[gab, 2013-08-06 18:47:07]

https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
File content/browser/gpu/compositor_util_browsertest.cc (right):

https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
content/browser/gpu/compositor_util_browsertest.cc:18:
IN_PROC_BROWSER_TEST_F(CompositorUtilTest, CompositingModeAsExpected) {
Maybe this can be a unit test rather than a browser test; wasn't sure whether
that would be sufficient to make sure the blacklist is loaded, etc. so that
IsForceCompositingModeEnabled() and IsThreadedCompositingEnabled() actually
return the same thing here that they will return in full browser tests.

It felt safer to just make this a browser test to make sure we never end up in a
case where something is not applied for this test (and it thus passes) when the
reality is not the same...

[apatrick_chromium, 2013-08-08 20:06:44]

LGTM but please hold off on committing until we've completed our experimental
disabling of all GPU features on Canary so this has stability coverage when it
goes in and doesn't affect the experiment.

[gab, 2013-08-09 14:17:02]

https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
File content/browser/gpu/compositor_util_browsertest.cc (right):

https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
content/browser/gpu/compositor_util_browsertest.cc:31: if
(base::win::GetVersion() >= base::win::VERSION_VISTA)
Actually this won't work because the blacklist is disabled on the bots, right?

Talking with wiltzius@ that seemed to be for a legacy reason since we wanted to
blacklist VM GPUs, but not our VMs' GPUs? Now that we have real GPUs on bots (is
that right?) we should be ok with using the blacklist on the bots and then
having this test to make sure that only the bots we expect (XP bots) are
actually blacklisted.

Does that make sense?

[wiltzius, 2013-08-09 21:20:04]

Complexity, complexity. There are two kinds of bots (relevant here), the
main waterfall bots (and associated trybots, and winaura bots), which use
VMs; the perf bots, which have real GPUs.

It's not really for a legacy reason that we wanted to blacklist VM GPUs but
not our VMs GPUs, this still applies. When using Chrome in the wild with
most VMs using the virtual GPU would only slow you down, although we
allowed it by default and only blacklisted as people complained about
specific VMs. On the main waterfall bots we care less about speed and more
about making sure the right code paths are exercised, hence want to use the
GPU path even in the VM. On the perf bots we care about speed, and hence
got real hardware for those.





On Fri, Aug 9, 2013 at 7:17 AM, <gab@chromium.org> wrote:

>
> https://codereview.chromium.**org/22198004/diff/21002/**
>
content/browser/gpu/**compositor_util_browsertest.cc<https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compositor_util_browsertest.cc>
> File content/browser/gpu/**compositor_util_browsertest.cc (right):
>
> https://codereview.chromium.**org/22198004/diff/21002/**
>
content/browser/gpu/**compositor_util_browsertest.**cc#newcode31<https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compositor_util_browsertest.cc#newcode31>
> content/browser/gpu/**compositor_util_browsertest.**cc:31: if
> (base::win::GetVersion() >= base::win::VERSION_VISTA)
> Actually this won't work because the blacklist is disabled on the bots,
> right?
>
> Talking with wiltzius@ that seemed to be for a legacy reason since we
> wanted to blacklist VM GPUs, but not our VMs' GPUs? Now that we have
> real GPUs on bots (is that right?) we should be ok with using the
> blacklist on the bots and then having this test to make sure that only
> the bots we expect (XP bots) are actually blacklisted.
>
> Does that make sense?
>
>
https://codereview.chromium.**org/22198004/<https://codereview.chromium.org/2...
>

[gab, 2013-08-10 02:11:07]

Ok, so then blacklisting all XP machines won't work because we will enable FCM
on the XP bots which ignore the blacklist... 


On 2013/08/09 21:20:04, wiltzius wrote:
> Complexity, complexity. There are two kinds of bots (relevant here), the
> main waterfall bots (and associated trybots, and winaura bots), which use
> VMs; the perf bots, which have real GPUs.
> 
> It's not really for a legacy reason that we wanted to blacklist VM GPUs but
> not our VMs GPUs, this still applies. When using Chrome in the wild with
> most VMs using the virtual GPU would only slow you down, although we
> allowed it by default and only blacklisted as people complained about
> specific VMs. On the main waterfall bots we care less about speed and more
> about making sure the right code paths are exercised, hence want to use the
> GPU path even in the VM. On the perf bots we care about speed, and hence
> got real hardware for those.
> 
> 
> 
> 
> 
> On Fri, Aug 9, 2013 at 7:17 AM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/22198004/diff/21002/**
> >
>
content/browser/gpu/**compositor_util_browsertest.cc<https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compositor_util_browsertest.cc>
> > File content/browser/gpu/**compositor_util_browsertest.cc (right):
> >
> > https://codereview.chromium.**org/22198004/diff/21002/**
> >
>
content/browser/gpu/**compositor_util_browsertest.**cc#newcode31<https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compositor_util_browsertest.cc#newcode31>
> > content/browser/gpu/**compositor_util_browsertest.**cc:31: if
> > (base::win::GetVersion() >= base::win::VERSION_VISTA)
> > Actually this won't work because the blacklist is disabled on the bots,
> > right?
> >
> > Talking with wiltzius@ that seemed to be for a legacy reason since we
> > wanted to blacklist VM GPUs, but not our VMs' GPUs? Now that we have
> > real GPUs on bots (is that right?) we should be ok with using the
> > blacklist on the bots and then having this test to make sure that only
> > the bots we expect (XP bots) are actually blacklisted.
> >
> > Does that make sense?
> >
> >
>
https://codereview.chromium.**org/22198004/%3Chttps://codereview.chromium.org...>
> >

[gab, 2013-08-13 21:05:24]

https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
File content/browser/gpu/compositor_util_browsertest.cc (right):

https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
content/browser/gpu/compositor_util_browsertest.cc:31: if
(base::win::GetVersion() >= base::win::VERSION_VISTA)
On 2013/08/09 14:17:03, gab wrote:
> Actually this won't work because the blacklist is disabled on the bots, right?
> 
> Talking with wiltzius@ that seemed to be for a legacy reason since we wanted
to
> blacklist VM GPUs, but not our VMs' GPUs? Now that we have real GPUs on bots
(is
> that right?) we should be ok with using the blacklist on the bots and then
> having this test to make sure that only the bots we expect (XP bots) are
> actually blacklisted.
> 
> Does that make sense?

So, should we change the bots to use the blacklist? And have a test (written in
this CL) to make sure the bots use the config we expect them to?

If not, blacklisting XP via the blacklist will not be sufficient to prevent FCM
on the XP bots... and we will need to otherwise hardcode this hard restriction
in code...

[wiltzius, 2013-08-13 21:26:51]

+zmo

Mo, this is the patch that forms the beginning of what I just talked to you
about.

For now I want the XP bots to continue using compositing-on-demand but the Win7
bots to start using force compositing mode.

As soon as we throw the Aura I want us to start using FCM + the software
compositor on the XP bots and FCM + the hardware compositor on the Win7 bots.

We had intended to achieve this using the blacklist; where FCM is blacklisted
we'd go down to the software compositing path and when it isn't we'd go down the
hardware compositing path. As Gab says, we would add a test to make sure the
intended configuration is running on all bots.

Does that sound reasonable to you, or would you prefer to hardcode the
restriction for the test harnesses?

On 2013/08/13 21:05:24, gab wrote:
>
https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
> File content/browser/gpu/compositor_util_browsertest.cc (right):
> 
>
https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
> content/browser/gpu/compositor_util_browsertest.cc:31: if
> (base::win::GetVersion() >= base::win::VERSION_VISTA)
> On 2013/08/09 14:17:03, gab wrote:
> > Actually this won't work because the blacklist is disabled on the bots,
right?
> > 
> > Talking with wiltzius@ that seemed to be for a legacy reason since we wanted
> to
> > blacklist VM GPUs, but not our VMs' GPUs? Now that we have real GPUs on bots
> (is
> > that right?) we should be ok with using the blacklist on the bots and then
> > having this test to make sure that only the bots we expect (XP bots) are
> > actually blacklisted.
> > 
> > Does that make sense?
> 
> So, should we change the bots to use the blacklist? And have a test (written
in
> this CL) to make sure the bots use the config we expect them to?
> 
> If not, blacklisting XP via the blacklist will not be sufficient to prevent
FCM
> on the XP bots... and we will need to otherwise hardcode this hard restriction
> in code...

[Zhenyao Mo, 2013-08-13 21:35:07]

On 2013/08/13 21:26:51, wiltzius wrote:
> +zmo
> 
> Mo, this is the patch that forms the beginning of what I just talked to you
> about.
> 
> For now I want the XP bots to continue using compositing-on-demand but the
Win7
> bots to start using force compositing mode.
> 
> As soon as we throw the Aura I want us to start using FCM + the software
> compositor on the XP bots and FCM + the hardware compositor on the Win7 bots.
> 
> We had intended to achieve this using the blacklist; where FCM is blacklisted
> we'd go down to the software compositing path and when it isn't we'd go down
the
> hardware compositing path. As Gab says, we would add a test to make sure the
> intended configuration is running on all bots.
> 
> Does that sound reasonable to you, or would you prefer to hardcode the
> restriction for the test harnesses?

Please see
https://code.google.com/p/chromium/codesearch#search/&q=kSkipGpuDataLoading&s...

Basically blacklist will be skipped in most of the tests.  So even with this
patch, our bots very likely will still run with the wrong config, except a few
that's not part of the affected test harness, say, Telemetry.

I am fine with turning blacklist back on all bots if we have tests to make sure
the bots are running with intended config.  So a future blacklist entry won't
accidentally change the bot behavior implicitly.

> 
> On 2013/08/13 21:05:24, gab wrote:
> >
>
https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
> > File content/browser/gpu/compositor_util_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
> > content/browser/gpu/compositor_util_browsertest.cc:31: if
> > (base::win::GetVersion() >= base::win::VERSION_VISTA)
> > On 2013/08/09 14:17:03, gab wrote:
> > > Actually this won't work because the blacklist is disabled on the bots,
> right?
> > > 
> > > Talking with wiltzius@ that seemed to be for a legacy reason since we
wanted
> > to
> > > blacklist VM GPUs, but not our VMs' GPUs? Now that we have real GPUs on
bots
> > (is
> > > that right?) we should be ok with using the blacklist on the bots and then
> > > having this test to make sure that only the bots we expect (XP bots) are
> > > actually blacklisted.
> > > 
> > > Does that make sense?
> > 
> > So, should we change the bots to use the blacklist? And have a test (written
> in
> > this CL) to make sure the bots use the config we expect them to?
> > 
> > If not, blacklisting XP via the blacklist will not be sufficient to prevent
> FCM
> > on the XP bots... and we will need to otherwise hardcode this hard
restriction
> > in code...

[wiltzius, 2013-08-13 22:52:05]

OK that's great, sounds like we have a way forward then (drop that skip gpu
loading command line flag in the test harnesses, but add tests to ensure the
right configuration is on for the bots)?

On 2013/08/13 21:35:07, Zhenyao Mo wrote:
> On 2013/08/13 21:26:51, wiltzius wrote:
> > +zmo
> > 
> > Mo, this is the patch that forms the beginning of what I just talked to you
> > about.
> > 
> > For now I want the XP bots to continue using compositing-on-demand but the
> Win7
> > bots to start using force compositing mode.
> > 
> > As soon as we throw the Aura I want us to start using FCM + the software
> > compositor on the XP bots and FCM + the hardware compositor on the Win7
bots.
> > 
> > We had intended to achieve this using the blacklist; where FCM is
blacklisted
> > we'd go down to the software compositing path and when it isn't we'd go down
> the
> > hardware compositing path. As Gab says, we would add a test to make sure the
> > intended configuration is running on all bots.
> > 
> > Does that sound reasonable to you, or would you prefer to hardcode the
> > restriction for the test harnesses?
> 
> Please see
>
https://code.google.com/p/chromium/codesearch#search/&q=kSkipGpuDataLoading&s...
> 
> Basically blacklist will be skipped in most of the tests.  So even with this
> patch, our bots very likely will still run with the wrong config, except a few
> that's not part of the affected test harness, say, Telemetry.
> 
> I am fine with turning blacklist back on all bots if we have tests to make
sure
> the bots are running with intended config.  So a future blacklist entry won't
> accidentally change the bot behavior implicitly.
> 
> > 
> > On 2013/08/13 21:05:24, gab wrote:
> > >
> >
>
https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
> > > File content/browser/gpu/compositor_util_browsertest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/22198004/diff/21002/content/browser/gpu/compo...
> > > content/browser/gpu/compositor_util_browsertest.cc:31: if
> > > (base::win::GetVersion() >= base::win::VERSION_VISTA)
> > > On 2013/08/09 14:17:03, gab wrote:
> > > > Actually this won't work because the blacklist is disabled on the bots,
> > right?
> > > > 
> > > > Talking with wiltzius@ that seemed to be for a legacy reason since we
> wanted
> > > to
> > > > blacklist VM GPUs, but not our VMs' GPUs? Now that we have real GPUs on
> bots
> > > (is
> > > > that right?) we should be ok with using the blacklist on the bots and
then
> > > > having this test to make sure that only the bots we expect (XP bots) are
> > > > actually blacklisted.
> > > > 
> > > > Does that make sense?
> > > 
> > > So, should we change the bots to use the blacklist? And have a test
(written
> > in
> > > this CL) to make sure the bots use the config we expect them to?
> > > 
> > > If not, blacklisting XP via the blacklist will not be sufficient to
prevent
> > FCM
> > > on the XP bots... and we will need to otherwise hardcode this hard
> restriction
> > > in code...

[gab, 2013-08-15 20:07:48]

@zmo: Does this LGTY? I added a bunch of comments for you on patch set 5 (both
to guide you understand code changes and some questions about other things I'm
not sure about).

Cheers!
Gab

https://codereview.chromium.org/22198004/diff/61001/chrome/browser/gpu/chrome...
File chrome/browser/gpu/chrome_gpu_util.cc (left):

https://codereview.chromium.org/22198004/diff/61001/chrome/browser/gpu/chrome...
chrome/browser/gpu/chrome_gpu_util.cc:46: return false;
This didn't make sense as CHROME_OS always returns true in
IsThreadedCompositingEnabled() et al.

https://codereview.chromium.org/22198004/diff/61001/chrome/browser/gpu/chrome...
chrome/browser/gpu/chrome_gpu_util.cc:52: return false;
This is now applied by the blacklist on the bots, no need to special case for
tests.

https://codereview.chromium.org/22198004/diff/61001/chrome/browser/gpu/chrome...
chrome/browser/gpu/chrome_gpu_util.cc:65: return false;
This is now applied by the blacklist on the bots, no need to special case for
tests.

https://codereview.chromium.org/22198004/diff/61001/content/browser/gpu/compo...
File content/browser/gpu/compositor_util.cc (left):

https://codereview.chromium.org/22198004/diff/61001/content/browser/gpu/compo...
content/browser/gpu/compositor_util.cc:40: return
GpuDataManager::GetInstance()->IsFeatureBlacklisted(
This is already checked by CanDoAcceleratedCompositing().

https://codereview.chromium.org/22198004/diff/61001/content/browser/gpu/compo...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/22198004/diff/61001/content/browser/gpu/compo...
content/browser/gpu/compositor_util.cc:50: // Command line switches take
precedence over blacklist and field trials.
Group command-line switches check together and moved
CanDoAcceleratedCompositing() check below to respect this comment.

https://codereview.chromium.org/22198004/diff/61001/content/browser/gpu/compo...
File content/browser/gpu/compositor_util_browsertest.cc (right):

https://codereview.chromium.org/22198004/diff/61001/content/browser/gpu/compo...
content/browser/gpu/compositor_util_browsertest.cc:18:
IN_PROC_BROWSER_TEST_F(CompositorUtilTest, CompositingModeAsExpected) {
The only thing that scares me about this test is that it is not impossible for
this scenario to happen:

- Tests are sharded.
- The bot running this test is in the right config and passes.
- Another bot not running this particular test is in the wrong config and we
don't know (or get flaky test results).

The only way I see to fix this is to hook into BrowserTestBase to make sure this
is always correct before running any such tests.

Does this concern worry you or are we confident that a set of bots running
sharded tests have identical configs?

https://codereview.chromium.org/22198004/diff/61001/content/public/test/brows...
File content/public/test/browser_test_base.cc (right):

https://codereview.chromium.org/22198004/diff/61001/content/public/test/brows...
content/public/test/browser_test_base.cc:147: if (allow_osmesa_) {
Even with all of this we will still use the OSMesa GL context on Windows bots
(and the test compositor on WinAura; see line 112 above); is that desired?

https://codereview.chromium.org/22198004/diff/61001/content/shell/app/shell_m...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/22198004/diff/61001/content/shell/app/shell_m...
content/shell/app/shell_main_delegate.cc:128:
command_line.AppendSwitch(switches::kDisableGpuDriverBugWorkarounds);
This more or less replicates the old behavior; if desire is to absolutely not
load GPU info then I guess we can keep --skip-gpu-data-loading around... WDYT?

[Zhenyao Mo, 2013-08-15 20:23:30]

LGTM with these issues addressed

[Zhenyao Mo, 2013-08-15 20:23:37]

https://chromiumcodereview.appspot.com/22198004/diff/61001/content/browser/gp...
File content/browser/gpu/compositor_util.cc (right):

https://chromiumcodereview.appspot.com/22198004/diff/61001/content/browser/gp...
content/browser/gpu/compositor_util.cc:90: #endif
This should be below the CanDOAcceleratedCompositing(), because if it's
blacklisted, you don't want to turn it on.

https://chromiumcodereview.appspot.com/22198004/diff/61001/content/browser/gp...
content/browser/gpu/compositor_util.cc:103: #else
According to code style, this #else is unnecessary and should not be here.

https://chromiumcodereview.appspot.com/22198004/diff/61001/content/browser/gp...
File content/browser/gpu/compositor_util_browsertest.cc (right):

https://chromiumcodereview.appspot.com/22198004/diff/61001/content/browser/gp...
content/browser/gpu/compositor_util_browsertest.cc:18:
IN_PROC_BROWSER_TEST_F(CompositorUtilTest, CompositingModeAsExpected) {
On 2013/08/15 20:07:49, gab wrote:
> The only thing that scares me about this test is that it is not impossible for
> this scenario to happen:
> 
> - Tests are sharded.
> - The bot running this test is in the right config and passes.
> - Another bot not running this particular test is in the wrong config and we
> don't know (or get flaky test results).
> 
> The only way I see to fix this is to hook into BrowserTestBase to make sure
this
> is always correct before running any such tests.
> 
> Does this concern worry you or are we confident that a set of bots running
> sharded tests have identical configs?

I think it is OK.  I assume with sharding, we still will at least run this more
than once per day, which guards against regression.

https://chromiumcodereview.appspot.com/22198004/diff/71001/gpu/config/softwar...
File gpu/config/software_rendering_list_json.cc (right):

https://chromiumcodereview.appspot.com/22198004/diff/71001/gpu/config/softwar...
gpu/config/software_rendering_list_json.cc:1165: "id": 77,
It would be better if you have a bug associated with this, explaining why we
don't have fcm on older windows.

[gab, 2013-08-15 21:03:19]

https://codereview.chromium.org/22198004/diff/61001/content/browser/gpu/compo...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/22198004/diff/61001/content/browser/gpu/compo...
content/browser/gpu/compositor_util.cc:90: #endif
On 2013/08/15 20:23:37, Zhenyao Mo wrote:
> This should be below the CanDOAcceleratedCompositing(), because if it's
> blacklisted, you don't want to turn it on.

Done.

https://codereview.chromium.org/22198004/diff/61001/content/browser/gpu/compo...
content/browser/gpu/compositor_util.cc:103: #else
On 2013/08/15 20:23:37, Zhenyao Mo wrote:
> According to code style, this #else is unnecessary and should not be here.

Done.

https://codereview.chromium.org/22198004/diff/71001/gpu/config/software_rende...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/22198004/diff/71001/gpu/config/software_rende...
gpu/config/software_rendering_list_json.cc:1165: "id": 77,
On 2013/08/15 20:23:37, Zhenyao Mo wrote:
> It would be better if you have a bug associated with this, explaining why we
> don't have fcm on older windows.

Done.

[gab, 2013-08-15 21:45:55]

FYI, had to bring back --skip-gpu-data-loading, as added to the description:

"Keep --skip-gpu-data-loading around only for the purpose of using it when
initializing content_shell.exe; see http://crbug.com/190942 for details"

Cheers,
Gab

[commit-bot: I haz the power, 2013-08-15 21:46:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/22198004/100001

[Zhenyao Mo, 2013-08-15 21:53:41]

https://codereview.chromium.org/22198004/diff/71001/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.cc (left):

https://codereview.chromium.org/22198004/diff/71001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:565: {
Here you should check if kUseGL is "osmesa" (I think there is a const char* for
the name).  If no, call the CollectBasicGraphicsInfo.  If yes, construct a fake
GPUInfo, say, fill the vendor_id, device_id, etc, but make sure it doesn't
conflict with any real ones, so it will never be blacklisted.

[gab, 2013-08-16 16:37:07]

PTAL, I don't really like the hardcoded approach (see comment below).

https://codereview.chromium.org/22198004/diff/71001/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.cc (left):

https://codereview.chromium.org/22198004/diff/71001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:565: {
On 2013/08/15 21:53:42, Zhenyao Mo wrote:
> Here you should check if kUseGL is "osmesa" (I think there is a const char*
for
> the name).  If no, call the CollectBasicGraphicsInfo.  If yes, construct a
fake
> GPUInfo, say, fill the vendor_id, device_id, etc, but make sure it doesn't
> conflict with any real ones, so it will never be blacklisted.

Ok done, although I couldn't really find a good value for things such as version
strings...

Do we have a better way to grab gl_version, etc. for OSMesa than to hardcode
it...?

I don't like my current hardcoded solution in gpu_data_manager_impl_private.cc
at all...

[Zhenyao Mo, 2013-08-16 16:59:01]

https://codereview.chromium.org/22198004/diff/95002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/22198004/diff/95002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:566:
gpu_info.gpu.vendor_string = gfx::kGLImplementationOSMesaName;
Actually you don't have to fake these data.  The only data you need to supply so
the blacklist logic don't think the GPUInfo is empty is the gpu.vendor_id and
gpu.device_id.  You probably can set both to 0x9999, which I don't think will be
conflicting with any real vendor/device.

[gab, 2013-08-16 17:04:45]

https://codereview.chromium.org/22198004/diff/95002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/22198004/diff/95002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:566:
gpu_info.gpu.vendor_string = gfx::kGLImplementationOSMesaName;
On 2013/08/16 16:59:01, Zhenyao Mo wrote:
> Actually you don't have to fake these data.  

Ah ok, good to know, why is that? (it's not clear to me why that is so I'd like
to know so I can add a comment here for any future reader of this code).

> The only data you need to supply so
> the blacklist logic don't think the GPUInfo is empty is the gpu.vendor_id and
> gpu.device_id.  You probably can set both to 0x9999, which I don't think will
be
> conflicting with any real vendor/device.

[Zhenyao Mo, 2013-08-16 17:15:52]

On 2013/08/16 17:04:45, gab wrote:
>
https://codereview.chromium.org/22198004/diff/95002/content/browser/gpu/gpu_d...
> File content/browser/gpu/gpu_data_manager_impl_private.cc (right):
> 
>
https://codereview.chromium.org/22198004/diff/95002/content/browser/gpu/gpu_d...
> content/browser/gpu/gpu_data_manager_impl_private.cc:566:
> gpu_info.gpu.vendor_string = gfx::kGLImplementationOSMesaName;
> On 2013/08/16 16:59:01, Zhenyao Mo wrote:
> > Actually you don't have to fake these data.  
> 
> Ah ok, good to know, why is that? (it's not clear to me why that is so I'd
like
> to know so I can add a comment here for any future reader of this code).
> 
> > The only data you need to supply so
> > the blacklist logic don't think the GPUInfo is empty is the gpu.vendor_id
and
> > gpu.device_id.  You probably can set both to 0x9999, which I don't think
will
> be
> > conflicting with any real vendor/device.

Because we handle special cases when gpu info collection fails.  The way to
recognize it on some platforms is vendor_id==0 && device_id==0.  As far as these
two are filled, blacklist logic will be working, but most entries won't apply
because vendor_id/device_id won't match the fake ones, but the OS entries (like
FCM ones) will successfully apply.

[gab, 2013-08-16 19:42:15]

PTAL.

Thanks!
Gab

https://codereview.chromium.org/22198004/diff/95002/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/22198004/diff/95002/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:566:
gpu_info.gpu.vendor_string = gfx::kGLImplementationOSMesaName;
On 2013/08/16 16:59:01, Zhenyao Mo wrote:
> Actually you don't have to fake these data.  The only data you need to supply
so
> the blacklist logic don't think the GPUInfo is empty is the gpu.vendor_id and
> gpu.device_id.  You probably can set both to 0x9999, which I don't think will
be
> conflicting with any real vendor/device.

Done.

[Zhenyao Mo, 2013-08-16 21:45:49]

LGTM

[gab, 2013-08-21 20:59:06]

TBR jcivelli: chrome\test\base\test_launcher_utils.cc
TBR piman: content\content_tests.gypi
content\public\common\content_switches.cc
content\public\common\content_switches.h
content\public\test\browser_test_base.cc
content\shell\app\shell_main_delegate.cc

TBR'ed files are trivial side-effects of removing --skip-gpu-data-loading (and
one gyp change)


@zmo PTAL at patch set 16.

A cleaner approach imo would be to pass in a list of rules to be ignored when
constructing the GpuBlacklist in InitializeImpl, WDYT? If you think that's a
better approach I'm happy to do that as a follow-up CL, but would like to get
this in for now.

[gab, 2013-08-21 21:09:36]

On 2013/08/21 20:59:06, gab wrote:
> TBR jcivelli: chrome\test\base\test_launcher_utils.cc
> TBR piman: content\content_tests.gypi
> content\public\common\content_switches.cc
> content\public\common\content_switches.h
> content\public\test\browser_test_base.cc
> content\shell\app\shell_main_delegate.cc
> 
> TBR'ed files are trivial side-effects of removing --skip-gpu-data-loading (and
> one gyp change)

Re-sending with TBR'ed OWNERS actually added as reviewers.

Cheers,
Gab

> 
> 
> @zmo PTAL at patch set 16.
> 
> A cleaner approach imo would be to pass in a list of rules to be ignored when
> constructing the GpuBlacklist in InitializeImpl, WDYT? If you think that's a
> better approach I'm happy to do that as a follow-up CL, but would like to get
> this in for now.

[Zhenyao Mo, 2013-08-21 21:20:37]

LGTM again

[piman, 2013-08-21 21:23:33]

lgtm

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/22198004/164001

[commit-bot: I haz the power, 2013-08-22 01:12:07]

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

[gab, 2013-08-22 02:46:59]

Added OS_ANDROID to the list of expected OSes running in threaded compositing
mode.

Re-introduced --skip-gpu-data-loading on Linux/Mac temporarily because of
failures on Mac/Linux bots and the main motivation for this CL is to get FCM
running on Windows; we can investigate issues with Mac/Linux separately (opened
http://crbug.com/277242 to track this).

Cheers,
Gab

[commit-bot: I haz the power, 2013-08-22 02:49:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/22198004/178001

[commit-bot: I haz the power, 2013-08-22 04:29:54]

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

[commit-bot: I haz the power, 2013-08-22 15:28:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/22198004/211001

[commit-bot: I haz the power, 2013-08-22 22:26:41]

Change committed as 219132

[ccameron, 2013-08-28 20:04:03]

On 2013/08/22 22:26:41, I haz the power (commit-bot) wrote:
> Change committed as 219132

FYI, this was reverted due to GpuTabTest.testScreenshot  -- that test failed on
Mac enabling FCM on 10.6 as well. It has since been disabled as a flake.

[gab, 2013-08-28 20:09:06]

On 2013/08/28 20:04:03, ccameron1 wrote:
> On 2013/08/22 22:26:41, I haz the power (commit-bot) wrote:
> > Change committed as 219132
> 
> FYI, this was reverted due to GpuTabTest.testScreenshot  -- that test failed
on
> Mac enabling FCM on 10.6 as well. It has since been disabled as a flake.

Thanks, there were actually other issues as well, another CL is in progress to
re-land this @ https://codereview.chromium.org/23534006/