Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2147693002:
Struct traits used by GpuInfo (Closed)
|
Created:
4 years, 5 months ago by Alex Z. Modified:
4 years, 4 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStruct traits used by GpuInfo
This CL contains struct traits of the structs used by gpu::GPUInfo
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/a639771dc3abd8e8030757c3c04e26a37c86348c
Cr-Commit-Position: refs/heads/master@{#410053}
Patch Set 1 : base::Version struct traits #Patch Set 2 : "base::Version struct traits implementation" #Patch Set 3 : base::Version unit test #Patch Set 4 : Fixed unit test #Patch Set 5 : DxDiagNode #Patch Set 6 : gpu::VideoCodecProfile enum traits #Patch Set 7 : VideoDecodeAcceleratorSupportedProfile #Patch Set 8 : Format #Patch Set 9 : VideoDecodeAcceleratorCapabilities #Patch Set 10 : Removed unnecessary include #Patch Set 11 : VideoEncodeAcceleratorSupportedProfile #
Total comments: 8
Patch Set 12 : Changed according to comments #
Total comments: 20
Patch Set 13 : Changed according to comments #
Total comments: 1
Patch Set 14 : Fixed a typo #
Total comments: 14
Patch Set 15 : Changed according to comments #
Total comments: 2
Patch Set 16 : Construct base::Version from a vector of components #
Total comments: 2
Patch Set 17 : Moved DxDiagNode struct traits Read() to a cc file #
Total comments: 2
Patch Set 18 : Moved Read() to .cc file #
Total comments: 21
Patch Set 19 : Changed comment to be more readable #Patch Set 20 : Merge conflicts #Patch Set 21 : Merge conflicts #Patch Set 22 : Added //ui/gfx/geometry/mojo to public_deps of gpu/ipc/common:interfaces #
Total comments: 1
Patch Set 23 : The right public_deps #Messages
Total messages: 113 (63 generated)
Description was changed from ========== Version struct traits BUG=622707 ========== to ========== Version struct traits BUG=622707 ==========
Description was changed from ========== Version struct traits BUG=622707 ========== to ========== Version struct traits BUG=622707 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;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...
Your CL relies on deprecated CQ feature(s): * Specifying master names without "master." prefix is deprecated: tryserver.chromium.linux, tryserver.chromium.mac, tryserver.chromium.win. For more details, see http://crbug.com/617627.
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...
Your CL relies on deprecated CQ feature(s): * Specifying master names without "master." prefix is deprecated: tryserver.chromium.linux, tryserver.chromium.mac, tryserver.chromium.win. For more details, see http://crbug.com/617627.
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on 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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
staraz@chromium.org changed reviewers: + dcheng@chromium.org, fsamuel@chromium.org, sky@chromium.org
sky@: Please review the changes under mojo/common fsamuel@: Please review the changes in the struct traits files dcheng@: Please do a security review on the changes under gpu/ipc/
https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:64: return static_cast<gpu::mojom::VideoCodecProfile>(video_codec_profile); Use switch statement. https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:71: *out = static_cast<gpu::VideoCodecProfile>(input); use switch statement. https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.h (right): https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.h:66: static const gpu::VideoCodecProfile& profile( Just return by value. This is a small type. https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.h:108: static const gpu::VideoCodecProfile& profile( pass by value.
https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:64: return static_cast<gpu::mojom::VideoCodecProfile>(video_codec_profile); On 2016/07/15 18:08:38, Fady Samuel wrote: > Use switch statement. Done. https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:71: *out = static_cast<gpu::VideoCodecProfile>(input); On 2016/07/15 18:08:38, Fady Samuel wrote: > use switch statement. Done. https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.h (right): https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.h:66: static const gpu::VideoCodecProfile& profile( On 2016/07/15 18:08:38, Fady Samuel wrote: > Just return by value. This is a small type. Done. https://codereview.chromium.org/2147693002/diff/200001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.h:108: static const gpu::VideoCodecProfile& profile( On 2016/07/15 18:08:38, Fady Samuel wrote: > pass by value. Done.
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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
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...)
https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/dx_diag... File gpu/ipc/common/dx_diag_node.mojom (right): https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/dx_diag... gpu/ipc/common/dx_diag_node.mojom:7: struct DxDiagNode { nit: add comment to path to where the C++ version of this type is defined. https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/dx_diag... File gpu/ipc/common/dx_diag_node_struct_traits.h (right): https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/dx_diag... gpu/ipc/common/dx_diag_node_struct_traits.h:18: static const std::map<std::string, std::string> values( return reference. const std::map<std::string, std::string>& This code creates a whole copy of the map. https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/dx_diag... gpu/ipc/common/dx_diag_node_struct_traits.h:23: static const std::map<std::string, gpu::DxDiagNode> children( return reference: const std::map<std::string, std::string>& https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info.mojom (right): https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:28: VIDEO_CODEC_PROFILE_MIN = VIDEO_CODEC_PROFILE_UNKNOWN, Let's get rid of min and max. We don't need them in the wire format. They exist for validation but StructTraits will take care of that for us. https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:51: struct VideoDecodeAcceleratorSupportedProfile { nit: comment with path to C++ type. https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:58: struct VideoDecodeAcceleratorCapabilities { nit: comment with path to C++ type. https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:62: struct VideoEncodeAcceleratorSupportedProfile { nit: comment with path to C++ type. https://codereview.chromium.org/2147693002/diff/220001/mojo/common/common_cus... File mojo/common/common_custom_types_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/220001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.cc:15: std::string ConstructVersionString(std::vector<uint32_t> components) { const std::vector<uint32_t>& https://codereview.chromium.org/2147693002/diff/220001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.cc:29: std::vector<uint32_t> const std::vector<uint32_t>& https://codereview.chromium.org/2147693002/diff/220001/mojo/common/common_cus... File mojo/common/common_custom_types_struct_traits.h (right): https://codereview.chromium.org/2147693002/diff/220001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.h:18: static std::vector<uint32_t> components(const base::Version& version); const std::vector<uint32_t>&
sky@chromium.org changed reviewers: + rockot@chromium.org
sky->rockot
https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/dx_diag... File gpu/ipc/common/dx_diag_node.mojom (right): https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/dx_diag... gpu/ipc/common/dx_diag_node.mojom:7: struct DxDiagNode { On 2016/07/18 15:59:39, Fady Samuel wrote: > nit: add comment to path to where the C++ version of this type is defined. Done. https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/dx_diag... File gpu/ipc/common/dx_diag_node_struct_traits.h (right): https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/dx_diag... gpu/ipc/common/dx_diag_node_struct_traits.h:18: static const std::map<std::string, std::string> values( On 2016/07/18 15:59:39, Fady Samuel wrote: > return reference. > > const std::map<std::string, std::string>& > > This code creates a whole copy of the map. Done. https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/dx_diag... gpu/ipc/common/dx_diag_node_struct_traits.h:23: static const std::map<std::string, gpu::DxDiagNode> children( On 2016/07/18 15:59:39, Fady Samuel wrote: > return reference: > > const std::map<std::string, std::string>& Done. https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info.mojom (right): https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:28: VIDEO_CODEC_PROFILE_MIN = VIDEO_CODEC_PROFILE_UNKNOWN, On 2016/07/18 15:59:39, Fady Samuel wrote: > Let's get rid of min and max. We don't need them in the wire format. They exist > for validation but StructTraits will take care of that for us. Done. https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:51: struct VideoDecodeAcceleratorSupportedProfile { On 2016/07/18 15:59:39, Fady Samuel wrote: > nit: comment with path to C++ type. Done. https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:58: struct VideoDecodeAcceleratorCapabilities { On 2016/07/18 15:59:39, Fady Samuel wrote: > nit: comment with path to C++ type. Done. https://codereview.chromium.org/2147693002/diff/220001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:62: struct VideoEncodeAcceleratorSupportedProfile { On 2016/07/18 15:59:39, Fady Samuel wrote: > nit: comment with path to C++ type. Done. https://codereview.chromium.org/2147693002/diff/220001/mojo/common/common_cus... File mojo/common/common_custom_types_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/220001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.cc:15: std::string ConstructVersionString(std::vector<uint32_t> components) { On 2016/07/18 15:59:39, Fady Samuel wrote: > const std::vector<uint32_t>& Done. https://codereview.chromium.org/2147693002/diff/220001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.cc:29: std::vector<uint32_t> On 2016/07/18 15:59:39, Fady Samuel wrote: > const std::vector<uint32_t>& Done. https://codereview.chromium.org/2147693002/diff/220001/mojo/common/common_cus... File mojo/common/common_custom_types_struct_traits.h (right): https://codereview.chromium.org/2147693002/diff/220001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.h:18: static std::vector<uint32_t> components(const base::Version& version); On 2016/07/18 15:59:39, Fady Samuel wrote: > const std::vector<uint32_t>& Done.
https://codereview.chromium.org/2147693002/diff/240001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info.mojom (right): https://codereview.chromium.org/2147693002/diff/240001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:19: gpu::CollectInfoResult This isn't a comment? I don't think this compiles. https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:120: // VIDEO_CODEC_PROFILE_MIN = VIDEO_CODEC_PROFILE_UNKNOWN Delete this comment. https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:181: // VIDEO_CODEC_PROFILE_MAX = HEVCPROFILE_MAIN_STILL_PICTURE Just delete these. https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/struct_... File gpu/ipc/common/struct_traits_unittest.cc (right): https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/struct_... gpu/ipc/common/struct_traits_unittest.cc:219: uint32_t max_framerate_numerator = 144; const https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/struct_... gpu/ipc/common/struct_traits_unittest.cc:220: uint32_t max_framerate_denominator = 12; const https://codereview.chromium.org/2147693002/diff/260001/mojo/common/common_cus... File mojo/common/common_custom_types.mojom (right): https://codereview.chromium.org/2147693002/diff/260001/mojo/common/common_cus... mojo/common/common_custom_types.mojom:25: struct Version { Where is this from? https://codereview.chromium.org/2147693002/diff/260001/mojo/common/common_cus... File mojo/common/common_custom_types_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/260001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.cc:15: const std::string& ConstructVersionString(std::vector<uint32_t> components) { std::string. This can't be const reference. The input parameter should be const reference though.
https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/OWNERS File gpu/ipc/common/OWNERS (right): https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/OWNERS#... gpu/ipc/common/OWNERS:13: per-file *.mojom=set noparent Why is this being added again? This is already in the OWNERS file.
https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/OWNERS File gpu/ipc/common/OWNERS (right): https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/OWNERS#... gpu/ipc/common/OWNERS:13: per-file *.mojom=set noparent On 2016/07/19 16:48:47, Fady Samuel wrote: > Why is this being added again? This is already in the OWNERS file. It might have been added by git during a merge. I have removed it in the next patch set. https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:120: // VIDEO_CODEC_PROFILE_MIN = VIDEO_CODEC_PROFILE_UNKNOWN On 2016/07/19 16:47:40, Fady Samuel wrote: > Delete this comment. Done. https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:181: // VIDEO_CODEC_PROFILE_MAX = HEVCPROFILE_MAIN_STILL_PICTURE On 2016/07/19 16:47:40, Fady Samuel wrote: > Just delete these. Done. https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/struct_... File gpu/ipc/common/struct_traits_unittest.cc (right): https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/struct_... gpu/ipc/common/struct_traits_unittest.cc:219: uint32_t max_framerate_numerator = 144; On 2016/07/19 16:47:40, Fady Samuel wrote: > const Done. https://codereview.chromium.org/2147693002/diff/260001/gpu/ipc/common/struct_... gpu/ipc/common/struct_traits_unittest.cc:220: uint32_t max_framerate_denominator = 12; On 2016/07/19 16:47:40, Fady Samuel wrote: > const Done. https://codereview.chromium.org/2147693002/diff/260001/mojo/common/common_cus... File mojo/common/common_custom_types.mojom (right): https://codereview.chromium.org/2147693002/diff/260001/mojo/common/common_cus... mojo/common/common_custom_types.mojom:25: struct Version { On 2016/07/19 16:47:40, Fady Samuel wrote: > Where is this from? Done. src/base/version.h base::Version I have added the location of the class in the next patch set. https://codereview.chromium.org/2147693002/diff/260001/mojo/common/common_cus... File mojo/common/common_custom_types_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/260001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.cc:15: const std::string& ConstructVersionString(std::vector<uint32_t> components) { On 2016/07/19 16:47:40, Fady Samuel wrote: > std::string. This can't be const reference. > > The input parameter should be const reference though. Done.
https://codereview.chromium.org/2147693002/diff/280001/mojo/common/common_cus... File mojo/common/common_custom_types_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/280001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.cc:15: std::string ConstructVersionString(const std::vector<uint32_t>& components) { I just looked at base::Version's constructor. It seems really silly to construct a string from a vector only to convert it back to a vector. I would give base::Version a move-only constructor
https://codereview.chromium.org/2147693002/diff/280001/mojo/common/common_cus... File mojo/common/common_custom_types_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/280001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.cc:15: std::string ConstructVersionString(const std::vector<uint32_t>& components) { On 2016/07/19 17:33:03, Fady Samuel wrote: > I just looked at base::Version's constructor. It seems really silly to construct > a string from a vector only to convert it back to a vector. I would give > base::Version a move-only constructor Done.
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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
https://codereview.chromium.org/2147693002/diff/300001/gpu/ipc/common/dx_diag... File gpu/ipc/common/dx_diag_node_struct_traits.h (right): https://codereview.chromium.org/2147693002/diff/300001/gpu/ipc/common/dx_diag... gpu/ipc/common/dx_diag_node_struct_traits.h:14: static bool Read(gpu::mojom::DxDiagNodeDataView data, gpu::DxDiagNode* out) { Move this to a cc file?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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/2147693002/diff/300001/gpu/ipc/common/dx_diag... File gpu/ipc/common/dx_diag_node_struct_traits.h (right): https://codereview.chromium.org/2147693002/diff/300001/gpu/ipc/common/dx_diag... gpu/ipc/common/dx_diag_node_struct_traits.h:14: static bool Read(gpu::mojom::DxDiagNodeDataView data, gpu::DxDiagNode* out) { On 2016/07/26 14:43:41, Fady Samuel wrote: > Move this to a cc file? Done.
lgtm once my comment is addressed. https://codereview.chromium.org/2147693002/diff/320001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.h (right): https://codereview.chromium.org/2147693002/diff/320001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.h:90: static bool Read(gpu::mojom::VideoDecodeAcceleratorCapabilitiesDataView data, Move to cc file. We generally move all Read methods (which tend to be non-trivial) to cc files. This one is trivial but we should probably move it there for consistency.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2147693002/diff/320001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.h (right): https://codereview.chromium.org/2147693002/diff/320001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.h:90: static bool Read(gpu::mojom::VideoDecodeAcceleratorCapabilitiesDataView data, On 2016/07/26 15:31:56, Fady Samuel wrote: > Move to cc file. We generally move all Read methods (which tend to be > non-trivial) to cc files. This one is trivial but we should probably move it > there for consistency. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Description was changed from ========== Version struct traits BUG=622707 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Version struct traits BUG=622707 ==========
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... File mojo/common/common_custom_types.mojom (right): https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... mojo/common/common_custom_types.mojom:26: // base::Version These comments aren't terribly useful as-is. Maybe: // Corresponds to |base::Version| in base/version.h. https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... File mojo/common/common_custom_types.typemap (right): https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... mojo/common/common_custom_types.typemap:16: sources = [ Do not add sources to typemaps. Please create a new source_set target like //mojo/common:common_custom_struct_traits and add the cc and h files to its sources. Then add that target to public_deps below. https://codereview.chromium.org/2147693002/diff/340001/mojo/common/traits_tes... File mojo/common/traits_test_service.mojom (right): https://codereview.chromium.org/2147693002/diff/340001/mojo/common/traits_tes... mojo/common/traits_test_service.mojom:11: interface TraitsTestService { nit: TraitsTest (it's not a service)
Also please make the CL description more meaningful
https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info.mojom (right): https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:29: VIDEO_CODEC_PROFILE_UNKNOWN = -1, Nit: Just call this UNKNOWN, mojo defaults to generating enum classes. https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:30: H264PROFILE_BASELINE = 0, Similar I'd just omit PROFILE from these names. https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:65: case gpu::VideoCodecProfile::VIDEO_CODEC_PROFILE_UNKNOWN: Oh I guess it's for consistency with the existing enums... I'd still suggest doing that cleanup separately to reduce the amount of typing, but it's up to you =) https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:189: data.ReadMinResolution(&out->min_resolution); Will anything interesting happen if min resolution > max resolution? Or is this purely for diagnostics? https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:207: out->max_framerate_denominator = data.max_framerate_denominator(); Are there any interesting checks to perform here? For example, is the numerator always supposed to be a multiple of the denominator? (I'm not familiar with the terminology here, so hopefully it's not a stupid question) https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... File mojo/common/common_custom_types_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.cc:17: return version.components(); Assuming this isn't doing any implicit conversions, this is ok to inline (it will probably be less code that way)
Description was changed from ========== Version struct traits BUG=622707 ========== to ========== Version struct traits 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 ==========
https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info.mojom (right): https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:29: VIDEO_CODEC_PROFILE_UNKNOWN = -1, On 2016/07/28 02:09:38, dcheng (OOO Aug 1 - Aug 11) wrote: > Nit: Just call this UNKNOWN, mojo defaults to generating enum classes. The long name is to keep the consistency with the existing enums. https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info.mojom:30: H264PROFILE_BASELINE = 0, On 2016/07/28 02:09:38, dcheng (OOO Aug 1 - Aug 11) wrote: > Similar I'd just omit PROFILE from these names. Same as above. https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:65: case gpu::VideoCodecProfile::VIDEO_CODEC_PROFILE_UNKNOWN: On 2016/07/28 02:09:39, dcheng (OOO Aug 1 - Aug 11) wrote: > Oh I guess it's for consistency with the existing enums... > > I'd still suggest doing that cleanup separately to reduce the amount of typing, > but it's up to you =) I'll do the cleanup in a separate CL. For now let's just keep it consistent with the existing enums. https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:189: data.ReadMinResolution(&out->min_resolution); On 2016/07/28 02:09:39, dcheng (OOO Aug 1 - Aug 11) wrote: > Will anything interesting happen if min resolution > max resolution? Or is this > purely for diagnostics? There isn't any validation in the existing ParamTraits for this struct, so we at least don't have to add validation here at the moment. It is possible that validation is done elsewhere. https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:207: out->max_framerate_denominator = data.max_framerate_denominator(); On 2016/07/28 02:09:38, dcheng (OOO Aug 1 - Aug 11) wrote: > Are there any interesting checks to perform here? For example, is the numerator > always supposed to be a multiple of the denominator? > > (I'm not familiar with the terminology here, so hopefully it's not a stupid > question) Same as above. https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... File mojo/common/common_custom_types.mojom (right): https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... mojo/common/common_custom_types.mojom:26: // base::Version On 2016/07/27 20:05:25, Ken Rockot wrote: > These comments aren't terribly useful as-is. Maybe: > > // Corresponds to |base::Version| in base/version.h. > Done. https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... File mojo/common/common_custom_types.typemap (right): https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... mojo/common/common_custom_types.typemap:16: sources = [ On 2016/07/27 20:05:25, Ken Rockot wrote: > Do not add sources to typemaps. > > Please create a new source_set target like > //mojo/common:common_custom_struct_traits and add the cc and h files to its > sources. Then add that target to public_deps below. I have found sources in other typemap files. For example, https://cs.chromium.org/chromium/src/cc/ipc/quads.typemap?q=_struct_traits.cc.... Is there any particular reason why sources shouldn't be in typemaps? https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... File mojo/common/common_custom_types_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... mojo/common/common_custom_types_struct_traits.cc:17: return version.components(); On 2016/07/28 02:09:39, dcheng (OOO Aug 1 - Aug 11) wrote: > Assuming this isn't doing any implicit conversions, this is ok to inline (it > will probably be less code that way) Done. https://codereview.chromium.org/2147693002/diff/340001/mojo/common/traits_tes... File mojo/common/traits_test_service.mojom (right): https://codereview.chromium.org/2147693002/diff/340001/mojo/common/traits_tes... mojo/common/traits_test_service.mojom:11: interface TraitsTestService { On 2016/07/27 20:05:25, Ken Rockot wrote: > nit: TraitsTest (it's not a service) Done.
lgtm % more verbose CL description https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... File mojo/common/common_custom_types.typemap (right): https://codereview.chromium.org/2147693002/diff/340001/mojo/common/common_cus... mojo/common/common_custom_types.typemap:16: sources = [ On 2016/07/28 at 19:40:44, StarAZ1 wrote: > On 2016/07/27 20:05:25, Ken Rockot wrote: > > Do not add sources to typemaps. > > > > Please create a new source_set target like > > //mojo/common:common_custom_struct_traits and add the cc and h files to its > > sources. Then add that target to public_deps below. > > I have found sources in other typemap files. For example, https://cs.chromium.org/chromium/src/cc/ipc/quads.typemap?q=_struct_traits.cc.... > Is there any particular reason why sources shouldn't be in typemaps? Hmm. I didn't think we supported this, but mojom.gni clearly does read sources from the typemap. Nevermind :)
https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:189: data.ReadMinResolution(&out->min_resolution); On 2016/07/28 19:40:43, StarAZ1 wrote: > On 2016/07/28 02:09:39, dcheng (OOO Aug 1 - Aug 11) wrote: > > Will anything interesting happen if min resolution > max resolution? Or is > this > > purely for diagnostics? > > There isn't any validation in the existing ParamTraits for this struct, so we at > least don't have to add validation here at the moment. It is possible that > validation is done elsewhere. Right: what I'm trying to understand, though, is if we do need validation. How will the new mojo struct be used? Is there a followup CL that uses this work?
Description was changed from ========== Version struct traits 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 ========== Struct traits used by GpuInfo This CL contains struct traits of the structs used by gpu::GPUInfo 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 ==========
https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... File gpu/ipc/common/gpu_info_struct_traits.cc (right): https://codereview.chromium.org/2147693002/diff/340001/gpu/ipc/common/gpu_inf... gpu/ipc/common/gpu_info_struct_traits.cc:189: data.ReadMinResolution(&out->min_resolution); On 2016/07/29 04:05:39, dcheng (OOO Aug 1 - Aug 11) wrote: > On 2016/07/28 19:40:43, StarAZ1 wrote: > > On 2016/07/28 02:09:39, dcheng (OOO Aug 1 - Aug 11) wrote: > > > Will anything interesting happen if min resolution > max resolution? Or is > > this > > > purely for diagnostics? > > > > There isn't any validation in the existing ParamTraits for this struct, so we > at > > least don't have to add validation here at the moment. It is possible that > > validation is done elsewhere. > > Right: what I'm trying to understand, though, is if we do need validation. How > will the new mojo struct be used? Is there a followup CL that uses this work? The corresponding structs are used by gpu::GPUInfo. I will upload another CL implementing struct traits for gpu::GPUInfo. And maybe more validations after that.
LGTM, since we already have an IPC version. As we implement the Mojo side of things, let's keep an eye out for if there are good things to validate down the road. THanks!
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/2147693002/#ps360001 (title: "Changed comment to be more readable")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by staraz@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) 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_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) 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
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, dcheng@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2147693002/#ps380001 (title: "Merge conflicts")
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 staraz@chromium.org
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, dcheng@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2147693002/#ps400001 (title: "Merge conflicts")
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
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, dcheng@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2147693002/#ps420001 (title: "Added //ui/gfx/geometry/mojo to public_deps of gpu/ipc/common:interfaces")
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
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2147693002/diff/420001/gpu/ipc/common/BUILD.gn File gpu/ipc/common/BUILD.gn (right): https://codereview.chromium.org/2147693002/diff/420001/gpu/ipc/common/BUILD.g... gpu/ipc/common/BUILD.gn:141: "//ui/gfx/geometry/mojo", wrong target. this is test_interfaces :)
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, dcheng@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2147693002/#ps440001 (title: "The right public_deps")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by staraz@chromium.org
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
Try jobs failed on following builders: 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
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.
Description was changed from ========== Struct traits used by GpuInfo This CL contains struct traits of the structs used by gpu::GPUInfo 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 ========== Struct traits used by GpuInfo This CL contains struct traits of the structs used by gpu::GPUInfo 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 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Struct traits used by GpuInfo This CL contains struct traits of the structs used by gpu::GPUInfo 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 ========== Struct traits used by GpuInfo This CL contains struct traits of the structs used by gpu::GPUInfo 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/a639771dc3abd8e8030757c3c04e26a37c86348c Cr-Commit-Position: refs/heads/master@{#410053} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/a639771dc3abd8e8030757c3c04e26a37c86348c Cr-Commit-Position: refs/heads/master@{#410053} |
