Added capability on Windows to get the physical dimensions of your attached monitors. Also added th…

ui/gfx/win/physical_size.cc

Index: ui/gfx/win/physical_size.cc
diff --git a/ui/gfx/win/physical_size.cc b/ui/gfx/win/physical_size.cc
new file mode 100644
index 0000000000000000000000000000000000000000..103df48abf294d2e090205dbd39e4c95ceedc77a
--- /dev/null
+++ b/ui/gfx/win/physical_size.cc
@@ -0,0 +1,154 @@
+// Copyright 2016 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/gfx/win/physical_size.h"

[robliao, 2016/01/13 00:22:10]

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

#include "ui/gfx/win/physical_size.h"

#include <windows.h> (Exception to the rule since things tend to break if it
isn't first)
#include <setupapi.h>

#include <iostream>

#include "base/logging.h"
#include "base/memory/scoped_ptr.h"
#include "base/scoped_generic.h"
#include "base/strings/utf_string_conversions.h"
#include "base/win/registry.h"

[Bret, 2016/01/13 18:02:48]

Done. The only problem seems to be missing a line break before <iostream>, if I
missed something let me know.

+
+#include <windows.h>
+#include <setupapi.h>
+#include <iostream>
+
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/scoped_generic.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/win/registry.h"
+
+// This GUID was taken from
+// https://msdn.microsoft.com/en-us/library/windows/hardware/ff545901.aspx

[robliao, 2016/01/13 00:22:11]

Nit: Add comment:
// {E6F07B5F-EE97-4A90-B076-33F57BF4EAA7}

[Bret, 2016/01/13 18:02:48]

Done.

