Connect up --disable-gl-multisampling to command buffer

Connect up --disable-gl-multisampling to command buffer

Plumb the --disable-gl-multisampling flag in to the command buffer, so it won't report the extension to any consumers.

BUG=75181
TEST=webgl antialias test

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

[zmo, 2011-03-07 22:25:32]

Add Gregg to the reviewer list as he is the master of command buffer.

[apatrick_chromium, 2011-03-07 22:31:39]

Drive by review :)

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/cont...
File gpu/command_buffer/service/context_group.h (right):

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/cont...
gpu/command_buffer/service/context_group.h:30: typedef struct
DisallowedExtensions DisallowedExtensions;
Can this just be

struct DisallowedExtensions;

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/gles...
File gpu/command_buffer/service/gles2_cmd_decoder.h (right):

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder.h:28: typedef struct
DisallowedExtensions {
And here

struct DisallowedExtensions {
  bool multisampling;
};

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/gpu_...
File gpu/command_buffer/service/gpu_processor.cc (right):

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/gpu_...
gpu/command_buffer/service/gpu_processor.cc:57: const
gles2::DisallowedExtensions& disallowed,
80 char limit

[zmo, 2011-03-07 23:34:29]

Thanks for fixing this.

http://codereview.chromium.org/6623063/diff/1/chrome/gpu/gpu_channel.cc
File chrome/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/6623063/diff/1/chrome/gpu/gpu_channel.cc#newco...
chrome/gpu/gpu_channel.cc:35: disallowed_extensions_.multisampling =
command_line->HasSwitch(switches::kDisableGLMultisampling);
here also: 80 per line.

http://codereview.chromium.org/6623063/diff/1/content/browser/gpu_process_hos...
File content/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/6623063/diff/1/content/browser/gpu_process_hos...
content/browser/gpu_process_host.cc:301: };
After copy switches from browser commandline, you need to append
kDisableGLMultisampling switch even it's not passed into browser, if
GpuDataManager::GpuFeatureAllowed(GpuFeatureFlags::kGpuFeatureMultisampling) is
false.

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/cont...
File gpu/command_buffer/service/context_group.h (right):

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/cont...
gpu/command_buffer/service/context_group.h:44: 
@gman: will there ever a conflict between disallowed and allowed?

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/gles...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder.cc:673: const DisallowedExtensions&
disalloowed,
an extra o, also, it would be better to use disallowed_extensions instead of
disallowed.  Here and a few other places.

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/gles...
File gpu/command_buffer/service/gles2_cmd_decoder.h (right):

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/gles...
gpu/command_buffer/service/gles2_cmd_decoder.h:68: const DisallowedExtensions
&disallowed,
should be const DisallowedExtensions& disallowed.

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/gpu_...
File gpu/command_buffer/service/gpu_processor_win.cc (right):

http://codereview.chromium.org/6623063/diff/1/gpu/command_buffer/service/gpu_...
gpu/command_buffer/service/gpu_processor_win.cc:48: disallowed
missing "," here.  You should use the trybots.  They definitely will catch such
typo.

[jbauman, 2011-03-08 19:59:18]

http://codereview.chromium.org/6623063/diff/1/content/browser/gpu_process_hos...
File content/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/6623063/diff/1/content/browser/gpu_process_hos...
content/browser/gpu_process_host.cc:301: };
On 2011/03/07 23:34:29, zmo wrote:
> After copy switches from browser commandline, you need to append
> kDisableGLMultisampling switch even it's not passed into browser, if
> GpuDataManager::GpuFeatureAllowed(GpuFeatureFlags::kGpuFeatureMultisampling)
is
> false.
Good point, although that's rather tricky because we're on the wrong thread to
access that info. I'll see what I can do.

[zmo, 2011-03-08 21:34:27]

On 2011/03/08 19:59:18, jbauman wrote:
>
http://codereview.chromium.org/6623063/diff/1/content/browser/gpu_process_hos...
> File content/browser/gpu_process_host.cc (right):
> 
>
http://codereview.chromium.org/6623063/diff/1/content/browser/gpu_process_hos...
> content/browser/gpu_process_host.cc:301: };
> On 2011/03/07 23:34:29, zmo wrote:
> > After copy switches from browser commandline, you need to append
> > kDisableGLMultisampling switch even it's not passed into browser, if
> > GpuDataManager::GpuFeatureAllowed(GpuFeatureFlags::kGpuFeatureMultisampling)
> is
> > false.
> Good point, although that's rather tricky because we're on the wrong thread to
> access that info. I'll see what I can do.

I see.  If there is no easy way around it, maybe we could remove the thread
checking and update in GpuFeatureAllowed, i.e., makes it a const function.  I
don't see much harm there.

If you do so, please add a comment in GpuFeatureAllowed.

[greggman, 2011-03-08 21:53:58]

I don't understand why this CL is so complicated.

