Write mojom and StructTraits for DisplaySnapshot.

ui/display/mojo/display_struct_traits_unittest.cc

Index: ui/display/mojo/display_struct_traits_unittest.cc
diff --git a/ui/display/mojo/display_struct_traits_unittest.cc b/ui/display/mojo/display_struct_traits_unittest.cc
index e087e68d63e240e979ba24140c472ba8a62b6b45..bb7f785ccabbac98077544811df21fe88b684857 100644
--- a/ui/display/mojo/display_struct_traits_unittest.cc
+++ b/ui/display/mojo/display_struct_traits_unittest.cc
@@ -2,12 +2,18 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <memory>
+
+#include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/display/display.h"
 #include "ui/display/display_layout.h"
+#include "ui/display/display_snapshot_mojo.h"
+#include "ui/display/mojo/display_mode_struct_traits.h"

[kylechar, 2017/02/13 22:01:24]

You shouldn't need to include the StructTraits I don't think.

[thanhph1, 2017/02/14 20:20:54]

I remove display_mode_struct_traits.h. display_struct_traits_test.mojom.h is
still needed.

 #include "ui/display/mojo/display_struct_traits_test.mojom.h"
+#include "ui/display/types/display_constants.h"
 #include "ui/display/types/display_mode.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
@@ -41,6 +47,12 @@ class DisplayStructTraitsTest : public testing::Test,
     callback.Run(std::move(in));
   }
 
