Take GL version and extensions correctly into account when binding functions

gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc

Index: gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc b/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
index 910cb0104b214ec80de4dd7872f5a70f1904a145..ca1b1dd76d58c39d7fb1d193e652e5190246f3fd 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
@@ -4739,7 +4739,7 @@ TEST_F(GLES2DecoderManualInitTest,
       .WillOnce(Return(GL_NO_ERROR))
       .WillOnce(Return(GL_OUT_OF_MEMORY))
       .RetiresOnSaturation();
-  EXPECT_CALL(*gl_, RenderbufferStorageMultisampleEXT(
+  EXPECT_CALL(*gl_, RenderbufferStorageMultisample(
       GL_RENDERBUFFER, 1, GL_RGBA, 100, 50))
       .Times(1)
       .RetiresOnSaturation();
@@ -4762,7 +4762,7 @@ TEST_F(GLES2DecoderManualInitTest,
       true);   // bind generates resource
   DoBindRenderbuffer(GL_RENDERBUFFER, client_renderbuffer_id_,
                     kServiceRenderbufferId);
-  EXPECT_CALL(*gl_, RenderbufferStorageMultisampleEXT(_, _, _, _, _))
+  EXPECT_CALL(*gl_, RenderbufferStorageMultisample(_, _, _, _, _))
       .Times(0)
       .RetiresOnSaturation();
   RenderbufferStorageMultisampleCHROMIUM cmd;
@@ -4798,11 +4798,11 @@ TEST_F(GLES2DecoderManualInitTest, RenderbufferStorageMultisampleCHROMIUM) {
       .RetiresOnSaturation();
   EXPECT_CALL(
       *gl_,
-      RenderbufferStorageMultisampleEXT(GL_RENDERBUFFER,
-                                        TestHelper::kMaxSamples,
-                                        GL_RGBA,
-                                        TestHelper::kMaxRenderbufferSize,
-                                        1))
+      RenderbufferStorageMultisample(GL_RENDERBUFFER,

[Ken Russell (switch to Gerrit), 2013/12/10 00:07:07]

This change is problematic. This test explicitly tries to initialize with the
GL_EXT_framebuffer_multisample extension. A fix is needed for this issue.

+                                     TestHelper::kMaxSamples,
+                                     GL_RGBA,
+                                     TestHelper::kMaxRenderbufferSize,
+                                     1))
       .Times(1)
       .RetiresOnSaturation();
   EXPECT_CALL(*gl_, GetError())
@@ -4872,11 +4872,11 @@ TEST_P(GLES2DecoderMultisampledRenderToTextureTest,
   } else {
     EXPECT_CALL(
         *gl_,
-        RenderbufferStorageMultisampleEXT(GL_RENDERBUFFER,
-                                          TestHelper::kMaxSamples,
-                                          GL_RGBA,
-                                          TestHelper::kMaxRenderbufferSize,
-                                          1))
+        RenderbufferStorageMultisample(GL_RENDERBUFFER,

[no sievers, 2013/12/05 20:51:57]

Here we'd still want RenderbufferStorageMultisampleEXT for the specific mobile
render_to_texture extension.

[oetuaho, 2013/12/09 17:17:07]

The root problem here is really that the unit tests don't initialize the
bindings with a proper extension string. If that would be fixed,
RenderbufferStorageMultisampleEXT would be called here. I think I'll try to see
if re-initializing the bindings in GLES2DecoderTestWithExtensions::SetUp could
work.

[no sievers, 2013/12/09 20:05:24]

Or maybe to make it simpler just initialize it with all extensions in
gpu/command_buffer/common/unittest_main.cc. That would be compatible with what
happens now, and I think good enough since at this level we don't really test
anything about the bindings but rather mock all of them. That means that we also
fail tests if we get unexpected mock calls.

You could use the mock version/extension trick you do in the other places. Maybe
the python script could generate a constant string with all known extensions for
this purpose?

[Ken Russell (switch to Gerrit), 2013/12/10 00:07:07]

There are attempts in other tests, like
GLES2DecoderManualInitTest::RenderbufferStorageMultisampleCHROMIUM, above, to
initialize the decoder with an appropriate extension string. Why isn't that
working any more?

The change of which function is called here and above -- from
RenderbufferStorageMultisampleEXT to RenderbufferStorageMultisample -- is really
problematic and needs to be resolved before this patch can land. These
multisampling APIs have very different semantics.

[oetuaho, 2013/12/10 10:39:05]

Yes, the decoder is initialized with the appropriate extension string in the
tests, but the bindings are not. I do intend to fix this.

Ah, and now I think I get the semantic difference. So to clarify, this binding
is a different function depending on whether we're using ES or desktop GL, since
in desktop GL RenderbufferStorageMultisample !=
RenderbufferStorageMultisampleEXT, but in ES RenderbufferStorageMultisample ==
RenderbufferStorageMultisampleEXT == RenderbufferStorageMultisampleIMG. In ES
world this fallback actually looks correct to me (this line), but in the desktop
world it is definitely not correct (the case above Ken commented on). That the
fallback exists in the bindings in the first place is wrong from the desktop
point of view, so it could use some further cleanup, but for now it should be
enough to fix the unit tests so that the bindings are done as expected.

And I don't think that initializing the unit test bindings once with all known
extensions is good enough, since the extensions can be in conflict with each
other like this. They need to be reinitialized per test.

+                                       TestHelper::kMaxSamples,
+                                       GL_RGBA,
+                                       TestHelper::kMaxRenderbufferSize,
+                                       1))
         .Times(1)
         .RetiresOnSaturation();
   }