Write mojom and StructTraits for DisplaySnapshot.

ui/display/mojo/display_snapshot_mojo_struct_traits.cc

Index: ui/display/mojo/display_snapshot_mojo_struct_traits.cc
diff --git a/ui/display/mojo/display_snapshot_mojo_struct_traits.cc b/ui/display/mojo/display_snapshot_mojo_struct_traits.cc
new file mode 100644
index 0000000000000000000000000000000000000000..263699b3b856dfacbce2cd964c74557818c4eabe
--- /dev/null
+++ b/ui/display/mojo/display_snapshot_mojo_struct_traits.cc
@@ -0,0 +1,130 @@
+// 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.
+
+#include "ui/display/mojo/display_snapshot_mojo_struct_traits.h"
+
+#include "ipc/ipc_message_utils.h"
+#include "ui/display/mojo/display_constants_struct_traits.h"
+#include "ui/display/mojo/display_mode_struct_traits.h"
+#include "ui/display/types/display_constants.h"
+#include "ui/gfx/geometry/mojo/geometry_struct_traits.h"
+#include "ui/gfx/geometry/size.h"
+
+namespace mojo {
+
+// static
+bool StructTraits<display::mojom::DisplaySnapshotMojoDataView,
+                  std::unique_ptr<display::DisplaySnapshotMojo>>::
+    compare_display_mode(const display::DisplayMode& display_mode_lhs,
+                         const display::DisplayMode& display_mode_rhs) {
+  return (display_mode_lhs.size() == display_mode_rhs.size()) &&
+         (display_mode_lhs.is_interlaced() ==
+          display_mode_rhs.is_interlaced()) &&
+         (display_mode_lhs.refresh_rate() == display_mode_rhs.refresh_rate());
+}
+
+// static
+std::vector<std::unique_ptr<display::DisplayMode>>
+StructTraits<display::mojom::DisplaySnapshotMojoDataView,
+             std::unique_ptr<display::DisplaySnapshotMojo>>::
+    modes(
+        const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+  std::vector<std::unique_ptr<display::DisplayMode>> display_mode_list;
+  display_mode_list.reserve(display_snapshot->modes().size());

[kylechar, 2017/02/10 21:40:29]

You are creating a vector with the default size then immediately resizing it.
Just provide it the correct size initially.

[thanhph1, 2017/02/13 20:09:11]

Done.

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

[kylechar, 2017/02/10 21:40:29]

if (!display_snapshot->current_mode())
  return base::Optional<uint64_t>();

[thanhph1, 2017/02/13 20:09:11]

Done, cool! Since we won't use base::Optional<uint64_t>() after discussion, I
use !mode.

+  auto& modes = display_snapshot->modes();
+  for (size_t i = 0; i < modes.size(); ++i) {
+    if (compare_display_mode(*modes[i].get(),
+                             *display_snapshot->current_mode()))
+      return i;

[kylechar, 2017/02/10 21:40:29]

You'll need to change this to work with base::Optional<uint64_t>. That should
help guard against any overflow problems.

[thanhph1, 2017/02/13 20:09:11]

As discussed, I'll use uint64_t and a bool to replace base::Optional<uint64_t>
because mojom doesn't allow uint64?

+  }
+  return -1;

[kylechar, 2017/02/10 21:40:29]

NOTREACHED();
return base::Optional<uint64_t>();

[thanhph1, 2017/02/13 20:09:11]

Done.

+}
+
+// static
+int64_t StructTraits<display::mojom::DisplaySnapshotMojoDataView,
+                     std::unique_ptr<display::DisplaySnapshotMojo>>::
+    native_mode_index(
+        const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+  auto& modes = display_snapshot->modes();

[kylechar, 2017/02/10 21:40:29]

Adding a helper function in an anonymous namespace to share for current_mode and
native_mode would help.

[thanhph1, 2017/02/13 20:09:11]

Done.

+  for (size_t i = 0; i < modes.size(); ++i)
+    if (compare_display_mode(*modes[i].get(), *display_snapshot->native_mode()))
+      return i;
+  return -1;
+}
+
+// static
+bool StructTraits<display::mojom::DisplaySnapshotMojoDataView,
+                  std::unique_ptr<display::DisplaySnapshotMojo>>::
+    Read(display::mojom::DisplaySnapshotMojoDataView data,
+         std::unique_ptr<display::DisplaySnapshotMojo>* out) {
+  gfx::Point origin;
+  if (!data.ReadOrigin(&origin))
+    return false;
+
+  gfx::Size physical_size;
+  if (!data.ReadPhysicalSize(&physical_size))
+    return false;
+
+  display::DisplayConnectionType type;
+  if (!data.ReadType(&type))
+    return false;
+
+  std::string display_name;
+  if (!data.ReadDisplayName(&display_name))
+    return false;
+
+  base::FilePath filePath;
+  if (!data.ReadSysPath(&filePath))
+    return false;
+
+  std::vector<std::unique_ptr<display::DisplayMode>> modes1;

[kylechar, 2017/02/10 21:40:29]

Please change the name, modes1 doesn't tell us anything, why is there modes and
modes1? Also, does this compile if modes1 is std::vector<std::unique_ptr<const
display::DisplayMode>>.

[thanhph1, 2017/02/13 20:09:11]

|modes1| contains non-const display::DisplayMode while |modes| contains const
display::DisplayMode. Mojom doesn't allow const variable so I need to use
non-const variable in |modes1| then later on convert this variable to const in
|modes|.

+  if (!data.ReadModes(&modes1))
+    return false;
+
+  display::DisplaySnapshot::DisplayModeList modes;
+  for_each(modes1.begin(), modes1.end(),

[kylechar, 2017/02/10 21:40:29]

A few things:
1. Sometimes it makes sense to do things using STL functions but not here. A
loop would take two lines vs four lines this used.
2. Did you mean std::for_each? I wonder what function you're actually using
here.
3. Assuming you can't just pass |modes| into ReadModes() and avoid this
entirely, you don't need to call Clone(). What exactly are you doing with the
contents of |modes1| when this function returns? It goes out of scope so the
contents get destroyed. You should be able to just move each individual
std::unique_ptr<DisplayMode> from |modes1| to |modes|.

[thanhph1, 2017/02/13 20:09:12]

Thanks, cool! mojom doesn't allow to use const variable so |modes1| contains
non-const unique pointers of DisplayMode which will be moved and auto-casted to
const in |modes|. 

This comment is also added in the code.

+           [&modes](std::unique_ptr<display::DisplayMode>& i) {
+             modes.push_back(i->Clone());
+           });
+
+  // Get current_mode pointer from modes1 array.
+  int current_mode_index = data.current_mode_index();

[kylechar, 2017/02/10 21:40:29]

You are converting int64_t to int here which could cause problems. Just FYI.
You'll need to change this to work the base::Optional<uint64_t> anyways.

[thanhph1, 2017/02/13 20:09:11]

Good catch, thanks! I'll change to int64_t instead of int.
base::Optional<uint64_t> won't work for mojom built-in type as discussed.

+  if (current_mode_index >= (int)modes1.size())

[kylechar, 2017/02/10 21:40:29]

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

[thanhph1, 2017/02/13 20:09:11]

Removed this line.

+    return false;
+  const display::DisplayMode* current_mode = nullptr;
+  if (current_mode_index > 0)
+    current_mode = modes[current_mode_index].get();
+
+  // Get native_mode pointer from modes1 array.

[kylechar, 2017/02/10 21:40:30]

Helper function again.

[thanhph1, 2017/02/13 20:09:12]

Done.

+  int native_mode_index = data.native_mode_index();
+  if (native_mode_index >= (int)modes1.size())
+    return false;
+  const display::DisplayMode* native_mode = nullptr;
+  if (native_mode_index > 0)
+    native_mode = modes[native_mode_index].get();
+
+  std::vector<uint8_t> edid;
+  if (!data.ReadEdid(&edid))
+    return false;
+
+  *out = base::MakeUnique<display::DisplaySnapshotMojo>(
+      data.display_id(), origin, physical_size, type,
+      data.is_aspect_preserving_scaling(), data.has_overscan(),
+      data.has_color_correction_matrix(), display_name, filePath,
+      data.product_id(), std::move(modes), edid, current_mode, native_mode);
+  return true;
+}
+
+}  // namespace mojo