Multiple DPI Tracking for ScreenWin

ui/display/win/screen_win.cc

Index: ui/display/win/screen_win.cc
diff --git a/ui/display/win/screen_win.cc b/ui/display/win/screen_win.cc
index 654059ce32228515e73d3a9a895d786d4d708976..d839b1ff6b827275cae9529145264efd9fa29f8f 100644
--- a/ui/display/win/screen_win.cc
+++ b/ui/display/win/screen_win.cc
@@ -18,15 +18,16 @@
 #include "ui/display/win/scaling_util.h"
 #include "ui/display/win/screen_win_display.h"
 #include "ui/gfx/geometry/point.h"
-#include "ui/gfx/geometry/point_conversions.h"
 #include "ui/gfx/geometry/rect.h"
-#include "ui/gfx/geometry/rect_conversions.h"
-#include "ui/gfx/geometry/size_conversions.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/gfx/geometry/vector2d.h"
 
 namespace display {
 namespace win {
 namespace {
 
+ScreenWin* g_screen_win_instance = nullptr;

[oshima, 2016/05/26 21:52:58]

nit: screen_win_instance_

https://www.chromium.org/developers/coding-style/cpp-dos-and-donts


I prefer to just use the utility function though:

gfx::ScreenWin* GetScreenWin() {
   return static_cast<ScreenWin*>(Screen::GetScreen());
}

Will this work?

[robliao, 2016/05/26 22:28:47]

https://google.github.io/styleguide/cppguide.html#Variable_Names
Global variables follow this style (g_hacker_case_var).

This is well established in Chromium
https://code.google.com/p/chromium/codesearch#search/&q=%5C%20g_%20-file:thir...



The utility function is unsafe and will crash with tests that use TestScreen
provided by Aura:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/test/test_...

This is the safest way to make sure we're actually using a ScreenWin.

[oshima, 2016/05/26 22:54:47]

which I'm trying to fix when I see it to be more compliant to the style guide.

g_ should mean that it's global that can be accessed in multiple files, and
discouraged, while this one is not.
Sounds like it should not happen, and such test should be updated to use
platform's screen instead, otherwise, there are two screen instances/impl, and
you may get incorrect results.

[robliao, 2016/05/26 23:19:24]

As commonly used, a global variable doesn't guarantee external linkage and it
doesn't guarantee that it can be accessed by other files.

From the style guide:
There are no special requirements for global variables, which should be rare in
any case, but if you use one, consider prefixing it with g_ or some other marker
to easily distinguish it from local variables.

This anonymous namespace makes this variable global to this file. 

The linked search results show also other consistent uses like
g_screen_win_instance.

Many global LazyInstances contained in anonymouse namespaces also follow the
same g_* naming pattern.


Trailing underscores are reserved for data members of classes:
https://google.github.io/styleguide/cppguide.html#Variable_Names


Which style reference do you refer to with regards to the naming of this
variable?
I suspect those tests are attempting to be platform agnostic. In either case, we
aren't guaranteed that the platform screen is always returned by
Screen::GetScreen.

[oshima, 2016/05/26 23:43:13]

extenral linkage has nothing to do with g_ naming. g_ simply means it's global
variable
and may be, and will be accessed by other files (otherwise, you can put it in
anonymous namespace).

It's discouraged and should be avoided if possible. One in anonymous namespace
is encouraged,
and that's big difference IMO.
It's talking about global variable, not anonymous namespace.

Also, do and don't, and this https://www.chromium.org/developers/coding-style
are addendum to google style guide.

[robliao, 2016/05/27 00:47:13]

Can you cite the rule you're discussing in those two docs? I can't seem to find
it.

https://www.chromium.org/developers/coding-style
Doesn't seem to discuss global variables or anonymous namespaces

https://www.chromium.org/developers/coding-style/cpp-dos-and-donts
Discusses moving static implementation details to anonymous namespaces, which is
what's going on here.

[oshima, 2016/05/27 02:02:04]

First of all, google style guide has *no recommendation* for a variable in
anonymous namespace. The following section is for global variable, not for
anonymous namespace. 

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

--------------
Global Variables

There are no special requirements for global variables, which should be rare in
any case, but if you use one, consider prefixing it with g_ or some other marker
to easily distinguish it from local variables.
--------------

g_ is a good choice (but not required), but again it's for global variable, not
for a variable in anonymous namespace.

Chromium has a recommendation:

https://www.chromium.org/developers/coding-style/cpp-dos-and-donts

and it has a clear example how to define such a variable.

[robliao, 2016/05/27 17:15:30]

Please note that the reference you cited from the style guide is the exact
reference I gave in Message 4 for this thread. 

The request here is to cite from 
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts

I think I see the reference now in the cpp-dos-and-donts. That's in fact a typo 
namespace {
BigImplementationDetail detail_;
}  // namespace

And in direct violation of 
https://google.github.io/styleguide/cppguide.html#Variable_Names

+
 std::vector<DisplayInfo> FindAndRemoveTouchingDisplayInfos(
     const DisplayInfo& ref_display_info,
     std::vector<DisplayInfo>* display_infos) {
@@ -141,6 +142,8 @@ BOOL CALLBACK EnumMonitorCallback(HMONITOR monitor,
   std::vector<DisplayInfo>* display_infos =
       reinterpret_cast<std::vector<DisplayInfo>*>(data);
   DCHECK(display_infos);
+  // TODO(robliao): When ready, replace the GetDPIScale with GetDpiForMonitor
+  // to get the actual DPI for the HMONITOR.
   display_infos->push_back(DisplayInfo(MonitorInfoFromHMONITOR(monitor),
                                        GetDPIScale()));
   return TRUE;
@@ -155,78 +158,114 @@ std::vector<DisplayInfo> GetDisplayInfosFromSystem() {
   return display_infos;
 }
 
+// Returns a point in |to_origin|'s coordinates and position scaled by
+// |scale_factor|.
+gfx::Point ScalePointRelative(const gfx::Point& from_origin,
+                              const gfx::Point& to_origin,
+                              const float scale_factor,
+                              const gfx::Point& point) {
+  gfx::Vector2d from_origin_vector(from_origin.x(), from_origin.y());
+  gfx::Vector2d to_origin_vector(to_origin.x(), to_origin.y());
+  gfx::Point scaled_relative_point(
+      gfx::ScaleToFlooredPoint(point - from_origin_vector, scale_factor));
+  return scaled_relative_point + to_origin_vector;
+}
+
 }  // namespace
 
 ScreenWin::ScreenWin() {
+  DCHECK(!g_screen_win_instance);
+  g_screen_win_instance = this;
   Initialize();
 }
 
-ScreenWin::~ScreenWin() = default;
+ScreenWin::~ScreenWin() {
+  DCHECK(g_screen_win_instance == this);

[oshima, 2016/05/26 21:52:58]

DCHECK_EQ

[robliao, 2016/05/26 22:28:47]

Done.

+  g_screen_win_instance = nullptr;
+}
 
 // static
 gfx::Point ScreenWin::ScreenToDIPPoint(const gfx::Point& pixel_point) {
-  return ScaleToFlooredPoint(pixel_point, 1.0f / GetDPIScale());
+  const ScreenWinDisplay screen_win_display =
+      GetScreenWinDisplayVia(&ScreenWin::GetScreenWinDisplayNearestScreenPoint,
+                             pixel_point);
+  const display::Display display = screen_win_display.display();
+  return ScalePointRelative(screen_win_display.pixel_bounds().origin(),
+                            display.bounds().origin(),
+                            1.0f / display.device_scale_factor(),
+                            pixel_point);
 }
 
 // static
 gfx::Point ScreenWin::DIPToScreenPoint(const gfx::Point& dip_point) {
-  return ScaleToFlooredPoint(dip_point, GetDPIScale());
+  const ScreenWinDisplay screen_win_display =
+      GetScreenWinDisplayVia(&ScreenWin::GetScreenWinDisplayNearestDIPPoint,
+                             dip_point);
+  const display::Display display = screen_win_display.display();
+  return ScalePointRelative(display.bounds().origin(),
+                            screen_win_display.pixel_bounds().origin(),
+                            display.device_scale_factor(),
+                            dip_point);
 }
 
 // static
 gfx::Point ScreenWin::ClientToDIPPoint(HWND hwnd,
                                        const gfx::Point& client_point) {
-  // TODO(robliao): Get the scale factor from |hwnd|.
-  return ScreenToDIPPoint(client_point);
+  return ScaleToFlooredPoint(client_point, 1.0f / GetScaleFactorForHWND(hwnd));
 }
 
 // static
 gfx::Point ScreenWin::DIPToClientPoint(HWND hwnd, const gfx::Point& dip_point) {
-  // TODO(robliao): Get the scale factor from |hwnd|.
-  return DIPToScreenPoint(dip_point);
+  float scale_factor = GetScaleFactorForHWND(hwnd);
+  return ScaleToFlooredPoint(dip_point, scale_factor);
 }
 
 // static
 gfx::Rect ScreenWin::ScreenToDIPRect(HWND hwnd, const gfx::Rect& pixel_bounds) {
-  // It's important we scale the origin and size separately. If we instead
-  // calculated the size from the floored origin and ceiled right the size could
-  // vary depending upon where the two points land. That would cause problems
-  // for the places this code is used (in particular mapping from native window
-  // bounds to DIPs).
-  return gfx::Rect(ScreenToDIPPoint(pixel_bounds.origin()),
-                   ScreenToDIPSize(hwnd, pixel_bounds.size()));
+  float scale_factor = hwnd ?
+      GetScaleFactorForHWND(hwnd) :
+      GetScreenWinDisplayVia(
+          &ScreenWin::GetScreenWinDisplayNearestScreenRect, pixel_bounds).
+              display().device_scale_factor();
+  gfx::Rect dip_rect = ScaleToEnclosingRect(pixel_bounds, 1.0f / scale_factor);
+  dip_rect.set_origin(ScreenToDIPPoint(pixel_bounds.origin()));
+  return dip_rect;
 }
 
 // static
 gfx::Rect ScreenWin::DIPToScreenRect(HWND hwnd, const gfx::Rect& dip_bounds) {
-  // See comment in ScreenToDIPRect for why we calculate size like this.
-  return gfx::Rect(DIPToScreenPoint(dip_bounds.origin()),
-                   DIPToScreenSize(hwnd, dip_bounds.size()));
+  float scale_factor = hwnd ?
+      GetScaleFactorForHWND(hwnd) :
+      GetScreenWinDisplayVia(
+          &ScreenWin::GetScreenWinDisplayNearestDIPRect, dip_bounds).display().
+              device_scale_factor();
+  gfx::Rect screen_rect = ScaleToEnclosingRect(dip_bounds, scale_factor);
+  screen_rect.set_origin(DIPToScreenPoint(dip_bounds.origin()));
+  return screen_rect;
 }
 
 // static
 gfx::Rect ScreenWin::ClientToDIPRect(HWND hwnd, const gfx::Rect& pixel_bounds) {
-  return ScreenToDIPRect(hwnd, pixel_bounds);
+  return ScaleToEnclosingRect(pixel_bounds, 1.0f / GetScaleFactorForHWND(hwnd));
 }
 
 // static
 gfx::Rect ScreenWin::DIPToClientRect(HWND hwnd, const gfx::Rect& dip_bounds) {
-  return DIPToScreenRect(hwnd, dip_bounds);
+  return ScaleToEnclosingRect(dip_bounds, GetScaleFactorForHWND(hwnd));
 }
 
 // static
 gfx::Size ScreenWin::ScreenToDIPSize(HWND hwnd,
                                      const gfx::Size& size_in_pixels) {
   // Always ceil sizes. Otherwise we may be leaving off part of the bounds.
-  // TODO(robliao): Get the scale factor from |hwnd|.
-  return ScaleToCeiledSize(size_in_pixels, 1.0f / GetDPIScale());
+  return ScaleToCeiledSize(size_in_pixels, 1.0f / GetScaleFactorForHWND(hwnd));
 }
 
 // static
 gfx::Size ScreenWin::DIPToScreenSize(HWND hwnd, const gfx::Size& dip_size) {
+  float scale_factor = GetScaleFactorForHWND(hwnd);
   // Always ceil sizes. Otherwise we may be leaving off part of the bounds.
-  // TODO(robliao): Get the scale factor from |hwnd|.
-  return ScaleToCeiledSize(dip_size, GetDPIScale());
+  return ScaleToCeiledSize(dip_size, scale_factor);
 }
 
 HWND ScreenWin::GetHWNDFromNativeView(gfx::NativeView window) const {
@@ -306,6 +345,18 @@ void ScreenWin::RemoveObserver(display::DisplayObserver* observer) {
   change_notifier_.RemoveObserver(observer);
 }
 
+gfx::Rect ScreenWin::ScreenToDIPRectInWindow(
+    gfx::NativeView view, const gfx::Rect& screen_rect) const {
+  HWND hwnd = view ? GetHWNDFromNativeView(view) : nullptr;
+  return ScreenToDIPRect(hwnd, screen_rect);
+}
+
+gfx::Rect ScreenWin::DIPToScreenRectInWindow(gfx::NativeView view,
+                                             const gfx::Rect& dip_rect) const {
+  HWND hwnd = view ? GetHWNDFromNativeView(view) : nullptr;
+  return DIPToScreenRect(hwnd, dip_rect);
+}
+
 void ScreenWin::UpdateFromDisplayInfos(
     const std::vector<DisplayInfo>& display_infos) {
   screen_win_displays_ = DisplayInfosToScreenWinDisplays(display_infos);
@@ -369,6 +420,39 @@ ScreenWinDisplay ScreenWin::GetScreenWinDisplayNearestScreenPoint(
   return GetScreenWinDisplay(MonitorInfoFromScreenPoint(screen_point));
 }
 
+ScreenWinDisplay ScreenWin::GetScreenWinDisplayNearestDIPPoint(
+    const gfx::Point& dip_point) const {
+  ScreenWinDisplay primary_screen_win_display;
+  for (const auto& screen_win_display : screen_win_displays_) {
+    display::Display display = screen_win_display.display();
+    const gfx::Rect dip_bounds = display.bounds();
+    if (dip_bounds.Contains(dip_point))
+      return screen_win_display;
+    else if (dip_bounds.origin().IsOrigin())
+      primary_screen_win_display = screen_win_display;
+  }
+  return primary_screen_win_display;
+}
+
+ScreenWinDisplay ScreenWin::GetScreenWinDisplayNearestDIPRect(
+    const gfx::Rect& dip_rect) const {
+  ScreenWinDisplay closest_screen_win_display;
+  int64_t closest_distance_squared = INT64_MAX;
+  for (const auto& screen_win_display : screen_win_displays_) {
+    display::Display display = screen_win_display.display();
+    gfx::Rect dip_bounds = display.bounds();
+    int64_t distance_squared = SquaredDistanceBetweenRects(dip_rect,
+                                                           dip_bounds);
+    if (distance_squared == 0) {
+      return screen_win_display;
+    } else if (distance_squared < closest_distance_squared) {
+      closest_distance_squared = distance_squared;
+      closest_screen_win_display = screen_win_display;
+    }
+  }
+  return closest_screen_win_display;
+}
+
 ScreenWinDisplay ScreenWin::GetPrimaryScreenWinDisplay() const {
   MONITORINFOEX monitor_info = MonitorInfoFromWindow(nullptr,
                                                      MONITOR_DEFAULTTOPRIMARY);
@@ -394,5 +478,27 @@ ScreenWinDisplay ScreenWin::GetScreenWinDisplay(
   return ScreenWinDisplay();
 }
 
+// static
+float ScreenWin::GetScaleFactorForHWND(HWND hwnd) {
+  if (!g_screen_win_instance)

[oshima, 2016/05/26 21:52:58]

when/how can this happen?

[robliao, 2016/05/26 22:28:47]

This happens in tests that don't use ScreenWin and yet call scaling APIs. These
tests rely on the default device scale factor for the scale instead.

[oshima, 2016/05/26 22:54:47]

Shouldn't such tests should create screen object? Again, I want to avoid having
mixing two unless it makes sense.

[robliao, 2016/05/26 23:19:24]

I don't think we want to force these tests to do this if a screen object is not
important to what they're checking. They can't easily mock a platform screen.
These tests aren't generally checking scaling computations.

[oshima, 2016/05/26 23:43:13]

Which test is it? I want to be convinced because we should avoid this type of
code in general.

Say, if someone add a production code that calls this before creating screen
object, it will use the wrong value without us knowing it.

At least, we should make it test specific (fails in production).

[robliao, 2016/05/27 00:47:13]

I'll get back to you on this at the patchset that had the global test failure is
gone by now (happened many months back).

[robliao, 2016/05/27 01:39:58]

The ash_unittests fail:
https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng...

As we expect since they're using their own test screen.

[oshima, 2016/05/27 02:02:04]

The link seems to be obsolete.
Ash does not use test screen. It use real ash screen, which means that we're
mixing two screen logic, which may imply real problem in ash. Can you run bots
again?

[robliao, 2016/05/27 17:15:30]

This is the full list of failures due to either the use of aura::TestScreen or
with an uninitialized Screen (ignore the display_unittests as those are expected
to fail since they test that the functions still work in the absence of a
ScreenWin).
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...

[robliao, 2016/05/27 17:51:34]

Since stacks aren't working, you can use this:
https://gist.github.com/anonymous/031563dc457d70e7c1181dee02053bbc

+    return ScreenWinDisplay().display().device_scale_factor();
+
+  DCHECK(hwnd);
+  HWND rootHwnd = g_screen_win_instance->GetRootWindow(hwnd);
+  ScreenWinDisplay screen_win_display =
+      g_screen_win_instance->GetScreenWinDisplayNearestHWND(rootHwnd);
+  return screen_win_display.display().device_scale_factor();
+}
+
+// static
+template <typename Getter, typename GetterType>
+ScreenWinDisplay ScreenWin::GetScreenWinDisplayVia(Getter getter,
+                                                   GetterType value) {
+  if (!g_screen_win_instance)
+    return ScreenWinDisplay();
+
+  return (g_screen_win_instance->*getter)(value);
+}
+
 }  // namespace win
 }  // namespace display