MacViews: Ensure WidgetDelegate::OnWidgetMove() is invoked consistently.

Since AppKit coordinates are flipped, NSWindowDelegate may not send
move changes when the top-left corner of the window moves relative to
the screen. Similarly, it will send a move when resizing a window
vertically via the bottom edge, which other platforms do not class as a
move.

To fix, track the top-left window corner relative to the top-left of the
screen and ensure the toolkit is notified when it changes.

OnWidgetMove() had no test coverage, so add a cross-platform widget test.
The test exposes a discrepancy on Windows only, which is likely a bug to
investigate in a follow-up.

Bug: 862217, 864938
Change-Id: I1fb0a6c2b463ea7cbd823fbe4d65545991564d50
Reviewed-on: https://chromium-review.googlesource.com/1141555Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576255}

ui/views/cocoa/bridged_native_widget.h

@@ -319,6 +319,13 @@ class VIEWS_EXPORT BridgedNativeWidget
   // has its own copy, but doesn't provide access to it).
   gfx::Rect bounds_before_fullscreen_;
 
+  // Tracks the origin of the window (from the top-left of the screen) when it
+  // was last reported to toolkit-views. Needed to trigger move notifications
+  // associated with a window resize since AppKit won't send move notifications
+  // when the top-left point of the window moves vertically. The origin of the
+  // window in AppKit coordinates is not actually changing in this case.
+  gfx::Point last_window_frame_origin_;
+
   // Whether this window wants to be fullscreen. If a fullscreen animation is in
   // progress then it might not be actually fullscreen.
   bool target_fullscreen_state_;

ui/views/cocoa/bridged_native_widget.mm

@@ -727,6 +727,14 @@ void BridgedNativeWidget::ToggleDesiredFullscreenState(bool async) {
 }
 
 void BridgedNativeWidget::OnSizeChanged() {
+  const gfx::Rect new_bounds = native_widget_mac_->GetWindowBoundsInScreen();
+  if (new_bounds.origin() != last_window_frame_origin_) {
+    native_widget_mac_->GetWidget()->OnNativeWidgetMove();
+    last_window_frame_origin_ = new_bounds.origin();
+  }
+
+  // Note we can't use new_bounds.size(), since it includes the titlebar for the
+  // purposes of detecting a window move.
   gfx::Size new_size = GetClientAreaSize();
   native_widget_mac_->GetWidget()->OnNativeWidgetSizeChanged(new_size);
   if (layer()) {
@@ -737,6 +745,13 @@ void BridgedNativeWidget::OnSizeChanged() {
 }
 
 void BridgedNativeWidget::OnPositionChanged() {
+  // When a window grows vertically, the AppKit origin changes, but as far as
+  // tookit-views is concerned, the window hasn't moved. Suppress these.
+  const gfx::Rect new_bounds = native_widget_mac_->GetWindowBoundsInScreen();
+  if (new_bounds.origin() == last_window_frame_origin_)
+    return;
+
+  last_window_frame_origin_ = new_bounds.origin();
   native_widget_mac_->GetWidget()->OnNativeWidgetMove();
 }
 

ui/views/widget/widget_unittest.cc

@@ -949,6 +949,65 @@ TEST_F(WidgetObserverTest, WidgetBoundsChangedNative) {
   EXPECT_FALSE(widget_bounds_changed());
 }
 
+namespace {
+
+class MoveTrackingTestDesktopWidgetDelegate : public TestDesktopWidgetDelegate {
+ public:
+  int move_count() const { return move_count_; }
+
+  // WidgetDelegate:
+  void OnWidgetMove() override { ++move_count_; }
+
+ private:
+  int move_count_ = 0;
+};
+
+}  // namespace
+
+// An extension to the WidgetBoundsChangedNative test above to ensure move
+// notifications propagate to the WidgetDelegate.
+TEST_F(WidgetObserverTest, OnWidgetMovedWhenOriginChangesNative) {
+  MoveTrackingTestDesktopWidgetDelegate delegate;
+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
+  delegate.InitWidget(params);
+  Widget* widget = delegate.GetWidget();
+  widget->Show();
+  widget->SetBounds(gfx::Rect(100, 100, 300, 200));
+
+  const int moves_during_init = delegate.move_count();
+
+#if defined(OS_WIN)
+  // Windows reliably notifies twice per origin change. https://crbug.com/864938
+  constexpr int kDeltaPerMove = 2;
+#else
+  constexpr int kDeltaPerMove = 1;
+#endif
+
+  // Resize without changing origin. No move.
+  widget->SetBounds(gfx::Rect(100, 100, 310, 210));
+  EXPECT_EQ(moves_during_init, delegate.move_count());
+
+  // Move without changing size. Moves.
+  widget->SetBounds(gfx::Rect(110, 110, 310, 210));
+  EXPECT_EQ(moves_during_init + kDeltaPerMove, delegate.move_count());
+
+  // Changing both moves.
+  widget->SetBounds(gfx::Rect(90, 90, 330, 230));
+  EXPECT_EQ(moves_during_init + 2 * kDeltaPerMove, delegate.move_count());
+
+  // Just grow vertically. On Mac, this changes the AppKit origin since it is
+  // from the bottom left of the screen, but there is no move as far as views is
+  // concerned.
+  widget->SetBounds(gfx::Rect(90, 90, 330, 240));
+  // No change.
+  EXPECT_EQ(moves_during_init + 2 * kDeltaPerMove, delegate.move_count());
+
+  // For a similar reason, move the widget down by the same amount that it grows
+  // vertically. The AppKit origin does not change, but it is a move.
+  widget->SetBounds(gfx::Rect(90, 100, 330, 250));
+  EXPECT_EQ(moves_during_init + 3 * kDeltaPerMove, delegate.move_count());
+}
+
 // Test correct behavior when widgets close themselves in response to visibility
 // changes.
 TEST_F(WidgetObserverTest, ClosingOnHiddenParent) {