Clean up of GLES2 Command Decoder by moving some of the error state into a separate class.

Clean up of GLES2 Command Decoder by moving some of the error state into a separate class.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195002

[kloveless, 2013-04-17 00:07:52]

This CL is just for changing to ErrorState. Many tests had to change making this
CL fairly large. (I will abandon the CL that had Logger and ErrorState in a
single CL).

[greggman, 2013-04-17 00:15:29]

lgtm

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:642: virtual ErrorState *
GetErrorState() OVERRIDE;
nit: no space before '*'

[dsinclair, 2013-04-17 00:39:09]

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
File gpu/command_buffer/service/error_state.h (right):

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/error_state.h:105: class ErrorStateImpl : public
ErrorState {
Does this need to be in the public header or can it be moved to the .cc file? 
Everything should just be using the ErrorState api so Impl should be able to go
into an anonymous namespace in .cc.

You can then add a static Create() method to ErrorState above that returns an
ErrorStateImpl to handle the creation.

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
File gpu/command_buffer/service/error_state_mock.h (right):

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/error_state_mock.h:5: // This file contains the mock
ErrorSTate class.
Lowercase T in ErrorState.

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/error_state_mock.h:46: }
nit: // namespace ...

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3092: ErrorState *
GLES2DecoderImpl::GetErrorState() {
nit: no space before *

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
File gpu/command_buffer/service/vertex_attrib_manager.cc (right):

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/vertex_attrib_manager.cc:170: ErrorState *
error_state = decoder->GetErrorState();
nit: no space before *.

[kloveless, 2013-04-17 14:58:01]

@dsinclair: PTAL

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
File gpu/command_buffer/service/error_state.h (right):

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/error_state.h:105: class ErrorStateImpl : public
ErrorState {
On 2013/04/17 00:39:09, dsinclair wrote:
> Does this need to be in the public header or can it be moved to the .cc file? 
> Everything should just be using the ErrorState api so Impl should be able to
go
> into an anonymous namespace in .cc.
> 
> You can then add a static Create() method to ErrorState above that returns an
> ErrorStateImpl to handle the creation.

Done.

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
File gpu/command_buffer/service/error_state_mock.h (right):

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/error_state_mock.h:5: // This file contains the mock
ErrorSTate class.
On 2013/04/17 00:39:09, dsinclair wrote:
> Lowercase T in ErrorState.

Done.

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/error_state_mock.h:46: }
On 2013/04/17 00:39:09, dsinclair wrote:
> nit: // namespace ...

Done.

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:642: virtual ErrorState *
GetErrorState() OVERRIDE;
On 2013/04/17 00:15:29, greggman wrote:
> nit: no space before '*'

Done.

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3092: ErrorState *
GLES2DecoderImpl::GetErrorState() {
On 2013/04/17 00:39:09, dsinclair wrote:
> nit: no space before *

Done.

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
File gpu/command_buffer/service/vertex_attrib_manager.cc (right):

https://codereview.chromium.org/14308014/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/vertex_attrib_manager.cc:170: ErrorState *
error_state = decoder->GetErrorState();
On 2013/04/17 00:39:09, dsinclair wrote:
> nit: no space before *.

Done.

[dsinclair, 2013-04-17 15:01:19]

lgtm.

[commit-bot: I haz the power, 2013-04-18 14:03:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kloveless@chromium.org/14308014/14006

[commit-bot: I haz the power, 2013-04-18 14:04:01]

Failed to apply patch for gpu/command_buffer/service/texture_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file gpu/command_buffer/service/texture_manager.cc
  Hunk #2 FAILED at 1044.
  Hunk #3 succeeded at 1066 (offset 4 lines).
  Hunk #4 succeeded at 1081 (offset 4 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
gpu/command_buffer/service/texture_manager.cc.rej

Patch:       gpu/command_buffer/service/texture_manager.cc
Index: gpu/command_buffer/service/texture_manager.cc
diff --git a/gpu/command_buffer/service/texture_manager.cc
b/gpu/command_buffer/service/texture_manager.cc
index
853e9f390ceeba93b4c301dff516cc436f2bb1d4..a8f84ea04c061b2ddabf2346a3d621242fe09600
100644
--- a/gpu/command_buffer/service/texture_manager.cc
+++ b/gpu/command_buffer/service/texture_manager.cc
@@ -6,6 +6,7 @@
 #include "base/bits.h"
 #include "base/stringprintf.h"
 #include "gpu/command_buffer/common/gles2_cmd_utils.h"
+#include "gpu/command_buffer/service/error_state.h"
 #include "gpu/command_buffer/service/feature_info.h"
 #include "gpu/command_buffer/service/gles2_cmd_decoder.h"
 #include "gpu/command_buffer/service/mailbox_manager.h"
@@ -1043,17 +1044,18 @@ bool TextureManager::Restore(
   texture->SetServiceId(definition->ReleaseServiceId());
   glBindTexture(texture->target(), texture->service_id());
   texture->SetImmutable(definition->immutable());
-  SetParameter(function_name, decoder, texture, GL_TEXTURE_MIN_FILTER,
+  ErrorState* error_state = decoder->GetErrorState();
+  SetParameter(function_name, error_state, texture, GL_TEXTURE_MIN_FILTER,
                definition->min_filter());
-  SetParameter(function_name, decoder, texture, GL_TEXTURE_MAG_FILTER,
+  SetParameter(function_name, error_state, texture, GL_TEXTURE_MAG_FILTER,
                definition->mag_filter());
-  SetParameter(function_name, decoder, texture, GL_TEXTURE_WRAP_S,
+  SetParameter(function_name, error_state, texture, GL_TEXTURE_WRAP_S,
                definition->wrap_s());
-  SetParameter(function_name, decoder, texture, GL_TEXTURE_WRAP_T,
+  SetParameter(function_name, error_state, texture, GL_TEXTURE_WRAP_T,
                definition->wrap_t());
   if (feature_info_->validators()->texture_parameter.IsValid(
       GL_TEXTURE_USAGE_ANGLE)) {
-    SetParameter(function_name, decoder, texture, GL_TEXTURE_USAGE_ANGLE,
+    SetParameter(function_name, error_state, texture, GL_TEXTURE_USAGE_ANGLE,
                  definition->usage());
   }
 
@@ -1061,9 +1063,9 @@ bool TextureManager::Restore(
 }
 
 void TextureManager::SetParameter(
-    const char* function_name, GLES2Decoder* decoder,
+    const char* function_name, ErrorState* error_state,
     Texture* texture, GLenum pname, GLint param) {
-  DCHECK(decoder);
+  DCHECK(error_state);
   DCHECK(texture);
   if (!texture->CanRender(feature_info_)) {
     DCHECK_NE(0, num_unrenderable_textures_);
@@ -1076,11 +1078,11 @@ void TextureManager::SetParameter(
   GLenum result = texture->SetParameter(feature_info_, pname, param);
   if (result != GL_NO_ERROR) {
     if (result == GL_INVALID_ENUM) {
-      GLESDECODER_SET_GL_ERROR_INVALID_ENUM(
-          decoder, function_name, param, "param");
+      ERRORSTATE_SET_GL_ERROR_INVALID_ENUM(
+          error_state, function_name, param, "param");
     } else {
-      GLESDECODER_SET_GL_ERROR_INVALID_PARAM(
-          decoder, result, function_name, pname, static_cast<GLint>(param));
+      ERRORSTATE_SET_GL_ERROR_INVALID_PARAM(
+          error_state, result, function_name, pname,
static_cast<GLint>(param));
     }
   } else {
     // Texture tracking pools exist only for the command decoder, so

[commit-bot: I haz the power, 2013-04-18 15:47:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kloveless@chromium.org/14308014/50001

[commit-bot: I haz the power, 2013-04-18 16:56:43]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-04-18 19:23:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kloveless@chromium.org/14308014/50001

[commit-bot: I haz the power, 2013-04-18 21:11:53]

Change committed as 195002