+const GUID GUID_DEVICEINTERFACE_MONITOR = {
+    0xE6F07B5F, 0xEE97, 0x4A90, 0xB0, 0x76, 0x33, 0xF5, 0x7B, 0xF4, 0xEA, 0xA7};
+
+namespace {
+
+struct DeviceInfoListScopedTraits {
+  static HDEVINFO InvalidValue() { return INVALID_HANDLE_VALUE; }
+
+  static void Free(HDEVINFO h) { SetupDiDestroyDeviceInfoList(h); }
+};
+
+bool GetSizeFromRegistry(HDEVINFO device_info_list,
+                         SP_DEVINFO_DATA* device_info,
+                         int* width_mm,
+                         int* height_mm) {
+  base::win::RegKey reg_key(SetupDiOpenDevRegKey(
+      device_info_list, device_info, DICS_FLAG_GLOBAL, 0, DIREG_DEV, KEY_READ));

[robliao, 2016/01/13 00:22:10]

The preference in ui/gfx/* seems to be to line up the arguments when the
argument list exceeds the 80 char limit, falling back to this format when single
argument lined up this way exceeds the 80 char limit.

  base::win::RegKey reg_key(SetupDiOpenDevRegKey(device_info_list, 
                                                 device_info,
                                                 DICS_FLAG_GLOBAL,
                                                 0,
                                                 DIREG_DEV,
                                                 KEY_READ));

fits. Similar rules apply to assignments:

type var_name = func_call(arg1,
                          arg2,
                          arg3)
(Only if arg1, arg2, arg3 would exceed the 80 char limit)

Apply to the rest of the file.

Personally, I prefer what you have right now, but we follow whatever is already
established in this section.

[Bret, 2016/01/13 18:02:48]

I just ran git cl format for this... Should I be doing
func(arg1,
     arg2,
     arg3,
     arg4)
everywhere, or is it okay to do
func(arg1, arg2,
     arg3, arg4)
? Should I ignore what git cl format does if it breaks that convention?

[scottmg, 2016/01/13 18:08:36]

Sticking to the output of clang-format/git cl format is fine.

I prefer one-per-line in general, but machine-formattable is way better than any
argument about it, so prefer not to deviate from git cl format unless it does
something very crazy (in which case you should also file a bug against
clang-format).

[robliao, 2016/01/13 18:15:07]

I'd defer to the owners on this one. I've gotten pushback in either way on
various parts of the codebase. The general guidance from the style guide is to
be consistent.

+
+  if (!reg_key.Valid())
+    return false;
+
+  BYTE data[128];
+  DWORD data_length = sizeof(data);
+  LONG return_value =
+      reg_key.ReadValue(L"EDID", &data[0], &data_length, nullptr);
+  if (return_value != ERROR_SUCCESS)
+    return false;
+
+  // Byte 54 is the start of the required descriptor block that contains the
+  // required timing information with the highest preference, and 12 bytes
+  // into that block is the size information.
+  // 66: width least significant bits
+  // 67: height least significant bits
+  // 68: 4 bits for each of width and height most significant bits
+  *width_mm = ((data[68] & 0xF0) << 4) + data[66];
+  *height_mm = ((data[68] & 0x0F) << 8) + data[67];
+
+  return true;
+}
+
+bool GetInterfaceDetailAndDeviceInfo(
+    HDEVINFO device_info_list,
+    SP_DEVICE_INTERFACE_DATA* interface_data,
+    scoped_ptr<SP_DEVICE_INTERFACE_DETAIL_DATA, base::FreeDeleter>*
+        interface_detail,
+    SP_DEVINFO_DATA* device_info) {
+  device_info->cbSize = sizeof(SP_DEVINFO_DATA);

[robliao, 2016/01/13 00:22:10]

Similar to Windows, the caller should be initializing this value. You can,
however, DCHECK(device_info->cbSize == sizeof(*device_info))

[Bret, 2016/01/13 18:02:48]

Done.

+  DWORD buffer_size;
+  // This call also populates device_info.
+  bool success =
+      SetupDiGetDeviceInterfaceDetail(device_info_list, interface_data, nullptr,
+                                      0, &buffer_size, device_info) != 0;
+  if (success || GetLastError() != ERROR_INSUFFICIENT_BUFFER) {

[robliao, 2016/01/13 00:22:10]

As set up, this call is guaranteed to fail, so a check for success is really
optional here. However, feel free to keep the ERROR_INSUFFICIENT_BUFFER check.

[Bret, 2016/01/13 18:02:49]

Done.

+    // The first call should fail.
+    return false;
+  }
+
+  interface_detail->reset(
+      reinterpret_cast<SP_DEVICE_INTERFACE_DETAIL_DATA*>(malloc(buffer_size)));
+  (*interface_detail)->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA);
+  return SetupDiGetDeviceInterfaceDetail(device_info_list, interface_data,
+                                         interface_detail->get(), buffer_size,
+                                         nullptr, nullptr) != 0;
+}
+
+}  // namespace
+
+namespace gfx {
+
+std::vector<PhysicalDisplaySize> GetPhysicalSizeForDisplays() {
+  std::vector<PhysicalDisplaySize> out;
+
+  // Get a handle to the list of device interfaces that are monitors via Setup.

[robliao, 2016/01/13 00:22:11]

Remove this comment.
See https://google.github.io/styleguide/cppguide.html#Comments under "Don'ts".
Apply to the rest of the file.

[Bret, 2016/01/13 18:02:48]

I moved all the important information into a big header comment, since I'm a bit
worried the flow of the function is hard to follow. Lemme know if it's
overwrought.

+  base::ScopedGeneric<HDEVINFO, DeviceInfoListScopedTraits> device_info_list(
+      SetupDiGetClassDevs(&GUID_DEVICEINTERFACE_MONITOR, nullptr, nullptr,
+                          DIGCF_PRESENT | DIGCF_DEVICEINTERFACE));
+
+  if (!device_info_list.is_valid())
+    return out;
+
+  // Loop over the device interfaces that are present according to Setup.
+  SP_DEVICE_INTERFACE_DATA interface_data = {0};
+  interface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
+  int interface_index = 0;
+  while (SetupDiEnumDeviceInterfaces(device_info_list.get(), nullptr,
+                                     &GUID_DEVICEINTERFACE_MONITOR,
+                                     interface_index++, &interface_data)) {
+    scoped_ptr<SP_DEVICE_INTERFACE_DETAIL_DATA, base::FreeDeleter>
+        interface_detail;
+    SP_DEVINFO_DATA device_info = {0};

[robliao, 2016/01/13 00:22:10]

cbSize should be set up here as part of initialization.
device_info.cbSize = sizeof(device_info);

Nit: {}

[Bret, 2016/01/13 18:02:48]

Done.

+    // This gives us the device path that we need to associate the device
+    // interface from Setup with the display device from EnumDisplayDevices.
+    bool get_info_succeeded =

[robliao, 2016/01/13 00:22:10]

Optional: You could plausibly inline this into the "if" (same with "found"
below). 

[Bret, 2016/01/13 18:02:48]

Keeping the temp variables reads a bit better imo

+        GetInterfaceDetailAndDeviceInfo(device_info_list.get(), &interface_data,
+                                        &interface_detail, &device_info);
+    if (!get_info_succeeded)
+      continue;
+
+    // Loop over the display interfaces via EnumDisplayDevices. this will give
+    // us the device name and allow us to loop over its attached monitors.
+    DISPLAY_DEVICE display_device = {0};

[robliao, 2016/01/13 00:22:11]

Nit: {}

[Bret, 2016/01/13 18:02:49]

Done.

+    display_device.cb = sizeof(DISPLAY_DEVICE);

[robliao, 2016/01/13 00:22:10]

Nit: sizeof(display_device)

[Bret, 2016/01/13 18:02:48]

Done.

+    int display_index = 0;
+    while (EnumDisplayDevices(nullptr, display_index++, &display_device,
+                              EDD_GET_DEVICE_INTERFACE_NAME)) {
+      // Loop over the monitors attached to the display interface. This gives us
+      // the device ID that associates the monitor with the setup device path
+      // from the earlier step.
+      DISPLAY_DEVICE attached_device = {0};

[robliao, 2016/01/13 00:22:10]

Nit: {}

[Bret, 2016/01/13 18:02:48]

Done.

+      attached_device.cb = sizeof(DISPLAY_DEVICE);

[robliao, 2016/01/13 00:22:11]

Nit: sizeof(attached_device)

[Bret, 2016/01/13 18:02:48]

Done.

+      int attached_index = 0;
+      while (EnumDisplayDevices(display_device.DeviceName, attached_index++,
+                                &attached_device,
+                                EDD_GET_DEVICE_INTERFACE_NAME)) {
+        wchar_t* attached_device_id = attached_device.DeviceID;
+        wchar_t* setup_device_path = interface_detail->DevicePath;
+        if (wcsicmp(attached_device_id, setup_device_path) == 0) {
+          // We've found the monitor that matches the Setup device.

[robliao, 2016/01/13 00:22:10]

Nit: Setup -> setup.

[Bret, 2016/01/13 18:02:48]

Done.

+          int width_mm;
+          int height_mm;
+          bool found = GetSizeFromRegistry(device_info_list.get(), &device_info,
+                                           &width_mm, &height_mm);
+          if (found) {
+            out.push_back(
+                PhysicalDisplaySize(base::WideToUTF8(display_device.DeviceName),
+                                    width_mm, height_mm));
+          }
+          break;
+        }
+      }
+    }
+  }
+  return out;
+}
+
+}  // namespace gfx