Implement blacklist's force GPU capability in dual GPU machines.

Implement blacklist's force GPU capability in dual GPU machines.

This is part 2.  In part one we implement the semantics in blacklist.  Now we hook it up with the "real" forcing code.

For now we only hook it up with Mac CGL port.

BUG=140114, 131276, 111720
TEST=tree
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=158076

[Zhenyao Mo, 2012-09-13 23:29:10]

kbr: please review.

[Zhenyao Mo, 2012-09-13 23:33:26]

csilv: webui owner
piman: content owner

[csilv, 2012-09-13 23:37:14]

/webui/ lgtm

[piman, 2012-09-13 23:51:43]

LGTM for content, but adding jam for content/public (new files etc.)

[piman, 2012-09-14 00:21:48]

+jam for content/public, for real this time

[piman, 2012-09-14 00:22:17]

3rd time?

[Ken Russell (switch to Gerrit), 2012-09-15 00:48:04]

This is pleasingly simple. LGTM if this has been tested on dual-GPU MacBook Pros
and the force-integrated, force-discrete, and automatic switching modes have
been verified (the latter with WebGL content, to make sure Chrome still
dynamically switches GPUs properly).

+jbauman for any additional comment.

[jam, 2012-09-17 17:08:55]

http://codereview.chromium.org/10909221/diff/11014/content/public/common/gpu_...
File content/public/common/gpu_switching_option.h (right):

http://codereview.chromium.org/10909221/diff/11014/content/public/common/gpu_...
content/public/common/gpu_switching_option.h:22: CONTENT_EXPORT extern const
char kGpuSwitchingOptionNameAutomatic[];
why are these here? these belong in content\public\common\content_switches.h
with the rest of the command line flags or options

[Zhenyao Mo, 2012-09-17 17:13:42]

http://codereview.chromium.org/10909221/diff/11014/content/public/common/gpu_...
File content/public/common/gpu_switching_option.h (right):

http://codereview.chromium.org/10909221/diff/11014/content/public/common/gpu_...
content/public/common/gpu_switching_option.h:22: CONTENT_EXPORT extern const
char kGpuSwitchingOptionNameAutomatic[];
On 2012/09/17 17:08:55, John Abd-El-Malek wrote:
> why are these here? these belong in content\public\common\content_switches.h
> with the rest of the commandline flags or options

These are not just for commandline flags, they are also for gpu_blacklist and
about:gpu pages.  So putting them in a common place (here) seems like better
than putting them in the commandline options.

[jam, 2012-09-17 17:16:08]

http://codereview.chromium.org/10909221/diff/11014/content/public/common/gpu_...
File content/public/common/gpu_switching_option.h (right):

http://codereview.chromium.org/10909221/diff/11014/content/public/common/gpu_...
content/public/common/gpu_switching_option.h:22: CONTENT_EXPORT extern const
char kGpuSwitchingOptionNameAutomatic[];
On 2012/09/17 17:13:43, Zhenyao Mo wrote:
> On 2012/09/17 17:08:55, John Abd-El-Malek wrote:
> > why are these here? these belong in content\public\common\content_switches.h
> > with the rest of the commandline flags or options
> 
> These are not just for commandline flags, they are also for gpu_blacklist and
> about:gpu pages.  So putting them in a common place (here) seems like better
> than putting them in the commandline options.

right, I realize that. these are anagalous to kRendererProcess/kPluginProcess
etc, however as they are command line arguments, they're still in
content_switches.h

[Zhenyao Mo, 2012-09-17 17:28:58]

http://codereview.chromium.org/10909221/diff/11014/content/public/common/gpu_...
File content/public/common/gpu_switching_option.h (right):

