Add a command-line switch --skip-gpu-data-loading.

Add a command-line switch --skip-gpu-data-loading.

With this switch,  GPU info collection, GPU blacklist loading, and GPU blacklist auto-update scheduling are skipped at browser startup time.

This CL also hooks this switch with UI tests and browser tests.

BUG=101220
TEST=no behavior change in existing browser tests and ui tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107459

[Zhenyao Mo, 2011-10-24 17:59:06]

thestig: can you review the part I add the switch to UI test and browser test?

vangelis: please review the rest.

[Lei Zhang, 2011-10-24 19:45:11]

LGTM on the testing side, but maybe Pawel will have additional input.

[Zhenyao Mo, 2011-10-24 19:48:15]

Pawel: please have a look.

[Ken Russell (switch to Gerrit), 2011-10-24 21:04:41]

http://codereview.chromium.org/8383008/diff/2002/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.cc (right):

http://codereview.chromium.org/8383008/diff/2002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:692: return NULL;
Does this test handle the case where --skip-gpu-data-loading is passed? Or
should the above test of ignore-gpu-blacklist be revised?

http://codereview.chromium.org/8383008/diff/2002/content/public/common/conten...
File content/public/common/content_switches.cc (right):

http://codereview.chromium.org/8383008/diff/2002/content/public/common/conten...
content/public/common/content_switches.cc:446: // scheduling at browser startup
time.
Could you describe the side-effects of this? In particular, does it allow or
disallow the use of all GPU features?

If this flag is a superset of --ignore-gpu-blacklist, would it make sense to
just add the new behaviors under the existing flag? Interactions between the two
flags could be confusing.

[Zhenyao Mo, 2011-10-24 21:42:18]

http://codereview.chromium.org/8383008/diff/2002/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.cc (right):

http://codereview.chromium.org/8383008/diff/2002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:692: return NULL;
On 2011/10/24 21:04:41, kbr wrote:
> Does this test handle the case where --skip-gpu-data-loading is passed? Or
> should the above test of ignore-gpu-blacklist be revised?

if --skip-gpu-data-loading is passed, gpu_blacklist_.get() will return NULL.

http://codereview.chromium.org/8383008/diff/2002/content/public/common/conten...
File content/public/common/content_switches.cc (right):

http://codereview.chromium.org/8383008/diff/2002/content/public/common/conten...
content/public/common/content_switches.cc:446: // scheduling at browser startup
time.
On 2011/10/24 21:04:41, kbr wrote:
> Could you describe the side-effects of this? In particular, does it allow or
> disallow the use of all GPU features?

Will do.

> 
> If this flag is a superset of --ignore-gpu-blacklist, would it make sense to
> just add the new behaviors under the existing flag? Interactions between the
two flags could be confusing.

Good point.  This is kinda tricky.  So here is what I think of:

--ignore-gpu-blacklist is more for the end users.  So for users who opt in this
flag, they could still see the about:gpu content.

--skip-gpu-data-loading will show empty content on about:gpu page, and it is
only for our tests.

Since --ignore-gpu-blacklist is already kinda well-known, I don't think we
should remove it.  So we could either

1) use two separate switches with overlapping effects

or

2) modify the --ignore-gpu-blacklist to do what --skip-gpu-data-loading does in
this CL (only that the name and the behavior will not exactly match).

What do you think?

[Ken Russell (switch to Gerrit), 2011-10-24 21:51:40]

On 2011/10/24 21:42:18, Zhenyao Mo wrote:
>
http://codereview.chromium.org/8383008/diff/2002/content/browser/gpu/gpu_data...
> File content/browser/gpu/gpu_data_manager.cc (right):
> 
>
http://codereview.chromium.org/8383008/diff/2002/content/browser/gpu/gpu_data...
> content/browser/gpu/gpu_data_manager.cc:692: return NULL;
> On 2011/10/24 21:04:41, kbr wrote:
> > Does this test handle the case where --skip-gpu-data-loading is passed? Or
> > should the above test of ignore-gpu-blacklist be revised?
> 
> if --skip-gpu-data-loading is passed, gpu_blacklist_.get() will return NULL.
> 
>
http://codereview.chromium.org/8383008/diff/2002/content/public/common/conten...
> File content/public/common/content_switches.cc (right):
> 
>
http://codereview.chromium.org/8383008/diff/2002/content/public/common/conten...
> content/public/common/content_switches.cc:446: // scheduling at browser
startup
> time.
> On 2011/10/24 21:04:41, kbr wrote:
> > Could you describe the side-effects of this? In particular, does it allow or
> > disallow the use of all GPU features?
> 
> Will do.
> 
> > 
> > If this flag is a superset of --ignore-gpu-blacklist, would it make sense to
> > just add the new behaviors under the existing flag? Interactions between the
> two flags could be confusing.
> 
> Good point.  This is kinda tricky.  So here is what I think of:
> 
> --ignore-gpu-blacklist is more for the end users.  So for users who opt in
this
> flag, they could still see the about:gpu content.
> 
> --skip-gpu-data-loading will show empty content on about:gpu page, and it is
> only for our tests.
> 
> Since --ignore-gpu-blacklist is already kinda well-known, I don't think we
> should remove it.  So we could either
> 
> 1) use two separate switches with overlapping effects
> 
> or
> 
> 2) modify the --ignore-gpu-blacklist to do what --skip-gpu-data-loading does
in
> this CL (only that the name and the behavior will not exactly match).
> 
> What do you think?

Ah, I didn't realize that about:gpu would effectively be disabled with
--skip-gpu-data-loading. Leaving it as a separate switch sounds better in that
case. It's worth documenting this side effect in content_switches.cc, and
indicating it's really intended only for unit tests.

