Clean up some of the shader compilation code.

Clean up some of the shader compilation code.

This cL contains some micro optimizations as well as cleaning up some
variables that are left over from old refactors.

One variable in particular that was deleted is the "compiler_options_"
member within the Shader class. This member was used to hold extra
compiler options such as extensions for cache key purposes. However,
at some point this mechanism has been replaced by querying the angle
compiler directly for a string which represents the resources.

BUG=453543
TEST=trybots

Committed: https://crrev.com/7b0b4ba57d1755c38baa1f569760683b12c49940
Cr-Commit-Position: refs/heads/master@{#314642}

[David Yen, 2015-02-03 22:33:04]

dyen@chromium.org changed reviewers:
+ vmiura@chromium.org

[Ken Russell (switch to Gerrit), 2015-02-04 00:52:36]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2015-02-04 00:52:36]

Thanks for doing this cleanup. It generally looks good but two comments.

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/s...
File gpu/command_buffer/service/shader_manager.cc (right):

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/s...
gpu/command_buffer/service/shader_manager.cc:71: translated_source_.resize(len);
Be very very careful. This could end up trimming the null terminator off the
end. That might be OK because it's a C++ string and the internal representation
doesn't require the null there, but regardless string manipulation like this has
been a cause of errors in the past.

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/s...
gpu/command_buffer/service/shader_manager.cc:88: log_info_.resize(len);
Same basic comment here.

[David Yen, 2015-02-04 01:01:05]

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/s...
File gpu/command_buffer/service/shader_manager.cc (right):

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/s...
gpu/command_buffer/service/shader_manager.cc:71: translated_source_.resize(len);
On 2015/02/04 00:52:36, Ken Russell wrote:
> Be very very careful. This could end up trimming the null terminator off the
> end. That might be OK because it's a C++ string and the internal
representation
> doesn't require the null there, but regardless string manipulation like this
has
> been a cause of errors in the past.

Yeah, to minimize the changes I just tried to mimic the old behavior so this is
simply equivalent to what used to be there (minus the extra temporary
allocation).

You are right that depending on what glGetTranslatedShaderSourceANGLE returns,
this could trim off the null terminator. But as you pointed out, std::string
does not require the null terminator. So as long as they are using c_str() when
needing const char* representations it should be okay either way.

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/s...
gpu/command_buffer/service/shader_manager.cc:88: log_info_.resize(len);
On 2015/02/04 00:52:36, Ken Russell wrote:
> Same basic comment here.

Acknowledged.

[vmiura, 2015-02-04 05:49:02]

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7055: str.append(data[ii],
length[ii]);
I believe the spec is that if length[ii] is NULL then it should be treated as a
0 terminated string.

[vmiura, 2015-02-04 05:55:51]

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7055: str.append(data[ii],
length[ii]);
On 2015/02/04 05:49:02, vmiura wrote:
> I believe the spec is that if length[ii] is NULL then it should be treated as
a
> 0 terminated string.

Btw, is this a bug fix that should be in it's own CL?

[David Yen, 2015-02-04 19:13:33]

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/900543004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7055: str.append(data[ii],
length[ii]);
On 2015/02/04 05:55:51, vmiura wrote:
> On 2015/02/04 05:49:02, vmiura wrote:
> > I believe the spec is that if length[ii] is NULL then it should be treated
as
> a
> > 0 terminated string.
> 
> Btw, is this a bug fix that should be in it's own CL?

Done. Strictly speaking this is not a bug fix. It is just a micro optimization
because the GLES2Implementation::ShaderSource() does the correct evaluations and
always passes a correct length array. To be sure I added a unit test also, the
unit tests passes without any changes.

[Ken Russell (switch to Gerrit), 2015-02-04 19:37:35]

lgtm

[vmiura, 2015-02-04 20:52:40]

lgtm

[David Yen, 2015-02-04 20:54:02]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2015-02-04 20:55:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/900543004/20001

[commit-bot: I haz the power, 2015-02-04 21:38:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-04 21:41:20]

Patchset 2 (id:??) landed as
https://crrev.com/7b0b4ba57d1755c38baa1f569760683b12c49940
Cr-Commit-Position: refs/heads/master@{#314642}