Exposed support for dynamically enabling extensions in command buffer...

Exposed support for dynamically enabling extensions in command buffer
implementation via new glGetRequestableExtensionsCHROMIUM and
glRequestExtensionCHROMIUM entry points. These entry points are needed in
order to allow WebGL to both query the available extensions and enable them
individually.

Added these entry points to WebGraphicsContext3DCommandBufferImpl. A
subsequent WebKit checkin under https://bugs.webkit.org/show_bug.cgi?id=40316
will utilize them and implement the OES_texture_float extension for WebGL.

BUG=none
TEST=none (ran with new oes-texture-float.html WebGL conformance test)


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

[Ken Russell (switch to Gerrit), 2010-12-04 00:53:13]

Please review. Thanks.

[greggman, 2010-12-04 01:28:44]

I'm not sure I understand why all the refactoring in feature_info.cc/h  You
could leave those as they were and implement
HandleGetRequestableExtensionsCHROMIUM like this

error::Error GLES2DecoderImpl::HandleGetRequestableExtensionsCHROMIUM(
  uint32 immediate_data_size,
  const gles2::GetRequestableExtensionsCHROMIUM& c) {
  Bucket* bucket = CreateBucket(c.bucket_id);
  scoped_ptr<FeatureInfo> info(new FeatureInfo());
  info->Initialize(NULL);
  bucket->SetFromString(info->extensions().c_str());
  return error::kNoError;
}

http://codereview.chromium.org/5626008/diff/20001/gpu/command_buffer/service/...
File gpu/command_buffer/service/feature_info.cc (right):

http://codereview.chromium.org/5626008/diff/20001/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.cc:345:
RemoveRequestableExtensionString(str);
Why remove added extensions? Shouldn't the list of requestable extensions
include extensions added?

http://codereview.chromium.org/5626008/diff/20001/gpu/command_buffer/service/...
File gpu/command_buffer/service/feature_info.h (right):

http://codereview.chromium.org/5626008/diff/20001/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.h:68: void
DoSimpleTestForEnablingExtension(
Why do these take void extension_helper? Why not ExtensionHelper*?

http://codereview.chromium.org/5626008/diff/20001/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.h:84: std::vector<std::string>
requestable_extensions_list_;
why a list and not a set? This would simplify the code where you add and remove
them

http://codereview.chromium.org/5626008/diff/20001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/5626008/diff/20001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5982: if (feature_str ==
"GL_OES_standard_derivatives" ||
FeatureInfo::AddFeatures works with multiple features. Otherwords, if I pass in
"GL_OES_standard_derivatives GL_CHROMIUM_webglsl" it will turn on both features
but this if will be false

You could remove the check, or you could just
feature_info_->feature_flags.oes_standard_derivatives ||
feature_info_->feature_flags.chromium_webglsl

http://codereview.chromium.org/5626008/diff/20001/gpu/command_buffer/service/...
File gpu/command_buffer/service/shader_translator.cc (right):

http://codereview.chromium.org/5626008/diff/20001/gpu/command_buffer/service/...
gpu/command_buffer/service/shader_translator.cc:114: // Pass down the original
shader's source rather than the
If you pass it down untranslated it will not have any of the re-writes needed to
fix bugs or add WebGL features. Shouldn't this stay as it was?

[Ken Russell (switch to Gerrit), 2010-12-04 02:17:46]

On 2010/12/04 01:28:44, greggman wrote:
> I'm not sure I understand why all the refactoring in feature_info.cc/h  You
> could leave those as they were and implement
> HandleGetRequestableExtensionsCHROMIUM like this
> 
> error::Error GLES2DecoderImpl::HandleGetRequestableExtensionsCHROMIUM(
>   uint32 immediate_data_size,
>   const gles2::GetRequestableExtensionsCHROMIUM& c) {
>   Bucket* bucket = CreateBucket(c.bucket_id);
>   scoped_ptr<FeatureInfo> info(new FeatureInfo());
>   info->Initialize(NULL);
>   bucket->SetFromString(info->extensions().c_str());
>   return error::kNoError;
> }

Thanks, this is a really good suggestion and simplifies the code a lot. I've
done this, which removes all of the changes from feature_info.cc/h. Re-tested.

http://codereview.chromium.org/5626008/diff/20001/gpu/command_buffer/service/...
File gpu/command_buffer/service/shader_translator.cc (right):

http://codereview.chromium.org/5626008/diff/20001/gpu/command_buffer/service/...
gpu/command_buffer/service/shader_translator.cc:114: // Pass down the original
shader's source rather than the
On 2010/12/04 01:28:44, greggman wrote:
> If you pass it down untranslated it will not have any of the re-writes needed
to
> fix bugs or add WebGL features. Shouldn't this stay as it was?

Previously we weren't using the shader validator at all on OpenGL ES 2.0
implementations. The shader validator doesn't have a GLSL ES backend, only a
desktop GL backend, and the two aren't compatible. I've filed
http://code.google.com/p/angleproject/issues/detail?id=81 to track this and
added a TODO.

[Ken Russell (switch to Gerrit), 2010-12-04 03:44:37]

I was wrong; one bug fix was needed to feature_info.cc to allow
GL_OES_texture_float and friends to be enabled individually on desktop GL. I've
added that fix.

[greggman, 2010-12-06 17:10:05]

lgtm