MacViews: Implement Tab Dragging

ui/views/cocoa/cocoa_window_move_loop.mm

+// Copyright 2016 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.
+
+#include "ui/views/cocoa/cocoa_window_move_loop.h"
+
+#import <Cocoa/Cocoa.h>
+
+#include "base/run_loop.h"
+#include "ui/display/screen.h"
+#import "ui/gfx/mac/coordinate_conversion.mm"
+#import "ui/views/cocoa/bridged_content_view.h"
+#import "ui/views/cocoa/bridged_native_widget.h"
+
+// CocoaWindowMoveLoop can be deleted just before its local event monitor is
+// processed and in that case it's too late to call removeMonitor: and we have a
+// dangling this pointer. Use a proxy Obj-C class to store the weakptr.

[tapted, 2016/05/23 07:29:28]

This needs to say something about your findings regarding how AppKit notifies
event monitors. E.g. like

~This is required because removing an event monitor is not enough to guarantee
it receives no more events. AppKit caches the list of event monitors and then
<does that other stuff you said it does>

[themblsha, 2016/05/26 15:13:25]

Reworded the comment. Actually the class is no longer required, as I'm no longer
registering for the NSKeyDownMask events, but potentially the similar stuff
could happen in the future for the other kind of events, so I think we should
leave this safeguard in place.