http://codereview.chromium.org/10909221/diff/11014/content/public/common/gpu_...
content/public/common/gpu_switching_option.h:22: CONTENT_EXPORT extern const
char kGpuSwitchingOptionNameAutomatic[];
On 2012/09/17 17:16:08, John Abd-El-Malek wrote:
> On 2012/09/17 17:13:43, Zhenyao Mo wrote:
> > On 2012/09/17 17:08:55, John Abd-El-Malek wrote:
> > > why are these here? these belong in
content\public\common\content_switches.h
> > > with the rest of the commandline flags or options
> > 
> > These are not just for commandline flags, they are also for gpu_blacklist
and
> > about:gpu pages.  So putting them in a common place (here) seems like better
> > than putting them in the commandline options.
> 
> right, I realize that. these are anagalous to kRendererProcess/kPluginProcess
> etc, however as they are command line arguments, they're still in
> content_switches.h

The commandline switch options only support force_discrete and force_integrated.
 We don't want to support automatic option because in older macbooks, automatic
switching cause disasters, whereas using only discrete GPU or integrated GPU
works fine.  So we hardwire to use discrete GPU on those machines, and we don't
want to provide an option to overwrite the force_discrete and starts automatic
switching.

So instead of sharing the strings here, I'll create two option strings in the
content_switches.h.

[Zhenyao Mo, 2012-09-17 18:35:39]

jam: revised, please have another look.

[jam, 2012-09-17 19:29:06]

On 2012/09/17 18:35:39, Zhenyao Mo wrote:
> jam: revised, please have another look.

content/public/common/gpu_switching_option.h still has them?

[Zhenyao Mo, 2012-09-17 20:11:48]

On 2012/09/17 19:29:06, John Abd-El-Malek wrote:
> On 2012/09/17 18:35:39, Zhenyao Mo wrote:
> > jam: revised, please have another look.
> 
> content/public/common/gpu_switching_option.h still has them?

Yes, as I explained, the commandline options do not include "automatic", but the
blacklist and about:gpu usage requires that.  So we treat the commandline
options and the gpu_switching_option names two different sets.

[jam, 2012-09-17 20:37:56]

On 2012/09/17 20:11:48, Zhenyao Mo wrote:
> On 2012/09/17 19:29:06, John Abd-El-Malek wrote:
> > On 2012/09/17 18:35:39, Zhenyao Mo wrote:
> > > jam: revised, please have another look.
> > 
> > content/public/common/gpu_switching_option.h still has them?
> 
> Yes, as I explained, the commandline options do not include "automatic", but
the
> blacklist and about:gpu usage requires that.  So we treat the commandline
> options and the gpu_switching_option names two different sets.

duplication isn't good.

since these are command line flags, except for the automatic one, put them in
conten_switches.h. even the one that's not used, put it there to be alongside
the other command line flags (with a comment, if you wish, that's it's not used
from the command line).

[Zhenyao Mo, 2012-09-17 21:19:32]

On 2012/09/17 20:37:56, John Abd-El-Malek wrote:
> On 2012/09/17 20:11:48, Zhenyao Mo wrote:
> > On 2012/09/17 19:29:06, John Abd-El-Malek wrote:
> > > On 2012/09/17 18:35:39, Zhenyao Mo wrote:
> > > > jam: revised, please have another look.
> > > 
> > > content/public/common/gpu_switching_option.h still has them?
> > 
> > Yes, as I explained, the commandline options do not include "automatic", but
> the
> > blacklist and about:gpu usage requires that.  So we treat the commandline
> > options and the gpu_switching_option names two different sets.
> 
> duplication isn't good.
> 
> since these are command line flags, except for the automatic one, put them in
> conten_switches.h. even the one that's not used, put it there to be alongside
> the other command line flags (with a comment, if you wish, that's it's not
used
> from the command line).

Revised.  Please have another look.

Now GPU blacklist and other code all access the strings defined in
ui/gl/gl_switches.h.  I moved the strings out of the switches namespace.  It
looks disturbing that GPU blacklist code access a string starts with switches::
whereas the semantics in the GPU blacklist code has nothing to do with switches.

