Write mojom and StructTraits for DisplaySnapshot.

ui/display/mojo/display_snapshot_mojo_struct_traits.h

Index: ui/display/mojo/display_snapshot_mojo_struct_traits.h
diff --git a/ui/display/mojo/display_snapshot_mojo_struct_traits.h b/ui/display/mojo/display_snapshot_mojo_struct_traits.h
new file mode 100644
index 0000000000000000000000000000000000000000..0bdf1f52a91ea77a8391c1647fcce2246630518f
--- /dev/null
+++ b/ui/display/mojo/display_snapshot_mojo_struct_traits.h
@@ -0,0 +1,124 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef UI_DISPLAY_MOJO_DISPLAY_SNAPSHOT_MOJO_STRUCT_TRAITS_H_
+#define UI_DISPLAY_MOJO_DISPLAY_SNAPSHOT_MOJO_STRUCT_TRAITS_H_
+
+#include "ui/display/mojo/display_mode.mojom.h"
+#include "ui/display/mojo/display_mode_struct_traits.h"
+#include "ui/display/mojo/display_snapshot_mojo.mojom.h"

[kylechar, 2017/02/09 15:05:54]

You are missing an import for DisplaySnapshotMojo.

[thanhph1, 2017/02/10 19:54:32]

Done.

+
+namespace mojo {
+
+template <>
+struct EnumTraits<display::mojom::DisplayConnectionType,

[kylechar, 2017/02/09 15:05:54]

Why is the defined twice?

[thanhph1, 2017/02/10 19:54:32]

Removed. 

+                  display::DisplayConnectionType> {
+  static display::mojom::DisplayConnectionType ToMojom(
+      display::DisplayConnectionType type);
+  static bool FromMojom(display::mojom::DisplayConnectionType type,
+                        display::DisplayConnectionType* out);
+};
+
+template <>
+struct StructTraits<display::mojom::DisplaySnapshotMojoDataView,
+                    std::unique_ptr<display::DisplaySnapshotMojo>> {
+  static int64_t display_id(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    return display_snapshot->display_id();
+  }
+
+  static gfx::Point origin(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    return display_snapshot->origin();
+  }
+
+  static gfx::Size physical_size(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    return display_snapshot->physical_size();
+  }
+
+  static display::DisplayConnectionType type(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    return display_snapshot->type();
+  }
+
+  static bool is_aspect_preserving_scaling(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    return display_snapshot->is_aspect_preserving_scaling();
+  }
+
+  static bool has_overscan(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    return display_snapshot->has_overscan();
+  }
+
+  static bool has_color_correction_matrix(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    return display_snapshot->has_color_correction_matrix();
+  }
+
+  static std::string display_name(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    return display_snapshot->display_name();
+  }
+
+  static const base::FilePath::StringType sys_path(

[kylechar, 2017/02/09 15:05:54]

I think you can just use FilePath in your mojom. There is a type and
StructTraits for it. If not, you're passing a string on some platform and
wstring on others. You'll need to double check that is okay, I'm not sure how it
works.

[thanhph1, 2017/02/10 19:54:32]

Nice, thanks. I didn't this exists. Updated!

+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    return display_snapshot->sys_path().value();
+  }
+
+  static bool compareDisplayMode(const display::DisplayMode* display_mode_a,

[kylechar, 2017/02/09 15:05:54]

Many problems here.
1. This doesn't belong in the header, it's an implementation detail and
shouldn't be exposed.
2. The method naming style is wrong.
3. Don't pass constant points, they need to be references.
3. nit: Name the variables left/right, lhs/rhs, a/b, etc. Those are much more
conventional names for an equality check.

[thanhph1, 2017/02/10 19:54:33]

Nice, thanks. I use the name display_mode_lhs and display_mode_rhs.

+                                 const display::DisplayMode* display_mode_b) {
+    if ((display_mode_a->size() != display_mode_b->size()) ||

[kylechar, 2017/02/09 15:05:54]

return lhs.size() == rhs.size() && ...;

[thanhph1, 2017/02/10 19:54:32]

Done.

+        (display_mode_a->is_interlaced() != display_mode_b->is_interlaced()) ||
+        (display_mode_a->refresh_rate() != display_mode_b->refresh_rate()))
+      return false;
+    return true;
+  }
+
+  static std::vector<uint8_t> edid(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    return display_snapshot->edid();
+  }
+
+  static std::vector<std::unique_ptr<display::DisplayMode>> modes(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    std::vector<std::unique_ptr<display::DisplayMode>> display_mode_list;

[kylechar, 2017/02/09 15:05:54]

You know the size, tell the constructor so it can allocate the correct size
initially.

[thanhph1, 2017/02/10 19:54:32]

Done.

+    for (const auto& display_mode : display_snapshot->modes())
+      display_mode_list.push_back(display_mode->Clone());
+    return display_mode_list;
+  }
+
+  static int64_t current_mode_index(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {

[kylechar, 2017/02/09 15:05:54]

Only one liner methods should go in the header. Move to source file.

[thanhph1, 2017/02/10 19:54:32]

Done.

+    for (int i = 0; i < (int)display_snapshot->modes().size(); i++) {

[kylechar, 2017/02/09 15:05:54]

You aren't allowed to use C style casts.

https://google.github.io/styleguide/cppguide.html#Casting

[thanhph1, 2017/02/10 19:54:32]

Cool, thanks!

+      if (compareDisplayMode(display_snapshot->modes()[i].get(),

[kylechar, 2017/02/09 15:05:54]

It would help for readability if you had done something like:

auto& modes = display_snapshot->modes();
for (size_t i = 0; i < modes.size(); ++i) {

[thanhph1, 2017/02/10 19:54:33]

nice, done!

+                             display_snapshot->current_mode()))
+        return i;
+    }
+    return -1;
+  }
+
+  static int64_t native_mode_index(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {

[kylechar, 2017/02/09 15:05:54]

Same as above.

[thanhph1, 2017/02/10 19:54:32]

Done.

+    for (int i = 0; i < (int)display_snapshot->modes().size(); i++) {
+      if (compareDisplayMode(display_snapshot->modes()[i].get(),
+                             display_snapshot->native_mode()))
+        return i;
+    }
+    return -1;
+  }
+
+  static int64_t product_id(
+      const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+    return display_snapshot->product_id();
+  }
+
+  static bool Read(display::mojom::DisplaySnapshotMojoDataView data,
+                   std::unique_ptr<display::DisplaySnapshotMojo>* out);
+};
+
+}  // namespace mojo
+
+#endif  // UI_DISPLAY_MOJO_DISPLAY_SNAPSHOT_MOJO_STRUCT_TRAITS_H_