Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220093002:
GpuInfo mojom struct and struct traits and unit test (Closed)
|
Created:
4 years, 4 months ago by Alex Z. Modified:
4 years, 4 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, piman+watch_chromium.org, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGpuInfo mojom struct and struct traits and unit test
The mojom struct corresponds to gpu::GPUInfo. Serializing GPUInfo in mojo
enables full support for hardware rendering.
BUG=622707
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/bf90a8e911d48f0055bd2676460f48b8c669324b
Cr-Commit-Position: refs/heads/master@{#411413}
Patch Set 1 #Patch Set 2 : Changed deps in gpu_info.typemap #Patch Set 3 : Added a missing field in GpuInfo #Patch Set 4 : Removed unnecessary change #Patch Set 5 : Fixed a dependency error and added more readable comments #Patch Set 6 : Added include #
Total comments: 7
Patch Set 7 : Used nullable field for dx_diagnostics; Improved unit test and bug fixes #Patch Set 8 : Compare only values for DxDiagNode #Patch Set 9 : Fixed a typo #
Messages
Total messages: 51 (37 generated)
Description was changed from ========== GpuInfo mojom struct and struct traits and unit test The mojom struct corresponds to gpu::GPUInfo. Serializing GPUInfo in mojo enables full support for hardware rendering. BUG=622707 ========== to ========== GpuInfo mojom struct and struct traits and unit test The mojom struct corresponds to gpu::GPUInfo. Serializing GPUInfo in mojo enables full support for hardware rendering. BUG=622707 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 staraz@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by staraz@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by staraz@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 checked by staraz@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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by staraz@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...
staraz@chromium.org changed reviewers: + dcheng@chromium.org, fsamuel@chromium.org
This should be the last CL for the GpuInfo mojom struct series :) fsamuel@: Please review the new struct traits for GpuInfo. dcheng@: Please do a security review on the IPC files.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info.mojom (right): https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:74: struct GpuInfo { reference to where this C++ type is located? https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/struct_... File gpu/ipc/common/struct_traits_unittest.cc (right): https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/struct_... gpu/ipc/common/struct_traits_unittest.cc:126: proxy->EchoGpuInfo(input, &output); This test is incomplete. Please update the test to use non-default values.
fsamuel@chromium.org changed reviewers: + yzshen@chromium.org
https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.h (right): https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.h:271: static const gpu::DxDiagNode dx_diag_node; This is very icky. I'm not sure if there's a better way though :( +yzshen@ for feedback.
https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.h (right): https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.h:271: static const gpu::DxDiagNode dx_diag_node; On 2016/08/10 12:13:24, Fady Samuel wrote: > This is very icky. I'm not sure if there's a better way though :( +yzshen@ for > feedback. This works correctly, although it is a little inefficient in that for platforms other than OS_WIN, we also have to serialize an dummy object. One way to avoid that cost: First, define the dx_diagnostics field as nullable in mojom. And then in this StructTraits, we can use: #if defined(OS_WIN): static const gpu::DxDiagNode& dx_diagnostics(const gpu::GPUInfo& input) { return input.dx_diagnostics; } #else static const base::Optional<gpu::DxDiagNode>& dx_diagnostics(const gpu::GPUInfo& input) { static const base::Optional<gpu::DxDiagNode> dx_diag_node(base::nullopt); return dx_diag_node; } #endif And in the deserialization code, we only read the dx_diagnostics field on OS_WIN, ignoring the others.
On 2016/08/10 16:29:19, yzshen1 wrote: > https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... > File gpu/ipc/common/gpu_info_struct_traits.h (right): > > https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... > gpu/ipc/common/gpu_info_struct_traits.h:271: static const gpu::DxDiagNode > dx_diag_node; > On 2016/08/10 12:13:24, Fady Samuel wrote: > > This is very icky. I'm not sure if there's a better way though :( +yzshen@ for > > feedback. > > This works correctly, although it is a little inefficient in that for platforms > other than OS_WIN, we also have to serialize an dummy object. > > One way to avoid that cost: > First, define the dx_diagnostics field as nullable in mojom. And then in this > StructTraits, we can use: > > #if defined(OS_WIN): > static const gpu::DxDiagNode& dx_diagnostics(const gpu::GPUInfo& input) { > return input.dx_diagnostics; > } > #else > static const base::Optional<gpu::DxDiagNode>& dx_diagnostics(const > gpu::GPUInfo& input) { > static const base::Optional<gpu::DxDiagNode> dx_diag_node(base::nullopt); > return dx_diag_node; > } > #endif > > > And in the deserialization code, we only read the dx_diagnostics field on > OS_WIN, ignoring the others. I changed the struct traits to use the code above and got an error from the echo unit test: "Invalid message: VALIDATION_ERROR_UNEXPECTED_NULL_POINTER (null dx_diagnostics field in GpuInfo)" It seems to be unhappy about GpuInfo having a null field. How can I fix it?
https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info.mojom (right): https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:74: struct GpuInfo { On 2016/08/08 19:06:20, Fady Samuel wrote: > reference to where this C++ type is located? Done. https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.h (right): https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.h:271: static const gpu::DxDiagNode dx_diag_node; On 2016/08/10 16:29:19, yzshen1 wrote: > On 2016/08/10 12:13:24, Fady Samuel wrote: > > This is very icky. I'm not sure if there's a better way though :( +yzshen@ for > > feedback. > > This works correctly, although it is a little inefficient in that for platforms > other than OS_WIN, we also have to serialize an dummy object. > > One way to avoid that cost: > First, define the dx_diagnostics field as nullable in mojom. And then in this > StructTraits, we can use: > > #if defined(OS_WIN): > static const gpu::DxDiagNode& dx_diagnostics(const gpu::GPUInfo& input) { > return input.dx_diagnostics; > } > #else > static const base::Optional<gpu::DxDiagNode>& dx_diagnostics(const > gpu::GPUInfo& input) { > static const base::Optional<gpu::DxDiagNode> dx_diag_node(base::nullopt); > return dx_diag_node; > } > #endif > > > And in the deserialization code, we only read the dx_diagnostics field on > OS_WIN, ignoring the others. > Done. https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/struct_... File gpu/ipc/common/struct_traits_unittest.cc (right): https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/struct_... gpu/ipc/common/struct_traits_unittest.cc:126: proxy->EchoGpuInfo(input, &output); On 2016/08/08 19:06:21, Fady Samuel wrote: > This test is incomplete. Please update the test to use non-default values. Done.
lgtm assuming all the bots are happy.
The CQ bit was checked by fsamuel@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...
On 2016/08/10 18:45:10, StarAZ1 wrote: > On 2016/08/10 16:29:19, yzshen1 wrote: > > > https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... > > File gpu/ipc/common/gpu_info_struct_traits.h (right): > > > > > https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... > > gpu/ipc/common/gpu_info_struct_traits.h:271: static const gpu::DxDiagNode > > dx_diag_node; > > On 2016/08/10 12:13:24, Fady Samuel wrote: > > > This is very icky. I'm not sure if there's a better way though :( +yzshen@ > for > > > feedback. > > > > This works correctly, although it is a little inefficient in that for > platforms > > other than OS_WIN, we also have to serialize an dummy object. > > > > One way to avoid that cost: > > First, define the dx_diagnostics field as nullable in mojom. And then in this > > StructTraits, we can use: > > > > #if defined(OS_WIN): > > static const gpu::DxDiagNode& dx_diagnostics(const gpu::GPUInfo& input) { > > return input.dx_diagnostics; > > } > > #else > > static const base::Optional<gpu::DxDiagNode>& dx_diagnostics(const > > gpu::GPUInfo& input) { > > static const base::Optional<gpu::DxDiagNode> dx_diag_node(base::nullopt); > > return dx_diag_node; > > } > > #endif > > > > > > And in the deserialization code, we only read the dx_diagnostics field on > > OS_WIN, ignoring the others. > > I changed the struct traits to use the code above and got an error from the echo > unit test: "Invalid message: VALIDATION_ERROR_UNEXPECTED_NULL_POINTER > (null dx_diagnostics field in GpuInfo)" It seems to be unhappy about GpuInfo > having > a null field. How can I fix it? Sorry for late reply. I think you have fixed it by making it nullable in mojom?
On 2016/08/10 20:01:08, yzshen1 wrote: > On 2016/08/10 18:45:10, StarAZ1 wrote: > > On 2016/08/10 16:29:19, yzshen1 wrote: > > > > > > https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... > > > File gpu/ipc/common/gpu_info_struct_traits.h (right): > > > > > > > > > https://codereview.chromium.org/2220093002/diff/100001/gpu/ipc/common/gpu_inf... > > > gpu/ipc/common/gpu_info_struct_traits.h:271: static const gpu::DxDiagNode > > > dx_diag_node; > > > On 2016/08/10 12:13:24, Fady Samuel wrote: > > > > This is very icky. I'm not sure if there's a better way though :( +yzshen@ > > for > > > > feedback. > > > > > > This works correctly, although it is a little inefficient in that for > > platforms > > > other than OS_WIN, we also have to serialize an dummy object. > > > > > > One way to avoid that cost: > > > First, define the dx_diagnostics field as nullable in mojom. And then in > this > > > StructTraits, we can use: > > > > > > #if defined(OS_WIN): > > > static const gpu::DxDiagNode& dx_diagnostics(const gpu::GPUInfo& input) { > > > return input.dx_diagnostics; > > > } > > > #else > > > static const base::Optional<gpu::DxDiagNode>& dx_diagnostics(const > > > gpu::GPUInfo& input) { > > > static const base::Optional<gpu::DxDiagNode> > dx_diag_node(base::nullopt); > > > return dx_diag_node; > > > } > > > #endif > > > > > > > > > And in the deserialization code, we only read the dx_diagnostics field on > > > OS_WIN, ignoring the others. > > > > I changed the struct traits to use the code above and got an error from the > echo > > unit test: "Invalid message: VALIDATION_ERROR_UNEXPECTED_NULL_POINTER > > (null dx_diagnostics field in GpuInfo)" It seems to be unhappy about GpuInfo > > having > > a null field. How can I fix it? > > Sorry for late reply. I think you have fixed it by making it nullable in mojom? Yes. At first I thought nullable means [nullable]DxDiagNode. Fady told me about the question mark.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by staraz@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by staraz@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.
staraz@chromium.org changed reviewers: + tsepez@chromium.org
tsepez@: Please do a security review on the changed IPC files.
On 2016/08/11 19:15:51, StarAZ1 wrote: > tsepez@: Please do a security review on the changed IPC files. LGTM
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2220093002/#ps160001 (title: "Fixed a typo")
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 #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== GpuInfo mojom struct and struct traits and unit test The mojom struct corresponds to gpu::GPUInfo. Serializing GPUInfo in mojo enables full support for hardware rendering. BUG=622707 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 ========== GpuInfo mojom struct and struct traits and unit test The mojom struct corresponds to gpu::GPUInfo. Serializing GPUInfo in mojo enables full support for hardware rendering. BUG=622707 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/bf90a8e911d48f0055bd2676460f48b8c669324b Cr-Commit-Position: refs/heads/master@{#411413} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/bf90a8e911d48f0055bd2676460f48b8c669324b Cr-Commit-Position: refs/heads/master@{#411413} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