[jam, 2012-09-18 19:42:58]

On 2012/09/17 21:19:32, Zhenyao Mo wrote:
> On 2012/09/17 20:37:56, John Abd-El-Malek wrote:
> > On 2012/09/17 20:11:48, Zhenyao Mo wrote:
> > > On 2012/09/17 19:29:06, John Abd-El-Malek wrote:
> > > > On 2012/09/17 18:35:39, Zhenyao Mo wrote:
> > > > > jam: revised, please have another look.
> > > > 
> > > > content/public/common/gpu_switching_option.h still has them?
> > > 
> > > Yes, as I explained, the commandline options do not include "automatic",
but
> > the
> > > blacklist and about:gpu usage requires that.  So we treat the commandline
> > > options and the gpu_switching_option names two different sets.
> > 
> > duplication isn't good.
> > 
> > since these are command line flags, except for the automatic one, put them
in
> > conten_switches.h. even the one that's not used, put it there to be
alongside
> > the other command line flags (with a comment, if you wish, that's it's not
> used
> > from the command line).
> 
> Revised.  Please have another look.
> 
> Now GPU blacklist and other code all access the strings defined in
> ui/gl/gl_switches.h.  I moved the strings out of the switches namespace.  It
> looks disturbing that GPU blacklist code access a string starts with
switches::
> whereas the semantics in the GPU blacklist code has nothing to do with
switches.

it's more consistent with how we use switches to put the two that are used as
switches in the switches namespace. I think it's better to move all three there,
and the comment that you have now is sufficient.

[Zhenyao Mo, 2012-09-18 19:53:08]

On 2012/09/18 19:42:58, John Abd-El-Malek wrote:
> On 2012/09/17 21:19:32, Zhenyao Mo wrote:
> > On 2012/09/17 20:37:56, John Abd-El-Malek wrote:
> > > On 2012/09/17 20:11:48, Zhenyao Mo wrote:
> > > > On 2012/09/17 19:29:06, John Abd-El-Malek wrote:
> > > > > On 2012/09/17 18:35:39, Zhenyao Mo wrote:
> > > > > > jam: revised, please have another look.
> > > > > 
> > > > > content/public/common/gpu_switching_option.h still has them?
> > > > 
> > > > Yes, as I explained, the commandline options do not include "automatic",
> but
> > > the
> > > > blacklist and about:gpu usage requires that.  So we treat the
commandline
> > > > options and the gpu_switching_option names two different sets.
> > > 
> > > duplication isn't good.
> > > 
> > > since these are command line flags, except for the automatic one, put them
> in
> > > conten_switches.h. even the one that's not used, put it there to be
> alongside
> > > the other command line flags (with a comment, if you wish, that's it's not
> > used
> > > from the command line).
> > 
> > Revised.  Please have another look.
> > 
> > Now GPU blacklist and other code all access the strings defined in
> > ui/gl/gl_switches.h.  I moved the strings out of the switches namespace.  It
> > looks disturbing that GPU blacklist code access a string starts with
> switches::
> > whereas the semantics in the GPU blacklist code has nothing to do with
> switches.
> 
> it's more consistent with how we use switches to put the two that are used as
> switches in the switches namespace. I think it's better to move all three
there,
> and the comment that you have now is sufficient.

I agree the switches options are better to stay inside the switches namespace. 
I just feel uncomfortable about the blacklist and other code referencing a
switches::my_string whereas the semantics has nothing to do with commandline
switches.  That's why originally I implemented two set of strings.  Even though
the content of strings are the same, their semantics are different.

Look at the gl_switches.h, the use-gl switch options are also not in the
switches namespace.  They are in the gfx namespace instead.  I think the
reasoning is the same: these strings are referenced not only by switches, but
some other code.

If putting the gpu-switching options in the global namespace makes you
uncomfortable, I can wrap them inside a namespace, say, gpu.  but for the above
reasons and the use-gl example, I think putting them inside switches is awkward.

