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

ui/gfx/win/physical_size.cc

+// Copyright (c) 2016 The Chromium Authors. All rights reserved.

[scottmg, 2016/01/09 01:00:05]

I think there's not supposed to be any "(c)" any more.

[Bret, 2016/01/12 00:23:47]

Done.

+// 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"
+
+#include <windows.h>
+#include <setupapi.h>
+#include <tchar.h>
+
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/scoped_generic.h"
+
+const GUID GUID_DEVICEINTERFACE_MONITOR = {

[scottmg, 2016/01/09 01:00:05]

Any documentation URL you could add for this magic value?

[Bret, 2016/01/12 00:23:47]

Done.

+    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) {
+  HKEY reg_key = SetupDiOpenDevRegKey(device_info_list, &device_info,
+                                      DICS_FLAG_GLOBAL, 0, DIREG_DEV, KEY_READ);
+
+  if (reg_key == INVALID_HANDLE_VALUE)
+    return false;
+
+  TCHAR value_name[256];

[scottmg, 2016/01/09 01:00:05]

We don't generally use TCHAR, _t, etc., just wchar_t and wcscmp(), etc. below is
probably more normal.

[Bret, 2016/01/12 00:23:47]

Done.

+  DWORD value_name_length = sizeof(value_name);
+  DWORD data_type;
+  BYTE data[128];
+  DWORD data_length = sizeof(data);
+
+  bool found = false;
+  int reg_value_index = 0;
+  while (ERROR_SUCCESS == RegEnumValue(reg_key, reg_value_index, &value_name[0],

[scottmg, 2016/01/09 01:00:05]

Can you use base/win/registry.h RegKey for this? ~RegKey makes sure the
RegCloseKey() doesn't get lost.

[Bret, 2016/01/12 00:23:47]

Done.

+                                       &value_name_length, NULL, &data_type,

[scottmg, 2016/01/09 01:00:06]

NULL -> nullptr throughout.

[Bret, 2016/01/12 00:23:47]

Done.

+                                       &data[0], &data_length)) {
+    if (_tcscmp(_T("EDID"), value_name) == 0) {
+      // 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 height and width (respectively) most

[scottmg, 2016/01/09 01:00:05]

"height and width (respectively)" sounds to me like it'd be 0xf0 for height, and
0x0f for width. Code correct as is? If so, maybe remove the respectively, since
it's clear which would is which.

[Bret, 2016/01/12 00:23:47]

Yeah that's confusing. I'll just let the bitmask speak for itself.

+      //     significant bits
+      *width_mm = ((data[68] & 0xF0) << 4) + data[66];
+      *height_mm = ((data[68] & 0x0F) << 8) + data[67];
+      found = true;

[scottmg, 2016/01/09 01:00:05]

Break here?

[Bret, 2016/01/12 00:23:47]

Done.

+    }
+    ++reg_value_index;
+  }
+
+  RegCloseKey(reg_key);
+  return found;
+}
+
+void 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);
+  int buffer_size;
+  SetupDiGetDeviceInterfaceDetail(device_info_list, &interface_data, NULL, 0,
+                                  (PDWORD)&buffer_size, device_info);

[scottmg, 2016/01/09 01:00:05]

Check that this returns false and GetLastError() is as expected.

[scottmg, 2016/01/09 01:00:06]

No c-style casts. I think buffer_size can just be a DWORD here and remove the
cast entirely?

[Bret, 2016/01/12 00:23:47]

Done.

[Bret, 2016/01/12 00:23:47]

Done.

+  interface_detail->reset(
+      reinterpret_cast<SP_DEVICE_INTERFACE_DETAIL_DATA*>(malloc(buffer_size)));
+  (*interface_detail)->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA);
+  SetupDiGetDeviceInterfaceDetail(device_info_list, &interface_data,
+                                  interface_detail->get(), buffer_size, NULL,
+                                  NULL);

[scottmg, 2016/01/09 01:00:05]

And that this one succeeds.

[Bret, 2016/01/12 00:23:47]

Done.

+}
+
+}  // namespace
+
+namespace gfx {
+
+void GetPhysicalSizeForDisplays(std::vector<DisplaySize>* out) {

[robliao, 2016/01/11 18:42:20]

Can this be part of ScreenWin Display generation?

[Bret, 2016/01/12 00:23:47]

It could, but I'm not clear on what the advantage of that would be. I don't
think most clients of Display care about the physical size? This is also easier
to call for the about:gpu page, since it doesn't have the Display objects easily
accessible.

[robliao, 2016/01/12 01:00:37]

Most, if not all calls to Display are quick cached value lookups. All of the
computation happens during the generation of the display. This would be
following that pattern.

This would also allow you to update the getter to something more display-like
like
gfx::Size GetPhysicalSize()

+  DCHECK(out);
+
+  // get a handle to the list of device interfaces that are monitors via Setup

[scottmg, 2016/01/09 01:00:05]

Capitals start and periods at the end of comments/sentences throughout.

[Bret, 2016/01/12 00:23:47]

Done.

+  base::ScopedGeneric<HDEVINFO, DeviceInfoListScopedTraits> device_info_list(
+      SetupDiGetClassDevs(&GUID_DEVICEINTERFACE_MONITOR, NULL, NULL,
+                          DIGCF_PRESENT | DIGCF_DEVICEINTERFACE));
+
+  if (!device_info_list.is_valid())
+    return;
+
+  // loop over the device interfaces 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(), NULL,
+                                     &GUID_DEVICEINTERFACE_MONITOR,
+                                     interface_index, &interface_data)) {
+    scoped_ptr<SP_DEVICE_INTERFACE_DETAIL_DATA, base::FreeDeleter>
+        interface_detail;
+    SP_DEVINFO_DATA device_info = {0};
+    // this gives us the device path that we need to associate the device
+    // interface from Setup with the display device from EnumDisplayDevices
+    GetInterfaceDetailAndDeviceInfo(device_info_list.get(), interface_data,
+                                    &interface_detail, &device_info);
+
+    // 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};
+    display_device.cb = sizeof(DISPLAY_DEVICE);
+    int display_index = 0;
+    while (EnumDisplayDevices(NULL, 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};
+      attached_device.cb = sizeof(DISPLAY_DEVICE);
+      int attached_index = 0;
+      while (EnumDisplayDevices(display_device.DeviceName, attached_index,
+                                &attached_device,
+                                EDD_GET_DEVICE_INTERFACE_NAME)) {
+        TCHAR* attached_device_id = attached_device.DeviceID;
+        TCHAR* setup_device_path = interface_detail->DevicePath;
+        if (_tcsicmp(attached_device_id, setup_device_path) == 0) {
+          // we've found the monitor that matches the Setup device
+          int width_mm;
+          int height_mm;
+          bool found = GetSizeFromRegistry(device_info_list.get(), device_info,
+                                           &width_mm, &height_mm);
+          if (found) {
+            DisplaySize size(display_device.DeviceName, width_mm, height_mm);
+            out->push_back(size);
+          }
+          break;
+        }
+        ++attached_index;
+      }
+
+      ++display_index;
+    }
+
+    ++interface_index;
+  }
+}
+
+}  // namespace gfx