+  void EchoDisplaySnapshotMojo(
+      std::unique_ptr<DisplaySnapshotMojo> in,
+      const EchoDisplaySnapshotMojoCallback& callback) override {
+    callback.Run(std::move(in));
+  }
+
   void EchoDisplayPlacement(
       const DisplayPlacement& in,
       const EchoDisplayPlacementCallback& callback) override {
@@ -108,12 +120,12 @@ TEST_F(DisplayStructTraitsTest, SetAllDisplayValues) {
 }
 
 TEST_F(DisplayStructTraitsTest, DefaultDisplayMode) {
+  // Prepare sample input with random values

[kylechar, 2017/02/13 22:01:24]

Undo this change?

[thanhph1, 2017/02/14 20:20:53]

Done.

   std::unique_ptr<DisplayMode> input =
       base::MakeUnique<DisplayMode>(gfx::Size(1024, 768), true, 61.0);
 
   mojom::DisplayStructTraitsTestPtr proxy = GetTraitsTestProxy();
   std::unique_ptr<DisplayMode> output;
-

[kylechar, 2017/02/13 22:01:24]

Undo this change?

   proxy->EchoDisplayMode(input->Clone(), &output);
 
   // We want to test each component individually to make sure each data member
@@ -219,4 +231,81 @@ TEST_F(DisplayStructTraitsTest, DisplayLayoutTwoMirrored) {
   CheckDisplayLayoutsEqual(*input, *output);
 }
 
+TEST_F(DisplayStructTraitsTest, DefaultDisplaySnapshotMojo) {

[kylechar, 2017/02/13 22:01:24]

This test is pretty massive and doesn't hit all the important cases. I'd say you
need at least the following tests:

(1) One display mode, current and native mode nullptr.
(2) One display mode that is the native mode and no current mode.
(3) Multiple display modes, both native and current mode set.
(4) Tests for both internal display and external display. An internal display
will only have one display mode and type=DISPLAY_CONNECT_TYPE_INTERNAL FYI.

You'll probably want some helper functions to make this more manageable. Also
you want to check a few different values in the important fields. A person
preference of mine is to use realistic looking data but that is less important.

[thanhph1, 2017/02/14 20:20:53]

Thanks! I added helper function CheckDisplaySnapShotMojoEqual() to compare
input, output. The test data input preparation/creation will be left in the same
unit test function for readability.

+  mojom::DisplayStructTraitsTestPtr proxy = GetTraitsTestProxy();
+
+  // prepare sample input with random values
+  const int64_t display_id = 7;
+  const gfx::Point origin(3, 12);
+  const gfx::Size physical_size(15, 29);
+  const gfx::Size maximum_cursor_size(30, 35);
+  const DisplayConnectionType type = display::DISPLAY_CONNECTION_TYPE_HDMI;
+  const bool is_aspect_preserving_scaling = true;
+  const bool has_overscan = true;
+  const bool has_color_correction_matrix = true;
+  const std::string display_name("whatever display_name");
+  const base::FilePath::StringType string_type_path =
+      FILE_PATH_LITERAL("a/b/c");
+  const base::FilePath::StringPieceType string_piece_path(string_type_path);
+  const base::FilePath sys_path(string_piece_path);
+  const int64_t product_id = 19;
+
+  const DisplayMode display_mode_1(gfx::Size(11, 12), true, 111.0);
+  const DisplayMode display_current_mode(gfx::Size(22, 23), true, 222.0);
+  const DisplayMode display_native_mode(gfx::Size(33, 34), true, 333.0);
+  const DisplayMode display_mode_2(gfx::Size(44, 45), true, 444.0);
+
+  display::DisplaySnapshot::DisplayModeList modes(4);
+  modes.push_back(display_mode_1.Clone());
+  modes.push_back(display_current_mode.Clone());
+  modes.push_back(display_native_mode.Clone());
+  modes.push_back(display_mode_2.Clone());
+
+  const DisplayMode* current_mode = modes[1].get();
+  const DisplayMode* native_mode = modes[2].get();
+
+  display::DisplaySnapshot::DisplayModeList expected_modes(4);
+  expected_modes.push_back(display_mode_1.Clone());
+  expected_modes.push_back(display_current_mode.Clone());
+  expected_modes.push_back(display_native_mode.Clone());
+  expected_modes.push_back(display_mode_2.Clone());
+
+  const std::vector<uint8_t> edid = {2, 3, 4, 5};
+
+  std::unique_ptr<DisplaySnapshotMojo> input =
+      base::MakeUnique<DisplaySnapshotMojo>(
+          display_id, origin, physical_size, type, is_aspect_preserving_scaling,
+          has_overscan, has_color_correction_matrix, display_name, sys_path,
+          product_id, std::move(modes), edid, current_mode, true, native_mode,
+          true, maximum_cursor_size);
+
+  std::unique_ptr<DisplaySnapshotMojo> output;
+  proxy->EchoDisplaySnapshotMojo(std::move(input), &output);
+
+  // We want to test each component individually to make sure each data member
+  // was correctly serialized and deserialized.
+  EXPECT_EQ(display_id, output->display_id());
+  EXPECT_EQ(origin, output->origin());
+  EXPECT_EQ(physical_size, output->physical_size());
+  EXPECT_EQ(type, output->type());
+  EXPECT_EQ(is_aspect_preserving_scaling,
+            output->is_aspect_preserving_scaling());
+  EXPECT_EQ(has_overscan, output->has_overscan());
+  EXPECT_EQ(has_color_correction_matrix, output->has_color_correction_matrix());
+  EXPECT_EQ(display_name, output->display_name());
+  EXPECT_EQ(sys_path, output->sys_path());
+  EXPECT_EQ(product_id, output->product_id());
+  EXPECT_EQ(4, (int)output->modes().size());

[kylechar, 2017/02/13 22:01:24]

You shouldn't downcast unless you absolutely need to, which isn't required here,
just use the proper value to compare against.

EXPECT_EQ(4u, output->modes().size());

You're not allowed to use C style casts anyways in any piece of code either.

[thanhph1, 2017/02/14 20:20:53]

Thanks, changed to EXPECT_EQ(input.modes().size(), output.modes().size());

+
+  for (int i = 0; i < (int)expected_modes.size(); i++)
+    EXPECT_EQ(*expected_modes[i].get(), *output->modes()[i].get());

[kylechar, 2017/02/13 22:01:24]

unique_ptr defines *, so no need to call .get() first.

[thanhph1, 2017/02/14 20:20:53]

Nice, done!

+
+  EXPECT_EQ(edid, output->edid());
+  EXPECT_EQ(display_current_mode, *output->current_mode());
+  EXPECT_EQ(true, output->current_mode_exist());
+  EXPECT_EQ(display_native_mode, *output->native_mode());
+  EXPECT_EQ(true, output->native_mode_exist());
+  EXPECT_EQ(maximum_cursor_size, output->maximum_cursor_size());
+}
+
 }  // namespace display