MacViews: Implement Tab Dragging

ui/views/cocoa/cocoa_window_move_loop.mm

Index: ui/views/cocoa/cocoa_window_move_loop.mm
diff --git a/ui/views/cocoa/cocoa_window_move_loop.mm b/ui/views/cocoa/cocoa_window_move_loop.mm
new file mode 100644
index 0000000000000000000000000000000000000000..5ed27de7af41d54f5550d4643b398f9a002e7586
--- /dev/null
+++ b/ui/views/cocoa/cocoa_window_move_loop.mm
@@ -0,0 +1,185 @@
+// 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"
+#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.
+@interface WeakCocoaWindowMoveLoop : NSObject {
+ @private
+  base::WeakPtr<views::CocoaWindowMoveLoop> weak_;
+}
+@end
+
+@implementation WeakCocoaWindowMoveLoop
+- (id)initWithWeakPtr:(const base::WeakPtr<views::CocoaWindowMoveLoop>&)weak {
+  if (self = [super init]) {

[tapted, 2016/03/11 09:38:28]

extra parens around assignment used as condition

[themblsha, 2016/04/05 17:20:42]

Done. Hmm, strange that it didn't result in any warning, maybe it's viewed as an
usual ObjC convention now :/

+    weak_ = weak;
+  }
+  return self;
+}
+
+- (base::WeakPtr<views::CocoaWindowMoveLoop>&)weak {
+  return weak_;
+}
+@end
+
+namespace {
+
+CGPoint GetCGMousePosition() {
+  return CGEventGetLocation(
+      base::ScopedCFTypeRef<CGEventRef>(CGEventCreate(nullptr)));
+}
+
+void SendCustomLeftMouseEvent(CGEventType type) {
+  base::ScopedCFTypeRef<CGEventRef> event(
+                  CGEventCreateMouseEvent(
+                      nullptr, type, GetCGMousePosition(),
+                      kCGMouseButtonLeft));
+  CGEventSetIntegerValueField(event, kCGEventSourceUserData, 1);

[tapted, 2016/03/11 09:38:28]

oops - this 1 should be the constant 

[themblsha, 2016/04/05 17:20:42]

Added kCocoaWindowMoveLoopSimulatedEventUserData constant :-)

+  CGEventPost(kCGSessionEventTap, event);
+}
+
+}  // namespace
+
+namespace views {
+
+CocoaWindowMoveLoop::CocoaWindowMoveLoop(BridgedNativeWidget* owner)
+    : owner_(owner),
+      run_loop_(new base::RunLoop),
+      quit_closure_(run_loop_->QuitClosure()),
+      weak_factory_(this) {
+  // AppKit continues to send mouse events to the window, but toolkit-views
+  // doesn't expect them during RunMoveLoop().
+  [owner_->ns_view() 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.
+  [owner_->ns_view() setIgnoreMouseEvents:NO];

[tapted, 2016/03/11 09:38:28]

Can this now use IsRunMoveLoopActive()?

[themblsha, 2016/04/05 17:20:42]

When the dragged tab is reattached to the window, an extra MouseDown simulated
event is sent to the parent window. Since IsRunMoveLoopActive() is not global it
would report different results for different widgets.

+
+  // Handle the pathological case, where |this| is destroyed while running.
+  if (exit_reason_ref_) {
+    *exit_reason_ref_ = WINDOW_DESTROYED;
+    quit_closure_.Run();
+  }
+
+  // Handle Run() never being called.
+  if (monitor_) {
+    [NSEvent removeMonitor:monitor_];
+    monitor_ = nil;
+  }
+  owner_ = nullptr;
+}
+
+Widget::MoveLoopResult CocoaWindowMoveLoop::Run() {
+  LoopExitReason exit_reason = ENDED_EXTERNALLY;
+  exit_reason_ref_ = &exit_reason;
+
+  // Move the RunLoop to the stack so our destructor can be called while it is
+  // running.
+  scoped_ptr<base::RunLoop> run_loop(std::move(run_loop_));
+
+  // A new window may have just been created, so post an event at the session
+  // tap level to initiate a window drag.
+  // TODO(tapted): Move this "inside" the window or stuff breaks when dragging

[tapted, 2016/03/11 09:38:28]

I think this needs to be done to fix one of those screencasts

[themblsha, 2016/04/05 17:20:42]

Removed the comment. Now I check for |expected_window| in
SendCustomLeftMouseEvent(), and it will DCHECK if the window under cursor is not
what we expect.

Is this what you meant?

+  // the last tab/s off a browser.
+  SendCustomLeftMouseEvent(kCGEventLeftMouseDown);
+
+  // Will be retained by the monitor handler block.
+  WeakCocoaWindowMoveLoop* proxy_quit_closure =
+      [[[WeakCocoaWindowMoveLoop alloc]
+          initWithWeakPtr:weak_factory_.GetWeakPtr()] autorelease];
+
+  NSEventMask mask = NSLeftMouseUpMask | NSKeyDownMask | NSLeftMouseDraggedMask;
+  monitor_ =

[tapted, 2016/03/11 09:38:28]

Does this need to be a data member still?

Can we just put it on the stack, and always call removeMonitor after
run_loop->Run(); returns?

[themblsha, 2016/04/05 17:20:42]

Done. Got no new test failures, so it seems to be fine.

+      [NSEvent addLocalMonitorForEventsMatchingMask:
+                   mask handler:^NSEvent * (NSEvent * event) {

[tapted, 2016/03/11 09:38:28]

I think we'll have to override clang format on this..

event_monitor_mac.mm formats these like:

  monitor_ = [NSEvent addLocalMonitorForEventsMatchingMask:mask
      handler:^NSEvent*(NSEvent* event) {
        if ([event...


or perhaps we can assign the handler to a local variable first so there's not so
much indenting.

[themblsha, 2016/04/05 17:20:42]

Done. Extracted the 'auto handler' variable.

+                     if ([event type] == NSLeftMouseDragged) {
+                       // AppKit doesn't supply position updates during a drag,
+                       // so post a task to notify observers once AppKit has
+                       // moved the window.
+                       base::MessageLoop::current()->PostTask(
+                           FROM_HERE,
+                           base::Bind(&BridgedNativeWidget::OnPositionChanged,
+                                      base::Unretained(owner_)));
+                       return event;
+                     }
+
+                     CocoaWindowMoveLoop* thiss =

[tapted, 2016/03/11 09:38:28]

perhaps thiss->`this_move_loop`, but can this weak pointer stuff be avoided just
by putting a reference to exit_reason on the stack?

e.g. at the top there is

  LoopExitReason exit_reason = ENDED_EXTERNALLY;
  exit_reason_ref_ = &exit_reason;

I think we can add

  // Blocks capture by value, so allow the block to read the exit reason on
  // the stack, even if |this| is destroyed, making exit_reason_ref_ invalid.
  LoopExitReason* exit_reason_for_block = &exit_reason;

Then here instead do

  if (*exit_reason_for_block == WINDOW_DESTROYED)
    return nil;

Also does this need to be moved further up to guard against accessing |owner_|?

[themblsha, 2016/04/05 17:20:42]

I guess I just got lucky that the event that got CocoaWindowMoveLoop deleted
wasn't the NSLeftMouseDragged, or it would crash on trying to post
OnPositionChanged to owner_ as well. Renamed the variables and moved the
condition earlier.

I don't think your solution will work, as the bug was as following (reproducible
by the PressEscapeWhileDetached test):
1. Several event monitors were installed and the CocoaWindowMoveLoop's was the
last one in chain
2. The Esc key press was posted and resulted in CocoaWindowMoveLoop untimely
death, the Loop's event monitor was scheduled to be removed
3. Loop's event monitor still got to process the event, as it was still
registered when the event got into the application, and it crashed on trying to
dereference an invalid 'this' pointer.

Your stack solution will not work, as it will still result in dereferencing an
invalid pointer. Please note that PressEscapeWhileDetached test crashes in a
somewhat flaky manner without the weak pointer but still does it often enough.

+                         [proxy_quit_closure weak].get();
+                     if (!thiss)
+                       return nil;
+
+                     thiss->quit_closure_.Run();
+                     if ([event type] == NSLeftMouseUp) {
+                       *thiss->exit_reason_ref_ = MOUSE_UP;
+                       return event;  // Process the MouseUp.
+                     }
+                     *thiss->exit_reason_ref_ = ESCAPE_PRESSED;
+                     return nil;  // Swallow the keypress.
+                   }];
+
+  // NSKeyDownMask doesn't work inside addLocalMonitorForEventsMatchingMask:
+  // 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.
+
+  run_loop->Run();
+
+  if (exit_reason != WINDOW_DESTROYED && exit_reason != ENDED_EXTERNALLY) {
+    exit_reason_ref_ = nullptr;  // Ensure End() doesn't replace the reason.
+    owner_->EndMoveLoop();       // Deletes |this|.
+  }
+
+  if (exit_reason != MOUSE_UP) {
+    // Tell AppKit to stop moving the window. Otherwise, AppKit will refuse to
+    // start a new window drag. Note the window being dragged is going away in
+    // this case, so it doesn't really matter where the event is located.
+    SendCustomLeftMouseEvent(kCGEventLeftMouseUp);
+
+    if (exit_reason == ENDED_EXTERNALLY) {
+      // When not canceled, the non-moving drag in the original window must
+      // resume. To do this, AppKit needs to see a mouseDown so that it sends
+      // the correct events. Ideally, it also needs to be at the original
+      // offset, so that it hits a non-draggable region of the original
+      // window: The tab being dragged may move some distance from the cursor
+      // when it "snaps in", so the cursor may not be over a tab. Sadly, this
+      // method doesn't know which window that is. But all that really needs
+      // to be done is to prevent a custom-dragging area from starting a
+      // window-drag. So hook into the logic in RepostEventIfHandledByWindow()
+      // by setting a flag here. Note this assumes the custom mouseDown event
+      // is guaranteed to hit another BridgedNativeWidget when it gets to the
+      // front of the event queue.
+      // TODO(tapted): A better fix would be to keep the temporary window
+      // around and never call EndMoveLoop() on Mac, making this block of code
+      // obsolete.
+      BridgedNativeWidget::IgnoreNextMouseDownForDraggableRegions();
+      SendCustomLeftMouseEvent(kCGEventLeftMouseDown);
+    }
+  }
+
+  return exit_reason == ESCAPE_PRESSED ? 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();
+  }
+}
+
+}  // namespace views