Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466763002:
command buffer: tweak error messages for a few more ES3 commands (Closed)
|
Created:
4 years, 1 month ago by Kai Ninomiya Modified:
4 years, 1 month ago Reviewers:
Zhenyao Mo CC:
chromium-reviews, piman+watch_chromium.org, Ken Russell (switch to Gerrit) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncommand buffer: tweak error messages for a few more ES3 commands
BUG=654787
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/9924d1a82210f09efd8ea904685cecb9308c9d00
Cr-Commit-Position: refs/heads/master@{#429119}
Patch Set 1 #
Total comments: 7
Patch Set 2 : address comments #Messages
Total messages: 17 (8 generated)
Description was changed from ========== command buffer: tweak error messages for a few more ES3 commands BUG=654787 ========== to ========== command buffer: tweak error messages for a few more ES3 commands BUG=654787 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 ==========
The CQ bit was checked by kainino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kainino@chromium.org changed reviewers: + zmo@chromium.org
zmo: PTAL
All look good to me except one place. https://codereview.chromium.org/2466763002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2466763002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:17216: NOTREACHED(); This could happen in context lost.
https://codereview.chromium.org/2466763002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2466763002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:17216: NOTREACHED(); On 2016/11/01 02:27:48, Zhenyao Mo wrote: > This could happen in context lost. Ah, okay. Is it okay that it returns kNoError in this case, then? https://codereview.chromium.org/2466763002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:17463: // This should mean GL_OUT_OF_MEMORY. Can context loss (or anything else) trigger this? If this is always supposed to be GL_OUT_OF_MEMORY, maybe it can be DCHECKed here.
https://codereview.chromium.org/2466763002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2466763002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:17216: NOTREACHED(); On 2016/11/01 03:16:01, Kai Ninomiya wrote: > On 2016/11/01 02:27:48, Zhenyao Mo wrote: > > This could happen in context lost. > > Ah, okay. Is it okay that it returns kNoError in this case, then? I think the LOSE_CONTEXT gl error will be handled somewhere, but if you want to return kContextLost, that's fine too. https://codereview.chromium.org/2466763002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:17463: // This should mean GL_OUT_OF_MEMORY. On 2016/11/01 03:16:01, Kai Ninomiya wrote: > Can context loss (or anything else) trigger this? If this is always supposed to > be GL_OUT_OF_MEMORY, maybe it can be DCHECKed here. In context lost, the behavior (returned value) is undefined.
Okay, I edited those two. Do they seem correct? https://codereview.chromium.org/2466763002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2466763002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:17216: NOTREACHED(); On 2016/11/01 04:49:44, Zhenyao Mo wrote: > On 2016/11/01 03:16:01, Kai Ninomiya wrote: > > On 2016/11/01 02:27:48, Zhenyao Mo wrote: > > > This could happen in context lost. > > > > Ah, okay. Is it okay that it returns kNoError in this case, then? > > I think the LOSE_CONTEXT gl error will be handled somewhere, but if you want to > return kContextLost, that's fine too. Done. https://codereview.chromium.org/2466763002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:17463: // This should mean GL_OUT_OF_MEMORY. On 2016/11/01 04:49:44, Zhenyao Mo wrote: > On 2016/11/01 03:16:01, Kai Ninomiya wrote: > > Can context loss (or anything else) trigger this? If this is always supposed > to > > be GL_OUT_OF_MEMORY, maybe it can be DCHECKed here. > > In context lost, the behavior (returned value) is undefined. Done.
lgtm
The CQ bit was checked by kainino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== command buffer: tweak error messages for a few more ES3 commands BUG=654787 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 ========== command buffer: tweak error messages for a few more ES3 commands BUG=654787 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/9924d1a82210f09efd8ea904685cecb9308c9d00 Cr-Commit-Position: refs/heads/master@{#429119} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9924d1a82210f09efd8ea904685cecb9308c9d00 Cr-Commit-Position: refs/heads/master@{#429119} |
