Fixes drag and drop crash. The problem was when I made

MenuController::SetExitType do a QuitNow it would prematurely exit out
of drag and drop, causing a crash. The fix is to not QuitNow if drag
and drop is on going. (Have I mentioned I hate nested message loops?).

I also changed DragDropController to QuitNow, this way tests can work
and seems like what we want anyway.

Lastly I made all the bookmarkbarviewtests work again as they provide
coverage of this.

BUG=127348
TEST=covered by tests
R=ben@chromium.org,varunjain@chromium.org

Review URL: https://chromiumcodereview.appspot.com/10388056

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@136365 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 79 insertions, 50 deletions

diff --git a/ash/drag_drop/drag_drop_controller.cc b/ash/drag_drop/drag_drop_controller.cc
index 09e0aad..5a3f7df 100644
--- a/ash/drag_drop/drag_drop_controller.cc
+++ b/ash/drag_drop/drag_drop_controller.cc
@@ -149,7 +149,7 @@ void DragDropController::Drop(aura::Window* target,
 
   Cleanup();
   if (should_block_during_drag_drop_)
-    MessageLoop::current()->Quit();
+    MessageLoop::current()->QuitNow();
 }
 
 void DragDropController::DragCancel() {
@@ -167,7 +167,7 @@ void DragDropController::DragCancel() {
   drag_operation_ = 0;
   StartCanceledAnimation();
   if (should_block_during_drag_drop_)
-    MessageLoop::current()->Quit();
+    MessageLoop::current()->QuitNow();
 }
 
 bool DragDropController::IsDragDropInProgress() {
diff --git a/ash/drag_drop/drag_drop_controller.h b/ash/drag_drop/drag_drop_controller.h
index dfaaf43..d44ba20 100644
--- a/ash/drag_drop/drag_drop_controller.h
+++ b/ash/drag_drop/drag_drop_controller.h
@@ -39,7 +39,7 @@ class ASH_EXPORT DragDropController
       public aura::EventFilter,
       public ui::ImplicitAnimationObserver,
       public aura::WindowObserver {
-public:
+ public:
   DragDropController();
   virtual ~DragDropController();
 
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc b/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc
index 61667ac..be13455b 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc
@@ -42,37 +42,6 @@ using content::OpenURLParams;
 using content::PageNavigator;
 using content::WebContents;
 
-#if defined(OS_LINUX)
-// See http://crbug.com/40040 for details.
-#define MAYBE_DND DISABLED_DND
-#define MAYBE_DNDToDifferentMenu DISABLED_DNDToDifferentMenu
-#define MAYBE_DNDBackToOriginatingMenu DISABLED_DNDBackToOriginatingMenu
-
-// See http://crbug.com/40039 for details.
-#define MAYBE_KeyEvents DISABLED_KeyEvents
-
-// Two bugs here. http://crbug.com/47089 for general Linux Views, and
-// http://crbug.com/47452 for ChromiumOS.
-#define MAYBE_CloseWithModalDialog DISABLED_CloseWithModalDialog
-// See http://crbug.com/47089 for details.
-#define MAYBE_CloseMenuAfterClosingContextMenu \
-        DISABLED_CloseMenuAfterClosingContextMenu
-
-// See bug http://crbug.com/60444 for details.
-#define MAYBE_ScrollButtonScrolls DISABLED_ScrollButtonScrolls
-#else
-
-#define MAYBE_DND DND
-#define MAYBE_DNDToDifferentMenu DNDToDifferentMenu
-#define MAYBE_DNDBackToOriginatingMenu DNDBackToOriginatingMenu
-#define MAYBE_DNDBackToOriginatingMenu DNDBackToOriginatingMenu
-#define MAYBE_KeyEvents KeyEvents
-#define MAYBE_CloseWithModalDialog CloseWithModalDialog
-#define MAYBE_CloseMenuAfterClosingContextMenu CloseMenuAfterClosingContextMenu
-#define MAYBE_ScrollButtonScrolls ScrollButtonScrolls
-
-#endif
-
 namespace {
 
 void MoveMouseAndPress(const gfx::Point& screen_pos,
@@ -652,7 +621,7 @@ class BookmarkBarViewTest5 : public BookmarkBarViewEventTestBase {
   GURL url_dragging_;
 };
 
-VIEW_TEST(BookmarkBarViewTest5, MAYBE_DND)
+VIEW_TEST(BookmarkBarViewTest5, DND)
 
 // Tests holding mouse down on overflow button, dragging such that menu pops up
 // then selecting an item.
@@ -731,12 +700,30 @@ class BookmarkBarViewTest7 : public BookmarkBarViewEventTestBase {
     gfx::Point loc(other_button->width() / 2, other_button->height() / 2);
     views::View::ConvertPointToScreen(other_button, &loc);
 
+#if defined(USE_AURA)
+    // TODO: fix this. Aura requires an additional mouse event to trigger drag
+    // and drop checking state.
+    ui_controls::SendMouseMoveNotifyWhenDone(loc.x() + 10, loc.y(),
+        base::Bind(&BookmarkBarViewTest7::Step3A, this));
+#else
     // Start a drag.
     ui_controls::SendMouseMoveNotifyWhenDone(loc.x() + 10, loc.y(),
         base::Bind(&BookmarkBarViewTest7::Step4, this));
 
     // See comment above this method as to why we do this.
     ScheduleMouseMoveInBackground(loc.x(), loc.y());
+#endif
+  }
+
+  void Step3A() {
+    // Drag over other button.
+    views::TextButton* other_button =
+        bb_view_->other_bookmarked_button();
+    gfx::Point loc(other_button->width() / 2, other_button->height() / 2);
+    views::View::ConvertPointToScreen(other_button, &loc);
+
+    ui_controls::SendMouseMoveNotifyWhenDone(loc.x(), loc.y(),
+        base::Bind(&BookmarkBarViewTest7::Step4, this));
   }
 
   void Step4() {
@@ -762,7 +749,7 @@ class BookmarkBarViewTest7 : public BookmarkBarViewEventTestBase {
   GURL url_dragging_;
 };
 
-VIEW_TEST(BookmarkBarViewTest7, MAYBE_DNDToDifferentMenu)
+VIEW_TEST(BookmarkBarViewTest7, DNDToDifferentMenu)
 
 // Drags from one menu to next so that original menu closes, then back to
 // original menu.
@@ -805,11 +792,28 @@ class BookmarkBarViewTest8 : public BookmarkBarViewEventTestBase {
     views::View::ConvertPointToScreen(other_button, &loc);
 
     // Start a drag.
+#if defined(USE_AURA)
+    // TODO: fix this. Aura requires an additional mouse event to trigger drag
+    // and drop checking state.
+    ui_controls::SendMouseMoveNotifyWhenDone(loc.x() + 10, loc.y(),
+        base::Bind(&BookmarkBarViewTest8::Step3A, this));
+#else
     ui_controls::SendMouseMoveNotifyWhenDone(loc.x() + 10, loc.y(),
         base::Bind(&BookmarkBarViewTest8::Step4, this));
-
     // See comment above this method as to why we do this.
     ScheduleMouseMoveInBackground(loc.x(), loc.y());
+#endif
+  }
+
+  void Step3A() {
+    // Drag over other button.
+    views::TextButton* other_button =
+        bb_view_->other_bookmarked_button();
+    gfx::Point loc(other_button->width() / 2, other_button->height() / 2);
+    views::View::ConvertPointToScreen(other_button, &loc);
+
+    ui_controls::SendMouseMoveNotifyWhenDone(loc.x() + 10, loc.y(),
+        base::Bind(&BookmarkBarViewTest8::Step4, this));
   }
 
   void Step4() {
@@ -849,7 +853,7 @@ class BookmarkBarViewTest8 : public BookmarkBarViewEventTestBase {
   GURL url_dragging_;
 };
 
-VIEW_TEST(BookmarkBarViewTest8, MAYBE_DNDBackToOriginatingMenu)
+VIEW_TEST(BookmarkBarViewTest8, DNDBackToOriginatingMenu)
 
 // Moves the mouse over the scroll button and makes sure we get scrolling.
 class BookmarkBarViewTest9 : public BookmarkBarViewEventTestBase {
@@ -920,7 +924,7 @@ class BookmarkBarViewTest9 : public BookmarkBarViewEventTestBase {
   views::MenuItemView* first_menu_;
 };
 
-VIEW_TEST(BookmarkBarViewTest9, MAYBE_ScrollButtonScrolls)
+VIEW_TEST(BookmarkBarViewTest9, ScrollButtonScrolls)
 
 // Tests up/down/left/enter key messages.
 class BookmarkBarViewTest10 : public BookmarkBarViewEventTestBase {
@@ -1026,7 +1030,7 @@ class BookmarkBarViewTest10 : public BookmarkBarViewEventTestBase {
   }
 };
 
-VIEW_TEST(BookmarkBarViewTest10, MAYBE_KeyEvents)
+VIEW_TEST(BookmarkBarViewTest10, KeyEvents)
 
 // Make sure the menu closes with the following sequence: show menu, show
 // context menu, close context menu (via escape), then click else where. This
@@ -1103,7 +1107,7 @@ class BookmarkBarViewTest11 : public BookmarkBarViewEventTestBase {
   ContextMenuNotificationObserver observer_;
 };
 
-VIEW_TEST(BookmarkBarViewTest11, MAYBE_CloseMenuAfterClosingContextMenu)
+VIEW_TEST(BookmarkBarViewTest11, CloseMenuAfterClosingContextMenu)
 
 // Tests showing a modal dialog from a context menu.
 class BookmarkBarViewTest12 : public BookmarkBarViewEventTestBase {
@@ -1185,7 +1189,7 @@ class BookmarkBarViewTest12 : public BookmarkBarViewEventTestBase {
   }
 };
 
-VIEW_TEST(BookmarkBarViewTest12, MAYBE_CloseWithModalDialog)
+VIEW_TEST(BookmarkBarViewTest12, CloseWithModalDialog)
 
 // Tests clicking on the separator of a context menu (this is for coverage of
 // bug 17862).
diff --git a/ui/views/controls/menu/menu_controller.cc b/ui/views/controls/menu/menu_controller.cc
index 01ace1f..6f43994 100644
--- a/ui/views/controls/menu/menu_controller.cc
+++ b/ui/views/controls/menu/menu_controller.cc
@@ -27,6 +27,7 @@
 
 #if defined(USE_AURA)
 #include "ui/aura/client/dispatcher_client.h"
+#include "ui/aura/client/drag_drop_client.h"
 #include "ui/aura/env.h"
 #include "ui/aura/root_window.h"
 #endif
@@ -320,9 +321,9 @@ MenuItemView* MenuController::Run(Widget* parent,
   message_loop_depth_++;
   DCHECK_LE(message_loop_depth_, 2);
 #if defined(USE_AURA)
-  aura::client::GetDispatcherClient(
-      parent->GetNativeWindow()->GetRootWindow())->
-          RunWithDispatcher(this, parent->GetNativeWindow(), true);
+  root_window_ = parent->GetNativeWindow()->GetRootWindow();
+  aura::client::GetDispatcherClient(root_window_)->
+      RunWithDispatcher(this, parent->GetNativeWindow(), true);
 #else
   {
     MessageLoopForUI* loop = MessageLoopForUI::current();
@@ -1025,6 +1026,9 @@ MenuController::MenuController(bool blocking,
       drop_target_(NULL),
       drop_position_(MenuDelegate::DROP_UNKNOWN),
       owner_(NULL),
+#if defined(USE_AURA)
+      root_window_(NULL),
+#endif
       possible_drag_(false),
       drag_in_progress_(false),
       valid_drop_coordinates_(false),
@@ -1990,7 +1994,18 @@ void MenuController::SetExitType(ExitType type) {
   //
   // It's safe to invoke QuitNow multiple times, it only effects the current
   // loop.
-  if (exit_type_ != EXIT_NONE && message_loop_depth_)
+  bool quit_now = exit_type_ != EXIT_NONE && message_loop_depth_;
+
+#if defined(USE_AURA)
+  // On aura drag and drop runs a nested messgae loop too. If drag and drop is
+  // active and we quit we would prematurely cancel drag and drop, which we
+  // don't want.
+  if (aura::client::GetDragDropClient(root_window_) &&
+      aura::client::GetDragDropClient(root_window_)->IsDragDropInProgress())
+    quit_now = false;
+#endif
+
+  if (quit_now)
     MessageLoop::current()->QuitNow();
 }
 
diff --git a/ui/views/controls/menu/menu_controller.h b/ui/views/controls/menu/menu_controller.h
index 8f3f054..088e813 100644
--- a/ui/views/controls/menu/menu_controller.h
+++ b/ui/views/controls/menu/menu_controller.h
@@ -20,11 +20,15 @@
 #include "ui/views/controls/menu/menu_delegate.h"
 #include "ui/views/controls/menu/menu_item_view.h"
 
+#if defined(USE_AURA)
+namespace aura {
+class RootWindow;
+}
+#endif
+
 namespace ui {
 class OSExchangeData;
 }
-using ui::OSExchangeData;
-
 namespace views {
 
 class DropTargetEvent;
@@ -110,9 +114,9 @@ class VIEWS_EXPORT MenuController : public MessageLoop::Dispatcher {
   bool GetDropFormats(
       SubmenuView* source,
       int* formats,
-      std::set<OSExchangeData::CustomFormat>* custom_formats);
+      std::set<ui::OSExchangeData::CustomFormat>* custom_formats);
   bool AreDropTypesRequired(SubmenuView* source);
-  bool CanDrop(SubmenuView* source, const OSExchangeData& data);
+  bool CanDrop(SubmenuView* source, const ui::OSExchangeData& data);
   void OnDragEntered(SubmenuView* source, const DropTargetEvent& event);
   int OnDragUpdated(SubmenuView* source, const DropTargetEvent& event);
   void OnDragExited(SubmenuView* source);
@@ -495,6 +499,12 @@ class VIEWS_EXPORT MenuController : public MessageLoop::Dispatcher {
   // Owner of child windows.
   Widget* owner_;
 
+#if defined(USE_AURA)
+  // |owner_|s RootWindow. Cached as at the time we need it |owner_| may have
+  // been deleted.
+  aura::RootWindow* root_window_;
+#endif
+
   // Indicates a possible drag operation.
   bool possible_drag_;