fix --show-composited-layer-borders on Mac

fix --show-composited-layer-borders on Mac

Fix GPU process crash due to incorrect glLineWidth value on Mac.

BUG=662682
R=piman
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/d5e3696056f67a01e595bc7c62b9270052deb1f2
Cr-Commit-Position: refs/heads/master@{#431102}

[Lof, 2016-11-05 21:18:03]

Description was changed from

==========
fix --show-composited-layer-borders on Mac

Fix GPU process crash due to incorrect glLineWidth value on Mac.

BUG=662682

R=piman
==========

to

==========
fix --show-composited-layer-borders on Mac

Fix GPU process crash due to incorrect glLineWidth value on Mac.

BUG=662682

R=piman
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2016-11-07 19:38:02]

https://codereview.chromium.org/2482673002/diff/1/gpu/command_buffer/build_gl...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/2482673002/diff/1/gpu/command_buffer/build_gl...
gpu/command_buffer/build_gles2_cmd_buffer.py:2743: 'type': 'Custom',
Rather than using "Custom" which completely removes the autogenerated code, can
you use a regular function (remove the 'type' attribute, leaving the
decoder_func), and move the logic (extra validation, state tracking) to the
DoLineWidth function?

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

https://codereview.chromium.org/2482673002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8469: state_.line_width =
DoLineWidth(width);
This is incorrect, though, because it means the value returned by
glGetIIntegerv(GL_LINE_WIDTH, ...) will not match the value passed to
glLineWidth any more (it's supposed to).

We either need to keep both the original value and the clamped value, or we need
to do the clamping again in ContextState::InitState

[Lof, 2016-11-07 22:09:34]

Hi piman!

Here another approach.
Is this OK?

[piman, 2016-11-07 22:34:40]

Overall approach is good, but we need to make sure the autogenerated file is
correctly (re-)generated by the script. Otherwise modifications will be lost
next time we run it.

https://codereview.chromium.org/2482673002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state.h (right):

https://codereview.chromium.org/2482673002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:169: void InitLineWidth(GLfloat min,
GLfloat max);
nit: maybe SetLineWidthBounds ?

https://codereview.chromium.org/2482673002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state_impl_autogen.h (right):

https://codereview.chromium.org/2482673002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state_impl_autogen.h:304:
DoLineWidth(line_width);
The approach is reasonable, but this file is autogenerated. I think you need a
modification in the code generator to produce this.

[Lof, 2016-11-08 20:11:58]

https://codereview.chromium.org/2482673002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state.h (right):

https://codereview.chromium.org/2482673002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:169: void InitLineWidth(GLfloat min,
GLfloat max);
On 2016/11/07 22:34:40, piman wrote:
> nit: maybe SetLineWidthBounds ?

Done.

https://codereview.chromium.org/2482673002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state_impl_autogen.h (right):

https://codereview.chromium.org/2482673002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state_impl_autogen.h:304:
DoLineWidth(line_width);
On 2016/11/07 22:34:40, piman wrote:
> The approach is reasonable, but this file is autogenerated. I think you need a
> modification in the code generator to produce this.

Done.

[piman, 2016-11-08 22:20:23]

Thanks for working on this - sorry it's taking a while. I think there's another
issue...

https://codereview.chromium.org/2482673002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_0_autogen.h (right):

https://codereview.chromium.org/2482673002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_0_autogen.h:79:
EXPECT_CALL(*gl_, DoLineWidth(1.0f)).Times(1).RetiresOnSaturation();
I think the generator picked 'func' for the autogenerated test as well, and this
will not compile. Maybe you can  skip generating this if "custom_function" is
set, and instead move it to SetupInitStateManualExpectations which is manually
written?

[Lof, 2016-11-09 08:57:11]

https://codereview.chromium.org/2482673002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_0_autogen.h (right):

https://codereview.chromium.org/2482673002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_0_autogen.h:79:
EXPECT_CALL(*gl_, DoLineWidth(1.0f)).Times(1).RetiresOnSaturation();
On 2016/11/08 22:20:23, piman wrote:
> I think the generator picked 'func' for the autogenerated test as well, and
this
> will not compile. Maybe you can  skip generating this if "custom_function" is
> set, and instead move it to SetupInitStateManualExpectations which is manually
> written?

Done.

[Lof, 2016-11-09 09:06:48]

Hi piman!

I think it's better to modify codegen rather than to add code manually.

[piman, 2016-11-09 16:32:13]

https://codereview.chromium.org/2482673002/diff/60001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/2482673002/diff/60001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:10540: func_name =
func_name[len(func_prefix):]
Mmh, the reason I suggested manual code for the test, is that a Do* function may
do much more than just calling the underlying gl function - e.g. calling several
gl functions, calling a different one, calling none, etc. So to set the
expectation automatically based on mangling the function name is a bit of a
stretch

[Lof, 2016-11-09 22:14:19]

https://codereview.chromium.org/2482673002/diff/60001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/2482673002/diff/60001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:10540: func_name =
func_name[len(func_prefix):]
On 2016/11/09 16:32:13, piman wrote:
> Mmh, the reason I suggested manual code for the test, is that a Do* function
may
> do much more than just calling the underlying gl function - e.g. calling
several
> gl functions, calling a different one, calling none, etc. So to set the
> expectation automatically based on mangling the function name is a bit of a
> stretch

Done.

[piman, 2016-11-09 22:31:21]

lgtm

https://codereview.chromium.org/2482673002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right):

https://codereview.chromium.org/2482673002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:2072:
EXPECT_CALL(*gl_, LineWidth(1.0f)).Times(1).RetiresOnSaturation();
nit: LineWidth(width) maybe?

[Lof, 2016-11-09 22:37:21]

https://codereview.chromium.org/2482673002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right):

https://codereview.chromium.org/2482673002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:2072:
EXPECT_CALL(*gl_, LineWidth(1.0f)).Times(1).RetiresOnSaturation();
On 2016/11/09 22:31:21, piman wrote:
> nit: LineWidth(width) maybe?

Done.

PS. Sorry 1am in Moscow.

[Lof, 2016-11-09 22:37:34]

The CQ bit was checked by lof84@yandex-team.ru

[Lof, 2016-11-09 22:37:35]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2482673002/#ps100001
(title: "small fix")

[commit-bot: I haz the power, 2016-11-09 22:38:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 00:36:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-10 00:38:29]

Description was changed from

==========
fix --show-composited-layer-borders on Mac

Fix GPU process crash due to incorrect glLineWidth value on Mac.

BUG=662682

R=piman
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
fix --show-composited-layer-borders on Mac

Fix GPU process crash due to incorrect glLineWidth value on Mac.

BUG=662682

R=piman
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/d5e3696056f67a01e595bc7c62b9270052deb1f2
Cr-Commit-Position: refs/heads/master@{#431102}
==========

[commit-bot: I haz the power, 2016-11-10 00:38:30]

Patchset 6 (id:??) landed as
https://crrev.com/d5e3696056f67a01e595bc7c62b9270052deb1f2
Cr-Commit-Position: refs/heads/master@{#431102}