+@interface WeakCocoaWindowMoveLoop : NSObject {
+ @private
+  base::WeakPtr<views::CocoaWindowMoveLoop> weak_;
+}
+@end
+
+@implementation WeakCocoaWindowMoveLoop
+- (id)initWithWeakPtr:(const base::WeakPtr<views::CocoaWindowMoveLoop>&)weak {
+  if ((self = [super init])) {
+    weak_ = weak;
+  }
+  return self;
+}
+
+- (base::WeakPtr<views::CocoaWindowMoveLoop>&)weak {
+  return weak_;
+}
+@end
+
+namespace views {
+
+CocoaWindowMoveLoop::CocoaWindowMoveLoop(
+    BridgedNativeWidget* owner,
+    const gfx::Point& initial_mouse_in_screen)
+    : owner_(owner),
+      initial_mouse_in_screen_(initial_mouse_in_screen),
+      weak_factory_(this) {
+  // AppKit continues to send mouse events to the window, but toolkit-views
+  // doesn't expect them during RunMoveLoop().
+  [BridgedContentView setIgnoreMouseEvents:YES];
+  owner_->SetDraggable(true);
+}
+
+CocoaWindowMoveLoop::~CocoaWindowMoveLoop() {
+  owner_->SetDraggable(false);
+  // Note the crafted events in Run() should not make their way through to
+  // toolkit-views, but regular events after that should be. So stop ignoring.
+  [BridgedContentView setIgnoreMouseEvents:NO];
+
+  // Handle the pathological case, where |this| is destroyed while running.
+  if (exit_reason_ref_) {
+    *exit_reason_ref_ = WINDOW_DESTROYED;
+    quit_closure_.Run();
+  }
+
+  owner_ = nullptr;
+}
+
+Widget::MoveLoopResult CocoaWindowMoveLoop::Run() {
+  LoopExitReason exit_reason = ENDED_EXTERNALLY;
+  exit_reason_ref_ = &exit_reason;
+  NSWindow* window = owner_->ns_window();
+  const gfx::Rect initial_frame = gfx::ScreenRectFromNSRect([window frame]);
+
+  base::RunLoop run_loop;
+  quit_closure_ = run_loop.QuitClosure();
+
+  // Will be retained by the monitor handler block.
+  WeakCocoaWindowMoveLoop* weak_cocoa_window_move_loop =
+      [[[WeakCocoaWindowMoveLoop alloc]
+          initWithWeakPtr:weak_factory_.GetWeakPtr()] autorelease];
+
+  // NSKeyDownMask doesn't work inside addLocalMonitorForEventsMatchingMask:

[tapted, 2016/05/23 07:29:28]

This comment isn't right. Instead there should be a comment about where the
escape keypress *is* handled (since it works now!)

[themblsha, 2016/05/26 15:13:25]

TabDragController now instantiates EscapeTracker which handles this, noted this
in the comment.

+  // the event is swallowed by the window move loop before it gets to -[NSApp
+  // sendEvent:]. To see an escape keypress, hook in an event tap lower.
+  NSEventMask mask = NSLeftMouseUpMask | NSLeftMouseDraggedMask;
+  auto handler = ^NSEvent*(NSEvent* event) {
+    CocoaWindowMoveLoop* strong = [weak_cocoa_window_move_loop weak].get();
+    if (!strong) {
+      // By this point CocoaWindowMoveLoop was deleted while processing this
+      // same event, and this event monitor was not unregistered in time. It is
+      // reproducible by the PressEscapeWhileDetached test.
+      // Continue processing the event.
+      return event;
+    }
+
+    if ([event type] == NSLeftMouseDragged) {
+      gfx::Point mouse_in_screen =
+          display::Screen::GetScreen()->GetCursorScreenPoint();

[tapted, 2016/05/23 07:29:28]

This should come off |event|, no?

[themblsha, 2016/05/26 15:13:25]

There's only instance-specific locationInWindow, and convert it to screen
coordinates. So it'll be more code and it'll lag a little behind the users's
mouse.

[tapted, 2016/06/01 11:29:55]

Hm - ok. I think I'd prefer this to use [NSEvent mouseLocation] directly then -
it's unclear that's how GetCursorScreenPoint works, and we don't really need to
flip. Then move the gfx::ScreenRectToNSRect(..) call to the
initial_mouse_in_screen, so we just get one flip at the start.

+
+      gfx::Rect frame = initial_frame;
+      frame.Offset(mouse_in_screen.x() - initial_mouse_in_screen_.x(),
+                   mouse_in_screen.y() - initial_mouse_in_screen_.y());
+
+      const NSRect ns_frame = gfx::ScreenRectToNSRect(frame);

[tapted, 2016/05/23 07:29:28]

There's no need to convert all these coordinates just do one on
|initial_mouse_in_screen|, then store initial_frame into an NSRect and use the
location from |event| to offset it manually (not with gfx:: stuff)

[themblsha, 2016/05/26 15:13:25]

Used NSOffsetRect in the end. Code definitely looks better now.

+      [window setFrame:ns_frame display:YES animate:NO];

[tapted, 2016/05/23 07:29:28]

Does display:NO work? It might do less work. We probably only need display:YES
when changing the size or doing other weird stuff.

[themblsha, 2016/05/26 15:13:25]

Works fine, thanks! I've even used display:NO in
BridgedNativeWidget::RunMoveLoop(), and it also seems to work fine.

+
+      return event;
+    }
+
+    strong->quit_closure_.Run();
+    if ([event type] == NSLeftMouseUp) {

[tapted, 2016/05/23 07:29:28]

DCHECK_EQ(NSLeftMouseUp, [event type])

no need for if

[themblsha, 2016/05/26 15:13:25]

Good idea, thanks.

+      *strong->exit_reason_ref_ = MOUSE_UP;
+      return event;  // Process the MouseUp.
+    }
+
+    NOTREACHED();
+    return event;
+  };
+  id monitor =
+      [NSEvent addLocalMonitorForEventsMatchingMask:mask handler:handler];
+
+  run_loop.Run();
+  [NSEvent removeMonitor:monitor];
+
+  if (exit_reason != WINDOW_DESTROYED && exit_reason != ENDED_EXTERNALLY) {
+    exit_reason_ref_ = nullptr;  // Ensure End() doesn't replace the reason.
+    owner_->EndMoveLoop();       // Deletes |this|.
+  }
+
+  return exit_reason != MOUSE_UP ? Widget::MOVE_LOOP_CANCELED
+                                 : Widget::MOVE_LOOP_SUCCESSFUL;
+}
+
+void CocoaWindowMoveLoop::End() {
+  if (exit_reason_ref_) {
+    DCHECK_EQ(*exit_reason_ref_, ENDED_EXTERNALLY);
+    // Ensure the destructor doesn't replace the reason.
+    exit_reason_ref_ = nullptr;
+    quit_closure_.Run();
+  }
+}
+
+void CocoaWindowMoveLoop::OnPositionChanged() {
+  return owner_->OnPositionChanged();
+}
+
+}  // namespace views