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..8be4b65d4e798fdd9cefb9ed4e0705f1bb38e0fb
--- /dev/null
+++ b/ui/display/mojo/display_snapshot_mojo_struct_traits.cc
@@ -0,0 +1,136 @@
+// 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 {
+
+namespace {
+
+static int64_t get_mode_index(

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

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

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

Oh cool, thanks!

+    const display::DisplaySnapshot::DisplayModeList& modes,
+    const display::DisplayMode* mode) {
+  if (!mode)
+    return -1;
+
+  for (size_t i = 0; i < modes.size(); ++i)

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

The for loop body is more than one line so add {}.

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

Done.

+    if (*modes[i].get() == *mode)
+      return i;
+
+  NOTREACHED();
+  return -1;
+}
+
+}  // namespace
+
+// 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_snapshot->modes().size());
+
+  for (const auto& display_mode : display_snapshot->modes())
+    display_mode_list.push_back(display_mode->Clone());
+  return display_mode_list;
+}
+
+// static
+uint64_t StructTraits<display::mojom::DisplaySnapshotMojoDataView,
+                      std::unique_ptr<display::DisplaySnapshotMojo>>::
+    current_mode_index(
+        const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+  return get_mode_index(display_snapshot->modes(),

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

get_mode_index returns an int64_t which you then return as uint64_t. This isn't
quite right. You'll either want two helper functions (uint64_t GetModeIndex() vs
bool HasMode()) or one method that returns a base::Optional<uint64_t>.

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

I change return value from int64_t to uint64_t in get_mode_index().

+                        display_snapshot->current_mode());
+}
+
+// static
+uint64_t StructTraits<display::mojom::DisplaySnapshotMojoDataView,
+                      std::unique_ptr<display::DisplaySnapshotMojo>>::
+    native_mode_index(
+        const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) {
+  return get_mode_index(display_snapshot->modes(),
+                        display_snapshot->native_mode());
+}
+
+// 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;

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

file_path

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

Done.

+  if (!data.ReadSysPath(&filePath))
+    return false;
+
+  // mojom doesn't allow to use const variable so |modes1| contains non-const

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

Maybe this would be clearer?

// There is a type mismatch between vectors containing unique_ptr<T> vs
unique_ptr<const T>. We deserialize into a vector of unique_ptr<T> then create a
vector of unique_ptr<const T> after.

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

Done.

+  // unique pointers of DisplayMode which will be moved and auto-casted to const
+  // in |modes|
+  std::vector<std::unique_ptr<display::DisplayMode>> modes1;

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

The same comment as last time, |modes1| isn't a good name.

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

Done. Changed to non_const_modes.

+  if (!data.ReadModes(&modes1))
+    return false;
+  display::DisplaySnapshot::DisplayModeList modes;
+  for (auto& it : modes1)

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

The variable |it| has nothing to do with an iterator so it's not an appropriate
name.

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

Done, change to it |mode|.

+    modes.push_back(std::move(it));
+
+  // Get current_mode pointer from modes array.
+  bool current_mode_exist = data.current_mode_exist();
+  const display::DisplayMode* current_mode = nullptr;
+  if (current_mode_exist) {

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

Your DCHECK won't be there in release builds so there isn't actually any
guarantee that current_mode_index isn't bigger than the modes vector. Read()
function should return false for bad data, so do that instead. Here is how I
would write this check.

if (data.has_current_mode()) {
  size_t current_mode_index = data.current_mode_index();
  if (current_mode_index >= modes.size())
    return false;
  current_mode = modes[current_mode_index].get();
}

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

Good to know DCHECK won't be in release builds, thanks!

+    uint64_t current_mode_index = data.current_mode_index();
+    DCHECK(current_mode_index < modes.size());
+    current_mode = modes[current_mode_index].get();
+  }
+
+  // Get native_mode pointer from modes array.
+  bool native_mode_exist = data.native_mode_exist();
+  const display::DisplayMode* native_mode = nullptr;
+  if (native_mode_exist) {
+    uint64_t native_mode_index = data.native_mode_index();
+    DCHECK(native_mode_index < modes.size());
+    native_mode = modes[native_mode_index].get();
+  }
+
+  std::vector<uint8_t> edid;
+  if (!data.ReadEdid(&edid))
+    return false;
+
+  gfx::Size maximum_cursor_size;
+  if (!data.ReadMaximumCursorSize(&maximum_cursor_size))
+    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,

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

std::move(edid)

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

Done.

+      current_mode_exist, native_mode, native_mode_exist, maximum_cursor_size);
+  return true;
+}
+
+}  // namespace mojo