It seems to me all you need to do is copy the commandline switch in
gpu_process_host.cc and look at the switch in gles2_cmd_decoder.cc. A 2 file
change.

Is there a reason it needs to be more complicated than that?

[jbauman, 2011-03-08 22:23:01]

On 2011/03/08 21:53:58, greggman wrote:
> I don't understand why this CL is so complicated.
> 
> It seems to me all you need to do is copy the commandline switch in
> gpu_process_host.cc and look at the switch in gles2_cmd_decoder.cc. A 2 file
> change.
> 
> Is there a reason it needs to be more complicated than that?

Causing the the command buffer implementation to depend on chrome's command line
processing caused the demos and unit tests to require that the command line
parser be set up and switches linked in. Either way requires changing a bunch of
files, so I figured that we might as well go the way that allows the most
separation of concerns. I can do it the other way, if you want.

[greggman, 2011-03-10 19:36:51]

On 2011/03/08 22:23:01, jbauman wrote:
> On 2011/03/08 21:53:58, greggman wrote:
> > I don't understand why this CL is so complicated.
> > 
> > It seems to me all you need to do is copy the commandline switch in
> > gpu_process_host.cc and look at the switch in gles2_cmd_decoder.cc. A 2 file
> > change.
> > 
> > Is there a reason it needs to be more complicated than that?
> 
> Causing the the command buffer implementation to depend on chrome's command
line
> processing caused the demos and unit tests to require that the command line
> parser be set up and switches linked in. Either way requires changing a bunch
of
> files, so I figured that we might as well go the way that allows the most
> separation of concerns. I can do it the other way, if you want.

got it.
LGTM

[Zhenyao Mo, 2011-03-10 23:52:49]

http://codereview.chromium.org/6623063/diff/10001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/6623063/diff/10001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host_ui_shim.cc:129:
GpuFeatureFlags::kGpuFeatureMultisampling);
style: 4 spaces instead of 8.

http://codereview.chromium.org/6623063/diff/10001/chrome/gpu/gpu_channel.cc
File chrome/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/6623063/diff/10001/chrome/gpu/gpu_channel.cc#n...
chrome/gpu/gpu_channel.cc:36:
command_line->HasSwitch(switches::kDisableGLMultisampling);
style: 4 spaces.

http://codereview.chromium.org/6623063/diff/10001/chrome/gpu/gpu_command_buff...
File chrome/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/6623063/diff/10001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.h:40: int32 render_view_id);
style: this is not legal function declaration style. See below for link.

http://codereview.chromium.org/6623063/diff/10001/content/browser/gpu_process...
File content/browser/gpu_process_host.h (right):

http://codereview.chromium.org/6623063/diff/10001/content/browser/gpu_process...
content/browser/gpu_process_host.h:18: };
GpuFeatureFlags is defined for this.  You really shouldn't add another struct
basically doing the same thing.

http://codereview.chromium.org/6623063/diff/10001/content/browser/gpu_process...
content/browser/gpu_process_host.h:42: const BlacklistedGpuFeatures&
blacklistedGpuFeatures);
style: naming should be aaa_bbb instead aaaBbb.

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
File gpu/command_buffer/service/context_group_unittest.cc (right):

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
gpu/command_buffer/service/context_group_unittest.cc:75: DisallowedExtensions(),
"");
style: 4 spaces.

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder_mock.h (right):

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_mock.h:31: const
DisallowedExtensions &disallowed_extensions,
style: & should go with the type, not the variable name

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_mock.h:35: uint32
parent_texture_id));
This is not one of the function parameter declaration styles.  See below for
link.

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right):

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:60:
DisallowedExtensions(), extensions);
style: 4 spaces.

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gpu_processor.cc (right):

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
gpu/command_buffer/service/gpu_processor.cc:61: uint32 parent_texture_id) {
style: this is not one of the function styles. You either align with the first
parameter, or bring the first one to a new line.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gpu_processor_linux.cc (right):

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
gpu/command_buffer/service/gpu_processor_linux.cc:18: uint32 parent_texture_id)
{
style: same here, incorrect function declaration style.

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gpu_processor_mac.cc (right):

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
gpu/command_buffer/service/gpu_processor_mac.cc:18: uint32 parent_texture_id) {
style: same here.

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gpu_processor_win.cc (right):

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
gpu/command_buffer/service/gpu_processor_win.cc:20: uint32 parent_texture_id) {
style: same here.

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
File gpu/command_buffer/service/test_helper.cc (right):

http://codereview.chromium.org/6623063/diff/10001/gpu/command_buffer/service/...
gpu/command_buffer/service/test_helper.cc:96: const DisallowedExtensions
&disallowed_extensions,
style: & should go with type.

[jbauman, 2011-03-11 23:37:05]

Hopefully this fixes all the style issues.

[zmo, 2011-03-12 00:13:53]

LGTM.  Thanks for fixing this bug.

On 2011/03/11 23:37:05, jbauman wrote:
> Hopefully this fixes all the style issues.