Fix hi-dpi transitions on Catalina

A few issues here:

First, actually wire up NativeWidgetNSWindowBridge as a DisplayObserver.
In the past, it didn't matter that this code was missing, because we
were getting notified via windowDidChangeBackingProperties. In Catalina,
that call is happening at a different time, resulting us using an
invalid cached version (which is the second issue).

Second, change GetDisplayNearestWindow to call UpdateDisplaysIfNeeded.
There was a bug here wherein we would return displays_[0], even if we
knew (because of displays_require_update_) that that value was out of
date.

Thid, make GetCachedDisplayForScreen be robust to getting a surprise
NSScreen* that we didn't know about. On Catalina, it happens that we
can read -[NSScreen screens] and see that it has changed, before having
received any notifications that such a change would happen (!).

Fourth, listen for NSApplicationDidChangeScreenParametersNotification
notifications. Just because it sounds like a healthy thing to be doing.

Bug: 1021340
Change-Id: Ibe5a6469d9e2c39cd81d0fb19ee2cfe3aedb1488
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1902508Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713490}

components/remote_cocoa/app_shim/native_widget_ns_window_bridge.h

@@ -187,6 +187,8 @@ class REMOTE_COCOA_APP_SHIM_EXPORT NativeWidgetNSWindowBridge
   bool RedispatchKeyEvent(NSEvent* event);
 
   // display::DisplayObserver:
+  void OnDisplayAdded(const display::Display& new_display) override;
+  void OnDisplayRemoved(const display::Display& old_display) override;
   void OnDisplayMetricsChanged(const display::Display& display,
                                uint32_t metrics) override;
 

components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm

