Refactor screen/monitor so that gfx::Screen returns monitor object.

ui/gfx/screen_win.cc

Index: ui/gfx/screen_win.cc
diff --git a/ui/gfx/screen_win.cc b/ui/gfx/screen_win.cc
index ea74c8245f742ce70071d8520827b85a56d92830..2aaef6184085215763b3b1a2387f8f198cbae902 100644
--- a/ui/gfx/screen_win.cc
+++ b/ui/gfx/screen_win.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -6,8 +6,13 @@
 
 #include <windows.h>
 
+#include "base/logging.h"
+#include "ui/gfx/monitor.h"
+
 namespace {
 
+gfx::SimpleMonitor* g_monitor = NULL;
+
 MONITORINFO GetMonitorInfoForMonitor(HMONITOR monitor) {
   MONITORINFO monitor_info = { 0 };
   monitor_info.cbSize = sizeof(monitor_info);
@@ -15,6 +20,22 @@ MONITORINFO GetMonitorInfoForMonitor(HMONITOR monitor) {
   return monitor_info;
 }
 
+gfx::Monitor* GetEmptyMonitor() {

[Ben Goodger (Google), 2012/04/11 21:05:58]

So even though you say it's temporary, it really troubles me that we'd add code
that has this bug. It means that code even within the same call stack could have
unpredictable behavior if any of these functions are called with different
monitors. I can't guarantee that doesn't happen in Chrome today, so as a result
I'm not really comfortable with this going in even temporarily.

Here's my question.

Monitor is actually pretty simple. It's bounds, work area dimensions, scale
factor etc. i.e. it's just data. It should just be a struct, like MONITORINFO,
but cross platform. The gfx::Screen methods that currently return a
gfx::Monitor* could take a gfx::Monitor* out param and prefill the struct the
caller holds on the stack.

If you need this struct to carry a handle to the native monitor you can invent a
typedef for that too.

[oshima, 2012/04/11 22:05:07]

Ok, that's good suggestion. I'll make that change.

+  if (!g_monitor)
+    g_monitor = new gfx::SimpleMonitor();
+  g_monitor->set_bounds(gfx::Rect());
+  g_monitor->set_work_area(gfx::Rect());
+  return g_monitor;
+}
+
+gfx::Monitor* GetMonitor(MONITORINFO& monitor_info) {
+  if (!g_monitor)
+    g_monitor = new gfx::SimpleMonitor();
+  g_monitor->set_bounds(gfx::Rect(monitor_info.rcMonitor));
+  g_monitor->set_work_area(gfx::Rect(monitor_info.rcWork));
+  return g_monitor;
+}
+
 }  // namespace
 
 namespace gfx {
@@ -27,79 +48,52 @@ gfx::Point Screen::GetCursorScreenPoint() {
 }
 
 // static
-gfx::Rect Screen::GetMonitorWorkAreaNearestWindow(gfx::NativeWindow window) {
-  MONITORINFO monitor_info;
-  monitor_info.cbSize = sizeof(monitor_info);
-  GetMonitorInfo(MonitorFromWindow(window, MONITOR_DEFAULTTONEAREST),
-                 &monitor_info);
-  return gfx::Rect(monitor_info.rcWork);
+gfx::NativeWindow Screen::GetWindowAtCursorScreenPoint() {
+  POINT location;
+  return GetCursorPos(&location) ? WindowFromPoint(location) : NULL;
+}
+
+// static
+int Screen::GetNumMonitors() {
+  return GetSystemMetrics(SM_CMONITORS);
 }
 
 // static
-gfx::Rect Screen::GetMonitorAreaNearestWindow(gfx::NativeWindow window) {
+const gfx::Monitor* Screen::GetMonitorNearestWindow(gfx::NativeWindow window) {
   MONITORINFO monitor_info;
   monitor_info.cbSize = sizeof(monitor_info);
   GetMonitorInfo(MonitorFromWindow(window, MONITOR_DEFAULTTONEAREST),
                  &monitor_info);
-  return gfx::Rect(monitor_info.rcMonitor);
+  return GetMonitor(monitor_info);
 }
 
-static gfx::Rect GetMonitorAreaOrWorkAreaNearestPoint(const gfx::Point& point,
-                                                      bool work_area) {
+// static
+const gfx::Monitor* Screen::GetMonitorNearestPoint(const gfx::Point& point) {
   POINT initial_loc = { point.x(), point.y() };
   HMONITOR monitor = MonitorFromPoint(initial_loc, MONITOR_DEFAULTTONEAREST);
   MONITORINFO mi = {0};
   mi.cbSize = sizeof(mi);
   if (monitor && GetMonitorInfo(monitor, &mi))
-    return gfx::Rect(work_area ? mi.rcWork : mi.rcMonitor);
-  return gfx::Rect();
-}
-
-// static
-gfx::Rect Screen::GetMonitorWorkAreaNearestPoint(const gfx::Point& point) {
-  return GetMonitorAreaOrWorkAreaNearestPoint(point, true);
-}
-
-// static
-gfx::Rect Screen::GetMonitorAreaNearestPoint(const gfx::Point& point) {
-  return GetMonitorAreaOrWorkAreaNearestPoint(point, false);
+    return GetMonitor(mi);
+  return GetEmptyMonitor();
 }
 
 // static
-gfx::Rect Screen::GetPrimaryMonitorWorkArea() {
-  return gfx::Rect(GetMonitorInfoForMonitor(MonitorFromWindow(NULL,
-      MONITOR_DEFAULTTOPRIMARY)).rcWork);
+const gfx::Monitor* Screen::GetPrimaryMonitor() {
+  MONITORINFO mi = GetMonitorInfoForMonitor(
+      MonitorFromWindow(NULL, MONITOR_DEFAULTTOPRIMARY));
+  gfx::Monitor* monitor = GetMonitor(mi);
+  DCHECK_EQ(GetSystemMetrics(SM_CXSCREEN), monitor->GetSize().width());
+  DCHECK_EQ(GetSystemMetrics(SM_CYSCREEN), monitor->GetSize().height());
+  return monitor;
 }
 
 // static
-gfx::Rect Screen::GetPrimaryMonitorBounds() {
-  return gfx::Rect(GetMonitorInfoForMonitor(MonitorFromWindow(NULL,
-      MONITOR_DEFAULTTOPRIMARY)).rcMonitor);
-}
-
-// static
-gfx::Rect Screen::GetMonitorWorkAreaMatching(const gfx::Rect& match_rect) {
+const gfx::Monitor* Screen::GetMonitorMatching(const gfx::Rect& match_rect) {
   RECT other_bounds_rect = match_rect.ToRECT();
   MONITORINFO monitor_info = GetMonitorInfoForMonitor(MonitorFromRect(
       &other_bounds_rect, MONITOR_DEFAULTTONEAREST));
-  return gfx::Rect(monitor_info.rcWork);
-}
-
-// static
-gfx::NativeWindow Screen::GetWindowAtCursorScreenPoint() {
-  POINT location;
-  return GetCursorPos(&location) ? WindowFromPoint(location) : NULL;
-}
-
-// static
-gfx::Size Screen::GetPrimaryMonitorSize() {
-  return gfx::Size(GetSystemMetrics(SM_CXSCREEN),
-                   GetSystemMetrics(SM_CYSCREEN));
-}
-
-// static
-int Screen::GetNumMonitors() {
-  return GetSystemMetrics(SM_CMONITORS);
+  return GetMonitor(monitor_info);
 }
 
 }  // namespace gfx