Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2636073002:
Create mojom and StructTraits for ui/display/types/display_mode.cc (Closed)
DescriptionCreate mojom and StructTraits for ui/display/types/display_mode.cc
BUG=680992
Review-Url: https://codereview.chromium.org/2636073002
Cr-Commit-Position: refs/heads/master@{#447394}
Committed: https://chromium.googlesource.com/chromium/src/+/bd13a9f7838046525e55350a7e491e014235aea1
Patch Set 1 #
Total comments: 8
Patch Set 2 : Clean up. use IPC mojom struct defined in display_mode_proxy.mojom. #Patch Set 3 : fix build target/deps #Patch Set 4 : clean up build.gn, assign out to deserialized values. #Patch Set 5 : build breaks #Patch Set 6 : convert mojo type to ui::DisplayMode_Params instead. #
Total comments: 38
Patch Set 7 : Refactor code. Add includes. Rename to add other struct traits in the future. #Patch Set 8 : undo deletion of includes in ui/ozone/common/BUILD.gn #
Total comments: 16
Patch Set 9 : Refactor file names/ build targets #Patch Set 10 : fix comment #
Total comments: 2
Patch Set 11 : Move /ui/ozone/common/mojo/ to /ui/display/types/mojo/ and Add DEP so including ozone_gpu_message_p… #Patch Set 12 : remove ui/display/types/mojo/DEPS #Patch Set 13 : Remove /ui/ozone/common dependencies. #Patch Set 14 : Dont need to make changes in ui/ozone/common/BUILD.gn. #
Total comments: 28
Patch Set 15 : add security owner for *.typemap #Patch Set 16 : Change structure not to use display_mode_mojo.(cc|h). Instead, use friend class in display_mode.h. #Patch Set 17 : Change structure not to use display_mode_mojo.(cc|h). Instead, use friend class in display_mode.h. #
Total comments: 15
Patch Set 18 : Fix format/dependecies/nits. #Patch Set 19 : add dep on mojo/public/cpp/bindings:struct_traits #
Total comments: 1
Patch Set 20 : add spaces for easy reading. #Patch Set 21 : Merge with master after cl 2640243004 landed. #Patch Set 22 : size_ is initialized by default. #
Total comments: 8
Patch Set 23 : use unique pointer for struct trait mojom #
Total comments: 2
Patch Set 24 : remove expect_true in void function #Messages
Total messages: 112 (80 generated)
Description was changed from ========== Create mojom, struct traits and unit test for display_mode.cc. BUG=680992 ========== to ========== Create mojom and StructTraits for ui/display/types/display_mode.cc BUG=680992 ==========
thanhph@google.com changed reviewers: + kylechar@chromium.org, thanhph@google.com
https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/gpu/ozone_g... File ui/ozone/common/gpu/ozone_gpu_message_params.h (left): https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/gpu/ozone_g... ui/ozone/common/gpu/ozone_gpu_message_params.h:24: DisplayMode_Params(); Not sure why you removed these but you didn't remove the implementation in the cc file. This code won't compile, meaning you're probably running an old version of the code and thus why the results don't match what you expect. https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/mojo/BUILD.gn File ui/ozone/common/mojo/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/mojo/BUILD.... ui/ozone/common/mojo/BUILD.gn:27: source_set("unit_test") { This target shouldn't exist. You added the test ozone_unittests so that is what you'll build and run. https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/mojo/BUILD.... ui/ozone/common/mojo/BUILD.gn:42: source_set("struct_traits") { This target shouldn't exist. https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/mojo/displa... File ui/ozone/common/mojo/display_mode_params_struct_traits.h (right): https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/mojo/displa... ui/ozone/common/mojo/display_mode_params_struct_traits.h:18: You're not setting is_interlaced or refresh_rate on |out|.
https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/gpu/ozone_g... File ui/ozone/common/gpu/ozone_gpu_message_params.h (left): https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/gpu/ozone_g... ui/ozone/common/gpu/ozone_gpu_message_params.h:24: DisplayMode_Params(); On 2017/01/16 23:08:24, kylechar wrote: > Not sure why you removed these but you didn't remove the implementation in the > cc file. This code won't compile, meaning you're probably running an old version > of the code and thus why the results don't match what you expect. I put this back. Thanks! https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/mojo/BUILD.gn File ui/ozone/common/mojo/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/mojo/BUILD.... ui/ozone/common/mojo/BUILD.gn:27: source_set("unit_test") { On 2017/01/16 23:08:24, kylechar wrote: > This target shouldn't exist. You added the test ozone_unittests so that is what > you'll build and run. Removed! https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/mojo/BUILD.... ui/ozone/common/mojo/BUILD.gn:42: source_set("struct_traits") { On 2017/01/16 23:08:24, kylechar wrote: > This target shouldn't exist. Removed! https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/mojo/displa... File ui/ozone/common/mojo/display_mode_params_struct_traits.h (right): https://codereview.chromium.org/2636073002/diff/1/ui/ozone/common/mojo/displa... ui/ozone/common/mojo/display_mode_params_struct_traits.h:18: On 2017/01/16 23:08:24, kylechar wrote: > You're not setting is_interlaced or refresh_rate on |out|. Done, thanks!
The CQ bit was checked by thanhph@google.com 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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
kylechar@chromium.org changed reviewers: - thanhph@google.com
https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/BUILD.gn File ui/ozone/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/BUILD.gn#newc... ui/ozone/BUILD.gn:234: test("ozone_struct_traits_unittests") { The test target should probably be in ui/ozone/common/mojo/BUILD.gn https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/B... File ui/ozone/common/mojo/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/B... ui/ozone/common/mojo/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/B... ui/ozone/common/mojo/BUILD.gn:7: mojom("ui_ozone_common_mojom_display_mode_proxy") { This name is too long and not accurate. You'll be building StructTraits for multiple classes and you don't need the full hierarchy. The name "interfaces" would probably be best since it's widely used elsewhere. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/B... ui/ozone/common/mojo/BUILD.gn:17: mojom("test_ui_ozone_common_mojom_display_mode_proxy") { mojom("test_interfaces") https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... File ui/ozone/common/mojo/display_mode_proxy.mojom (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy.mojom:1: module ui.mojom; You are missing a copyright string. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy.mojom:6: struct DisplayModeProxy { This struct should be named DisplayModeParams. Also, the filename should change. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... File ui/ozone/common/mojo/display_mode_proxy.typemap (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy.typemap:1: # Copyright 2016 The Chromium Authors. All rights reserved. 2017 (update all the new files to 2017). https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy.typemap:7: traits_headers = [ "//ui/ozone/common/mojo/display_mode_proxy_struct_traits.h" ] You are missing a public_deps on //ui/ozone/common. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... File ui/ozone/common/mojo/display_mode_proxy_struct_traits.h (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits.h:8: #include "base/logging.h" I don't think this is used. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits.h:10: You have no #include that contains mojom::DisplayModeProxyDataView or for the gfx::Size StructTrait definition. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits.h:16: static const gfx::Size& size(const ui::DisplayMode_Params& d) { I would call the parameter |mode| or |display_mode| for clarity, here and in the two methods below. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits.h:18: } Whitespace between methods. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits.h:28: if (data.is_null()) You shouldn't need this check. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... File ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc:9: #include "ui/ozone/common/mojo/display_mode_proxy_test.mojom.h" Missing #include for ui::DisplayMode_Params. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc:13: class DisplayModeProxyTestImpl : public mojom::DisplayModeProxyTest { You'll want to put the DisplaySnapshotParams test in here too. So I would recommend you change the name of the test class and test interface to something more general. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc:40: gfx::Size size(15, 29); If you aren't going to use these again, there is no point in defining the variables instead of just setting input directly. If you are going to compare against them, make them const. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc:55: EXPECT_NE(&input, &output); // Make sure they aren't the same object. You know what input and output aren't the same object here, since they are both defined in this function. If this was a separate function where both input and output were passed in that would be different. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc:56: EXPECT_EQ(input.size.width(), output.size.width()); gfx::Size has == defined, so you can compare them directly. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... File ui/ozone/common/mojo/display_mode_proxy_test.mojom (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_test.mojom:9: interface DisplayModeProxyTest { You'll want to use the same test interface for DisplaySnapshotParams and maybe others, so the name should be more general. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_test.mojom:11: BounceDisplayModeProxy(DisplayModeProxy in) => (DisplayModeProxy out); This should be EchoDisplayModeParams. It looks like one test uses "BounceStructName" but many tests uses "EchoStructName", so go with the more common usage.
thanhph@google.com changed reviewers: + thanhph@google.com
https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/BUILD.gn File ui/ozone/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/BUILD.gn#newc... ui/ozone/BUILD.gn:234: test("ozone_struct_traits_unittests") { On 2017/01/17 17:19:26, kylechar wrote: > The test target should probably be in ui/ozone/common/mojo/BUILD.gn Done. I moved this test in ui/ozone/common/mojo/BUILD.gn. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/B... File ui/ozone/common/mojo/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/B... ui/ozone/common/mojo/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/17 17:19:26, kylechar wrote: > 2017 Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/B... ui/ozone/common/mojo/BUILD.gn:7: mojom("ui_ozone_common_mojom_display_mode_proxy") { On 2017/01/17 17:19:26, kylechar wrote: > This name is too long and not accurate. You'll be building StructTraits for > multiple classes and you don't need the full hierarchy. > > The name "interfaces" would probably be best since it's widely used elsewhere. Done, thanks! https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/B... ui/ozone/common/mojo/BUILD.gn:17: mojom("test_ui_ozone_common_mojom_display_mode_proxy") { On 2017/01/17 17:19:26, kylechar wrote: > mojom("test_interfaces") Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... File ui/ozone/common/mojo/display_mode_proxy.mojom (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy.mojom:1: module ui.mojom; On 2017/01/17 17:19:26, kylechar wrote: > You are missing a copyright string. Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy.mojom:6: struct DisplayModeProxy { On 2017/01/17 17:19:26, kylechar wrote: > This struct should be named DisplayModeParams. Also, the filename should change. Done, I also renamed related files/ dependencies. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... File ui/ozone/common/mojo/display_mode_proxy.typemap (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy.typemap:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/17 17:19:26, kylechar wrote: > 2017 (update all the new files to 2017). Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy.typemap:7: traits_headers = [ "//ui/ozone/common/mojo/display_mode_proxy_struct_traits.h" ] On 2017/01/17 17:19:26, kylechar wrote: > You are missing a public_deps on //ui/ozone/common. Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... File ui/ozone/common/mojo/display_mode_proxy_struct_traits.h (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits.h:8: #include "base/logging.h" On 2017/01/17 17:19:26, kylechar wrote: > I don't think this is used. Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits.h:10: On 2017/01/17 17:19:27, kylechar wrote: > You have no #include that contains mojom::DisplayModeProxyDataView or for the > gfx::Size StructTrait definition. Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits.h:16: static const gfx::Size& size(const ui::DisplayMode_Params& d) { On 2017/01/17 17:19:26, kylechar wrote: > I would call the parameter |mode| or |display_mode| for clarity, here and in the > two methods below. Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits.h:18: } On 2017/01/17 17:19:26, kylechar wrote: > Whitespace between methods. Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits.h:28: if (data.is_null()) On 2017/01/17 17:19:27, kylechar wrote: > You shouldn't need this check. Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... File ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc (right): https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc:9: #include "ui/ozone/common/mojo/display_mode_proxy_test.mojom.h" On 2017/01/17 17:19:27, kylechar wrote: > Missing #include for ui::DisplayMode_Params. Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc:13: class DisplayModeProxyTestImpl : public mojom::DisplayModeProxyTest { On 2017/01/17 17:19:27, kylechar wrote: > You'll want to put the DisplaySnapshotParams test in here too. So I would > recommend you change the name of the test class and test interface to something > more general. Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc:40: gfx::Size size(15, 29); On 2017/01/17 17:19:27, kylechar wrote: > If you aren't going to use these again, there is no point in defining the > variables instead of just setting input directly. If you are going to compare > against them, make them const. Done. https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc:55: EXPECT_NE(&input, &output); // Make sure they aren't the same object. On 2017/01/17 17:19:27, kylechar wrote: > You know what input and output aren't the same object here, since they are both > defined in this function. If this was a separate function where both input and > output were passed in that would be different. Done, thanks! https://codereview.chromium.org/2636073002/diff/100001/ui/ozone/common/mojo/d... ui/ozone/common/mojo/display_mode_proxy_struct_traits_unittest.cc:56: EXPECT_EQ(input.size.width(), output.size.width()); On 2017/01/17 17:19:27, kylechar wrote: > gfx::Size has == defined, so you can compare them directly. Done.
The CQ bit was checked by thanhph@google.com 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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by thanhph@google.com 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...
https://codereview.chromium.org/2636073002/diff/80002/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/80002/BUILD.gn#newcode381 BUILD.gn:381: "//ui/ozone/common/mojo:ozone_struct_traits_unittests", Only include this target. It has a dependency on the other two targets. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/BUILD.gn File ui/ozone/common/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/BUILD.g... ui/ozone/common/BUILD.gn:38: "//ui/ozone/common/*", //ui/ozone/common/mojo/* https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/BU... File ui/ozone/common/mojo/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/BU... ui/ozone/common/mojo/BUILD.gn:20: "ozone_gpu_message_params_test.mojom", ozone_gpu_struct_traits_test.mojo, also change the interface name to match. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/BU... ui/ozone/common/mojo/BUILD.gn:28: test("ozone_struct_traits_unittests") { ozone_gpu_struct_traits_unittests, also change the interface name to match. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/BU... ui/ozone/common/mojo/BUILD.gn:32: "//ui/ozone/common/mojo/ozone_gpu_messages_params_struct_traits_unittest.cc", ozone_gpu_struct_traits_unittest.cc https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/oz... File ui/ozone/common/mojo/ozone_gpu_message_params_test.mojom (right): https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/oz... ui/ozone/common/mojo/ozone_gpu_message_params_test.mojom:11: BounceDisplayModeParams(DisplayModeParams in) => (DisplayModeParams out); Change name to EchoDisplayModeParams. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/oz... File ui/ozone/common/mojo/ozone_gpu_messages_params_struct_traits_unittest.cc (right): https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/oz... ui/ozone/common/mojo/ozone_gpu_messages_params_struct_traits_unittest.cc:14: class OzoneGpuMessageParamsTestImpl : public mojom::OzoneGpuMessageParamsTest { You can put this class in an anonymous namespace. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/oz... ui/ozone/common/mojo/ozone_gpu_messages_params_struct_traits_unittest.cc:33: TEST(MojoDisplayModeStructTraitsTest, Basic) { TEST(OzoneGpuStructTraitsTest, DisplayMode) {
Oh also, you aren't adding the test target to any trybots, so it won't be run.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
As discussed, we can add unit test to the try bot in the next cl. https://codereview.chromium.org/2636073002/diff/80002/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/80002/BUILD.gn#newcode381 BUILD.gn:381: "//ui/ozone/common/mojo:ozone_struct_traits_unittests", On 2017/01/17 21:39:46, kylechar wrote: > Only include this target. It has a dependency on the other two targets. Done. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/BUILD.gn File ui/ozone/common/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/BUILD.g... ui/ozone/common/BUILD.gn:38: "//ui/ozone/common/*", On 2017/01/17 21:39:46, kylechar wrote: > //ui/ozone/common/mojo/* Done. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/BU... File ui/ozone/common/mojo/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/BU... ui/ozone/common/mojo/BUILD.gn:20: "ozone_gpu_message_params_test.mojom", On 2017/01/17 21:39:46, kylechar wrote: > ozone_gpu_struct_traits_test.mojo, also change the interface name to match. Done. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/BU... ui/ozone/common/mojo/BUILD.gn:28: test("ozone_struct_traits_unittests") { On 2017/01/17 21:39:46, kylechar wrote: > ozone_gpu_struct_traits_unittests, also change the interface name to match. Done. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/BU... ui/ozone/common/mojo/BUILD.gn:32: "//ui/ozone/common/mojo/ozone_gpu_messages_params_struct_traits_unittest.cc", On 2017/01/17 21:39:46, kylechar wrote: > ozone_gpu_struct_traits_unittest.cc Done. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/oz... File ui/ozone/common/mojo/ozone_gpu_message_params_test.mojom (right): https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/oz... ui/ozone/common/mojo/ozone_gpu_message_params_test.mojom:11: BounceDisplayModeParams(DisplayModeParams in) => (DisplayModeParams out); On 2017/01/17 21:39:46, kylechar wrote: > Change name to EchoDisplayModeParams. Done. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/oz... File ui/ozone/common/mojo/ozone_gpu_messages_params_struct_traits_unittest.cc (right): https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/oz... ui/ozone/common/mojo/ozone_gpu_messages_params_struct_traits_unittest.cc:14: class OzoneGpuMessageParamsTestImpl : public mojom::OzoneGpuMessageParamsTest { On 2017/01/17 21:39:46, kylechar wrote: > You can put this class in an anonymous namespace. Done. https://codereview.chromium.org/2636073002/diff/80002/ui/ozone/common/mojo/oz... ui/ozone/common/mojo/ozone_gpu_messages_params_struct_traits_unittest.cc:33: TEST(MojoDisplayModeStructTraitsTest, Basic) { On 2017/01/17 21:39:46, kylechar wrote: > TEST(OzoneGpuStructTraitsTest, DisplayMode) { Done.
The CQ bit was checked by thanhph@google.com 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.
kylechar@chromium.org changed reviewers: + rjkroege@chromium.org - thanhph@google.com
lgtm.
thanhph@google.com changed reviewers: + thanhph@google.com
@tsepez: Please review my mojom struct trait files for security.
thanhph@google.com changed reviewers: + jam@chromium.org - kylechar@chromium.org, rjkroege@chromium.org
jam@chromium.org: Hi Jon, Please review changes in BUILD.gn mojo/public/tools/bindings/chromium_bindings_configuration.gni Thanks, Thanh.
tsepez@chromium.org changed reviewers: + tsepez@chromium.org
lgtm
thanhph@google.com changed reviewers: + kylechar@chromium.org, rjkroege@chromium.org
+kylechar, +rjkroege: I accidentally removed your names. @rjkroege: Please review my cl.
I don't think this code is in the right place. https://codereview.chromium.org/2636073002/diff/170001/ui/ozone/common/mojo/B... File ui/ozone/common/mojo/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/170001/ui/ozone/common/mojo/B... ui/ozone/common/mojo/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. Why is this not in ui/ozone/public/interfaces? Or ui/display somewhere? Regardless: it does not belong in ozone/common as that is for code common to all ozone platforms but internal to the ozone implementation. And these mojom are part of an external interface to ozone. https://codereview.chromium.org/2636073002/diff/170001/ui/ozone/common/mojo/DEPS File ui/ozone/common/mojo/DEPS (right): https://codereview.chromium.org/2636073002/diff/170001/ui/ozone/common/mojo/D... ui/ozone/common/mojo/DEPS:2: "+mojo/public/cpp", per previous comment: this seems like a layering violation
per email thread: I think that these files belong in ui/display/type/mojo. On 2017/01/20 18:56:13, rjkroege wrote: > I don't think this code is in the right place. > > https://codereview.chromium.org/2636073002/diff/170001/ui/ozone/common/mojo/B... > File ui/ozone/common/mojo/BUILD.gn (right): > > https://codereview.chromium.org/2636073002/diff/170001/ui/ozone/common/mojo/B... > ui/ozone/common/mojo/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All > rights reserved. > Why is this not in ui/ozone/public/interfaces? Or ui/display somewhere? > > Regardless: it does not belong in ozone/common as that is for code common to all > ozone platforms but internal to the ozone implementation. And these mojom are > part of an external interface to ozone. > > https://codereview.chromium.org/2636073002/diff/170001/ui/ozone/common/mojo/DEPS > File ui/ozone/common/mojo/DEPS (right): > > https://codereview.chromium.org/2636073002/diff/170001/ui/ozone/common/mojo/D... > ui/ozone/common/mojo/DEPS:2: "+mojo/public/cpp", > per previous comment: this seems like a layering violation
The CQ bit was checked by thanhph@google.com 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 thanhph@google.com
The CQ bit was checked by thanhph@google.com 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.
The CQ bit was checked by thanhph@google.com 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: 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 thanhph@google.com 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...
Based on the change of plan not lgtm. Also can you fix the CL description. https://codereview.chromium.org/2636073002/diff/250001/ui/display/BUILD.gn File ui/display/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/BUILD.gn#ne... ui/display/BUILD.gn:33: "mojo/display_mode_mojo.cc", Move the files to ui/display/ instead. https://codereview.chromium.org/2636073002/diff/250001/ui/display/BUILD.gn#ne... ui/display/BUILD.gn:165: "//base/test:run_all_unittests", Remove this since you added another target with a main(). https://codereview.chromium.org/2636073002/diff/250001/ui/display/BUILD.gn#ne... ui/display/BUILD.gn:171: "//ui/display/mojo:interfaces", Remove this, it's a public dep of test_interfaces. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/BUILD.gn File ui/display/mojo/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/BUILD.... ui/display/mojo/BUILD.gn:6: import("//testing/test.gni") Remove unused import. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... File ui/display/mojo/display_mode_mojo.cc (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.cc:10: DisplayModeMojo::DisplayModeMojo(): DisplayMode(gfx::Size(), false, 0.0) {}; No ; after. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... File ui/display/mojo/display_mode_mojo.h (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.h:5: #ifndef UI_DISPLAY_TYPES_MOJO_DISPLAY_MODE_MOJO_H_ UI_DISPLAY_DISPLAY_MODE_MOJO_H_ https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.h:13: //Move-only class. This comment isn't helpful. It would be better to explain why this implementation of DisplayMode exists. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.h:15: public: You will also need a constructor that lets you set size, is_interlaced and refresh_rate. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.h:18: DisplayModeMojo(DisplayModeMojo &&) = default; Move to cc file. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.h:19: DisplayModeMojo& operator=(DisplayModeMojo &&) = default; Move to cc file. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.h:20: }; Missing DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:46: input.set_refresh_rate(61); 61.0f https://codereview.chromium.org/2636073002/diff/250001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/types/displ... ui/display/types/display_mode.h:23: DisplayMode(DisplayMode&&) = default; Move to cc file. https://codereview.chromium.org/2636073002/diff/250001/ui/display/types/displ... ui/display/types/display_mode.h:24: DisplayMode& operator=(DisplayMode&&) = default; Move to cc file. https://codereview.chromium.org/2636073002/diff/250001/ui/display/types/displ... ui/display/types/display_mode.h:29: void set_size(const gfx::Size& size) { We really don't want to make DisplayMode mutable. Can you think of any other ways to do this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2636073002/diff/250001/ui/display/BUILD.gn File ui/display/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/BUILD.gn#ne... ui/display/BUILD.gn:33: "mojo/display_mode_mojo.cc", On 2017/01/24 21:15:07, kylechar wrote: > Move the files to ui/display/ instead. Done, thanks! https://codereview.chromium.org/2636073002/diff/250001/ui/display/BUILD.gn#ne... ui/display/BUILD.gn:165: "//base/test:run_all_unittests", On 2017/01/24 21:15:07, kylechar wrote: > Remove this since you added another target with a main(). Done, thanks! https://codereview.chromium.org/2636073002/diff/250001/ui/display/BUILD.gn#ne... ui/display/BUILD.gn:171: "//ui/display/mojo:interfaces", On 2017/01/24 21:15:07, kylechar wrote: > Remove this, it's a public dep of test_interfaces. Done. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/BUILD.gn File ui/display/mojo/BUILD.gn (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/BUILD.... ui/display/mojo/BUILD.gn:6: import("//testing/test.gni") On 2017/01/24 21:15:07, kylechar wrote: > Remove unused import. Done. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... File ui/display/mojo/display_mode_mojo.cc (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.cc:10: DisplayModeMojo::DisplayModeMojo(): DisplayMode(gfx::Size(), false, 0.0) {}; On 2017/01/24 21:15:07, kylechar wrote: > No ; after. Done. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... File ui/display/mojo/display_mode_mojo.h (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.h:5: #ifndef UI_DISPLAY_TYPES_MOJO_DISPLAY_MODE_MOJO_H_ On 2017/01/24 21:15:07, kylechar wrote: > UI_DISPLAY_DISPLAY_MODE_MOJO_H_ Done. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.h:13: //Move-only class. On 2017/01/24 21:15:07, kylechar wrote: > This comment isn't helpful. It would be better to explain why this > implementation of DisplayMode exists. Done. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.h:15: public: On 2017/01/24 21:15:07, kylechar wrote: > You will also need a constructor that lets you set size, is_interlaced and > refresh_rate. Done. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_mode_mojo.h:18: DisplayModeMojo(DisplayModeMojo &&) = default; On 2017/01/24 21:15:07, kylechar wrote: > Move to cc file. Done. https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:46: input.set_refresh_rate(61); On 2017/01/24 21:15:07, kylechar wrote: > 61.0f Done. https://codereview.chromium.org/2636073002/diff/250001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2636073002/diff/250001/ui/display/types/displ... ui/display/types/display_mode.h:23: DisplayMode(DisplayMode&&) = default; On 2017/01/24 21:15:07, kylechar wrote: > Move to cc file. Done. https://codereview.chromium.org/2636073002/diff/250001/ui/display/types/displ... ui/display/types/display_mode.h:24: DisplayMode& operator=(DisplayMode&&) = default; On 2017/01/24 21:15:07, kylechar wrote: > Move to cc file. Done. https://codereview.chromium.org/2636073002/diff/250001/ui/display/types/displ... ui/display/types/display_mode.h:29: void set_size(const gfx::Size& size) { On 2017/01/24 21:15:07, kylechar wrote: > We really don't want to make DisplayMode mutable. Can you think of any other > ways to do this? As discussed, I removed display_mode_mojo.(cc|h) and used display_mode.cc and friend class.
https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/OWNERS File ui/display/mojo/OWNERS (right): https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/OWNERS... ui/display/mojo/OWNERS:5: per-file *.typemap=set noparent Just curious, why did you add the *.typemap per file? https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... File ui/display/mojo/display_mode.typemap (right): https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... ui/display/mojo/display_mode.typemap:10: ] deps = [ "//ui/gfx/geometry", ] https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... File ui/display/mojo/display_mode_struct_traits.h (right): https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... ui/display/mojo/display_mode_struct_traits.h:16: display::DisplayMode> { Please run "git cl format". https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:43: DisplayMode input(gfx::Size(15, 29), true, 61.0); const DisplayMode input https://codereview.chromium.org/2636073002/diff/310001/ui/display/types/displ... File ui/display/types/display_mode.cc (right): https://codereview.chromium.org/2636073002/diff/310001/ui/display/types/displ... ui/display/types/display_mode.cc:14: is_interlaced_(0), This is a bool not an int. https://codereview.chromium.org/2636073002/diff/310001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2636073002/diff/310001/ui/display/types/displ... ui/display/types/display_mode.h:16: #if !defined(OS_IOS) Get rid of all the !defined(OS_IOS) ifdefs, here and below. https://codereview.chromium.org/2636073002/diff/310001/ui/display/types/displ... ui/display/types/display_mode.h:17: #include "mojo/public/cpp/bindings/struct_traits.h" // nogncheck Remove the // nogncheck. https://codereview.chromium.org/2636073002/diff/310001/ui/display/types/displ... ui/display/types/display_mode.h:32: DisplayMode(); Can you make this private? I'm not sure what instantiates it. If it's not DisplayModeDataView then don't worry, leave this here.
Thanks Kyle. Please review my cl! https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/OWNERS File ui/display/mojo/OWNERS (right): https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/OWNERS... ui/display/mojo/OWNERS:5: per-file *.typemap=set noparent On 2017/01/25 16:59:44, kylechar wrote: > Just curious, why did you add the *.typemap per file? I got this error message on "git cl upload" if I dont have *.typemap lines ** Presubmit Warnings ** Found changes to IPC files without a security OWNER! *************** ui/display/mojo/OWNERS is missing the following lines: per-file *.typemap=set noparent per-file *.typemap=file://ipc/SECURITY_OWNERS for changed files: ui/display/mojo/display_mode.mojom ui/display/mojo/display_struct_traits_test.mojom ui/display/mojo/display_mode_struct_traits.h ui/display/mojo/display_struct_traits_unittest.cc ui/display/mojo/display_mode.typemap https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... File ui/display/mojo/display_mode.typemap (right): https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... ui/display/mojo/display_mode.typemap:10: ] On 2017/01/25 16:59:44, kylechar wrote: > deps = [ > "//ui/gfx/geometry", > ] Done, thanks! https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... File ui/display/mojo/display_mode_struct_traits.h (right): https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... ui/display/mojo/display_mode_struct_traits.h:16: display::DisplayMode> { On 2017/01/25 16:59:45, kylechar wrote: > Please run "git cl format". Done. https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2636073002/diff/310001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:43: DisplayMode input(gfx::Size(15, 29), true, 61.0); On 2017/01/25 16:59:45, kylechar wrote: > const DisplayMode input Done. https://codereview.chromium.org/2636073002/diff/310001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2636073002/diff/310001/ui/display/types/displ... ui/display/types/display_mode.h:16: #if !defined(OS_IOS) On 2017/01/25 16:59:45, kylechar wrote: > Get rid of all the !defined(OS_IOS) ifdefs, here and below. Done. https://codereview.chromium.org/2636073002/diff/310001/ui/display/types/displ... ui/display/types/display_mode.h:17: #include "mojo/public/cpp/bindings/struct_traits.h" // nogncheck On 2017/01/25 16:59:45, kylechar wrote: > Remove the // nogncheck. Done. https://codereview.chromium.org/2636073002/diff/310001/ui/display/types/displ... ui/display/types/display_mode.h:32: DisplayMode(); On 2017/01/25 16:59:45, kylechar wrote: > Can you make this private? I'm not sure what instantiates it. If it's not > DisplayModeDataView then don't worry, leave this here. This display_struct_traits_test.mojom.cc uses it so I can't put it in private. gen/ui/display/mojo/display_struct_traits_test.mojom.cc:61:24: error: calling a private constructor of class 'display::DisplayMode' display::DisplayMode p_out{};
The CQ bit was checked by thanhph@google.com 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...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) 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_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_asan_rel_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_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_...)
The CQ bit was checked by thanhph@google.com 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...
lgtm https://codereview.chromium.org/2636073002/diff/350001/ui/display/mojo/OWNERS File ui/display/mojo/OWNERS (right): https://codereview.chromium.org/2636073002/diff/350001/ui/display/mojo/OWNERS... ui/display/mojo/OWNERS:2: per-file *_struct_traits*.*=file://ipc/SECURITY_OWNERS nit: spaces between the entries make it easier to read.
The CQ bit was checked by thanhph@google.com 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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by thanhph@google.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Patchset #21 (id:390001) has been deleted
The CQ bit was checked by thanhph@google.com 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.
The CQ bit was checked by thanhph@google.com 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...
thanhph@google.com changed reviewers: - jam@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... File ui/display/types/display_mode.cc (right): https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... ui/display/types/display_mode.cc:23: DisplayMode::~DisplayMode() = default; reasoning? https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... ui/display/types/display_mode.h:29: DisplayMode(DisplayMode&&); needs explicit. Also: without std::move(<display object>) everywhere, does it add much value given the size of this class? https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... ui/display/types/display_mode.h:30: DisplayMode& operator=(DisplayMode&&); Do we need this? Can we remove? https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... ui/display/types/display_mode.h:44: gfx::Size size_; I don't like seeing the same structure defined in two places. Someone can add a field to here and not update the mojom and there would be a odd bug to track down at some point. Could we embed the DisplayModeDataView here instead? I think the struct traits could be adjusted successfully given the friend declaration above. (I'm not thrilled about friend but I understand why you need it pending the de-virtualization of this class.)
https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... ui/display/types/display_mode.h:44: gfx::Size size_; On 2017/01/27 18:54:55, rjkroege wrote: > I don't like seeing the same structure defined in two places. Someone can add a > field to here and not update the mojom and there would be a odd bug to track > down at some point. > > Could we embed the DisplayModeDataView here instead? I think the struct traits > could be adjusted successfully given the friend declaration above. (I'm not > thrilled about friend but I understand why you need it pending the > de-virtualization of this class.) Having to update a C++ class, mojom and StructTraits in sync is going to be a pain point for at least a couple hundred classes :) You can't embed a DisplayModeDataView here though. DisplayModeDataView doesn't actually hold any data, it just provides methods to deserialize data from a serialized blob. If you wanted to use the same structure you could just use a display::mojom::DisplayModePtr directly and you don't need a StructTraits. StructTraits were added to avoid doing that though...
The duplication seems regrettable. lgtm
Patchset #23 (id:450001) has been deleted
Patchset #23 (id:470001) has been deleted
Patchset #23 (id:490001) has been deleted
Patchset #23 (id:510001) has been deleted
Hi Rob, I leave out changes in display_mode.(cc|h) and use unique pointer as discussed with Kyle since the unique pointer will improve de-serialization time for DisplayMode when we do DisplaySnapshot struct traits. Could you review my new cl? Thanks, Thanh. https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... ui/display/types/display_mode.h:29: DisplayMode(DisplayMode&&); On 2017/01/27 18:54:55, rjkroege wrote: > needs explicit. > > Also: without std::move(<display object>) everywhere, does it add much value > given the size of this class? Acknowledged. I removed this line. https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... ui/display/types/display_mode.h:30: DisplayMode& operator=(DisplayMode&&); On 2017/01/27 18:54:55, rjkroege wrote: > Do we need this? Can we remove? Done. https://codereview.chromium.org/2636073002/diff/430001/ui/display/types/displ... ui/display/types/display_mode.h:44: gfx::Size size_; On 2017/01/27 19:34:18, kylechar wrote: > On 2017/01/27 18:54:55, rjkroege wrote: > > I don't like seeing the same structure defined in two places. Someone can add > a > > field to here and not update the mojom and there would be a odd bug to track > > down at some point. > > > > Could we embed the DisplayModeDataView here instead? I think the struct traits > > could be adjusted successfully given the friend declaration above. (I'm not > > thrilled about friend but I understand why you need it pending the > > de-virtualization of this class.) > > Having to update a C++ class, mojom and StructTraits in sync is going to be a > pain point for at least a couple hundred classes :) > > You can't embed a DisplayModeDataView here though. DisplayModeDataView doesn't > actually hold any data, it just provides methods to deserialize data from a > serialized blob. > > If you wanted to use the same structure you could just use a > display::mojom::DisplayModePtr directly and you don't need a StructTraits. > > StructTraits were added to avoid doing that though... I chat to Kyle and removed changes in display_mode.(cc|h) are untouched.
The CQ bit was checked by thanhph@google.com to run a CQ dry run
new changes lgtm
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I lied, one small thing to fix. https://codereview.chromium.org/2636073002/diff/530001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2636073002/diff/530001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:97: EXPECT_TRUE(proxy->EchoDisplayMode(input->Clone(), &output)); This returns null, don't EXPECT_TRUE on it.
The CQ bit was checked by thanhph@google.com 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...
Good catch Kyle, thanks! Thanh. https://codereview.chromium.org/2636073002/diff/530001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2636073002/diff/530001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:97: EXPECT_TRUE(proxy->EchoDisplayMode(input->Clone(), &output)); On 2017/01/27 21:15:19, kylechar wrote: > This returns null, don't EXPECT_TRUE on it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanhph@google.com changed reviewers: + oshima@chromium.org
oshima@chromium.org: Please review changes in ui/display. Thanks, Thanh.
still lgtm. thanks
lgtm
The CQ bit was checked by thanhph@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, kylechar@chromium.org Link to the patchset: https://codereview.chromium.org/2636073002/#ps550001 (title: "remove expect_true in void function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 550001, "attempt_start_ts": 1485897334632910,
"parent_rev": "76cd3bbdad924d4bee7264e8efb9d6f8e360e4d7", "commit_rev":
"bd13a9f7838046525e55350a7e491e014235aea1"}
CQ is committing da patch.
Bot data: {"patchset_id": 550001, "attempt_start_ts": 1485897334632910,
"parent_rev": "76cd3bbdad924d4bee7264e8efb9d6f8e360e4d7", "commit_rev":
"bd13a9f7838046525e55350a7e491e014235aea1"}
Message was sent while issue was closed.
Description was changed from ========== Create mojom and StructTraits for ui/display/types/display_mode.cc BUG=680992 ========== to ========== Create mojom and StructTraits for ui/display/types/display_mode.cc BUG=680992 Review-Url: https://codereview.chromium.org/2636073002 Cr-Commit-Position: refs/heads/master@{#447394} Committed: https://chromium.googlesource.com/chromium/src/+/bd13a9f7838046525e55350a7e49... ==========
Message was sent while issue was closed.
Committed patchset #24 (id:550001) as https://chromium.googlesource.com/chromium/src/+/bd13a9f7838046525e55350a7e49... |
