Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1747803003:
MacViews: Implement Tab Dragging (Closed)
DescriptionMacViews: Implement Tab Dragging
BridgedNativeWidget::RunMoveLoop() drags the window by manually processing mouse move events.
Most of the tests in tab_drag_controller_interactive_uitest.cc pass, and I've added several Mac-specific ones to test for the edge cases.
BUG=594079
COLLABORATOR=tapted@chromium.org
Also reviewed in http://crrev.com/1259913003/
Committed: https://crrev.com/6984eac05884a72bc81697589817df83bcde0551
Cr-Commit-Position: refs/heads/master@{#400463}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Removed debug logging, use CGEvents for drag-n-drop. #
Total comments: 6
Patch Set 3 : Wait for CocoaWindowMoveLoop to finish, fix PressEscapeWhileDetached test. #Patch Set 4 : Extract DragsWindowUsingCocoaMoveLoop test, cleanup code. #
Total comments: 54
Patch Set 5 : Extract CocoaWindowMoveLoop, fix review issues. #
Total comments: 21
Patch Set 6 : Add CocoaShakeDetach test. #Patch Set 7 : Fix dragging edge conditions, add tests for them (incomplete, sorry). #Patch Set 8 : Fix dragging edge conditions, add tests for them. #Patch Set 9 : Fix review issues. #
Total comments: 6
Patch Set 10 : Fix tests and review issues (incomplete, sorry). #Patch Set 11 : Fix tests and review issues. #
Total comments: 70
Patch Set 12 : Fix review issues. #Patch Set 13 : Replace GCD with DelegateSimpleThread, remove ConstrainToEnclosingRect function, cleanup code. #
Total comments: 4
Patch Set 14 : Replace MacDetachesWindowOnTopOfMacMenuBar with MacDetachesWindowAtTopOfScreen, rename kCocoaWindow… #Patch Set 15 : Removed click simulation, reimplemented CocoaWindowMoveLoop without relying on the WindowServer. #
Total comments: 60
Patch Set 16 : Removed the traces of old code that relied on moving the window using WindowServer. #Patch Set 17 : Remove CGEvent-generating code from ui_controls_mac.mm #
Total comments: 39
Patch Set 18 : Fix review issues. #
Total comments: 12
Patch Set 19 : Fix review issues. #
Total comments: 4
Patch Set 20 : Fix review issues. #
Total comments: 1
Patch Set 21 : Don't set the window position inside the RunMoveLoop call. #
Total comments: 2
Patch Set 22 : Remove crbug.com/518740-fixing code, remove DragAndDrop helpers that are no longer required without… #
Total comments: 2
Patch Set 23 : FIXME --> TODO #Patch Set 24 : git cl upload --target_branch=master #Patch Set 25 : Rebased on top of master. #Patch Set 26 : Fix include. #Messages
Total messages: 77 (15 generated)
I've added one test for dragging. What do you think of this approach? I could port the rest. Will it be possible to merge this in that case? Also regarding the "slipping off" bug. It should be reproducible by making -[NSEvent mouseLocation] return incorrect location at the moment the mouse event is actually processed. Did I understand it correctly? P.S. Finally got the chance to use the associated object trick in order to send the 'event processed' callback notifications -- it should make the Mac interactive_ui_tests much more robust.
On 2016/02/29 14:58:20, themblsha wrote: > I've added one test for dragging. What do you think of this approach? I could > port the rest. Will it be possible to merge this in that case? Seems like a good approach, but I didn't have much time today, so I've only had a quick skim. There's a lot of things to clean up from my experimental CL. E.g. CocoaWindowMoveLoop should be in its own file, and all the logging needs to be dropped. > Also regarding the "slipping off" bug. It should be reproducible by making > -[NSEvent mouseLocation] return incorrect location at the moment the mouse event > is actually processed. Did I understand it correctly? Yep - that might do it. I actually started exploring a generic fix in https://codereview.chromium.org/1272223005/ - but it was so long ago, I'm not sure how much it actually achieved. > P.S. Finally got the chance to use the associated object trick in order to send > the 'event processed' callback notifications -- it should make the Mac > interactive_ui_tests much more robust. It does seem like a useful approach - did you identify a case or sequence of events where EventQueueWatcher would flake out? If it's clear-cut, we can land just the associated object stuff separately. https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_drag_controller.cc:91: #if 0 && defined(OS_MACOSX) So, this doesn't work yet. It's fine for a follow-up (until then this constant should be removed). However, this really should be enabled on Mac. The drag and drop that occurs on Aura is quite janky compared to the technique in Cocoa Chrome. We should experiment to see if we can get it here. https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:820: gfx::Screen::GetScreen()->GetPrimaryDisplay().work_area(); did GetDisplayNearestWindow() not work? https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma... ui/base/test/ui_controls_mac.mm:59: bool g_mouse_button_down[3] = { false, false, false }; comment (mention that it's indexed by ui_controls::MouseButton) https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma... ui/base/test/ui_controls_mac.mm:183: int kEventDeletedTaskRunnerKey; can be merged with the namespace above. needs a comment. https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma... ui/base/test/ui_controls_mac.mm:186: @interface EventDeletedTaskRunner : NSObject { What's the reason why EventQueueWatcher couldn't support this? https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma... ui/base/test/ui_controls_mac.mm:289: // we want to destroy the autoreleased event ASAP Comments should all be complete sentences. Capitalize first letter, end in a fullstop. https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma... ui/base/test/ui_controls_mac.mm:301: for (std::vector<NSEvent*>::iterator iter = events.begin(); for (NSEvent* event : events) https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma... ui/base/test/ui_controls_mac.mm:340: NSEventType etype = NSMouseMoved; etype -> event_type https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma... ui/base/test/ui_controls_mac.mm:411: g_mouse_button_down[RIGHT] = true; these can all be collapsed into g_mouse_button_down[type] = state == DOWN; after the if-else{} https://codereview.chromium.org/1747803003/diff/1/ui/gfx/screen_mac.mm File ui/gfx/screen_mac.mm (right): https://codereview.chromium.org/1747803003/diff/1/ui/gfx/screen_mac.mm#newcode27 ui/gfx/screen_mac.mm:27: gfx::Rect ConvertCoordinateSystem(NSRect ns_rect) { this function isn't needed any more -- can just call gfx::ScreenRectFromNSRect(ns_rect) at the call sites https://codereview.chromium.org/1747803003/diff/1/ui/gfx/screen_mac.mm#newcod... ui/gfx/screen_mac.mm:103: return gfx::ScreenPointFromNSPoint(mouseLocation); nit: can just pass [NSEvent mouseLocation]; in on this line https://codereview.chromium.org/1747803003/diff/1/ui/views/event_monitor_mac.mm File ui/views/event_monitor_mac.mm (right): https://codereview.chromium.org/1747803003/diff/1/ui/views/event_monitor_mac.... ui/views/event_monitor_mac.mm:32: return gfx::Screen::GetScreen()->GetCursorScreenPoint(); these refactorings are good,but they should land separately to keep the WindowDragging CL small
https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2377: #elif defined(USE_ASH) oh, and this [not stuff you added :)] actually looks bad. I don't think we have ASH that isn't ChromeOS any more, so this is dead code. Which means we are missing out on test coverage. So, I've put up https://codereview.chromium.org/1755513002/ to fix this bit
https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma... ui/base/test/ui_controls_mac.mm:186: @interface EventDeletedTaskRunner : NSObject { On 2016/03/01 08:11:58, tapted wrote: > What's the reason why EventQueueWatcher couldn't support this? I'm spawning these on a separate thread, as the main one is inside a nested eventloop — and the queue on the separate thread is always empty as the events are magically posted to the UI thread. But turns out there's an error in implementation — NSErrors are copied and the copy is actually processed, so the tasks are added to the queue before the event finishes processing. This currently results in a race condition when g_mouse_location is updated while the previous message is still being processed — that's why the window move works. And currently the simulated events don't move the window while in CocoaWindowMoveLoop, still investigating how it works internally — maybe I'm sending wrong messages or not enough messages or something else. Need to do additional reverse-engineering on this. Another option which I did at first involved sending CG-events which I've created using CGEventCreateMouseEvent — but they have two drawbacks: 1. They move the real mouse 2. Don't know how to know when they've finished being processed
So I'm currently fixing the CocoaWindowMoveLoop not really working in the test.
On 2016/03/01 18:42:45, themblsha wrote: > So I'm currently fixing the CocoaWindowMoveLoop not really working in the test. Ah, I thought you had some success with that already. I suspect we will need to generate CGEvents rather than NSEvents, in which case ui_controls may be too far away from what we need. ui::EventGenerator may be a better shot - EventGeneratorDelegateMac::OnMouseEvent and EventGeneratorDelegateMac::OnKeyEvent can be adjusted to also be able to target the "system" with [ns_event CGEvent].
Turns out the cocoa window moving works even when the application is completely frozen, so we can't really filter the moving events, and it appears to work on a WindowServer level. So that's why simulated NSEvents don't work — WindowServer doesn't see them. Creating CGEvents per se is not tricky, here's our current code for doing so: https://gist.github.com/anonymous/a046f16fbf82eb7d4b53 (but needs a minor update to generate kosher move events: NSLeftMouseDragged and etc). They're #ifdef OS_MACOSX in ui_controls.h, and implemented only on OS X. I'm planning on integrating it back, as I don't see how using an EventGenerator with a custom CGEvent-generating EventGeneratorDelegateMac would allow for waiting for events to finish processing. Am I missing the point?
On 2016/03/02 17:34:29, themblsha wrote: > Turns out the cocoa window moving works even when the application is completely > frozen, so we can't really filter the moving events, and it appears to work on a > WindowServer level. So that's why simulated NSEvents don't work — WindowServer > doesn't see them. > > Creating CGEvents per se is not tricky, here's our current code for doing so: > https://gist.github.com/anonymous/a046f16fbf82eb7d4b53 (but needs a minor update > to generate kosher move events: NSLeftMouseDragged and etc). They're #ifdef > OS_MACOSX in ui_controls.h, and implemented only on OS X. > > I'm planning on integrating it back, as I don't see how using an EventGenerator > with a custom CGEvent-generating EventGeneratorDelegateMac would allow for > waiting for events to finish processing. Am I missing the point? You might have to add in the waiting as well. EventGenerator is just a nicer encapsulation of event-generating than ui_controls, but using it isn't essential. There's some stuff in bridged_native_widget_interactive_uitest.mm that uses CGEvents to test low-level window dragging.
What do you think of this approach? Should I continue with fixing the remaining issues? https://codereview.chromium.org/1747803003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1747803003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:393: RunMoveLoop(GetWindowOffset(start_point_in_screen_)); Without this fix the first mouse move after pressing the button won't actually move the window, only the second mouse move will. This fix has the side effect that now window moves will work even with NSEvents, that's why I've added an intermediate step and carefully check for the resulting window bounds. https://codereview.chromium.org/1747803003/diff/20001/chrome/test/base/intera... File chrome/test/base/interactive_test_utils_mac.mm (right): https://codereview.chromium.org/1747803003/diff/20001/chrome/test/base/intera... chrome/test/base/interactive_test_utils_mac.mm:121: usleep(100000.0); Now the code works very reliably with the exception of the last step. How to properly wait until CocoaWindowMoveLoop is finished?
Yeah - the g_use_cgevents approach looks like a good way to me. https://codereview.chromium.org/1747803003/diff/20001/chrome/test/base/intera... File chrome/test/base/interactive_test_utils_mac.mm (right): https://codereview.chromium.org/1747803003/diff/20001/chrome/test/base/intera... chrome/test/base/interactive_test_utils_mac.mm:121: usleep(100000.0); On 2016/03/03 17:56:51, themblsha wrote: > Now the code works very reliably with the exception of the last step. How to > properly wait until CocoaWindowMoveLoop is finished? There's a BridgedNativeWidgetTestApi which has access to private members of BridgedNativeWidget -- you could add something that watches events, and waits for BridgedNativeWidget::window_move_loop_ to become null https://codereview.chromium.org/1747803003/diff/20001/ui/base/test/ui_control... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/20001/ui/base/test/ui_control... ui/base/test/ui_controls_mac.mm:15: #include "base/mac/scoped_nsobject.h" the objc headers should be #imported
Turns out that simply waiting for window_move_loop_ to become NULL is not enough, as there'll be an asynchronous NSWindowMovedEventType notification and we need to wait for that to get the actual resulting window position. Now it works really reliably. But it seems that this CGEvent magic is not necessary for vast majority (or all?) of tab_drag_controller_interactive_uitests, so I've enabled it only for a single test. Probably I need to write a separate Mac-specific test that drags a window by a single tab, and leave the rest as they are if they work without any additional work. Also fixed the crasher in CocoaWindowMoveLoop. Will continue the clean-up work on wednesday.
Phew. Fixed all review issues.
Some tests fail/timeout due to gray bar that appears on top of windows after
resizing (and dragging doesn't work when clicking on top of the grey bar, which
occupies the central portion of the tab), I think it should be fixed separately:
4 tests failed:
TabDragging/DetachToBrowserTabDragControllerTest.CancelOnNewTabWhenDragging/0
(../../chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:544)
TabDragging/DetachToBrowserTabDragControllerTest.DetachFromFullsizeWindow/0
(../../chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:544)
TabDragging/DetachToBrowserTabDragControllerTest.DetachToOwnWindowFromMaximizedWindow/0
(../../chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:544)
TabDragging/DetachToBrowserTabDragControllerTest.DragDirectlyToSecondWindow/0
(../../chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:544)
4 tests timed out:
TabDragging/DetachToBrowserTabDragControllerTest.DragAllToSeparateWindow/0
(../../chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:544)
TabDragging/DetachToBrowserTabDragControllerTest.DragAllToSeparateWindowAndCancel/0
(../../chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:544)
TabDragging/DetachToBrowserTabDragControllerTest.DragSingleTabToSeparateWindow/0
(../../chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:544)
TabDragging/DetachToBrowserTabDragControllerTest.DragToSeparateWindow/0
(../../chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:544)
https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):
https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:91: #if 0 &&
defined(OS_MACOSX)
On 2016/03/01 08:11:58, tapted wrote:
> So, this doesn't work yet. It's fine for a follow-up (until then this constant
> should be removed).
>
> However, this really should be enabled on Mac. The drag and drop that occurs
on
> Aura is quite janky compared to the technique in Cocoa Chrome. We should
> experiment to see if we can get it here.
Done.
https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):
https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:820:
gfx::Screen::GetScreen()->GetPrimaryDisplay().work_area();
On 2016/03/01 08:11:58, tapted wrote:
> did GetDisplayNearestWindow() not work?
GetDisplayNearestWindow() expects NativeView, but we can get only NativeWindow
from BrowserWindow. Did I miss a good way to convert one to the other?
https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2377:
#elif defined(USE_ASH)
On 2016/03/01 08:30:10, tapted wrote:
> oh, and this [not stuff you added :)] actually looks bad. I don't think we
have
> ASH that isn't ChromeOS any more, so this is dead code. Which means we are
> missing out on test coverage. So, I've put up
> https://codereview.chromium.org/1755513002/ to fix this bit
Added OS_MACOSX block.
https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma...
File ui/base/test/ui_controls_mac.mm (right):
https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma...
ui/base/test/ui_controls_mac.mm:59: bool g_mouse_button_down[3] = { false,
false, false };
On 2016/03/01 08:11:58, tapted wrote:
> comment (mention that it's indexed by ui_controls::MouseButton)
Done.
https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma...
ui/base/test/ui_controls_mac.mm:183: int kEventDeletedTaskRunnerKey;
On 2016/03/01 08:11:58, tapted wrote:
> can be merged with the namespace above. needs a comment.
Done.
https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma...
ui/base/test/ui_controls_mac.mm:289: // we want to destroy the autoreleased
event ASAP
On 2016/03/01 08:11:58, tapted wrote:
> Comments should all be complete sentences. Capitalize first letter, end in a
> fullstop.
Done.
https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma...
ui/base/test/ui_controls_mac.mm:340: NSEventType etype = NSMouseMoved;
On 2016/03/01 08:11:58, tapted wrote:
> etype -> event_type
Done.
https://codereview.chromium.org/1747803003/diff/1/ui/base/test/ui_controls_ma...
ui/base/test/ui_controls_mac.mm:411: g_mouse_button_down[RIGHT] = true;
On 2016/03/01 08:11:58, tapted wrote:
> these can all be collapsed into
>
> g_mouse_button_down[type] = state == DOWN;
>
> after the if-else{}
Done.
https://codereview.chromium.org/1747803003/diff/20001/chrome/test/base/intera...
File chrome/test/base/interactive_test_utils_mac.mm (right):
https://codereview.chromium.org/1747803003/diff/20001/chrome/test/base/intera...
chrome/test/base/interactive_test_utils_mac.mm:121: usleep(100000.0);
On 2016/03/04 06:33:08, tapted wrote:
> On 2016/03/03 17:56:51, themblsha wrote:
> > Now the code works very reliably with the exception of the last step. How to
> > properly wait until CocoaWindowMoveLoop is finished?
>
> There's a BridgedNativeWidgetTestApi which has access to private members of
> BridgedNativeWidget -- you could add something that watches events, and waits
> for BridgedNativeWidget::window_move_loop_ to become null
Done.
https://codereview.chromium.org/1747803003/diff/20001/ui/base/test/ui_control...
File ui/base/test/ui_controls_mac.mm (right):
https://codereview.chromium.org/1747803003/diff/20001/ui/base/test/ui_control...
ui/base/test/ui_controls_mac.mm:15: #include "base/mac/scoped_nsobject.h"
On 2016/03/04 06:33:08, tapted wrote:
> the objc headers should be #imported
Done.
It'll need a proper CL description (the one on my earlier experiment is a bit casual) And I've only done a first pass on the review -- I want to play around with it some more, and will probably have more comments. Ran out of time today. https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:820: gfx::Screen::GetScreen()->GetPrimaryDisplay().work_area(); On 2016/03/09 17:40:22, themblsha wrote: > On 2016/03/01 08:11:58, tapted wrote: > > did GetDisplayNearestWindow() not work? > > GetDisplayNearestWindow() expects NativeView, but we can get only NativeWindow > from BrowserWindow. Did I miss a good way to convert one to the other? oh weird. There's platform_util::GetViewForWindow which should work in a cross-platform way. We should probably use that in case the test runs on a secondary screen or something. https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:385: RunMoveLoop(GetWindowOffset(start_point_in_screen_)); I think this should land first and separately with its own explainer since it affects all platforms and might cause a subtle regression.. Also someone more familiar with tab_drag_controller.cc might know a good reason why it was point_in_screen rather than start_point_in_screen. (although I seem to recall that Windows would even ignore its argument to `RunMoveLoop` or something, but that was hard to fix -- if this just fixes the "slippage" on Ash it's probably worth doing -- there is http://crbug.com/518740 for that) https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:527: bool result = sqrt(pow(static_cast<float>(x_offset), 2) + nit: this change not needed https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:612: nit: remove added line https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:175: // On Macs if we try to drag on an inactive window it won't work. Two Hm - this should work. Or at least, it works with the Cocoa browser. Is it just in the test situation that it doesn't work? Maybe the tab is obscured by another window - that would probably break the simulated events. But we should try to explain why it doesn't work in the comment. https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:248: #if !defined(OS_CHROMEOS) && !defined(OS_MACOSX) I'd go with && defined(USE_AURA) instead - since the code in-between relies a lot on Aura APIs https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1199: // Makes sure we can drag the window using WindowServer by dragging on a tab nit: full-stop on the end. Also say why this is only relevant for Mac (e.g. will it fail, or is it that ui_test_utils::DragAndDrop is special) https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1212: const unsigned int steps = 10; // use anything greater than 1 to verify that we tend not to use `unsigned` on Chrome if int works - I think there's something in the style guide about it. So perhaps `cont int kSteps = 10;` https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1213: // CocoaWindowMoveLoop is working correctly nit: Comment above the declaration. Also sentence-style. https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1220: ASSERT_FALSE(tab_strip->IsDragSessionActive()); nit: typically we just use EXPECT_ unless things really explode, since it can allow a tryjob to capture more debugging output upon a failure. https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2413: #elif defined(OS_MACOSX) this shouldn't be needed any more -- we can just use the `else` (also I'm amazed that compilers don't complain about #elif coming after an #else :) https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/window_finder_mac.mm (right): https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/window_finder_mac.mm:7: #include <AppKit/AppKit.h> nit: import https://codereview.chromium.org/1747803003/diff/60001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/1747803003/diff/60001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1731: #'browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc', delete rather than commenting out. Also is there a BUILD.gn somewhere that needs to change too? https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera... File chrome/test/base/interactive_test_utils.h (right): https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera... chrome/test/base/interactive_test_utils.h:32: base::TimeDelta delay = base::TimeDelta(), can the delay argument be removed? It might help while debugging the test, but if people are tempted to use it, it could lead to test flakiness. https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera... chrome/test/base/interactive_test_utils.h:33: unsigned int steps = 1); just int https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera... File chrome/test/base/interactive_test_utils_mac.mm (right): https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera... chrome/test/base/interactive_test_utils_mac.mm:23: class BridgedNativeWidgetTestApi { ooh interesting - there's already a BridgedNativeWidgetTestApi. I guess _technically_ this doesn't violate the ODR since one is only built in views_unittests and one is only built in interactive_ui_tests, but it could be a weird thing for someone to discover later on. But I think the bigger problem is that chrome/test/base shouldn't have so much that's specific to MacViews -- I think DragAndDrop should move to a new file like ui/views/cocoa/test/macviews_interactive_test_utils.h But then we can't have the window_finder.h dependency -- perhaps the window can just be passed into DragAndDrop from the test for that case. Then can you merge the two declarations of BridgedNativeWidgetTestApi -- that should go into ui/views/cocoa/test/bridged_native_widget_test_api.h/cc https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera... chrome/test/base/interactive_test_utils_mac.mm:200: for (unsigned int i = 1; i <= steps; ++i) { nit: just int https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera... chrome/test/base/interactive_test_utils_mac.mm:201: const double progress = double(i) / steps; we tend to use static_cast instead of this style - I think it's called "functional cast" https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera... chrome/test/base/interactive_test_utils_mac.mm:210: usleep(delay.InMicroseconds()); yeah - I think we can remove this for now? - `usleep` tends to lead to test flakes, so it would need a stronger justification. https://codereview.chromium.org/1747803003/diff/60001/ui/base/test/ui_control... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/60001/ui/base/test/ui_control... ui/base/test/ui_controls_mac.mm:405: // We want to destroy the autoreleased event ASAP. what happens if we don't? I think since we rely on refcounting to signal the end there might be a chance of getting trapped somehow. Perhaps give a long explainer comment here, and put a comment on the others like "See SendKeyPressNotifyWhenDone()." https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.h:46: BOOL ignoreMouseEvents_; needs a comment https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:35: bool g_ignore_mouse_events = false; needs a comment. (it should say why we need both a global and an instance member - maybe we don't and this was just me being lazy while experimenting :) https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:314: class CocoaWindowMoveLoop { Move this to its own file (with WeakCocoaMoveLoop?) - ui/views/cocoa/cocoa_window_move_loop.{h,mm}. I think this grew in line count unexpectedly while experimenting.. https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:322: // goes berserk if it gets them during RunMoveLoop(). hehe - I remember writing `berserk`... Perhaps "but toolkit-views doesn't expect them during RunMoveLoop()." https://codereview.chromium.org/1747803003/diff/60001/ui/views/widget/native_... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/1747803003/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac.mm:82: //DCHECK([NSEvent pressedMouseButtons] == 0 || This DCHECK would actually fail, which surprised me. I think we actually need to get the origin from the window server and the size from [window frame] to ensure that the window layout is in-step, synchronously with NSEvents that cause it. But also I think we should only use FrameIncludingDrag at all when there's a RunMoveLoop installed -- I think using it in other places could lead to test flakiness because of asynchronous stuff around the window server. https://codereview.chromium.org/1747803003/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac.mm:228: // if (IsMouseButtonDown()) nit: uncomment (sorry for the log spam - drag and drop is still in the pipeline).
Didn't have the time yet to address bridged_content_view.{h,mm} issues yet, will
do it tomorrow.
Also turns out I've included a wrong dependency in ui_controls_mac.mm — Illegal
include: "content/public/browser/browser_thread.h"
I could use dispatch_async(dispatch_get_main_queue(), {}) to post it, and
dispatch_get_current_queue() == dispatch_get_main_queue() in the DCHECK, but
maybe there are better options that don't violate the checkdeps?
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:385:
RunMoveLoop(GetWindowOffset(start_point_in_screen_));
On 2016/03/10 11:51:18, tapted wrote:
> I think this should land first and separately with its own explainer since it
> affects all platforms and might cause a subtle regression.. Also someone more
> familiar with tab_drag_controller.cc might know a good reason why it was
> point_in_screen rather than start_point_in_screen.
>
> (although I seem to recall that Windows would even ignore its argument to
> `RunMoveLoop` or something, but that was hard to fix -- if this just fixes the
> "slippage" on Ash it's probably worth doing -- there is
http://crbug.com/518740
> for that)
Without this fix the DetachToBrowserTabDragControllerTest.DragAll won't pass for
sure, and it'll also affect all other tests that rely on
CocoaWindowMoveLoop-related code to move the window.
Maybe it's better to ensure that it doesn't break no other platforms in this CL
and add one more reviewer for that?
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:527: bool result =
sqrt(pow(static_cast<float>(x_offset), 2) +
On 2016/03/10 11:51:18, tapted wrote:
> nit: this change not needed
Done.
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:612:
On 2016/03/10 11:51:18, tapted wrote:
> nit: remove added line
Done.
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:175: //
On Macs if we try to drag on an inactive window it won't work. Two
On 2016/03/10 11:51:19, tapted wrote:
> Hm - this should work. Or at least, it works with the Cocoa browser. Is it
just
> in the test situation that it doesn't work? Maybe the tab is obscured by
another
> window - that would probably break the simulated events. But we should try to
> explain why it doesn't work in the comment.
Did some tests a couple more times: in order for dragging to work window needs
to be activated. In non-MacViews Chromium (as well as in Safari) we *can* start
drag on an inactive window.
I tried to investigate why windows in interactive_ui_tests can't be activated
using normal means: BrowserWindow::Activate() doesn't work, mouse clicking
doesn't work. NSBundle seems to be swizzled the same way as in non-MacViews
builds, NSApp's activationPolicy is the same. [NSApp
activateIgnoringOtherApps:YES] didn't seem to work.
Added -[BridgedContentView acceptsFirstMouse:], and now it works just fine. But
ui_test_utils::DragAndDrop() won't be able to monitor incoming MouseMove events,
so I've moved explicit browser window activation to that test and added a
comment.
Also acceptsFirstMouse: == YES is enabled only for handful of objects in Cocoa
browser, and there seems to be nothing at the moment that would enable us to
enable acceptsFirstMouse: only for Views that require it. Probably this should
be handled in another issue?
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:248: #if
!defined(OS_CHROMEOS) && !defined(OS_MACOSX)
On 2016/03/10 11:51:19, tapted wrote:
> I'd go with && defined(USE_AURA) instead - since the code in-between relies a
> lot on Aura APIs
Done.
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1199: //
Makes sure we can drag the window using WindowServer by dragging on a tab
On 2016/03/10 11:51:19, tapted wrote:
> nit: full-stop on the end. Also say why this is only relevant for Mac (e.g.
will
> it fail, or is it that ui_test_utils::DragAndDrop is special)
Done.
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1212:
const unsigned int steps = 10; // use anything greater than 1 to verify that
On 2016/03/10 11:51:19, tapted wrote:
> we tend not to use `unsigned` on Chrome if int works - I think there's
something
> in the style guide about it. So perhaps `cont int kSteps = 10;`
Done.
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1213: //
CocoaWindowMoveLoop is working correctly
On 2016/03/10 11:51:18, tapted wrote:
> nit: Comment above the declaration. Also sentence-style.
Done.
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1220:
ASSERT_FALSE(tab_strip->IsDragSessionActive());
On 2016/03/10 11:51:19, tapted wrote:
> nit: typically we just use EXPECT_ unless things really explode, since it can
> allow a tryjob to capture more debugging output upon a failure.
Done. But I used the DragAll test as a base, and it uses ASSERTs.
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2413:
#elif defined(OS_MACOSX)
On 2016/03/10 11:51:19, tapted wrote:
> this shouldn't be needed any more -- we can just use the `else` (also I'm
amazed
> that compilers don't complain about #elif coming after an #else :)
Whoops, clang indeed complains about that, but at the time of 'git cl upload' it
was still recompiling on top of recent Chromium.
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/window_finder_mac.mm (right):
https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/window_finder_mac.mm:7: #include <AppKit/AppKit.h>
On 2016/03/10 11:51:19, tapted wrote:
> nit: import
Done.
https://codereview.chromium.org/1747803003/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):
https://codereview.chromium.org/1747803003/diff/60001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:1731:
#'browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc',
On 2016/03/10 11:51:19, tapted wrote:
> delete rather than commenting out. Also is there a BUILD.gn somewhere that
needs
> to change too?
Exactly, forgot about that, thanks!
https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera...
File chrome/test/base/interactive_test_utils.h (right):
https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera...
chrome/test/base/interactive_test_utils.h:32: base::TimeDelta delay =
base::TimeDelta(),
On 2016/03/10 11:51:19, tapted wrote:
> can the delay argument be removed? It might help while debugging the test, but
> if people are tempted to use it, it could lead to test flakiness.
I thought that this delay would be useful for testing
tab_strip_drag_controller.mm as it has kTearDuration, and if we don't wait for
enough time the precise window bounds comparisons will fail.
https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera...
chrome/test/base/interactive_test_utils.h:33: unsigned int steps = 1);
On 2016/03/10 11:51:19, tapted wrote:
> just int
Done.
https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera...
File chrome/test/base/interactive_test_utils_mac.mm (right):
https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera...
chrome/test/base/interactive_test_utils_mac.mm:23: class
BridgedNativeWidgetTestApi {
On 2016/03/10 11:51:19, tapted wrote:
> ooh interesting - there's already a BridgedNativeWidgetTestApi. I guess
> _technically_ this doesn't violate the ODR since one is only built in
> views_unittests and one is only built in interactive_ui_tests, but it could be
a
> weird thing for someone to discover later on.
>
> But I think the bigger problem is that chrome/test/base shouldn't have so much
> that's specific to MacViews -- I think DragAndDrop should move to a new file
> like ui/views/cocoa/test/macviews_interactive_test_utils.h
>
> But then we can't have the window_finder.h dependency -- perhaps the window
can
> just be passed into DragAndDrop from the test for that case.
>
> Then can you merge the two declarations of BridgedNativeWidgetTestApi -- that
> should go into ui/views/cocoa/test/bridged_native_widget_test_api.h/cc
I've added BridgedNativeWidget::IsRunMoveLoopActive(), so
NativeWidgetMac::GetClientAreaBoundsInScreen() could use that, and also removed
this test-only class.
https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera...
chrome/test/base/interactive_test_utils_mac.mm:200: for (unsigned int i = 1; i
<= steps; ++i) {
On 2016/03/10 11:51:19, tapted wrote:
> nit: just int
Done.
https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera...
chrome/test/base/interactive_test_utils_mac.mm:201: const double progress =
double(i) / steps;
On 2016/03/10 11:51:19, tapted wrote:
> we tend to use static_cast instead of this style - I think it's called
> "functional cast"
Done.
https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera...
chrome/test/base/interactive_test_utils_mac.mm:210:
usleep(delay.InMicroseconds());
On 2016/03/10 11:51:19, tapted wrote:
> yeah - I think we can remove this for now? - `usleep` tends to lead to test
> flakes, so it would need a stronger justification.
Okay :-)
https://codereview.chromium.org/1747803003/diff/60001/ui/base/test/ui_control...
File ui/base/test/ui_controls_mac.mm (right):
https://codereview.chromium.org/1747803003/diff/60001/ui/base/test/ui_control...
ui/base/test/ui_controls_mac.mm:405: // We want to destroy the autoreleased
event ASAP.
On 2016/03/10 11:51:19, tapted wrote:
> what happens if we don't? I think since we rely on refcounting to signal the
end
> there might be a chance of getting trapped somehow. Perhaps give a long
> explainer comment here, and put a comment on the others like "See
> SendKeyPressNotifyWhenDone()."
It was necessary when I've added an associated object to them to signal that
they've started processing, but it's not necessary anymore since
EventQueueWatcher() work fine and I use swizzled -[NSApp sendEvent:] function to
wait till the events start processing.
Removed the pools.
https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):
https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:314: class CocoaWindowMoveLoop {
On 2016/03/10 11:51:19, tapted wrote:
> Move this to its own file (with WeakCocoaMoveLoop?) -
> ui/views/cocoa/cocoa_window_move_loop.{h,mm}. I think this grew in line count
> unexpectedly while experimenting..
Oh, right, you told me about that and it seems that I forgot when reviewing the
earlier comments. Sorry, should be fixed now!
https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:322: // goes berserk if it gets them
during RunMoveLoop().
On 2016/03/10 11:51:19, tapted wrote:
> hehe - I remember writing `berserk`... Perhaps "but toolkit-views doesn't
expect
> them during RunMoveLoop()."
Done.
https://codereview.chromium.org/1747803003/diff/60001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (right):
https://codereview.chromium.org/1747803003/diff/60001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:82: //DCHECK([NSEvent pressedMouseButtons]
== 0 ||
On 2016/03/10 11:51:19, tapted wrote:
> This DCHECK would actually fail, which surprised me. I think we actually need
to
> get the origin from the window server and the size from [window frame] to
ensure
> that the window layout is in-step, synchronously with NSEvents that cause it.
> But also I think we should only use FrameIncludingDrag at all when there's a
> RunMoveLoop installed -- I think using it in other places could lead to test
> flakiness because of asynchronous stuff around the window server.
Done.
https://codereview.chromium.org/1747803003/diff/60001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:228: // if (IsMouseButtonDown())
On 2016/03/10 11:51:19, tapted wrote:
> nit: uncomment (sorry for the log spam - drag and drop is still in the
> pipeline).
Done.
I uploaded a couple of tricky test cases to a bug I created for this - http://crbug.com/594079 we need to be sure these can be resolved using this approach with CGEvents -- it might be a matter of fixing one of those TODOs, but if it's not possible we may have to revisit some of the techniques Chrome on Cocoa uses (which isn't as nice -- you can't drag a tab to another desktop, for example). Still going with the review -- I might need to tinker with the code for a bit. It's taking a while for me to upload all the oddities back into my brain from when I was experimenting with this before. It would be nice to have a test that destroys the window mid-move too.. https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:385: RunMoveLoop(GetWindowOffset(start_point_in_screen_)); On 2016/03/10 17:18:58, themblsha wrote: > On 2016/03/10 11:51:18, tapted wrote: > > I think this should land first and separately with its own explainer since it > > affects all platforms and might cause a subtle regression.. Also someone more > > familiar with tab_drag_controller.cc might know a good reason why it was > > point_in_screen rather than start_point_in_screen. > > > > (although I seem to recall that Windows would even ignore its argument to > > `RunMoveLoop` or something, but that was hard to fix -- if this just fixes the > > "slippage" on Ash it's probably worth doing -- there is > http://crbug.com/518740 > > for that) > > Without this fix the DetachToBrowserTabDragControllerTest.DragAll won't pass for > sure, and it'll also affect all other tests that rely on > CocoaWindowMoveLoop-related code to move the window. > > Maybe it's better to ensure that it doesn't break no other platforms in this CL > and add one more reviewer for that? The problem currently is that it's hard for a reviewer to understand why it's needed and reason about whether it could break things. Maybe it just needs a comment above it, like // Always use the start point <because reason>. https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:175: // On Macs if we try to drag on an inactive window it won't work. Two On 2016/03/10 17:18:58, themblsha wrote: > On 2016/03/10 11:51:19, tapted wrote: > > Hm - this should work. Or at least, it works with the Cocoa browser. Is it > just > > in the test situation that it doesn't work? Maybe the tab is obscured by > another > > window - that would probably break the simulated events. But we should try to > > explain why it doesn't work in the comment. > > Did some tests a couple more times: in order for dragging to work window needs > to be activated. In non-MacViews Chromium (as well as in Safari) we *can* start > drag on an inactive window. > > I tried to investigate why windows in interactive_ui_tests can't be activated > using normal means: BrowserWindow::Activate() doesn't work, mouse clicking > doesn't work. NSBundle seems to be swizzled the same way as in non-MacViews > builds, NSApp's activationPolicy is the same. [NSApp > activateIgnoringOtherApps:YES] didn't seem to work. > > Added -[BridgedContentView acceptsFirstMouse:], and now it works just fine. But > ui_test_utils::DragAndDrop() won't be able to monitor incoming MouseMove events, > so I've moved explicit browser window activation to that test and added a > comment. > > Also acceptsFirstMouse: == YES is enabled only for handful of objects in Cocoa > browser, and there seems to be nothing at the moment that would enable us to > enable acceptsFirstMouse: only for Views that require it. Probably this should > be handled in another issue? Yeah I think acceptsFirstMouse returning YES is good -- playing around in the browser most things seem to respond immediately. https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera... File chrome/test/base/interactive_test_utils_mac.mm (right): https://codereview.chromium.org/1747803003/diff/60001/chrome/test/base/intera... chrome/test/base/interactive_test_utils_mac.mm:23: class BridgedNativeWidgetTestApi { On 2016/03/10 17:18:58, themblsha wrote: > On 2016/03/10 11:51:19, tapted wrote: > > ooh interesting - there's already a BridgedNativeWidgetTestApi. I guess > > _technically_ this doesn't violate the ODR since one is only built in > > views_unittests and one is only built in interactive_ui_tests, but it could be > a > > weird thing for someone to discover later on. > > > > But I think the bigger problem is that chrome/test/base shouldn't have so much > > that's specific to MacViews -- I think DragAndDrop should move to a new file > > like ui/views/cocoa/test/macviews_interactive_test_utils.h > > > > But then we can't have the window_finder.h dependency -- perhaps the window > can > > just be passed into DragAndDrop from the test for that case. > > > > Then can you merge the two declarations of BridgedNativeWidgetTestApi -- that > > should go into ui/views/cocoa/test/bridged_native_widget_test_api.h/cc > > I've added BridgedNativeWidget::IsRunMoveLoopActive(), so > NativeWidgetMac::GetClientAreaBoundsInScreen() could use that, and also removed > this test-only class. nice - I like IsRunMoveLoopActive But did you see my comment about DragAndDrop: > > But I think the bigger problem is that chrome/test/base shouldn't have so much > > that's specific to MacViews -- I think DragAndDrop should move to a new file > > like ui/views/cocoa/test/macviews_interactive_test_utils.h > > > > But then we can't have the window_finder.h dependency -- perhaps the window > can > > just be passed into DragAndDrop from the test for that case. it won't help with the browser_thread.h dependency - but we should be able to access the UI thread in other ways -- it's typically just the IO/FILE/etc. threads that are hard to get at. https://codereview.chromium.org/1747803003/diff/80001/ui/base/test/ui_control... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/80001/ui/base/test/ui_control... ui/base/test/ui_controls_mac.mm:259: content::BrowserThread::PostTask( Can you just post to task_runner_? (sorry - I've still only had time to skim this file) https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:623: return window_move_loop_.get(); is the .get() needed? https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:24: if (self = [super init]) { extra parens around assignment used as condition https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:47: CGEventSetIntegerValueField(event, kCGEventSourceUserData, 1); oops - this 1 should be the constant https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:70: [owner_->ns_view() setIgnoreMouseEvents:NO]; Can this now use IsRunMoveLoopActive()? https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:96: // TODO(tapted): Move this "inside" the window or stuff breaks when dragging I think this needs to be done to fix one of those screencasts https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:106: monitor_ = 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? https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:108: mask handler:^NSEvent * (NSEvent * event) { 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. https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:120: CocoaWindowMoveLoop* thiss = 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_|? https://codereview.chromium.org/1747803003/diff/80001/ui/views/widget/native_... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/1747803003/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_mac.mm:82: const NSRect rect = ScreenRectToNSRect(gfx::Rect(bounds)); perhaps NSRect rect = ScreenRectToNSRect(gfx::Rect(bounds)); rect.size = [window frame].size; return rect;
On 2016/03/11 09:38:28, tapted wrote: > It would be nice to have a test that destroys the window mid-move too.. There is! DetachToBrowserTabDragControllerTest.PressEscapeWhileDetached does exactly that, and it's the reason why I had to add the WeakCocoaWindowMoveLoop class.
Spent the day writing tests today, and only CocoaShakeDetach is somewhat usable: https://bugs.chromium.org/p/chromium/issues/detail?id=594079#c3
Description was changed from ========== MacViews: Implement Tab Dragging Looks like it's actually not too hard to *improve* on Cocoa and actually allow the window to be dragged between spaces straight from a tab. We'll see. R=tapted BUG= COLLABORATOR=tapted@chromium.org Also reviewed in http://crrev.com/1259913003/ ========== to ========== MacViews: Implement Tab Dragging BridgedNativeWidget::RunMoveLoop() drags the window using the WindowServer, as such the window could be dragged between Spaces. Most of the tests in tab_drag_controller_interactive_uitest.cc pass, and I've added several Mac-specific ones to test for the edge cases. BUG=594079 COLLABORATOR=tapted@chromium.org Also reviewed in http://crrev.com/1259913003/ ==========
Hmm, botched the previous 'git cl upload' somehow, sorry for extra patchset :-/ Fixed most of the detachment/reattachment bugs, added tests for the edge cases, and fixed two small review issues. Will continue the work on the remaining issues next week. https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.h:46: BOOL ignoreMouseEvents_; On 2016/03/10 11:51:19, tapted (OOO.travel to Apr 5th) wrote: > needs a comment Done. This local instance property wasn't used at all :-) https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1747803003/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:35: bool g_ignore_mouse_events = false; On 2016/03/10 11:51:19, tapted (OOO.travel to Apr 5th) wrote: > needs a comment. (it should say why we need both a global and an instance member > - maybe we don't and this was just me being lazy while experimenting :) Done.
DetachToBrowserTabDragControllerTest.CancelOnNewTabWhenDragging fails with a weird reason I wasn't yet able to fix: ending RunMoveLoop() somehow results in RunLoop of CancelOnNewTabWhenDraggingStep2's waiter to immediately quit on start. I'm still investigating that. And two tests fail because fullscreen is not yet working right, but I guess that should solved first. And there's an issue of inkosher dependensy on content/public/browser/browser_thread.h in ui/base/test/ui_controls_mac.mm — would it be OK to add it to the DEPS? https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1747803003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:385: RunMoveLoop(GetWindowOffset(start_point_in_screen_)); On 2016/03/11 09:38:28, tapted (travelling to Apr 5th) wrote: > On 2016/03/10 17:18:58, themblsha wrote: > > On 2016/03/10 11:51:18, tapted wrote: > > > I think this should land first and separately with its own explainer since > it > > > affects all platforms and might cause a subtle regression.. Also someone > more > > > familiar with tab_drag_controller.cc might know a good reason why it was > > > point_in_screen rather than start_point_in_screen. > > > > > > (although I seem to recall that Windows would even ignore its argument to > > > `RunMoveLoop` or something, but that was hard to fix -- if this just fixes > the > > > "slippage" on Ash it's probably worth doing -- there is > > http://crbug.com/518740 > > > for that) > > > > Without this fix the DetachToBrowserTabDragControllerTest.DragAll won't pass > for > > sure, and it'll also affect all other tests that rely on > > CocoaWindowMoveLoop-related code to move the window. > > > > Maybe it's better to ensure that it doesn't break no other platforms in this > CL > > and add one more reviewer for that? > > The problem currently is that it's hard for a reviewer to understand why it's > needed and reason about whether it could break things. Maybe it just needs a > comment above it, like // Always use the start point <because reason>. Added this comment: // Always use the start point so the MacViews' // BridgedNativeWidget::RunMoveLoop() could position the window so the // mouse would be directly on top of start_point_in_screen_. Otherwise the // point by which the window is dragged will be wrong. // // The DetachToBrowserTabDragControllerTest.DragAll/0 test on MacViews // will detect the inconsistencies. https://codereview.chromium.org/1747803003/diff/80001/ui/base/test/ui_control... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/80001/ui/base/test/ui_control... ui/base/test/ui_controls_mac.mm:259: content::BrowserThread::PostTask( On 2016/03/11 09:38:28, tapted (travelling to Apr 5th) wrote: > Can you just post to task_runner_? task_runner_ is used here to notify the original requester, which could be on a background thread (that's what the DragAndDrop functions do). We can't simply wait using EventQueueWatcher from the start, because with CGEvents they're posted asynchronously and we have to wait for them to start, and that's what the EventMonitor class is for. https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:623: return window_move_loop_.get(); On 2016/03/11 09:38:28, tapted (travelling to Apr 5th) wrote: > is the .get() needed? Yes. error: no viable conversion from returned value of type 'const scoped_ptr<views::CocoaWindowMoveLoop>' (aka 'const std::__1::unique_ptr<views::CocoaWindowMoveLoop, std::__1::default_delete<views::CocoaWindowMoveLoop> >') to function return type 'bool' https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:24: if (self = [super init]) { On 2016/03/11 09:38:28, tapted (travelling to Apr 5th) wrote: > extra parens around assignment used as condition Done. Hmm, strange that it didn't result in any warning, maybe it's viewed as an usual ObjC convention now :/ https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:47: CGEventSetIntegerValueField(event, kCGEventSourceUserData, 1); Added kCocoaWindowMoveLoopSimulatedEventUserData constant :-) https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:70: [owner_->ns_view() setIgnoreMouseEvents:NO]; On 2016/03/11 09:38:28, tapted (travelling to Apr 5th) wrote: > Can this now use IsRunMoveLoopActive()? 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. https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:96: // TODO(tapted): Move this "inside" the window or stuff breaks when dragging On 2016/03/11 09:38:28, tapted (travelling to Apr 5th) wrote: > I think this needs to be done to fix one of those screencasts 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? https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:106: monitor_ = On 2016/03/11 09:38:28, tapted (travelling to Apr 5th) wrote: > 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? Done. Got no new test failures, so it seems to be fine. https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:108: mask handler:^NSEvent * (NSEvent * event) { On 2016/03/11 09:38:28, tapted (travelling to Apr 5th) wrote: > I think we'll have to override clang format on this.. Done. Extracted the 'auto handler' variable. https://codereview.chromium.org/1747803003/diff/80001/ui/views/cocoa/cocoa_wi... ui/views/cocoa/cocoa_window_move_loop.mm:120: CocoaWindowMoveLoop* thiss = On 2016/03/11 09:38:28, tapted (travelling to Apr 5th) wrote: > 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_|? 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. https://codereview.chromium.org/1747803003/diff/80001/ui/views/widget/native_... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/1747803003/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_mac.mm:82: const NSRect rect = ScreenRectToNSRect(gfx::Rect(bounds)); On 2016/03/11 09:38:28, tapted (travelling to Apr 5th) wrote: > perhaps > > NSRect rect = ScreenRectToNSRect(gfx::Rect(bounds)); > rect.size = [window frame].size; > return rect; Done. Although the code now lives inside NativeWidgetMac::WindowServerFrame().
I'll do a more thorough review tomorrow, but it's a super complex CL. Just addressing a couple of things below, but there's more I need to follow up on too. A general comment - we've been working around some text flakiness elsewhere when using the ObjectiveC dispatch stuff in tests -- it might be more robust (and familiar to more Chrome engineers) to use our own TaskRunner APIs, if that's possible. But I need to dive a lot deeper into the CL to see what is justified here. https://codereview.chromium.org/1747803003/diff/80001/ui/base/test/ui_control... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/80001/ui/base/test/ui_control... ui/base/test/ui_controls_mac.mm:259: content::BrowserThread::PostTask( On 2016/04/05 17:20:42, themblsha wrote: > On 2016/03/11 09:38:28, tapted (travelling to Apr 5th) wrote: > > Can you just post to task_runner_? > > task_runner_ is used here to notify the original requester, which could be on a > background thread (that's what the DragAndDrop functions do). We can't simply > wait using EventQueueWatcher from the start, because with CGEvents they're > posted asynchronously and we have to wait for them to start, and that's what the > EventMonitor class is for. Ah, this was my plan for removing the browser_thread dependency -- it would be a tough proposition to OWNERS to allow that DEPS change -- src/ui shouldn't depend on src/content since it will make a dependency loop. I think we will need to pass in a task runner for the UI thread at an appropriate place and store it on a data member. It might be enough simply to grab it from MessageLoop::current()->task_runner(), but that might need an explicit Init() call rather than relying on lazy initialization in instance() https://codereview.chromium.org/1747803003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1747803003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:385: // Always use the start point so the MacViews' This affects all platforms, so we need to say why it's right to use start_point_in_screen_ in general. Also it's typical in a comment not just to say something will be wrong, but to say how it's wrong. I think the gist of this is something like // Use the start point to initialize the move loop, otherwise the point under the cursor will be offset by the drag threshold from where the user originally clicked, which could be entirely off the window. https://codereview.chromium.org/1747803003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/window_finder_mac.mm (right): https://codereview.chromium.org/1747803003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/window_finder_mac.mm:27: if (![window isVisible]) { nit: no curlies https://codereview.chromium.org/1747803003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/window_finder_mac.mm:31: if (NSPointInRect(ns_point, [window frame])) { nit: no curlies
I'm working from home today and for some reason was unable to get 'git cl upload' to actually upload. Will try from work tomorrow. Here's the diff: https://gist.github.com/mblsha/83efbf4d15b154a6562b65ef705d41de The CancelOnNewTabWhenDragging issue was weird. 1. CancelOnNewTabWhenDraggingStep2 is getting invoked from CocoaWindowMoveLoop's MessagePumpNSApplication. 2. chrome::AddTabAt() is called, which results in a BridgedNativeWidget::EndMoveLoop() call. 3. chrome::AddTabAt() returns, but we're still inside CocoaWindowMoveLoop's MessagePumpNSApplication nested call. 4. observer.Wait() is called while current MessagePumpNSApplication is cancelled, but it didn't have a chance to process the Quit call, as it's still inside the nested call. 5. observer.Wait() immediately quits and DCHECKs on the seen_ flag, as it didn't have a chance to wait for the notification. So instead I moved the NOTIFICATION_LOAD_STOP to the main test body, so that it would not create a nested RunLoop while its parent RunLoop was supposed to exit. Also had to add a global ui_test_utils::BringBrowserWindowToFront() call, otherwise all tests failed when there were some obstructing windows. Something changed in MacViews in the last week or two, previously it worked without that line, even though the windows weren't 'key', they were shown in front of other windows. I think this should be dealt with separately. Fixed the invalid DEPS as well, the issue was me not using base::Lock for synchronization. And fixed a couple of nits. https://codereview.chromium.org/1747803003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1747803003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:385: // Always use the start point so the MacViews' On 2016/04/06 09:38:51, tapted wrote: > This affects all platforms, so we need to say why it's right to use > start_point_in_screen_ in general. Also it's typical in a comment not just to > say something will be wrong, but to say how it's wrong. Thanks, your suggestion is correct and succinct. https://codereview.chromium.org/1747803003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/window_finder_mac.mm (right): https://codereview.chromium.org/1747803003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/window_finder_mac.mm:27: if (![window isVisible]) { On 2016/04/06 09:38:51, tapted wrote: > nit: no curlies Done. https://codereview.chromium.org/1747803003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/window_finder_mac.mm:31: if (NSPointInRect(ns_point, [window frame])) { On 2016/04/06 09:38:51, tapted wrote: > nit: no curlies Done.
Uploaded yesterdays fixes.
On 2016/04/07 10:30:02, themblsha wrote: > Uploaded yesterdays fixes. There's still an issue with dragging up really quickly from the top edge of the tab - the window starts to resize. I think we need to nudge the mouse cursor back into the window by some threshold so that the simulated mousedown isn't interpreted by the window server as a request to resize. Apart from that, I've started a thorough review, and have a bunch of drafts, but wanted to give you a cohesive set of comments. There's some stuff I want to play around with too - if there's a neat way to avoid GCD in ui_test_utils::DragAndDrop, I think that would be a big improvement. Maybe I can figure out something for that window-resizing issue.. I'm time-poor for the next couple of days, so leave it with me for a bit.
OK this is enough to go on. Some (but not all) of the review recommendations I've uploaded to https://codereview.chromium.org/1877043003 so you can see the code I tried out. You can diff against earlier patchsets to set the changes. Lots of stuff here, but still two major things: - ESC doesn't work (see comments) - I think the changes to interactive_test_utils_mac.mm can be a lot simpler I haven't explored the interactive_test_utils_mac changes in depth yet. Ideally, we would not use objective-C blocks there at all, and just use base::Closure instead -- more Chrome engineers understand those, whereas ObjectiveC blocks and dispatch queues are really niche, have tricky rules, and have been causing us headaches with flaky tests -- see e.g. https://codereview.chromium.org/1829603002 https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:179: EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser)); Let's move this to TabDragControllerTest::SetUpOnMainThread and ensure DetachToBrowserTabDragControllerTest::SetUpOnMainThread calls its superclass's TabDragControllerTest::SetUpOnMainThread() nothing else should have to call BringBrowserWindowToFront then https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:920: EXPECT_TRUE(new_browser->window()->IsMaximized()); this will fail on Mac (likewise a similar check on line 942) - I haven't mapped out exactly what "maximized" means on Mac - for other platforms it typically changes the window frame, but that doesn't happen for Mac's "zoom" - only fullscreen. We also can't just fill the screen with the browser window, since that's not how "zoom" works. I think this needs a separate crbug filed to track, and this line should be #fidef'd out on Mac since, regardless of how it works, it doesn't seem right to automatically maximize a window when there were no maximized windows to start with - not on mac at least. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:942: ASSERT_TRUE(browser()->window()->IsMaximized()); fails - I think here this entire test can be can be disabled on mac with a comment citing that bug https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1259: const gfx::Rect final_bounds = browser()->window()->GetBounds(); Comment ~ // Size unchanged, but it should have moved down. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1275: // Perhaps a note here, since it will be confusing for those unfamiliar, to say // Note that Mac doesn't simply rely on NSEvents, since they would not allow // correct interactions with the window server. E.g., a window could not be // dragged to another desktop Space. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1278: IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest, A comment specific to this test? E.g. // Test dragging all the tabs in the window. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1344: ui_test_utils::DragAndDrop(tab_0_center, on_top_of_menu_bar); using a sequence of DragAndDropOperation in place of this causes a failure for me. E.g. I did browser()->window()->SetBounds(gfx::Rect(100, 200, 400, 200)); DCHECK_GT(browser()->window()->GetBounds().y(), GetDetachY(tab_strip)); gfx::Point tab_0_center(GetCenterInScreenCoordinates(tab_strip->tab_at(0))); gfx::Point at_top_of_screen(tab_0_center.x(), 0); gfx::Point below_top_of_screen(tab_0_center.x(), 100); std::list<DragAndDropOperation> operations; operations.push_back(DragAndDropOperation::Move(tab_0_center)); operations.push_back(DragAndDropOperation::MouseDown()); operations.push_back(DragAndDropOperation::Move(at_top_of_screen)); operations.push_back(DragAndDropOperation::Move(below_top_of_screen)); operations.push_back(DragAndDropOperation::MouseUp()); ui_test_utils::DragAndDrop(operations); If at_top_of_screen is 50px instead of 0px from the top, the test works fine. But with 0px the test fails - the window moves up, but doesn't initiate a drag. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1361: using namespace ui_test_utils; nit: using ui_test_utils::DragAndDropOperation, and move it to above the std::list declaration https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1376: // will deadlock. nit: this comment doesn't need to repeat https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1380: operations.emplace_back(DragAndDropOperation::Move(tab_1_center)); The rule at Google is to use push_back unless that doesn't work. DragAndDropOperation is copyable, so it should be push_back here (The downside of emplace_back is that it invisibly calls explicit constructors which can lead to unexpected stuff like vector<vector<int>> baz; baz.emplace_back(1 << 10); // Inserts a vector of size 1024. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1679: // to exercise past crash when model/tabstrip got out of sync (474082). This comment should move as well, since it talks about the bug (http://crbug.com/474082) which has "a new webcontents being created in the middle of a drag, resulting in a canceled drag where the tabstrip view and its model are out of sync." - i.e. it needs the LOAD_STOP stuff to exercise that properly https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1714: // On MacViews the Drag ends while waiting for NOTIFICATION_LOAD_STOP. can you say more about this? (i.e. why it ends early on Mac) https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1715: if (TabDragController::IsActive()) { nit: no curlies https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:198: if (g_ignore_next_mouse_down_for_draggable_regions) { Is it possible to scrap g_ignore_next_mouse_down_for_draggable_regions if we check for kCGEventSourceUserData == views::kCocoaWindowMoveLoopSimulatedEventUserData? Otherwise we should comment on the g_ignore_next_mouse_down_for_draggable_regions declaration about why it's necessary https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:311: extern "C" { keep this where it was? https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:641: gfx::ConstrainToEnclosingRect(display.work_area(), mouse_in_screen); This isn't right. Only the menu bar affects this constraint -- not the Dock (which also reduces the work area). My fix was to replace lines 636 to 654 with: // A window can't be moved vertically up out of the work area. Treat this case // as if the mouse location is at the point it would be when the window first // stopped moving. That is, vertically down such that the top edge of the // window touches the menubar (or top of the screen in a dual-screen setup). // Note on Mac we can assume that the y-coordinate of the work area origin is // the bottom of the menu bar and not the Dock which doesn't affect window // movement, but can reduce the work area on the bottom, left and right. const gfx::Display display = gfx::Screen::GetScreen()->GetDisplayNearestPoint(mouse_in_screen); int min_y = display.work_area().y() + drag_offset.y(); if (mouse_in_screen.y() < min_y) mouse_in_screen.set_y(min_y); gfx::Rect frame = gfx::ScreenRectFromNSRect([window_ frame]); frame.set_x(mouse_in_screen.x() - drag_offset.x()); frame.set_y(mouse_in_screen.y() - drag_offset.y()); DCHECK_GE(frame.y(), display.work_area().y()); ConstrainToEnclosingRect isn't needed https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:661: // Animating may provide a less janky UX, but something custom would be This comment can go - I've changed my mind since I wrote it (animating wouldn't work here) https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:673: // I was able to always replicate old window coordinates from Shouldn't use "I " in comments (typically not "we" either). Maybe // El Capitan would always replicate the old window coordinates when calling CGSGetWindowBounds(). https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.h (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:16: extern const int kCocoaWindowMoveLoopSimulatedEventUserData; nit: declare as a static member on CocoaWindowMoveLoop https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:18: // Used by views::BridgedNativeWidget This can say more. E.g. // Used when dragging tabs, to manually position the window. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:21: explicit CocoaWindowMoveLoop(BridgedNativeWidget* owner, nit: explicit not needed https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:22: gfx::Point initial_mouse_in_screen); nit: const ref https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:25: Widget::MoveLoopResult Run(); this looks weird without a comment. Perhaps it's worth saying under what conditions the Run will end e.g. // Initiates the drag with a simulated mouse click then monitors window server events until a mouse up or Escape keypress is observed, or End() is called. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:40: // created. Cache this as the mouse could be moved when we're inside Run(), remove "we're" https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:41: // and then sending the MouseDown event will fail. say why it will fail? https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:17: @interface WeakCocoaWindowMoveLoop : NSObject { I still think this is unnecessary -- I think the issue is just that the event monitor was only checking for WINDOW_DESTROYED but it also needs to check for ENDED_EXTERNALLY -- either of those may indicate the destructor being called (since End() prevents the destructor updating to ENDED_EXTERNALLY). *except* It can't check for ENDED_EXTERNALLY since that's what the exit reason is initialized with :p. So I think it needs a new enum value - that's probably clearer anyway. However, the weak_factory_ is needed for another reason - see below. See also https://codereview.chromium.org/1877043003 where I've removed this @interface https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:49: gfx::Point mouse_position, nit: const ref https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:55: NSPoint ns_mouse_position = gfx::ScreenPointToNSPoint(mouse_position); This should move to a separate function, since it's not used in a release build and the compiler can't optimize out the calls because of possible side-effects. E.g. NSWindow* WindowAtScreenPoint(const gfx::Point& point) { ... DCHECK_EQ(expected_window, WindowAtScreenPoint(mouse_position)); https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:114: scoped_ptr<base::RunLoop> run_loop(std::move(run_loop_)); I think this might not be necessary, and we can just do base::RunLoop run_loop; quit_closure_ = run_loop.QuitClosure(); (and remove the run_loop_ data member). https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:142: base::Unretained(strong->owner_))); I don't think this can be Unretained - this should pass weak_factory_.GetWeakPtr() and call a method on CocoaWindowMoveLoop which invokes owner_->OnPositionChanged see what I did in https://codereview.chromium.org/1877043003 https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:157: // NSKeyDownMask doesn't work inside addLocalMonitorForEventsMatchingMask: This is still the case right? Did you investigate ways to fix this and get Escape to work properly? I think we need this to work. It "works" in tests because the test event goes in at a different level, but when *actually* dragging tabs and pressing ESC, the tab isn't going back into the window it was previously in. https://codereview.chromium.org/1747803003/diff/200001/ui/views/widget/native... File ui/views/widget/native_widget_mac.h (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_mac.h:51: gfx::Rect WindowServerFrame() const; Can this go on BridgedNativeWidget instead? It's fine for NativeWidgetMac to call bridge_->WindowServerFrame. (NativeWidgetMac should be lightweight, since things in chrome/browser can inherit from it, and it will be harder to share stuff we put on it when we need things for aura) And I think it should be something like NSRect BridgedNativeWidget::GetFrameIncludingLiveDrag() { if (IsRunMoveLoopActive()) { return WindowServerFrame(window_); } return [window_ frame]; } WindowServerFrame can probably be put in an anonymous namespace, or even integrated into GetFrameIncludingLiveDrag() this will allow GetWindowBoundsInScreen and GetClientAreaBoundsInScreen to share more logic
Guess I now have to clean up ui/gfx/geometry/rect* after your simplification of menu bar frame adjustment code, and try to get rid of GCD :-) Regarding the GCD / CFRunLoopPerformBlock: The interactive_ui_tests are not supposed to run concurrently with other tests, as they deal with focus issues, and if some other process could steal it, it will result in an incredible amount of flakiness. The drag'n'drop code won't work for sure when several tests run in parallel. My CL uses GCD only as a syntax sugar to send Drag-n-Drop events from the background queue, so it could easily be rewritten in some other way as well. Also CFRunLoopPerformBlock and GCD seem to differ in their behavior, with GCD being more low-level: http://stackoverflow.com/questions/12871737/cfrunloopperformblock-vs-dispatch... https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:179: EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser)); On 2016/04/13 08:28:12, tapted wrote: > Let's move this to TabDragControllerTest::SetUpOnMainThread and ensure > DetachToBrowserTabDragControllerTest::SetUpOnMainThread calls its superclass's > TabDragControllerTest::SetUpOnMainThread() > > nothing else should have to call BringBrowserWindowToFront then Done. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:920: EXPECT_TRUE(new_browser->window()->IsMaximized()); On 2016/04/13 08:28:12, tapted wrote: > this will fail on Mac (likewise a similar check on line 942) - I haven't mapped > out exactly what "maximized" means on Mac - for other platforms it typically > changes the window frame, but that doesn't happen for Mac's "zoom" - only > fullscreen. We also can't just fill the screen with the browser window, since > that's not how "zoom" works. > > I think this needs a separate crbug filed to track, and this line should be > #fidef'd out on Mac since, regardless of how it works, it doesn't seem right to > automatically maximize a window when there were no maximized windows to start > with - not on mac at least. Yeah, the two test that fail deal with maximization. Maybe IsMaximized / Maximize functions need to be #ifdef'd on Mac as well, so all the places would have to be explicitly audited? Created http://crbug.com/603562, but can't edit its fields to match the rest of MacViews issues. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:942: ASSERT_TRUE(browser()->window()->IsMaximized()); On 2016/04/13 08:28:12, tapted wrote: > fails - I think here this entire test can be can be disabled on mac with a > comment citing that bug Done. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1259: const gfx::Rect final_bounds = browser()->window()->GetBounds(); On 2016/04/13 08:28:12, tapted wrote: > Comment ~ > > // Size unchanged, but it should have moved down. Done. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1275: // On 2016/04/13 08:28:12, tapted wrote: > Perhaps a note here, since it will be confusing for those unfamiliar, to say > > // Note that Mac doesn't simply rely on NSEvents, since they would not allow > // correct interactions with the window server. E.g., a window could not be > // dragged to another desktop Space. Done. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1278: IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest, On 2016/04/13 08:28:12, tapted wrote: > A comment specific to this test? > > E.g. > > // Test dragging all the tabs in the window. Done. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1344: ui_test_utils::DragAndDrop(tab_0_center, on_top_of_menu_bar); On 2016/04/13 08:28:12, tapted wrote: > using a sequence of DragAndDropOperation in place of this causes a failure for > me. E.g. I did > > > browser()->window()->SetBounds(gfx::Rect(100, 200, 400, 200)); > DCHECK_GT(browser()->window()->GetBounds().y(), GetDetachY(tab_strip)); > > gfx::Point tab_0_center(GetCenterInScreenCoordinates(tab_strip->tab_at(0))); > gfx::Point at_top_of_screen(tab_0_center.x(), 0); > gfx::Point below_top_of_screen(tab_0_center.x(), 100); > > std::list<DragAndDropOperation> operations; > operations.push_back(DragAndDropOperation::Move(tab_0_center)); > operations.push_back(DragAndDropOperation::MouseDown()); > operations.push_back(DragAndDropOperation::Move(at_top_of_screen)); > operations.push_back(DragAndDropOperation::Move(below_top_of_screen)); > operations.push_back(DragAndDropOperation::MouseUp()); > ui_test_utils::DragAndDrop(operations); > > If at_top_of_screen is 50px instead of 0px from the top, the test works fine. > But with 0px the test fails - the window moves up, but doesn't initiate a drag. Added additional check to my code: EXPECT_GE(work_area.y(), 22) << "Without the MenuBar on top of work_area this test is pointless."; Maybe somehow you don't have the menu bar on top of your screen? https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1361: using namespace ui_test_utils; On 2016/04/13 08:28:12, tapted wrote: > nit: using ui_test_utils::DragAndDropOperation, and move it to above the > std::list declaration Done. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1376: // will deadlock. On 2016/04/13 08:28:12, tapted wrote: > nit: this comment doesn't need to repeat Done. https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1380: operations.emplace_back(DragAndDropOperation::Move(tab_1_center)); On 2016/04/13 08:28:12, tapted wrote: > The rule at Google is to use push_back unless that doesn't work. > DragAndDropOperation is copyable, so it should be push_back here (The downside > of emplace_back is that it invisibly calls explicit constructors which can lead > to unexpected stuff like > > vector<vector<int>> baz; > baz.emplace_back(1 << 10); // Inserts a vector of size 1024. > Done. Style Guide is absolute :-) https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1679: // to exercise past crash when model/tabstrip got out of sync (474082). On 2016/04/13 08:28:12, tapted wrote: > This comment should move as well, since it talks about the bug > (http://crbug.com/474082) which has "a new webcontents being created in the > middle of a drag, resulting in a canceled drag where the tabstrip view and its > model are out of sync." - i.e. it needs the LOAD_STOP stuff to exercise that > properly Done, thanks! https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1714: // On MacViews the Drag ends while waiting for NOTIFICATION_LOAD_STOP. On 2016/04/13 08:28:12, tapted wrote: > can you say more about this? (i.e. why it ends early on Mac) Maybe after moving the stuff around it's not Mac-specific anymore but I haven't tested. Previously it issued an async command to drag, then immediately executed QuitWhenNotDragging(). Now on Mac NOTIFICATION_LOAD_STOP executes after the drag'n'drop was cancelled. Thinking about it now I think this should be removed, and testbots will show if my assumption is wrong. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:198: if (g_ignore_next_mouse_down_for_draggable_regions) { On 2016/04/13 08:28:12, tapted wrote: > Is it possible to scrap g_ignore_next_mouse_down_for_draggable_regions if we > check for kCGEventSourceUserData == > views::kCocoaWindowMoveLoopSimulatedEventUserData? > > Otherwise we should comment on the > g_ignore_next_mouse_down_for_draggable_regions declaration about why it's > necessary Yep, kCocoaWindowMoveLoopSimulatedEventUserData works perfectly fine, thanks for the idea! https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:311: extern "C" { On 2016/04/13 08:28:12, tapted wrote: > keep this where it was? Done. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:641: gfx::ConstrainToEnclosingRect(display.work_area(), mouse_in_screen); On 2016/04/13 08:28:12, tapted wrote: > This isn't right. Only the menu bar affects this constraint -- not the Dock > (which also reduces the work area). > > My fix was to replace lines 636 to 654 with: > > // A window can't be moved vertically up out of the work area. Treat this case > // as if the mouse location is at the point it would be when the window first > // stopped moving. That is, vertically down such that the top edge of the > // window touches the menubar (or top of the screen in a dual-screen setup). > // Note on Mac we can assume that the y-coordinate of the work area origin is > // the bottom of the menu bar and not the Dock which doesn't affect window > // movement, but can reduce the work area on the bottom, left and right. > const gfx::Display display = > gfx::Screen::GetScreen()->GetDisplayNearestPoint(mouse_in_screen); > int min_y = display.work_area().y() + drag_offset.y(); > if (mouse_in_screen.y() < min_y) > mouse_in_screen.set_y(min_y); > > gfx::Rect frame = gfx::ScreenRectFromNSRect([window_ frame]); > frame.set_x(mouse_in_screen.x() - drag_offset.x()); > frame.set_y(mouse_in_screen.y() - drag_offset.y()); > DCHECK_GE(frame.y(), display.work_area().y()); > > > ConstrainToEnclosingRect isn't needed Tests still pass after these changes, so it's good :-) https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:661: // Animating may provide a less janky UX, but something custom would be On 2016/04/13 08:28:12, tapted wrote: > This comment can go - I've changed my mind since I wrote it (animating wouldn't > work here) Done. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:673: // I was able to always replicate old window coordinates from On 2016/04/13 08:28:12, tapted wrote: > Shouldn't use "I " in comments (typically not "we" either). > > Maybe > > // El Capitan would always replicate the old window coordinates when calling > CGSGetWindowBounds(). Thanks :) https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.h (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:16: extern const int kCocoaWindowMoveLoopSimulatedEventUserData; On 2016/04/13 08:28:13, tapted wrote: > nit: declare as a static member on CocoaWindowMoveLoop Done. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:18: // Used by views::BridgedNativeWidget On 2016/04/13 08:28:12, tapted wrote: > This can say more. E.g. > > // Used when dragging tabs, to manually position the window. Done. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:21: explicit CocoaWindowMoveLoop(BridgedNativeWidget* owner, On 2016/04/13 08:28:12, tapted wrote: > nit: explicit not needed Done. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:22: gfx::Point initial_mouse_in_screen); On 2016/04/13 08:28:13, tapted wrote: > nit: const ref Done. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:25: Widget::MoveLoopResult Run(); On 2016/04/13 08:28:13, tapted wrote: > this looks weird without a comment. Perhaps it's worth saying under what > conditions the Run will end e.g. > > // Initiates the drag with a simulated mouse click then monitors window server > events until a mouse up or Escape keypress is observed, or End() is called. Thanks, done. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:40: // created. Cache this as the mouse could be moved when we're inside Run(), On 2016/04/13 08:28:13, tapted wrote: > remove "we're" Done. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:41: // and then sending the MouseDown event will fail. On 2016/04/13 08:28:12, tapted wrote: > say why it will fail? Done. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:17: @interface WeakCocoaWindowMoveLoop : NSObject { On 2016/04/13 08:28:13, tapted wrote: > I still think this is unnecessary -- I think the issue is just that the event > monitor was only checking for WINDOW_DESTROYED but it also needs to check for > ENDED_EXTERNALLY -- either of those may indicate the destructor being called > (since End() prevents the destructor updating to ENDED_EXTERNALLY). *except* It > can't check for ENDED_EXTERNALLY since that's what the exit reason is > initialized with :p. So I think it needs a new enum value - that's probably > clearer anyway. > > However, the weak_factory_ is needed for another reason - see below. > > See also https://codereview.chromium.org/1877043003 where I've removed this > @interface In my testing the sometimes run_loop was Quit()'d, the monitor was unregistered, the *whole stack frame* unwound, and only then I got the monitor notification. Your approach would result access to the memory that was in the removed stack frame, and results would be undetermined. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:49: gfx::Point mouse_position, On 2016/04/13 08:28:13, tapted wrote: > nit: const ref Done. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:55: NSPoint ns_mouse_position = gfx::ScreenPointToNSPoint(mouse_position); On 2016/04/13 08:28:13, tapted wrote: > This should move to a separate function, since it's not used in a release build > and the compiler can't optimize out the calls because of possible side-effects. > > E.g. > > NSWindow* WindowAtScreenPoint(const gfx::Point& point) { ... > DCHECK_EQ(expected_window, WindowAtScreenPoint(mouse_position)); Done. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:114: scoped_ptr<base::RunLoop> run_loop(std::move(run_loop_)); On 2016/04/13 08:28:13, tapted wrote: > I think this might not be necessary, and we can just do > > base::RunLoop run_loop; > quit_closure_ = run_loop.QuitClosure(); > > (and remove the run_loop_ data member). Yep, works fine. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:142: base::Unretained(strong->owner_))); On 2016/04/13 08:28:13, tapted wrote: > I don't think this can be Unretained - this should pass > weak_factory_.GetWeakPtr() and call a method on CocoaWindowMoveLoop which > invokes owner_->OnPositionChanged > > see what I did in https://codereview.chromium.org/1877043003 Nice idea, thanks. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:157: // NSKeyDownMask doesn't work inside addLocalMonitorForEventsMatchingMask: On 2016/04/13 08:28:13, tapted wrote: > This is still the case right? Did you investigate ways to fix this and get > Escape to work properly? I think we need this to work. It "works" in tests > because the test event goes in at a different level, but when *actually* > dragging tabs and pressing ESC, the tab isn't going back into the window it was > previously in. For me it works fine on live Chromium. The block views::CocoaMouseCapture::ActiveEventTap::Init() captures the Esc keypress and correctly ends the drag. So I consider that comment to be accurate. Here's my stacktrace: http://pastie.org/10799043 https://codereview.chromium.org/1747803003/diff/200001/ui/views/widget/native... File ui/views/widget/native_widget_mac.h (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_mac.h:51: gfx::Rect WindowServerFrame() const; On 2016/04/13 08:28:13, tapted wrote: > Can this go on BridgedNativeWidget instead? > > It's fine for NativeWidgetMac to call bridge_->WindowServerFrame. > (NativeWidgetMac should be lightweight, since things in chrome/browser can > inherit from it, and it will be harder to share stuff we put on it when we need > things for aura) > > And I think it should be something like > > NSRect BridgedNativeWidget::GetFrameIncludingLiveDrag() { > if (IsRunMoveLoopActive()) { > return WindowServerFrame(window_); > } > return [window_ frame]; > } > > WindowServerFrame can probably be put in an anonymous namespace, or even > integrated into GetFrameIncludingLiveDrag() > > > this will allow GetWindowBoundsInScreen and GetClientAreaBoundsInScreen to share > more logic Well, I moved it to BridgedNativeWidget, as it uses WindowServerFrame() after -setFrame: in a DCHECK.
I tried replacing GCD with base::Thread but it resulted in base::RunLoop never quitting, probably as a result of nested RunLoops again. DelegateSimpleThread work splendidly though.
Another pass - I'll try to absorb all the ui::DragAndDrop stuff this week and do a full review there https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1344: ui_test_utils::DragAndDrop(tab_0_center, on_top_of_menu_bar); On 2016/04/18 09:30:01, themblsha wrote: > On 2016/04/13 08:28:12, tapted wrote: > > using a sequence of DragAndDropOperation in place of this causes a failure for > > me. E.g. I did > > > > > > browser()->window()->SetBounds(gfx::Rect(100, 200, 400, 200)); > > DCHECK_GT(browser()->window()->GetBounds().y(), GetDetachY(tab_strip)); > > > > gfx::Point tab_0_center(GetCenterInScreenCoordinates(tab_strip->tab_at(0))); > > gfx::Point at_top_of_screen(tab_0_center.x(), 0); > > gfx::Point below_top_of_screen(tab_0_center.x(), 100); > > > > std::list<DragAndDropOperation> operations; > > operations.push_back(DragAndDropOperation::Move(tab_0_center)); > > operations.push_back(DragAndDropOperation::MouseDown()); > > operations.push_back(DragAndDropOperation::Move(at_top_of_screen)); > > operations.push_back(DragAndDropOperation::Move(below_top_of_screen)); > > operations.push_back(DragAndDropOperation::MouseUp()); > > ui_test_utils::DragAndDrop(operations); > > > > If at_top_of_screen is 50px instead of 0px from the top, the test works fine. > > But with 0px the test fails - the window moves up, but doesn't initiate a > drag. > > Added additional check to my code: > > EXPECT_GE(work_area.y(), 22) > << "Without the MenuBar on top of work_area this test is pointless."; > > Maybe somehow you don't have the menu bar on top of your screen? On one of them :). If you run two screens side-by-side, and don't use "Displays have separate spaces" then one of the monitors will not have a menu bar at the top. And the screen edge restricts movement in the same way as the menubar does. But the point of my comment was that the test previously failed (regardless of the menu bar presence). The checks were not enough to pick up the problem that required fixes to move the cursor not just into the work area, but to ensure it was in the window in the same place as the work area would have forced it to move to. So we should make sure we have a test case for this. In fact I wrote one already at https://codereview.chromium.org/1877043003/diff2/20001:60001/chrome/browser/u... https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:17: @interface WeakCocoaWindowMoveLoop : NSObject { On 2016/04/18 09:30:02, themblsha wrote: > On 2016/04/13 08:28:13, tapted wrote: > > I still think this is unnecessary -- I think the issue is just that the event > > monitor was only checking for WINDOW_DESTROYED but it also needs to check for > > ENDED_EXTERNALLY -- either of those may indicate the destructor being called > > (since End() prevents the destructor updating to ENDED_EXTERNALLY). *except* > It > > can't check for ENDED_EXTERNALLY since that's what the exit reason is > > initialized with :p. So I think it needs a new enum value - that's probably > > clearer anyway. > > > > However, the weak_factory_ is needed for another reason - see below. > > > > See also https://codereview.chromium.org/1877043003 where I've removed this > > @interface > > In my testing the sometimes run_loop was Quit()'d, the monitor was unregistered, > the *whole stack frame* unwound, and only then I got the monitor notification. > Your approach would result access to the memory that was in the removed stack > frame, and results would be undetermined. I'm not sure this is true - did asan confirm or deny this after applying the code to remove the @interface. We don't want the code to suggest the problem is more complex than it is. Here, I think we would only require the @interface if calls could be made to the EventMonitor after CocoaWindowMoveLoop::Run() has returned (regardless of whether CocoaWindowMoveLoop itself has been destroyed). Since CocoaWindowMoveLoop::Run() removes the EventMonitor before it returns, that should be impossible. So, as long as EventMonitor verifies its state first by using stack variables declared inside Run(), we should be able to avoid UAF. https://codereview.chromium.org/1747803003/diff/240001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.h (right): https://codereview.chromium.org/1747803003/diff/240001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:28: static const int kCocoaWindowMoveLoopSimulatedEventUserData = 1; sorry - I should have said to remove the 'CocoaWindowMoveLoop' bit too - it's redundant now it's a class member https://codereview.chromium.org/1747803003/diff/240001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/240001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:184: // by setting a flag here. Note this assumes the custom mouseDown event this Epic comment needs rewording now that we no longer set a flag here
https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1344: ui_test_utils::DragAndDrop(tab_0_center, on_top_of_menu_bar); On 2016/04/20 07:30:22, tapted wrote: > On one of them :). > > If you run two screens side-by-side, and don't use "Displays have separate > spaces" then one of the monitors will not have a menu bar at the top. And the > screen edge restricts movement in the same way as the menubar does. > > But the point of my comment was that the test previously failed (regardless of > the menu bar presence). The checks were not enough to pick up the problem that > required fixes to move the cursor not just into the work area, but to ensure it > was in the window in the same place as the work area would have forced it to > move to. > > So we should make sure we have a test case for this. In fact I wrote one already > at > https://codereview.chromium.org/1877043003/diff2/20001:60001/chrome/browser/u... Curiously enough, your test only passes when I have two 27" monitors plugged in, when only a single one is plugged it fails. But after replacing 2500 with 100 it works fine even in single-monitor configuration. And your test will fail if the BridgedNativeWidget::RunMoveLoop() mouse y position shifting is removed even when the menu bar is not present, so it's better than my version. Replaced my test function with yours. https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:17: @interface WeakCocoaWindowMoveLoop : NSObject { Sorry for not being clear in my explanations at what's happening here. On 2016/04/20 07:30:22, tapted wrote: > I'm not sure this is true - did asan confirm or deny this after applying the > code to remove the @interface. I didn't try it on an ASAN build as it takes a while to recompile, but even without ASAN it crashes fairly frequently to see the issue. > Here, I think we would only require the @interface if calls could be made to the > EventMonitor after CocoaWindowMoveLoop::Run() has returned (regardless of > whether CocoaWindowMoveLoop itself has been destroyed). > > Since CocoaWindowMoveLoop::Run() removes the EventMonitor before it returns, > that should be impossible. So, as long as EventMonitor verifies its state first > by using stack variables declared inside Run(), we should be able to avoid UAF. How the crash happens (http://pastie.org/10799043): 1. We get an ESC keypress event, -[NSApplication sendEvent:] calls _NSSendEventToObservers and it *caches* the list of all registered event monitors. 2. views::EventMonitorMac::EventMonitorMac()'s monitor gets the ESC keypress, and it ends the drag and the CocoaMoveLoop. 3. CocoaMoveLoop::End() quits the run_loop. 4. CocoaMoveLoop event monitor is unregistered, but is still cached by _NSSendEventToObservers. 5. _NSSendEventToObservers sends ESC keypress to the CocoaMoveLoop's monitor, even though it's supposed to be unregistered at this point. At this point the CocoaMoveLoop::Run()'s stack frame is unwound and the object is destroyed. You might not be getting this crash because the ESC-cancelling of drag operation is somehow not working for you, but the event propagation to the event monitors should be similar. https://codereview.chromium.org/1747803003/diff/240001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.h (right): https://codereview.chromium.org/1747803003/diff/240001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:28: static const int kCocoaWindowMoveLoopSimulatedEventUserData = 1; On 2016/04/20 07:30:22, tapted wrote: > sorry - I should have said to remove the 'CocoaWindowMoveLoop' bit too - it's > redundant now it's a class member Ah, yeah, that's much better :-) https://codereview.chromium.org/1747803003/diff/240001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/240001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:184: // by setting a flag here. Note this assumes the custom mouseDown event On 2016/04/20 07:30:22, tapted wrote: > this Epic comment needs rewording now that we no longer set a flag here Done.
https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:17: @interface WeakCocoaWindowMoveLoop : NSObject { On 2016/04/20 13:38:40, themblsha wrote: > Sorry for not being clear in my explanations at what's happening here. > > On 2016/04/20 07:30:22, tapted wrote: > > I'm not sure this is true - did asan confirm or deny this after applying the > > code to remove the @interface. > > I didn't try it on an ASAN build as it takes a while to recompile, but even > without ASAN it crashes fairly frequently to see the issue. > > > Here, I think we would only require the @interface if calls could be made to > the > > EventMonitor after CocoaWindowMoveLoop::Run() has returned (regardless of > > whether CocoaWindowMoveLoop itself has been destroyed). > > > > Since CocoaWindowMoveLoop::Run() removes the EventMonitor before it returns, > > that should be impossible. So, as long as EventMonitor verifies its state > first > > by using stack variables declared inside Run(), we should be able to avoid > UAF. > > How the crash happens (http://pastie.org/10799043): > 1. We get an ESC keypress event, -[NSApplication sendEvent:] calls > _NSSendEventToObservers and it *caches* the list of all registered event > monitors. > 2. views::EventMonitorMac::EventMonitorMac()'s monitor gets the ESC keypress, > and it ends the drag and the CocoaMoveLoop. > 3. CocoaMoveLoop::End() quits the run_loop. > 4. CocoaMoveLoop event monitor is unregistered, but is still cached by > _NSSendEventToObservers. > 5. _NSSendEventToObservers sends ESC keypress to the CocoaMoveLoop's monitor, > even though it's supposed to be unregistered at this point. At this point the > CocoaMoveLoop::Run()'s stack frame is unwound and the object is destroyed. > > You might not be getting this crash because the ESC-cancelling of drag operation > is somehow not working for you, but the event propagation to the event monitors > should be similar. Ah, that is likely. So it does sound like we need WeakCocoaWindowMoveLoop - that's fine. The bigger problem is that, for me, ESC is definitely not working. This is on a machine running El Capital 10.11.4 -- does that match your configuration? I've spent a good chunk of time tracing this. But the NSKeyDown event simply isn't delivered to [NSApp sendEvent] during a window drag -- the EscapeTracker in tab_drag_controller.cc never sees it. I traced this further by adding code like: CGEventMask event_mask = CGEventMaskBit(kCGEventKeyDown) | CGEventMaskBit(kCGEventLeftMouseUp); mouse_mach_port_.reset(CGEventTapCreate( kCGAnnotatedSessionEventTap, kCGHeadInsertEventTap, kCGEventTapOptionListenOnly, // passive listener. event_mask, &CallBack, this)); DLOG(INFO) << "made mach port"; if (!mouse_mach_port_) return Widget::MOVE_LOOP_CANCELED; mouse_run_loop_source_.reset( CFMachPortCreateRunLoopSource(NULL, mouse_mach_port_, 0)); if (!mouse_run_loop_source_) return Widget::MOVE_LOOP_CANCELED; DLOG(INFO) << "EventTAp!"; CFRunLoopAddSource( CFRunLoopGetMain(), mouse_run_loop_source_, kCFRunLoopCommonModes); CGEventTapEnable(mouse_mach_port_, true); This allows the key down event to be seen, but only when I run the application as root, or with accessibility modes on. This lets me see the event with the stack: * frame #0: 0x0000000111c4ac76 libbase.dylib`base::debug::BreakDebugger() + 38 at debugger_posix.cc:249 frame #1: 0x00000001293f377a libviews.dylib`CallBack(proxy=0x000000012a026d10, type=kCGEventKeyDown, event=0x00000001002cf310, refcon=0x000000013af72c40) + 458 at cocoa_window_move_loop.mm:30 frame #2: 0x00007fff8d7b4e73 CoreGraphics`processEventTapData + 641 frame #3: 0x00007fff8d696b2f CoreGraphics`_XPostEventTapData + 269 frame #4: 0x00007fff8d7b4b9c CoreGraphics`eventTapMessageHandler + 133 frame #5: 0x00007fff8e7a712c CoreFoundation`__CFMachPortPerform + 252 frame #6: 0x00007fff8e7a7019 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__ + 41 frame #7: 0x00007fff8e7a6f89 CoreFoundation`__CFRunLoopDoSource1 + 473 frame #8: 0x00007fff8e79e9bb CoreFoundation`__CFRunLoopRun + 2171 frame #9: 0x00007fff8e79ded8 CoreFoundation`CFRunLoopRunSpecific + 296 frame #10: 0x00007fff8cd1d935 HIToolbox`RunCurrentEventLoopInMode + 235 frame #11: 0x00007fff8cd1d76f HIToolbox`ReceiveNextEventCommon + 432 frame #12: 0x00007fff8cd1d5af HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71 frame #13: 0x00007fff9bec6efa AppKit`_DPSNextEvent + 1067 frame #14: 0x00007fff9bec632a AppKit`-[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454 frame #15: 0x0000000111bf86d9 libbase.dylib`base::MessagePumpNSApplication::DoRun(this=0x000000013af0ad90, delegate=0x000000013af0ab20) + 505 at message_pump_mac.mm:671 frame #16: 0x0000000111bf6ecd libbase.dylib`base::MessagePumpCFRunLoopBase::Run(this=0x000000013af0ad90, delegate=0x000000013af0ab20) + 125 at message_pump_mac.mm:238 frame #17: 0x0000000111d22b7a libbase.dylib`base::MessageLoop::RunHandler(this=0x000000013af0ab20) + 298 at message_loop.cc:443 frame #18: 0x0000000111dce525 libbase.dylib`base::RunLoop::Run(this=0x00007fff5fbfbd58) + 85 at run_loop.cc:35 frame #19: 0x00000001293f4791 libviews.dylib`views::CocoaWindowMoveLoop::Run(this=0x000000013af72c40) + 1649 at cocoa_window_move_loop.mm:213 but then the event is swallowed while still inside CoreFoundation -- _nextEventMatchingEventMask:untilDate:inMode:dequeue only returns when another event comes in, so the keydown is getting lost. I'm suspecting this is something that has changed in 10.11. Sadly, I don't really have time right now to tear apart the assembly code to diagnose it further. Do you want to have a go at this, if you can get into a state where you can reproduce the esc-not-working. If we can't get this working with Esc, that might be a deal-breaker :(. I don't like it, but we might need to fallback to the crappy method that TabStripDragController uses in the Cocoa browser, which doesn't let you move tabs between spaces. Or.... we might be willing to sacrifice ESC. It's actually broken on Cocoa right now -- ESC will cancel the drag, but the dragged-out tab doesn't move back into the window it came from the way it does on other platforms. If we do that we shouldn't bother with adding code to handle it at all (e.g. not bother adding NSKeyDownMask to the event Mask. Interestingly, it's actually a bug on the Cooca browser that it accepts key events while a tab is being dragged. E.g. selecting an input field on some web contents, then dragging that tab, then typing, then releasing will make the typed text appear in the input field. But of course dragging the whole window suppresses the keydowns "somewhere". Can we figure out a way to switch that "off" perhaps?
https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:17: @interface WeakCocoaWindowMoveLoop : NSObject { Turns out I had TextExpander 5 installed, and it made the Esc-drag-cancelling work, so that's one mystery solved (it's not related to OS X version, I've tested both on Yosemite and El Capitan). Safari also accepts keyboard events into input fields while the window is being dragged. I've started reverse-engineering the -[NSApplication run], it consists of this: rbx = [NSAutoreleasePool new]; [self setWindowsNeedUpdate:0x1]; [rbx drain]; while (self->_running != 0x0) { id pool = [NSAutoreleasePool new]; [self nextEventMatchingMask:0xffffffffffffffff untilDate:[NSDate distantFuture] inMode:_kCFRunLoopDefaultMode dequeue:YES]; if (objc_collectingEnabled() != 0x0) { objc_clear_stack(0x0); } BOOL suddenTerminationEnabled = [self _disableSuddenTermination]; id event = [self->_currentEvent retain]; [self sendEvent:event]; [event release]; [NSUndoManager _endTopLevelGroupings]; if (suddenTerminationEnabled != 0x0) { [self _enableSuddenTermination]; } [pool drain]; } So it seems that the -nextEventMatchingMask:untilDate:inMode:dequeue: is the actual culprit, will continue the hunt tomorrow. And then I'll be on a 2-week vacation.
https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/200001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:17: @interface WeakCocoaWindowMoveLoop : NSObject { Ah, I went on a false start as both the Esc keypress and the mouse dragging events are both getting sent using the MessagePumpNSApplication::DoRun(). But the cause for the error is still that the -nextEventMatchingMask:untilDate:inMode:dequeue: is not returning the Esc keypress for some reason. So I'm going to investigate that.
Tried tracing malloc/free for the CGEvent using malloc_history, and it was a
temporary object:
stack[0]: addr = 0x14921d7a0, type=malloc, frames:
[0] 0x00007fff941a95c5 libsystem_malloc.dylib`malloc_zone_malloc + 107
[1] 0x00007fff8cf2e4bd CoreFoundation`_CFRuntimeCreateInstance + 301
[2] 0x00007fff8a5d96f5 CoreGraphics`CGTypeCreateInstance + 46
[3] 0x00007fff8a636e9d CoreGraphics`CGEventCreateFromDataAndSource + 198
[4] 0x00007fff8a85590c CoreGraphics`processEventTapData + 528
[5] 0x00007fff8a7c8bb8 CoreGraphics`_XPostEventTapData + 288
[6] 0x00007fff8a8556a6 CoreGraphics`eventTapMessageHandler + 133
[7] 0x00007fff8cfbd12c CoreFoundation`__CFMachPortPerform + 252
[8] 0x00007fff8cfbd019
CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__ + 41
[9] 0x00007fff8cfbcf89 CoreFoundation`__CFRunLoopDoSource1 + 473
[10] 0x00007fff8cfb49bb CoreFoundation`__CFRunLoopRun + 2171
[11] 0x00007fff8cfb3ed8 CoreFoundation`CFRunLoopRunSpecific + 296
[12] 0x00007fff921be935 HIToolbox`RunCurrentEventLoopInMode + 235
[13] 0x00007fff921be76f HIToolbox`ReceiveNextEventCommon + 432
[14] 0x00007fff921be5af
HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71
[15] 0x00007fff8961aefa AppKit`_DPSNextEvent + 1067
[16] 0x00007fff8961a32a AppKit`-[NSApplication
_nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454
[17] 0x0000000111dca301
libbase.dylib`base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*)
+ 465 at message_pump_mac.mm:671:24
[18] 0x0000000111dc8d2d
libbase.dylib`base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*)
+ 125 at message_pump_mac.mm:241:15
[19] 0x0000000111ee34e0 libbase.dylib`base::MessageLoop::RunHandler() + 288
at message_loop.cc:443:3
[20] 0x0000000111f7d78e libbase.dylib`base::RunLoop::Run() + 78 at
run_loop.cc:35:3
[21] 0x0000000127e87f83 libviews.dylib`views::CocoaWindowMoveLoop::Run() +
1507 at cocoa_window_move_loop.mm:194:3
It was created and deleted in processEventTapData.
So far it seems that _DPSNextEvent is not returning the Esc keypress for some
reason.
Didn't have enough time for a more through investigation today, and I'll be on a
2-week vacation, will resume the work after that point.
Did a bit more research on the issue. ### Reverse-engineering update: Turns out there are useful NSUserDefaults / command line arguments: * "-NSTraceEvents YES" — prints information about all events the application receives * "-NSDebugViewDrawing 1" — opaque / draggable window regions logging Draggable window regions are posted to the WindowServer using the CGSAddDragRegion function, (called by -[NSNextStepFrame _resetDragMargins] and some other _resetFrameMargins functions). WindowServer is implemented inside CoreGraphics.framework. Draggable regions seem to be accessed using the _testDragRegion function which is called by _tryTiledSpaceDragForWindow and _prepareToDragWindow. Actual dragging could happen using the _dragWindow (not sure). Haven't discovered yet how the actual event handling is implemented inside the WindowServer. ### Esc-key is not working when window is being dragged by WindowServer: Drag is handled by the WindowServer, and it somehow passes mouse events to the application, without doing the same for the keyboard events. Installing a CGEventTap installs it globally on the WindowServer and requires an Accessibility permission. I think requiring it is overkill for this feature, and a potential security risk. Maybe there is a way to make WindowServer post keyboard events during the dragging, need to do more research to determine that conclusively. Did some testing with Safari: I can't drag a dragged tab it into a new space, and pressing Esc does nothing. I get the same behaviour in Xcode. Also tried out Terminal: I can initiate the Space switch when dragging out a tab from Terminal, after which the drag operation stops. Pressing Esc when dragging a Terminal tab also stops the dragging without re-attaching it back. So the current do-nothing-on-esc-press behaviour seems to be consistent with some native apps.
On 2016/05/17 14:43:33, themblsha wrote: > Did a bit more research on the issue. > > ### Reverse-engineering update: > > Turns out there are useful NSUserDefaults / command line arguments: > * "-NSTraceEvents YES" — prints information about all events the application > receives > * "-NSDebugViewDrawing 1" — opaque / draggable window regions logging > > Draggable window regions are posted to the WindowServer using the > CGSAddDragRegion function, (called by -[NSNextStepFrame _resetDragMargins] and > some other _resetFrameMargins functions). > > WindowServer is implemented inside CoreGraphics.framework. Draggable regions > seem to be accessed using the _testDragRegion function which is called by > _tryTiledSpaceDragForWindow and _prepareToDragWindow. Actual dragging could > happen using the _dragWindow (not sure). > > Haven't discovered yet how the actual event handling is implemented inside the > WindowServer. > > ### Esc-key is not working when window is being dragged by WindowServer: > > Drag is handled by the WindowServer, and it somehow passes mouse events to the > application, without doing the same for the keyboard events. > > Installing a CGEventTap installs it globally on the WindowServer and requires an > Accessibility permission. I think requiring it is overkill for this feature, and > a potential security risk. > > Maybe there is a way to make WindowServer post keyboard events during the > dragging, need to do more research to determine that conclusively. > > Did some testing with Safari: I can't drag a dragged tab it into a new space, > and pressing Esc does nothing. I get the same behaviour in Xcode. Also tried out > Terminal: I can initiate the Space switch when dragging out a tab from Terminal, > after which the drag operation stops. Pressing Esc when dragging a Terminal tab > also stops the dragging without re-attaching it back. So the current > do-nothing-on-esc-press behaviour seems to be consistent with some native apps. I'm ok with trying this initially with ESC-does-nothing. We just need to be clear in the code that it doesn't work (and that's intentional), then not try to add handling for the case. We could add a use-counter in chrome to see how often reporting users actually press ESC while tab dragging. (and I do also have some background concerns that the generate-fake-click approach could have subtle glitches in "real" situations, but that might turn out to be nothing. Need to try it and see I guess. But we may want to structure things so that it's easy to substitute a different window-dragging backend if needed.)
I reimplemented CocoaWindowMoveLoop without relying on the WindowServer. Should be more robust, but no more dragging to different spaces (behaviour will be comparable to Safari and old Chrome). Cleaned up some code that's no longer necessary. I think the newly added tests / CGEvent-based DragAndDrop could be potentially useful in the future if the need, and I'll be using them in the Yandex.Browser for sure (and I'd probably restore the WindowServer-based move loop, as I'd want to implement easy window dragging by child views::Widgets). So what do you think? Esc drag canceling now works fine, and there are no more simulated clicks that could potentially miss the target :-)
I haven't reviewed ui_controls_mac.mm thoroughly yet. And, conceptually, I'm OK with keeping the CGEvent stuff for testing drag and drop, but I think interactive_test_utils_mac.mm has a ton of complexity that's not required. Certainly not now that we're using NSWindow methods to move the window anyway. I experimented locally, and uploaded a diff here so you get the gist: https://codereview.chromium.org/2005773002 https://codereview.chromium.org/1747803003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:867: // TODO(tapted,mblsha): Disabled as the IsMaximized behavior is not consistent nit: add "Mac, parens" in ".. as the Mac IsMaximized() behavior ..". https://codereview.chromium.org/1747803003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:938: // TODO(tapted,mblsha): Disabled as the IsMaximized behavior is not consistent ".. as the Mac IsMaximized() behavior ..". https://codereview.chromium.org/1747803003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1781: // Add another tab. This should trigger exiting the nested loop. Add at the I realise this is just moved, but there's a word missing. Looking at the CL that added it, the missing word is "beginning": it should say "Add at the beginning to exercise.." https://codereview.chromium.org/1747803003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1782: // to exercise past crash when model/tabstrip got out of sync (474082). nit: update bug reference: crbug.com/474082 https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... File chrome/test/base/interactive_test_utils.h (right): https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:28: class DragAndDropOperation { add a comment, e.g. // Represents an operation in a sequence of simulated drag and drop events executed asynchronously by DragAndDropSequence. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:32: MoveWithoutAck, Can this (and the creator function) be removed? I don't think there's a compelling use case for this and it seems to add unnecessary complexity. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:37: DebugDelay I think we can remove DebugDelay. If we really want to keep it, it should be documented heavily below. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:49: static DragAndDropOperation DebugDelay(); E.g. // Creates a one-second delay. This is for debugging complex drag and drop operations only. Do not use it in a test. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:63: base::TimeDelta delay_; (then I think all the delays are one second, and we don't need to keep this extra data member around) https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:70: // that Drag'n'Drop is finished before trying to return from the function. nit: "Drag'n'Drop" -> "sequence" https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:71: void DragAndDrop(const std::list<DragAndDropOperation>& operations); I don't think overloading is appropriate here - perhaps call this DragAndDropSequence. And is there any advantage to using std::list? Nothing is inserting in weird places, so I think vector is more appropriate. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... File chrome/test/base/interactive_test_utils_mac.mm (right): https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:23: bool WaitForEvent(bool (^block)(const base::Closure& quit_closure)) { I replaced everything except `ScopedCGEventsEnabler` below down to line 272 with the following. No threads, no blocks. It's much simpler and all we need for now. // comment class OperationRunner { public: static void Run(const std::list<DragAndDropOperation>& operations) { OperationRunner runner(operations); base::RunLoop run_loop; runner.quit_closure_ = run_loop.QuitClosure(); base::MessageLoop::current()->PostTask( FROM_HERE, base::Bind(&OperationRunner::Next, base::Unretained(&runner))); run_loop.Run(); } private: explicit OperationRunner(const std::list<DragAndDropOperation>& operations) : operations_(operations.begin(), operations.end()) {} void Next() { if (progress_ == operations_.size()) { base::MessageLoop::current()->PostTask(FROM_HERE, quit_closure_); return; } const DragAndDropOperation& op = operations_[progress_++]; auto next = base::Bind(&OperationRunner::Next, base::Unretained(this)); switch (op.type()) { case DragAndDropOperation::Type::Move: ui_controls::SendMouseMoveNotifyWhenDone( op.point().x(), op.point().y(), next); break; case DragAndDropOperation::Type::MouseDown: ui_controls::SendMouseEventsNotifyWhenDone( ui_controls::LEFT, ui_controls::DOWN, next); break; case DragAndDropOperation::Type::MouseUp: ui_controls::SendMouseEventsNotifyWhenDone( ui_controls::LEFT, ui_controls::UP, next); break; case DragAndDropOperation::Type::SetMousePositionOverride: ui_controls::SetMousePositionOverride(true, op.point()); base::MessageLoop::current()->PostTask(FROM_HERE, next); break; case DragAndDropOperation::Type::UnsetMousePositionOverride: ui_controls::SetMousePositionOverride(false, gfx::Point()); base::MessageLoop::current()->PostTask(FROM_HERE, next); break; case DragAndDropOperation::Type::DebugDelay: base::MessageLoop::current()->PostDelayedTask( FROM_HERE, next, base::TimeDelta::FromSeconds(1)); break; } } ~OperationRunner() { DCHECK_EQ(operations_.size(),progress_); } base::Closure quit_closure_; size_t progress_ = 0; std::vector<DragAndDropOperation> operations_; DISALLOW_COPY_AND_ASSIGN(OperationRunner); }; https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:31: const base::TimeDelta& delay = base::TimeDelta()) { |delay| argument isn't needed if MoveWithoutAck is gone https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:58: std::vector<ui_test_utils::DragAndDropOperation> DragAndDropMoveOperations( It looks like this is only ever needed to calculate a count of the number of move operations. So let's go with something like int CountMoves(const std::list<DragAndDropOperation>& operations) { int sum = 0; for (const auto& op : operations) sum += op.type() == DragAndDropOperation::Type::Move; return sum; } But, even better I think is to do something like the following (see later comments) int RemainingMoves(const std::vector<DragAndDropOperation>& operations, int start) { int sum = 0; for (int i = start; start < operations.size(); ++i) sum += operations[i].type() == DragAndDropOperation::Type::Move; return sum; } https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:69: class ScopedCGEventsEnabler { comment this, e.g. // While in scope, causes event generators in ui_controls to generate CGEvents rather than NSEvents. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:81: bool enable_cgevents_; Is this member needed? (i.e. do we need the complexity of having multiple of these on the stack?). Can we just DCHECK(!ui_controls::SendMouseEventsAsCGEvents()); in the constructor? https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:82: }; nit: DISALLOW_COPY_AND_ASSIGN(..) https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:84: class BlockRunner : public base::DelegateSimpleThread::Delegate { needs a comment https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:90: ~BlockRunner() override {} remove this? I don't think it's needed https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:93: std::unique_ptr<base::MessageLoop> loop( Can this just be on the stack? base::MessageLoop message_loop(base::MessageLoop::TYPE_DEFAULT); https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:105: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:123: bool WindowIsMoving(NSWindow* window) { Perhaps WindowHasActiveDragger https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:127: return bridge->IsRunMoveLoopActive(); It's not good to couple together this file with BridgedNativeWidget. `WaitForSystemWindowMoveToStop` needs to move to the browsertest file. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:131: bool WindowIsMovingBySystem() { `BySystem` is not accurate any longer. Perhaps WindowUnderCursorIsDragging https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:133: views::EventMonitor::GetLastMouseLocation(), Use gfx::ScreenPointFromNSPoint([NSEvent mouseLocation]); (remove event_monitor.h) https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:184: namespace ui_test_utils { move this to the top of the file before the anonymous namespace, and remove a bunch of unnecessary ui_test_utils:: prefixes https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:227: while (!mutable_operations.empty()) { This can just iterate normally - I don't see any need for the separate list after changing `DragAndDropMoveOperations` to a simple counter. |operations| should be a vector instead of a list. https://codereview.chromium.org/1747803003/diff/280001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/280001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:208: class EventMonitor { This should be in the anonymous namespace https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:671: [[NSRunLoop currentRunLoop] is this still needed? https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:17: // dangling this pointer. Use a proxy Obj-C class to store the weakptr. 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> https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:80: // NSKeyDownMask doesn't work inside addLocalMonitorForEventsMatchingMask: This comment isn't right. Instead there should be a comment about where the escape keypress *is* handled (since it works now!) https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:96: display::Screen::GetScreen()->GetCursorScreenPoint(); This should come off |event|, no? https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:102: const NSRect ns_frame = gfx::ScreenRectToNSRect(frame); 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) https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:103: [window setFrame:ns_frame display:YES animate:NO]; Does display:NO work? It might do less work. We probably only need display:YES when changing the size or doing other weird stuff. https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:109: if ([event type] == NSLeftMouseUp) { DCHECK_EQ(NSLeftMouseUp, [event type]) no need for if
And do a pass yourself for other ways to simplify. E.g. SetMousePositionOverride and related methods don't seem to be used - they should be removed. Generally we're quite strict in Chrome about adding non-essential code to the repo. If an optimizing linker could remove a method without breaking stuff, then people will generally just delete it. Otherwise, someone has to maintain it, and it puts an unnecessary burden on everyone. You typically even have to fight to justify things like `DebugDelay` - things that make debugging easier, but aren't explicitly used in tests also tend to be quite rare.
Phew, I think this is it. Cleaned up the comments and some other stuff as well. BridgedNativeWidget::OnPositionChanged() must stay, or the tests will deadlock. CGEvents are no longer required strictly speaking, but the code will be nice to have if the need to move the window using the WindowServer returns. https://codereview.chromium.org/1747803003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:867: // TODO(tapted,mblsha): Disabled as the IsMaximized behavior is not consistent On 2016/05/23 07:29:27, tapted wrote: > nit: add "Mac, parens" in ".. as the Mac IsMaximized() behavior ..". Done. https://codereview.chromium.org/1747803003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:938: // TODO(tapted,mblsha): Disabled as the IsMaximized behavior is not consistent On 2016/05/23 07:29:27, tapted wrote: > ".. as the Mac IsMaximized() behavior ..". Done. https://codereview.chromium.org/1747803003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1781: // Add another tab. This should trigger exiting the nested loop. Add at the On 2016/05/23 07:29:27, tapted wrote: > I realise this is just moved, but there's a word missing. Looking at the CL that > added it, the missing word is "beginning": it should say > > "Add at the beginning to exercise.." Done. https://codereview.chromium.org/1747803003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1782: // to exercise past crash when model/tabstrip got out of sync (474082). On 2016/05/23 07:29:27, tapted wrote: > nit: update bug reference: crbug.com/474082 Done. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... File chrome/test/base/interactive_test_utils.h (right): https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:28: class DragAndDropOperation { On 2016/05/23 07:29:27, tapted wrote: > add a comment, e.g. > > // Represents an operation in a sequence of simulated drag and drop events > executed asynchronously by DragAndDropSequence. Done. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:32: MoveWithoutAck, On 2016/05/23 07:29:27, tapted wrote: > Can this (and the creator function) be removed? I don't think there's a > compelling use case for this and it seems to add unnecessary complexity. At the moment there's no need for it, yeah. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:37: DebugDelay On 2016/05/23 07:29:27, tapted wrote: > I think we can remove DebugDelay. If we really want to keep it, it should be > documented heavily below. Yeah, I used it to look at what was actually happening on the screen while I was debugging the tests. There's no need for it when tests are working fine. If some tough-to-debug problem happens again this (or something similar) could be re-added, I guess. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:49: static DragAndDropOperation DebugDelay(); On 2016/05/23 07:29:27, tapted wrote: > E.g. > > // Creates a one-second delay. This is for debugging complex drag and drop > operations only. Do not use it in a test. Should I re-add this even though there's no immediate need for it? :-) https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:70: // that Drag'n'Drop is finished before trying to return from the function. On 2016/05/23 07:29:27, tapted wrote: > nit: "Drag'n'Drop" -> "sequence" Done. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:71: void DragAndDrop(const std::list<DragAndDropOperation>& operations); On 2016/05/23 07:29:27, tapted wrote: > I don't think overloading is appropriate here - perhaps call this > DragAndDropSequence. And is there any advantage to using std::list? Nothing is > inserting in weird places, so I think vector is more appropriate. I'm removing items from the front as they're processed (there's some performance overhead with list but it should be negligible in this case). Isn't that a good enough reason? https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... File chrome/test/base/interactive_test_utils_mac.mm (right): https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:23: bool WaitForEvent(bool (^block)(const base::Closure& quit_closure)) { On 2016/05/23 07:29:28, tapted wrote: > I replaced everything except `ScopedCGEventsEnabler` below down to line 272 with > the following. No threads, no blocks. It's much simpler and all we need for now. Wow, amazing. It even works when simulating a drag on the window title (which is handled by the WindowServer). Thanks!! https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:69: class ScopedCGEventsEnabler { On 2016/05/23 07:29:28, tapted wrote: > comment this, e.g. > > // While in scope, causes event generators in ui_controls to generate CGEvents > rather than NSEvents. Done. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:81: bool enable_cgevents_; On 2016/05/23 07:29:28, tapted wrote: > Is this member needed? (i.e. do we need the complexity of having multiple of > these on the stack?). Can we just > > DCHECK(!ui_controls::SendMouseEventsAsCGEvents()); > > in the constructor? No, I guess I was trying to write a generic version of the class by default, even though there's yet no need for it outside of this helper. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:93: std::unique_ptr<base::MessageLoop> loop( On 2016/05/23 07:29:28, tapted wrote: > Can this just be on the stack? > > base::MessageLoop message_loop(base::MessageLoop::TYPE_DEFAULT); It can, thanks :) https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:184: namespace ui_test_utils { On 2016/05/23 07:29:28, tapted wrote: > move this to the top of the file before the anonymous namespace, and remove a > bunch of unnecessary ui_test_utils:: prefixes Done. Also removed the unneeded functions you've also removed. https://codereview.chromium.org/1747803003/diff/280001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:227: while (!mutable_operations.empty()) { On 2016/05/23 07:29:28, tapted wrote: > This can just iterate normally - I don't see any need for the separate list > after changing `DragAndDropMoveOperations` to a simple counter. |operations| > should be a vector instead of a list. I think the code is simpler with a list, see the revised version. https://codereview.chromium.org/1747803003/diff/280001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/280001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:208: class EventMonitor { On 2016/05/23 07:29:28, tapted wrote: > This should be in the anonymous namespace Done. https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:671: [[NSRunLoop currentRunLoop] On 2016/05/23 07:29:28, tapted wrote: > is this still needed? Nope, removed. https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:17: // dangling this pointer. Use a proxy Obj-C class to store the weakptr. On 2016/05/23 07:29:28, tapted wrote: > 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> 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. https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:80: // NSKeyDownMask doesn't work inside addLocalMonitorForEventsMatchingMask: On 2016/05/23 07:29:28, tapted wrote: > This comment isn't right. Instead there should be a comment about where the > escape keypress *is* handled (since it works now!) TabDragController now instantiates EscapeTracker which handles this, noted this in the comment. https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:96: display::Screen::GetScreen()->GetCursorScreenPoint(); On 2016/05/23 07:29:28, tapted wrote: > This should come off |event|, no? 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. https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:102: const NSRect ns_frame = gfx::ScreenRectToNSRect(frame); On 2016/05/23 07:29:28, tapted wrote: > 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) Used NSOffsetRect in the end. Code definitely looks better now. https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:103: [window setFrame:ns_frame display:YES animate:NO]; On 2016/05/23 07:29:28, tapted wrote: > Does display:NO work? It might do less work. We probably only need display:YES > when changing the size or doing other weird stuff. Works fine, thanks! I've even used display:NO in BridgedNativeWidget::RunMoveLoop(), and it also seems to work fine. https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:109: if ([event type] == NSLeftMouseUp) { On 2016/05/23 07:29:28, tapted wrote: > DCHECK_EQ(NSLeftMouseUp, [event type]) > > no need for if Good idea, thanks.
Removed all the CGEvent-related code from this CL. Should be bare-bones now.
On 2016/05/30 14:19:21, themblsha wrote: > Removed all the CGEvent-related code from this CL. Should be bare-bones now. Sorry - I've been swamped - hopefully I'll get to this soon.
OK - I think things are in pretty good shape. I should be able to turn around on review quicker for the remaining stuff. Please update the CL description too - it still talks about `drags the window using the WindowServer` https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:96: display::Screen::GetScreen()->GetCursorScreenPoint(); On 2016/05/26 15:13:25, themblsha wrote: > On 2016/05/23 07:29:28, tapted wrote: > > This should come off |event|, no? > > 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. 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. https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... File chrome/test/base/interactive_test_utils.h (right): https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:26: void DragAndDrop(const gfx::Point& from, const gfx::Point& to, int steps = 1); nit: move below DragAndDropSequnce (it's usually nice to have types/class declarations ordered first in any scope) https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:54: // main thread. If there are nested eventloops on the main thread's queue, our I don't think we need the sentence "If there are nested eventloops...stuck." https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... File chrome/test/base/interactive_test_utils_mac.mm (right): https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:11: #include "chrome/browser/ui/views/tabs/window_finder.h" remove? https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:47: operations_.pop_front(); move this to the end of the function, or don't use a reference for |op| (the object |op| points to is freed on this line) https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:108: void DragAndDropSequence(const std::list<DragAndDropOperation>& operations) { nit: order should match the header: - move everything up above HideNativeWindow - reorder - first DragAndDropOperation::Move etc, then these functions https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:136: DCHECK_EQ(dispatch_get_current_queue(), dispatch_get_main_queue()) This DCHECK can probably go since we're not using GCD any more https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:200: for (unsigned int i = 0; i < arraysize(buttons); ++i) { nit: unsigned int -> size_t (or just `int`, but that probably warns) https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:213: - (NSPoint)mouseLocationOutsideOfEventStream { Is anything being tested that requires this? (remove?). https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:219: namespace { nit: blank line after https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:220: class NSEventSwizzler { needs a comment https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:223: static NSEventSwizzler* swizzler = nullptr; I'm unsure about this -- the swizzler is "Scoped" for a reason - but this breaks that. We should move this class declaration to the header and allow the test harness to construct it and store it in a data member on |DetachToBrowserTabDragControllerTest| in SetUpOnMainThread() where we bring the browser to the front. ScopedObjCClassSwizzler will need to be forward declared. And we need a better name than NSEventSwizzler. Maybe ScopedMockNSEventClassMethods https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:251: }; nit: DISALLOW_COPY_AND_ASSIGN(..) https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:252: } // namespace nit: blank line before https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:347: clickCount:(event_type == NSMouseMoved ? 0 : 1) outer () parens not required, same below https://codereview.chromium.org/1747803003/diff/320001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.h (right): https://codereview.chromium.org/1747803003/diff/320001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:38: gfx::Point initial_mouse_in_screen_; NSPoint - see later comments https://codereview.chromium.org/1747803003/diff/320001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/320001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:48: initial_mouse_in_screen_(initial_mouse_in_screen), use gfx::ScreenRectToNSRect() here https://codereview.chromium.org/1747803003/diff/320001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:94: display::Screen::GetScreen()->GetCursorScreenPoint(); (per earlier comment, use [NSEvent mouseLocation] directly - assign to an NSPoint
Description was changed from ========== MacViews: Implement Tab Dragging BridgedNativeWidget::RunMoveLoop() drags the window using the WindowServer, as such the window could be dragged between Spaces. Most of the tests in tab_drag_controller_interactive_uitest.cc pass, and I've added several Mac-specific ones to test for the edge cases. BUG=594079 COLLABORATOR=tapted@chromium.org Also reviewed in http://crrev.com/1259913003/ ========== to ========== MacViews: Implement Tab Dragging BridgedNativeWidget::RunMoveLoop() drags the window by manually processing mouse move events. Most of the tests in tab_drag_controller_interactive_uitest.cc pass, and I've added several Mac-specific ones to test for the edge cases. BUG=594079 COLLABORATOR=tapted@chromium.org Also reviewed in http://crrev.com/1259913003/ ==========
https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... File chrome/test/base/interactive_test_utils.h (right): https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:26: void DragAndDrop(const gfx::Point& from, const gfx::Point& to, int steps = 1); On 2016/06/01 11:29:55, tapted wrote: > nit: move below DragAndDropSequnce (it's usually nice to have types/class > declarations ordered first in any scope) Done. https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... chrome/test/base/interactive_test_utils.h:54: // main thread. If there are nested eventloops on the main thread's queue, our On 2016/06/01 11:29:55, tapted wrote: > I don't think we need the sentence "If there are nested eventloops...stuck." Done. https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... File chrome/test/base/interactive_test_utils_mac.mm (right): https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:11: #include "chrome/browser/ui/views/tabs/window_finder.h" On 2016/06/01 11:29:55, tapted wrote: > remove? Done. https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:47: operations_.pop_front(); On 2016/06/01 11:29:55, tapted wrote: > move this to the end of the function, or don't use a reference for |op| (the > object |op| points to is freed on this line) ohshi, thanks! Wonder why it wasn't crashing on me :-/ https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:108: void DragAndDropSequence(const std::list<DragAndDropOperation>& operations) { On 2016/06/01 11:29:55, tapted wrote: > nit: order should match the header: > > - move everything up above HideNativeWindow > - reorder - first DragAndDropOperation::Move etc, then these functions Thanks. It's so easy to miss, isn't there an automated test for that? https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:136: DCHECK_EQ(dispatch_get_current_queue(), dispatch_get_main_queue()) On 2016/06/01 11:29:55, tapted wrote: > This DCHECK can probably go since we're not using GCD any more Done. https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:200: for (unsigned int i = 0; i < arraysize(buttons); ++i) { On 2016/06/01 11:29:55, tapted wrote: > nit: unsigned int -> size_t (or just `int`, but that probably warns) Done. https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:213: - (NSPoint)mouseLocationOutsideOfEventStream { It's widely used in Chromium, and without this swizzle the mouse position returned there would be inconsistent with g_mouse_location, as we're not really moving the mouse. That being said, removing this doesn't make any of the DetachToBrowserTabDragControllerTests fail. https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:219: namespace { On 2016/06/01 11:29:55, tapted wrote: > nit: blank line after Done. https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:220: class NSEventSwizzler { On 2016/06/01 11:29:55, tapted wrote: > needs a comment Done. https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:223: static NSEventSwizzler* swizzler = nullptr; On 2016/06/01 11:29:55, tapted wrote: > I'm unsure about this -- the swizzler is "Scoped" for a reason - but this breaks > that. > > We should move this class declaration to the header and allow the test harness > to construct it and store it in a data member on > |DetachToBrowserTabDragControllerTest| in SetUpOnMainThread() where we bring the > browser to the front. ScopedObjCClassSwizzler will need to be forward declared. > > And we need a better name than NSEventSwizzler. Maybe > ScopedMockNSEventClassMethods In tests the entire browser state gets reconstructed anew for each test, isn't it? And even if it isn't, I think it's a benefit, as after installing the NSEventSwizzler you're not supposed to interact with the tested browser directly without using ui_controls.h functions. I feel that this swizzler could benefit tons of other tests as well, as global mouse position is used pretty widely, both directly and through display::Screen::GetCursorScreenPoint(). EventGeneratorDelegateMac::SetContext() also swizzles several calls, although I'm not sure when it's destroyed. https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:251: }; On 2016/06/01 11:29:55, tapted wrote: > nit: DISALLOW_COPY_AND_ASSIGN(..) Done. https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:252: } // namespace On 2016/06/01 11:29:55, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:347: clickCount:(event_type == NSMouseMoved ? 0 : 1) On 2016/06/01 11:29:55, tapted wrote: > outer () parens not required, same below Done. Although there are parens in SendMouseEventsNotifyWhenDone, with an extra space before the closing one. https://codereview.chromium.org/1747803003/diff/320001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.h (right): https://codereview.chromium.org/1747803003/diff/320001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:38: gfx::Point initial_mouse_in_screen_; On 2016/06/01 11:29:56, tapted wrote: > NSPoint - see later comments Done. https://codereview.chromium.org/1747803003/diff/320001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.mm (right): https://codereview.chromium.org/1747803003/diff/320001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:48: initial_mouse_in_screen_(initial_mouse_in_screen), On 2016/06/01 11:29:56, tapted wrote: > use gfx::ScreenRectToNSRect() here Done. https://codereview.chromium.org/1747803003/diff/320001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.mm:94: display::Screen::GetScreen()->GetCursorScreenPoint(); On 2016/06/01 11:29:56, tapted wrote: > (per earlier comment, use [NSEvent mouseLocation] directly - assign to an > NSPoint Done.
https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... File chrome/test/base/interactive_test_utils_mac.mm (right): https://codereview.chromium.org/1747803003/diff/320001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:108: void DragAndDropSequence(const std::list<DragAndDropOperation>& operations) { On 2016/06/03 17:42:08, themblsha wrote: > On 2016/06/01 11:29:55, tapted wrote: > > nit: order should match the header: > > > > - move everything up above HideNativeWindow > > - reorder - first DragAndDropOperation::Move etc, then these functions > > Thanks. It's so easy to miss, isn't there an automated test for that? Sadly not :/ https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:213: - (NSPoint)mouseLocationOutsideOfEventStream { On 2016/06/03 17:42:08, themblsha wrote: > It's widely used in Chromium, and without this swizzle the mouse position > returned there would be inconsistent with g_mouse_location, as we're not really > moving the mouse. > > That being said, removing this doesn't make any of the > DetachToBrowserTabDragControllerTests fail. `git grep mouseLocationOutsideOfEventStream` shows up a bunch of stuff in c/b/ui/cocoa code, but nothing interesting under src/ui, which is pretty much all we use for toolkit-views on Mac. This adds complexity, so we should remove it until we know it's needed. At this stage it's possible all the uses of mouseLocationOutsideOfEventStream get phased out of Chrome. https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:223: static NSEventSwizzler* swizzler = nullptr; On 2016/06/03 17:42:08, themblsha wrote: > On 2016/06/01 11:29:55, tapted wrote: > > I'm unsure about this -- the swizzler is "Scoped" for a reason - but this > breaks > > that. > > > > We should move this class declaration to the header and allow the test harness > > to construct it and store it in a data member on > > |DetachToBrowserTabDragControllerTest| in SetUpOnMainThread() where we bring > the > > browser to the front. ScopedObjCClassSwizzler will need to be forward > declared. > > > > And we need a better name than NSEventSwizzler. Maybe > > ScopedMockNSEventClassMethods > > In tests the entire browser state gets reconstructed anew for each test, isn't > it? And even if it isn't, I think it's a benefit, as after installing the > NSEventSwizzler you're not supposed to interact with the tested browser directly > without using ui_controls.h functions. Not always -- there is `--single-process-tests` which is currently broken for MacViews -- See http://crbug.com/594033 Maybe EventGeneratorDelegateMac is the culprit. > I feel that this swizzler could benefit tons of other tests as well, as global > mouse position is used pretty widely, both directly and through > display::Screen::GetCursorScreenPoint(). > > EventGeneratorDelegateMac::SetContext() also swizzles several calls, although > I'm not sure when it's destroyed. EventGeneratorDelegateMac is explicitly installed on ViewsTestHelperMac, with `test::InitializeMacEventGeneratorDelegate();` so it's less likely that things like sharding or randomizing the order that tests are run in will change the behaviour. Calling `Install()` from multiple places, and as a side-effect of sending the first test event (for some subset of test event types) is not a robust approach. There's also void ViewsTestHelperMac::SetUp() which installs some correctly scoped swizzler methods for tests, some specifically for interactive_ui_tests, so we need to be careful of conflicts here. In particular, ViewsTestHelperMac::SetUp() swizzles some stuff on NSWindow, but after removing `FakeNSWindowTestingDonor` from this CL we keep some separation if we only swizzle NSEvent stuff. How about calling ScopedMockNSEventClassMethods::Init() in ui_controls::EnableUIControls? This avoids the downsides of having platform-specific initialization in the tests that a fine-grained scoped approach would need. https://codereview.chromium.org/1747803003/diff/340001/chrome/test/base/inter... File chrome/test/base/interactive_test_utils_mac.mm (right): https://codereview.chromium.org/1747803003/diff/340001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:129: DragAndDropOperation DragAndDropOperation::Move(const gfx::Point& p) { nit (per previous comment) This should move up as well, to come before DragAndDropSequence https://codereview.chromium.org/1747803003/diff/340001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/340001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:418: pressure:(state == DOWN ? 1.0 : 0.0 )]; yeah, let's fix this too. I think even `git cl format` would complain about that extra space. https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.h (right): https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:33: void OnPositionChanged(); remove - I think it's orphaned now https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:43: base::Closure quit_closure_; nit: #include "base/callback.h" https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:46: base::WeakPtrFactory<CocoaWindowMoveLoop> weak_factory_; nit: #include "base/memory/weak_ptr.h" https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/views_n... File ui/views/cocoa/views_nswindow_delegate.mm (right): https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/views_n... ui/views/cocoa/views_nswindow_delegate.mm:84: // is also windowWillMove: sent at the start, also once. When window is being nit: window -> the window
https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:213: - (NSPoint)mouseLocationOutsideOfEventStream { On 2016/06/06 07:12:36, tapted wrote: > On 2016/06/03 17:42:08, themblsha wrote: > > It's widely used in Chromium, and without this swizzle the mouse position > > returned there would be inconsistent with g_mouse_location, as we're not > really > > moving the mouse. > > > > That being said, removing this doesn't make any of the > > DetachToBrowserTabDragControllerTests fail. > > `git grep mouseLocationOutsideOfEventStream` shows up a bunch of stuff in > c/b/ui/cocoa code, but nothing interesting under src/ui, which is pretty much > all we use for toolkit-views on Mac. > > This adds complexity, so we should remove it until we know it's needed. At this > stage it's possible all the uses of mouseLocationOutsideOfEventStream get phased > out of Chrome. Done. https://codereview.chromium.org/1747803003/diff/320001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:223: static NSEventSwizzler* swizzler = nullptr; On 2016/06/06 07:12:36, tapted wrote: > On 2016/06/03 17:42:08, themblsha wrote: > > On 2016/06/01 11:29:55, tapted wrote: > > > I'm unsure about this -- the swizzler is "Scoped" for a reason - but this > > breaks > > > that. > > > > > > We should move this class declaration to the header and allow the test > harness > > > to construct it and store it in a data member on > > > |DetachToBrowserTabDragControllerTest| in SetUpOnMainThread() where we bring > > the > > > browser to the front. ScopedObjCClassSwizzler will need to be forward > > declared. > > > > > > And we need a better name than NSEventSwizzler. Maybe > > > ScopedMockNSEventClassMethods > > > > In tests the entire browser state gets reconstructed anew for each test, isn't > > it? And even if it isn't, I think it's a benefit, as after installing the > > NSEventSwizzler you're not supposed to interact with the tested browser > directly > > without using ui_controls.h functions. > > Not always -- there is `--single-process-tests` which is currently broken for > MacViews -- See http://crbug.com/594033 Maybe EventGeneratorDelegateMac is the > culprit. > > > I feel that this swizzler could benefit tons of other tests as well, as global > > mouse position is used pretty widely, both directly and through > > display::Screen::GetCursorScreenPoint(). > > > > EventGeneratorDelegateMac::SetContext() also swizzles several calls, although > > I'm not sure when it's destroyed. > > EventGeneratorDelegateMac is explicitly installed on ViewsTestHelperMac, with > `test::InitializeMacEventGeneratorDelegate();` so it's less likely that things > like sharding or randomizing the order that tests are run in will change the > behaviour. > > Calling `Install()` from multiple places, and as a side-effect of sending the > first test event (for some subset of test event types) is not a robust approach. > > There's also void ViewsTestHelperMac::SetUp() which installs some correctly > scoped swizzler methods for tests, some specifically for interactive_ui_tests, > so we need to be careful of conflicts here. In particular, > ViewsTestHelperMac::SetUp() swizzles some stuff on NSWindow, but after removing > `FakeNSWindowTestingDonor` from this CL we keep some separation if we only > swizzle NSEvent stuff. > > How about calling ScopedMockNSEventClassMethods::Init() in > ui_controls::EnableUIControls? This avoids the downsides of having > platform-specific initialization in the tests that a fine-grained scoped > approach would need. If it's only installed in EnableUIControls() and never removed, I don't think it should have a Scoped prefix. Renamed the class and moved the installation there, that single place is definitely a better fit. https://codereview.chromium.org/1747803003/diff/340001/chrome/test/base/inter... File chrome/test/base/interactive_test_utils_mac.mm (right): https://codereview.chromium.org/1747803003/diff/340001/chrome/test/base/inter... chrome/test/base/interactive_test_utils_mac.mm:129: DragAndDropOperation DragAndDropOperation::Move(const gfx::Point& p) { On 2016/06/06 07:12:36, tapted wrote: > nit (per previous comment) This should move up as well, to come before > DragAndDropSequence Whoops, sorry. https://codereview.chromium.org/1747803003/diff/340001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/340001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:418: pressure:(state == DOWN ? 1.0 : 0.0 )]; On 2016/06/06 07:12:36, tapted wrote: > yeah, let's fix this too. I think even `git cl format` would complain about that > extra space. Done. https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/cocoa_w... File ui/views/cocoa/cocoa_window_move_loop.h (right): https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:33: void OnPositionChanged(); On 2016/06/06 07:12:36, tapted wrote: > remove - I think it's orphaned now Done. https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:43: base::Closure quit_closure_; On 2016/06/06 07:12:36, tapted wrote: > nit: #include "base/callback.h" Done. https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/cocoa_w... ui/views/cocoa/cocoa_window_move_loop.h:46: base::WeakPtrFactory<CocoaWindowMoveLoop> weak_factory_; On 2016/06/06 07:12:36, tapted wrote: > nit: #include "base/memory/weak_ptr.h" Done. https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/views_n... File ui/views/cocoa/views_nswindow_delegate.mm (right): https://codereview.chromium.org/1747803003/diff/340001/ui/views/cocoa/views_n... ui/views/cocoa/views_nswindow_delegate.mm:84: // is also windowWillMove: sent at the start, also once. When window is being On 2016/06/06 07:12:36, tapted wrote: > nit: window -> the window Done.
lgtm - just some final stuff since `MockNSEventClassMethods` is now staying in the .mm file. You'll need another OWNER - probably sky, since he can cover src/chrome/test as well as chrome/browser/ui/views (make sure you point out the files you'd like him to review) https://codereview.chromium.org/1747803003/diff/360001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/1747803003/diff/360001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:219: protected: nit: private: (and remove private: below) https://codereview.chromium.org/1747803003/diff/360001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:231: std::unique_ptr<base::mac::ScopedObjCClassSwizzler> mouse_location_swizzler_; Ah, sorry - these should be regular members, not unique_ptr. (we couldn't have done that if this class declaration moved to the header, but while it's wholly contained here, there's no point these being unique_ptr).
mblsha@yandex-team.ru changed reviewers: + sky@chromium.org
+sky
Please review these files:
chrome/browser/ui/views/tabs/tab_drag_controller.cc
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
chrome/browser/ui/views/tabs/window_finder_mac.mm
chrome/test/base/interactive_test_utils.h
chrome/test/base/interactive_test_utils_mac.mm
ui/views/views.gyp
chrome/browser/ui/views/tabs/tab_drag_controller.cc: The offset in RunMoveLoop
doesn't appear to be used on other platforms, but on MacViews we can't get the
offset to pressed mouse location using other way.
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc: Fix
compilation on MacViews, fix deadlock in CancelOnNewTabWhenDragging:
The CancelOnNewTabWhenDragging issue was weird.
1. CancelOnNewTabWhenDraggingStep2 is getting invoked from CocoaWindowMoveLoop's
MessagePumpNSApplication.
2. chrome::AddTabAt() is called, which results in a
BridgedNativeWidget::EndMoveLoop() call.
3. chrome::AddTabAt() returns, but we're still inside CocoaWindowMoveLoop's
MessagePumpNSApplication nested call.
4. observer.Wait() is called while current MessagePumpNSApplication is
cancelled, but it didn't have a chance to process the Quit call, as it's still
inside the nested call.
5. observer.Wait() immediately quits and DCHECKs on the seen_ flag, as it didn't
have a chance to wait for the notification.
So instead I moved the NOTIFICATION_LOAD_STOP to the main test body, so that it
would not create a nested RunLoop while its parent RunLoop was supposed to exit.
chrome/browser/ui/views/tabs/window_finder_mac.mm: Required for
TabDragController to work on MacViews.
chrome/test/base/interactive_test_utils.h
chrome/test/base/interactive_test_utils_mac.mm: Add helper class to perform
complex drag&drop operations without deadlocks.
ui/views/views.gyp: Add files to the project.
https://codereview.chromium.org/1747803003/diff/360001/ui/base/test/ui_contro...
File ui/base/test/ui_controls_mac.mm (right):
https://codereview.chromium.org/1747803003/diff/360001/ui/base/test/ui_contro...
ui/base/test/ui_controls_mac.mm:219: protected:
On 2016/06/07 00:49:14, tapted wrote:
> nit: private: (and remove private: below)
Done.
https://codereview.chromium.org/1747803003/diff/360001/ui/base/test/ui_contro...
ui/base/test/ui_controls_mac.mm:231:
std::unique_ptr<base::mac::ScopedObjCClassSwizzler> mouse_location_swizzler_;
On 2016/06/07 00:49:14, tapted wrote:
> Ah, sorry - these should be regular members, not unique_ptr. (we couldn't
have
> done that if this class declaration moved to the header, but while it's wholly
> contained here, there's no point these being unique_ptr).
Done.
https://codereview.chromium.org/1747803003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1747803003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:396: RunMoveLoop(GetWindowOffset(start_point_in_screen_)); I think the right thing here is to move the window by point_in_screen-start_point_in_screen and then start the move loop using point_in_screen.
On 2016/06/07 15:57:55, sky wrote: > https://codereview.chromium.org/1747803003/diff/380001/chrome/browser/ui/view... > File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): > > https://codereview.chromium.org/1747803003/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/tabs/tab_drag_controller.cc:396: > RunMoveLoop(GetWindowOffset(start_point_in_screen_)); > I think the right thing here is to move the window by > point_in_screen-start_point_in_screen and then start the move loop using > point_in_screen. This solution won't be enough. RunMoveLoop could be started in three different code paths: 1. TabDragController::Drag() — when all tabs are selected, moves the window. I've patched this call. 2. TabDragController::DetachIntoNewBrowserAndRunMoveLoop() — when all tabs are selected, moves the window. Not sure how to reproduce this code path, and I haven't patched this call. 3. TabDragController::DetachIntoNewBrowserAndRunMoveLoop() — detaches the tab and moves it around, I haven't patched this call. There's a bug in Views implementation that's reproducible on Windows (and maybe Linux as well): https://yadi.sk/i/Ia655yNGsMXhZ On current Cocoa version the window correctly snaps to the point where the mouse was clicked. Your proposed solution would fix this bug on all platforms. But if I remove window moving from BridgedNativeWidget::RunMoveLoop() it will break case number 3: https://yadi.sk/i/t7cJm5_TsMXzt I've tested my change on Windows, and the result of case 1 is as if nothing was happening, so it's not changing any observable behaviour :-) Do you have an alternate approach I could try? Personally I'm in favour of the current approach, as we're using RunMoveLoop() in Yandex.Browser for moving the main Browser window using child Widgets, and I don't want to insert duplicate window moving code there as well.
On Wed, Jun 8, 2016 at 10:17 AM, <mblsha@yandex-team.ru> wrote: > On 2016/06/07 15:57:55, sky wrote: >> > https://codereview.chromium.org/1747803003/diff/380001/chrome/browser/ui/view... >> File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): >> >> > https://codereview.chromium.org/1747803003/diff/380001/chrome/browser/ui/view... >> chrome/browser/ui/views/tabs/tab_drag_controller.cc:396: >> RunMoveLoop(GetWindowOffset(start_point_in_screen_)); >> I think the right thing here is to move the window by >> point_in_screen-start_point_in_screen and then start the move loop using >> point_in_screen. > > This solution won't be enough. RunMoveLoop could be started in three > different > code paths: > 1. TabDragController::Drag() — when all tabs are selected, moves the window. > I've patched this call. > 2. TabDragController::DetachIntoNewBrowserAndRunMoveLoop() — when all tabs > are > selected, moves the window. Not sure how to reproduce this code path, and I > haven't patched this call. > 3. TabDragController::DetachIntoNewBrowserAndRunMoveLoop() — detaches the > tab > and moves it around, I haven't patched this call. > > There's a bug in Views implementation that's reproducible on Windows (and > maybe > Linux as well): https://yadi.sk/i/Ia655yNGsMXhZ This is the bug I was thinking of when I suggested we need to move the window by the offset, then start the move loop. If we do this, the window should snap to the drag location and won't be offset as it is in the video. > On current Cocoa version the window correctly snaps to the point where the > mouse > was clicked. Your proposed solution would fix this bug on all platforms. > > But if I remove window moving from BridgedNativeWidget::RunMoveLoop() it > will > break case number 3: https://yadi.sk/i/t7cJm5_TsMXzt I don't think RunMoveLoop should move the window, TabDragController needs to move the window first, then call RunMoveLoop. > I've tested my change on Windows, and the result of case 1 is as if nothing > was > happening, so it's not changing any observable behaviour :-) Do you have an > alternate approach I could try? Personally I'm in favour of the current > approach, as we're using RunMoveLoop() in Yandex.Browser for moving the main > Browser window using child Widgets, and I don't want to insert duplicate > window > moving code there as well. Not sure I follow. What happens if you move the window before starting the move loop? > > https://codereview.chromium.org/1747803003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry, didn't notice your reply last week :-( I think I found the solution that should work on all platforms now, please take a look.
https://codereview.chromium.org/1747803003/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1747803003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:394: views::Widget* widget = GetAttachedBrowserWidget(); As this part is really a separate bug, could you create a patch just for this and add coverage in existing interactive ui tests? Also, I think this should be in the else case of the if that starts on 371.
If you're ok with this CL, I can start fixing crbug.com/518740 after this one lands. https://codereview.chromium.org/1747803003/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1747803003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:394: views::Widget* widget = GetAttachedBrowserWidget(); On 2016/06/16 15:56:12, sky wrote: > As this part is really a separate bug, could you create a patch just for this > and add coverage in existing interactive ui tests? > > Also, I think this should be in the else case of the if that starts on 371. There's already a bug for this: https://bugs.chromium.org/p/chromium/issues/detail?id=518740#c2 Ok, reverted this code. DetachToBrowserTabDragControllerTest.DragAll / DetachToBrowserTabDragControllerTest.MacDragsWindowUsingCocoaMoveLoop both tested for this bug, guess I'll revert their changes as well.
LGTM https://codereview.chromium.org/1747803003/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1328: // FIXME(mblsha): Fails on MacViews because of crbug.com/518740 FIXME->TODO
https://codereview.chromium.org/1747803003/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1747803003/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1328: // FIXME(mblsha): Fails on MacViews because of crbug.com/518740 On 2016/06/17 15:15:17, sky wrote: > FIXME->TODO Done.
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1747803003/#ps440001 (title: "FIXME --> TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1747803003/440001
The CQ bit was unchecked by commit-bot@chromium.org
CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1747803003/#ps460001 (title: "git cl upload --target_branch=master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1747803003/460001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1747803003/#ps480001 (title: "Rebased on top of master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1747803003/480001
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1747803003/#ps500001 (title: "Fix include.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1747803003/500001
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implement Tab Dragging BridgedNativeWidget::RunMoveLoop() drags the window by manually processing mouse move events. Most of the tests in tab_drag_controller_interactive_uitest.cc pass, and I've added several Mac-specific ones to test for the edge cases. BUG=594079 COLLABORATOR=tapted@chromium.org Also reviewed in http://crrev.com/1259913003/ ========== to ========== MacViews: Implement Tab Dragging BridgedNativeWidget::RunMoveLoop() drags the window by manually processing mouse move events. Most of the tests in tab_drag_controller_interactive_uitest.cc pass, and I've added several Mac-specific ones to test for the edge cases. BUG=594079 COLLABORATOR=tapted@chromium.org Also reviewed in http://crrev.com/1259913003/ ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implement Tab Dragging BridgedNativeWidget::RunMoveLoop() drags the window by manually processing mouse move events. Most of the tests in tab_drag_controller_interactive_uitest.cc pass, and I've added several Mac-specific ones to test for the edge cases. BUG=594079 COLLABORATOR=tapted@chromium.org Also reviewed in http://crrev.com/1259913003/ ========== to ========== MacViews: Implement Tab Dragging BridgedNativeWidget::RunMoveLoop() drags the window by manually processing mouse move events. Most of the tests in tab_drag_controller_interactive_uitest.cc pass, and I've added several Mac-specific ones to test for the edge cases. BUG=594079 COLLABORATOR=tapted@chromium.org Also reviewed in http://crrev.com/1259913003/ Committed: https://crrev.com/6984eac05884a72bc81697589817df83bcde0551 Cr-Commit-Position: refs/heads/master@{#400463} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/6984eac05884a72bc81697589817df83bcde0551 Cr-Commit-Position: refs/heads/master@{#400463} |