@@ -312,9 +312,11 @@ NativeWidgetNSWindowBridge::NativeWidgetNSWindowBridge(
       text_input_host_(text_input_host) {
   DCHECK(GetIdToWidgetImplMap().find(id_) == GetIdToWidgetImplMap().end());
   GetIdToWidgetImplMap().insert(std::make_pair(id_, this));
+  display::Screen::GetScreen()->AddObserver(this);
 }
 
 NativeWidgetNSWindowBridge::~NativeWidgetNSWindowBridge() {
+  display::Screen::GetScreen()->RemoveObserver(this);
   // The delegate should be cleared already. Note this enforces the precondition
   // that -[NSWindow close] is invoked on the hosted window before the
   // destructor is called.
@@ -1122,7 +1124,17 @@ DragDropClient* NativeWidgetNSWindowBridge::drag_drop_client() {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-// NativeWidgetNSWindowBridge, ui::CATransactionObserver
+// NativeWidgetNSWindowBridge, display::DisplayObserver:
+
+void NativeWidgetNSWindowBridge::OnDisplayAdded(
+    const display::Display& display) {
+  UpdateWindowDisplay();
+}
+
+void NativeWidgetNSWindowBridge::OnDisplayRemoved(
+    const display::Display& display) {
+  UpdateWindowDisplay();
+}
 
 void NativeWidgetNSWindowBridge::OnDisplayMetricsChanged(
     const display::Display& display,

ui/display/mac/screen_mac.mm

@@ -32,8 +32,8 @@ Boolean CGDisplayUsesForceToGray(void);
 namespace display {
 namespace {
 
-// The delay to handle the display configuration changes.
-// See comments in ScreenMac::HandleDisplayReconfiguration.
+// The delay to handle the display configuration changes. This is in place to
+// coalesce display update notifications and thereby avoid thrashing.
 const int64_t kConfigureDelayMs = 500;
 
 NSScreen* GetMatchingScreen(const gfx::Rect& match_rect) {
@@ -157,20 +157,27 @@ class ScreenMac : public Screen {
     CGDisplayRegisterReconfigurationCallback(
         ScreenMac::DisplayReconfigurationCallBack, this);
 
+    auto update_block = ^(NSNotification* notification) {
+      OnNSScreensMayHaveChanged();
+    };
+
     NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
     screen_color_change_observer_.reset(
         [[center addObserverForName:NSScreenColorSpaceDidChangeNotification
                              object:nil
                               queue:nil
-                         usingBlock:^(NSNotification* notification) {
-                           configure_timer_.Reset();
-                           displays_require_update_ = true;
-                         }] retain]);
+                         usingBlock:update_block] retain]);
+    screen_params_change_observer_.reset([[center
+        addObserverForName:NSApplicationDidChangeScreenParametersNotification
+                    object:nil
+                     queue:nil
+                usingBlock:update_block] retain]);
   }
 
   ~ScreenMac() override {
     NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
     [center removeObserver:screen_color_change_observer_];
+    [center removeObserver:screen_params_change_observer_];
 
     CGDisplayRemoveReconfigurationCallback(
         ScreenMac::DisplayReconfigurationCallBack, this);
@@ -195,16 +202,18 @@ class ScreenMac : public Screen {
   int GetNumDisplays() const override { return GetAllDisplays().size(); }
 
   const std::vector<Display>& GetAllDisplays() const override {
+    UpdateDisplaysIfNeeded();
     return displays_;
   }
 
   Display GetDisplayNearestWindow(
       gfx::NativeWindow native_window) const override {
-    NSWindow* window = native_window.GetNativeNSWindow();
-    EnsureDisplaysValid();
+    UpdateDisplaysIfNeeded();
+
     if (displays_.size() == 1)
       return displays_[0];
 
+    NSWindow* window = native_window.GetNativeNSWindow();
     if (!window)
       return GetPrimaryDisplay();
 
@@ -277,31 +286,30 @@ class ScreenMac : public Screen {
   static void DisplayReconfigurationCallBack(CGDirectDisplayID display,
                                              CGDisplayChangeSummaryFlags flags,
                                              void* userInfo) {
-    if (flags & kCGDisplayBeginConfigurationFlag)
-      return;
-
     ScreenMac* screen_mac = static_cast<ScreenMac*>(userInfo);
-
-    // Timer::Reset() ensures at least another interval passes before the
-    // associated task runs, effectively coalescing these events.
-    screen_mac->configure_timer_.Reset();
-    screen_mac->displays_require_update_ = true;
+    screen_mac->OnNSScreensMayHaveChanged();
   }
 
  private:
   Display GetCachedDisplayForScreen(NSScreen* screen) const {
-    EnsureDisplaysValid();
+    UpdateDisplaysIfNeeded();
     const CGDirectDisplayID display_id = [[[screen deviceDescription]
         objectForKey:@"NSScreenNumber"] unsignedIntValue];
     for (const Display& display : displays_) {
       if (display_id == display.id())
         return display;
     }
-    NOTREACHED();  // Asked for a hidden/sleeping/mirrored screen?
+    // In theory, this should not be reached, because |displays_require_update_|
+    // should have been set prior to -[NSScreen screens] changing. In practice,
+    // on Catalina, it has been observed that -[NSScreen screens] changes before
+    // any notifications are received.
+    // https://crbug.com/1021340.
+    OnNSScreensMayHaveChanged();
+    DLOG(ERROR) << "Value of -[NSScreen screens] changed before notification.";
     return BuildDisplayForScreen(screen);
   }
 
-  void EnsureDisplaysValid() const {
+  void UpdateDisplaysIfNeeded() const {
     if (displays_require_update_) {
       displays_ = BuildDisplaysFromQuartz();
       displays_require_update_ = false;
@@ -309,7 +317,7 @@ class ScreenMac : public Screen {
   }
 
   void ConfigureTimerFired() {
-    EnsureDisplaysValid();
+    UpdateDisplaysIfNeeded();
     change_notifier_.NotifyDisplaysChanged(old_displays_, displays_);
     old_displays_ = displays_;
   }
@@ -323,7 +331,7 @@ class ScreenMac : public Screen {
     // It would be ridiculous to have this many displays connected, but
     // CGDirectDisplayID is just an integer, so supporting up to this many
     // doesn't hurt.
-    CGDirectDisplayID online_displays[128];
+    CGDirectDisplayID online_displays[1024];
     CGDisplayCount online_display_count = 0;
     if (CGGetOnlineDisplayList(base::size(online_displays), online_displays,
                                &online_display_count) != kCGErrorSuccess) {
@@ -359,21 +367,32 @@ class ScreenMac : public Screen {
                             : displays;
   }
 
-  // The displays currently attached to the device. Cached.
+  void OnNSScreensMayHaveChanged() const {
+    // Timer::Reset() ensures at least another interval passes before the
+    // associated task runs, effectively coalescing these events.
+    configure_timer_.Reset();
+    displays_require_update_ = true;
+  }
+
+  // The displays currently attached to the device. Updated by
+  // UpdateDisplaysIfNeeded.
   mutable std::vector<Display> displays_;
 
-  // Set whenever the CGDisplayRegisterReconfigurationCallback is invoked and
-  // cleared when |displays_| is updated by BuildDisplaysFromQuartz().
+  // Whether or not |displays_| might need to be upated. Set in
+  // OnNSScreensMayHaveChanged, and un-set by UpdateDisplaysIfNeeded.
   mutable bool displays_require_update_ = false;
 
-  // The displays last communicated to DisplayChangeNotifier.
-  std::vector<Display> old_displays_;
+  // The timer to delay configuring outputs and notifying observers (to coalesce
+  // several updates into one update).
+  mutable base::RetainingOneShotTimer configure_timer_;
 
-  // The timer to delay configuring outputs and notifying observers.
-  base::RetainingOneShotTimer configure_timer_;
+  // The displays last communicated to the DisplayChangeNotifier.
+  std::vector<Display> old_displays_;
 
-  // The observer notified by NSScreenColorSpaceDidChangeNotification.
+  // The observers notified by NSScreenColorSpaceDidChangeNotification and
+  // NSApplicationDidChangeScreenParametersNotification.
   base::scoped_nsobject<id> screen_color_change_observer_;
+  base::scoped_nsobject<id> screen_params_change_observer_;
 
   DisplayChangeNotifier change_notifier_;