[Paweł Hajdan Jr., 2011-10-25 08:46:40]

Thank you for adding me to review (see also chrome/test/OWNERS, you should get
an approval anyway).

http://codereview.chromium.org/8383008/diff/5015/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/8383008/diff/5015/chrome/test/ui/ui_test.cc#ne...
chrome/test/ui/ui_test.cc:206:
launch_arguments_.AppendSwitch(switches::kSkipGpuDataLoading);
With the change in test_launcher_utils this shouldn't be needed. If it is, it's
a bug, and you should make sure we call test_launcher_utils at the right place.

It's just not maintainable to duplicate this code.

[Zhenyao Mo, 2011-10-25 15:51:12]

http://codereview.chromium.org/8383008/diff/5015/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/8383008/diff/5015/chrome/test/ui/ui_test.cc#ne...
chrome/test/ui/ui_test.cc:206:
launch_arguments_.AppendSwitch(switches::kSkipGpuDataLoading);
On 2011/10/25 08:46:40, Paweł Hajdan Jr. wrote:
> With the change in test_launcher_utils this shouldn't be needed. If it is,
it's
> a bug, and you should make sure we call test_launcher_utils at the right
place.
> 
> It's just not maintainable to duplicate this code.

I double-checked again, and
test_launcher_utils::PrepareBrowserCommandLineForTests are not called directly
or indirectly by ui_test at all.

If it is a bug, I need points where in the ui_test we should call the
test_launcher_utils::PrepareBrowserCommandLineForTests.  Apparently the current
ui tests run fine without calling it, that's the part I don't understand.

[Paweł Hajdan Jr., 2011-10-25 16:04:35]

http://codereview.chromium.org/8383008/diff/5015/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/8383008/diff/5015/chrome/test/ui/ui_test.cc#ne...
chrome/test/ui/ui_test.cc:206:
launch_arguments_.AppendSwitch(switches::kSkipGpuDataLoading);
On 2011/10/25 15:51:12, Zhenyao Mo wrote:
> On 2011/10/25 08:46:40, Paweł Hajdan Jr. wrote:
> > With the change in test_launcher_utils this shouldn't be needed. If it is,
> it's
> > a bug, and you should make sure we call test_launcher_utils at the right
> place.
> > 
> > It's just not maintainable to duplicate this code.
> 
> I double-checked again, and
> test_launcher_utils::PrepareBrowserCommandLineForTests are not called directly
> or indirectly by ui_test at all.
> 
> If it is a bug, I need points where in the ui_test we should call the
> test_launcher_utils::PrepareBrowserCommandLineForTests.  Apparently the
current
> ui tests run fine without calling it, that's the part I don't understand.

I think it's called by ProxyLauncher:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/test/automatio...

[Zhenyao Mo, 2011-10-25 17:48:13]

On 2011/10/25 16:04:35, Paweł Hajdan Jr. wrote:
> http://codereview.chromium.org/8383008/diff/5015/chrome/test/ui/ui_test.cc
> File chrome/test/ui/ui_test.cc (right):
> 
>
http://codereview.chromium.org/8383008/diff/5015/chrome/test/ui/ui_test.cc#ne...
> chrome/test/ui/ui_test.cc:206:
> launch_arguments_.AppendSwitch(switches::kSkipGpuDataLoading);
> On 2011/10/25 15:51:12, Zhenyao Mo wrote:
> > On 2011/10/25 08:46:40, Paweł Hajdan Jr. wrote:
> > > With the change in test_launcher_utils this shouldn't be needed. If it is,
> > it's
> > > a bug, and you should make sure we call test_launcher_utils at the right
> > place.
> > > 
> > > It's just not maintainable to duplicate this code.
> > 
> > I double-checked again, and
> > test_launcher_utils::PrepareBrowserCommandLineForTests are not called
directly
> > or indirectly by ui_test at all.
> > 
> > If it is a bug, I need points where in the ui_test we should call the
> > test_launcher_utils::PrepareBrowserCommandLineForTests.  Apparently the
> current
> > ui tests run fine without calling it, that's the part I don't understand.
> 
> I think it's called by ProxyLauncher:
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/test/automatio...

Thanks.  Removed the change in ui_test.cc.  Please review again.

[Paweł Hajdan Jr., 2011-10-25 18:50:21]

LGTM

[Zhenyao Mo, 2011-10-26 16:22:53]

Ken or Vangelis: could one of you give a final review on this CL?  I have a
bunch of tests depending on this CL.

[Ken Russell (switch to Gerrit), 2011-10-26 18:01:24]

LGTM. One minor comment issue.

http://codereview.chromium.org/8383008/diff/9003/content/public/common/conten...
File content/public/common/content_switches.cc (right):

http://codereview.chromium.org/8383008/diff/9003/content/public/common/conten...
content/public/common/content_switches.cc:447: // Therefore, all GPU features
are avalable, and about:gpu page shows empty
typo: available

[commit-bot: I haz the power, 2011-10-26 18:33:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zmo@chromium.org/8383008/16001

[commit-bot: I haz the power, 2011-10-26 18:33:52]

Presubmit check for 8383008-16001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/public/common/content_switches.cc,content/public/common/content_switches.h

Presubmit checks took 1.7s to calculate.

[Zhenyao Mo, 2011-10-26 19:50:59]

jam: could you please owner-stamp this CL?

[jam, 2011-10-26 21:54:25]

lgtm

[commit-bot: I haz the power, 2011-10-26 21:55:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zmo@chromium.org/8383008/16001

[commit-bot: I haz the power, 2011-10-26 23:04:05]

Change committed as 107459