[Zhenyao Mo, 2012-09-19 18:11:31]

jam: ping

> 
> I agree the switches options are better to stay inside the switches namespace.

> I just feel uncomfortable about the blacklist and other code referencing a
> switches::my_string whereas the semantics has nothing to do with commandline
> switches.  That's why originally I implemented two set of strings.  Even
though
> the content of strings are the same, their semantics are different.
> 
> Look at the gl_switches.h, the use-gl switch options are also not in the
> switches namespace.  They are in the gfx namespace instead.  I think the
> reasoning is the same: these strings are referenced not only by switches, but
> some other code.
> 
> If putting the gpu-switching options in the global namespace makes you
> uncomfortable, I can wrap them inside a namespace, say, gpu.  but for the
above
> reasons and the use-gl example, I think putting them inside switches is
awkward.

[jam, 2012-09-19 19:55:42]

On 2012/09/19 18:11:31, Zhenyao Mo wrote:
> jam: ping
> 
> > 
> > I agree the switches options are better to stay inside the switches
namespace.
> 
> > I just feel uncomfortable about the blacklist and other code referencing a
> > switches::my_string whereas the semantics has nothing to do with commandline
> > switches.  That's why originally I implemented two set of strings.  Even
> though
> > the content of strings are the same, their semantics are different.
> > 
> > Look at the gl_switches.h, the use-gl switch options are also not in the
> > switches namespace.  They are in the gfx namespace instead.  I think the
> > reasoning is the same: these strings are referenced not only by switches,
but
> > some other code.
> > 
> > If putting the gpu-switching options in the global namespace makes you
> > uncomfortable, I can wrap them inside a namespace, say, gpu.  but for the
> above
> > reasons and the use-gl example, I think putting them inside switches is
> awkward.

I'm not familiar with use-gl switch options, but if they're not like the
kRendererProcess ones, then they should be like that. 

I'm still not convinced that these switch options should be outside of the
switches namespace. Again, consistency is best, and we should do the same as
what we did for --type, even if one of them isn't used.

[Zhenyao Mo, 2012-09-20 19:25:16]

On 2012/09/19 19:55:42, John Abd-El-Malek wrote:
> On 2012/09/19 18:11:31, Zhenyao Mo wrote:
> > jam: ping
> > 
> > > 
> > > I agree the switches options are better to stay inside the switches
> namespace.
> > 
> > > I just feel uncomfortable about the blacklist and other code referencing a
> > > switches::my_string whereas the semantics has nothing to do with
commandline
> > > switches.  That's why originally I implemented two set of strings.  Even
> > though
> > > the content of strings are the same, their semantics are different.
> > > 
> > > Look at the gl_switches.h, the use-gl switch options are also not in the
> > > switches namespace.  They are in the gfx namespace instead.  I think the
> > > reasoning is the same: these strings are referenced not only by switches,
> but
> > > some other code.
> > > 
> > > If putting the gpu-switching options in the global namespace makes you
> > > uncomfortable, I can wrap them inside a namespace, say, gpu.  but for the
> > above
> > > reasons and the use-gl example, I think putting them inside switches is
> > awkward.
> 
> I'm not familiar with use-gl switch options, but if they're not like the
> kRendererProcess ones, then they should be like that. 
> 
> I'm still not convinced that these switch options should be outside of the
> switches namespace. Again, consistency is best, and we should do the same as
> what we did for --type, even if one of them isn't used.

Revised, please have a look.

[Zhenyao Mo, 2012-09-20 22:18:16]

jam: also this one.

[jam, 2012-09-21 20:42:04]

lgtm

http://codereview.chromium.org/10909221/diff/19001/ui/gl/gl_switches.cc
File ui/gl/gl_switches.cc (right):

http://codereview.chromium.org/10909221/diff/19001/ui/gl/gl_switches.cc#newco...
ui/gl/gl_switches.cc:64: 
nit: remove