Fix the bug of multiple overlays.

- keyboard overlay and partial_screenshot overlay should do nothing
  if there are already overlays.
- overlay_event_filter should cancel the existing delegate if
  another delegate is trying to activate.

BUG=341958
R=jamescook@chromium.org
TEST=manually

Review URL: https://codereview.chromium.org/461093003

Cr-Commit-Position: refs/heads/master@{#289199}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289199 0039d316-1c4b-4281-b951-d872f2087c98

ash/ash.gyp

@@ -689,6 +689,8 @@
       'test/test_activation_delegate.h',
       'test/test_lock_state_controller_delegate.cc',
       'test/test_lock_state_controller_delegate.h',
+      'test/test_overlay_delegate.cc',
+      'test/test_overlay_delegate.h',
       'test/test_screenshot_delegate.cc',
       'test/test_screenshot_delegate.h',
       'test/test_session_state_delegate.cc',
@@ -860,6 +862,7 @@
       'wm/window_util_unittest.cc',
       'wm/maximize_mode/workspace_backdrop_delegate.cc',
       'wm/maximize_mode/workspace_backdrop_delegate.h',
+      'wm/overlay_event_filter_unittest.cc',
       'wm/workspace/magnetism_matcher_unittest.cc',
       'wm/workspace/multi_window_resize_controller_unittest.cc',
       'wm/workspace/workspace_event_handler_test_helper.cc',

ash/first_run/first_run_helper_impl.cc

@@ -40,7 +40,7 @@ FirstRunHelperImpl::FirstRunHelperImpl()
 }
 
 FirstRunHelperImpl::~FirstRunHelperImpl() {
-  Shell::GetInstance()->overlay_filter()->Deactivate();
+  Shell::GetInstance()->overlay_filter()->Deactivate(this);
   if (IsTrayBubbleOpened())
     CloseTrayBubble();
   widget_->Close();

ash/keyboard_overlay/keyboard_overlay_view.cc

@@ -43,7 +43,7 @@ KeyboardOverlayView::~KeyboardOverlayView() {
 }
 
 void KeyboardOverlayView::Cancel() {
-  Shell::GetInstance()->overlay_filter()->Deactivate();
+  Shell::GetInstance()->overlay_filter()->Deactivate(this);
   views::Widget* widget = GetWidget();
   if (widget)
     widget->Close();
@@ -66,10 +66,14 @@ aura::Window* KeyboardOverlayView::GetWindow() {
   return GetWidget()->GetNativeWindow();
 }
 
+// static
 void KeyboardOverlayView::ShowDialog(
     content::BrowserContext* context,
     WebContentsHandler* handler,
     const GURL& url) {
+  if (Shell::GetInstance()->overlay_filter()->IsActive())
+    return;
+
   KeyboardOverlayDelegate* delegate = new KeyboardOverlayDelegate(
       l10n_util::GetStringUTF16(IDS_ASH_KEYBOARD_OVERLAY_TITLE), url);
   KeyboardOverlayView* view =

ash/test/test_overlay_delegate.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/test/test_overlay_delegate.h"
+
+namespace ash {
+namespace test {
+
+TestOverlayDelegate::TestOverlayDelegate()
+    : cancel_count_(0) {}
+TestOverlayDelegate::~TestOverlayDelegate() {}
+
+int TestOverlayDelegate::GetCancelCountAndReset() {
+  int count = cancel_count_;
+  cancel_count_ = 0;
+  return count;
+}
+
+void TestOverlayDelegate::Cancel() {
+  ++cancel_count_;
+}
+
+bool TestOverlayDelegate::IsCancelingKeyEvent(ui::KeyEvent* event) {
+  return false;
+}
+
+aura::Window* TestOverlayDelegate::GetWindow() {
+  return NULL;
+}
+
+}  // namespace test
+}  // namespace ash

ash/test/test_overlay_delegate.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef ASH_TEST_TEST_OVERLAY_DELEGATE_H_
+#define ASH_TEST_TEST_OVERLAY_DELEGATE_H_
+
+#include "ash/wm/overlay_event_filter.h"
+
+namespace ash {
+namespace test {
+
+class TestOverlayDelegate : public OverlayEventFilter::Delegate {
+ public:
+  TestOverlayDelegate();
+  virtual ~TestOverlayDelegate();
+
+  int GetCancelCountAndReset();
+
+ private:
+  // Overridden from OverlayEventFilter::Delegate:
+  virtual void Cancel() OVERRIDE;
+  virtual bool IsCancelingKeyEvent(ui::KeyEvent* event) OVERRIDE;
+  virtual aura::Window* GetWindow() OVERRIDE;
+
+  int cancel_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestOverlayDelegate);
+};
+
+}  // namespace test
+}  // namespace ash
+
+#endif  // ASH_TEST_TEST_OVERLAY_DELEGATE_H_

ash/wm/overlay_event_filter.cc

@@ -56,11 +56,14 @@ void OverlayEventFilter::OnLockStateChanged(bool locked) {
 }
 
 void OverlayEventFilter::Activate(Delegate* delegate) {
+  if (delegate_)
+    delegate_->Cancel();
   delegate_ = delegate;
 }
 
-void OverlayEventFilter::Deactivate() {
-  delegate_ = NULL;
+void OverlayEventFilter::Deactivate(Delegate* delegate) {
+  if (delegate_ == delegate)
+    delegate_ = NULL;
 }
 
 void OverlayEventFilter::Cancel() {
@@ -68,4 +71,8 @@ void OverlayEventFilter::Cancel() {
     delegate_->Cancel();
 }
 
+bool OverlayEventFilter::IsActive() {
+  return delegate_ != NULL;
+}
+
 }  // namespace ash

ash/wm/overlay_event_filter.h

@@ -46,11 +46,14 @@ class ASH_EXPORT OverlayEventFilter : public ui::EventHandler,
   void Activate(Delegate* delegate);
 
   // Ends the filtering of events.
-  void Deactivate();
+  void Deactivate(Delegate* delegate);
 
   // Cancels the partial screenshot UI.  Do nothing if it's not activated.
   void Cancel();
 
+  // Returns true if it's currently active.
+  bool IsActive();
+
   // ui::EventHandler overrides:
   virtual void OnKeyEvent(ui::KeyEvent* event) OVERRIDE;
 
@@ -60,6 +63,8 @@ class ASH_EXPORT OverlayEventFilter : public ui::EventHandler,
   virtual void OnLockStateChanged(bool locked) OVERRIDE;
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(PartialScreenshotViewTest, DontStartOverOverlay);
+
   Delegate* delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(OverlayEventFilter);

ash/wm/overlay_event_filter_unittest.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/wm/overlay_event_filter.h"
+
+#include "ash/shell.h"
+#include "ash/test/ash_test_base.h"
+#include "ash/test/test_overlay_delegate.h"
+
+namespace ash {
+namespace test {
+
+typedef AshTestBase OverlayEventFilterTest;
+
+// Tests of the multiple overlay delegates attempt to activate, in that case
+// Cancel() of the existing delegate should be called.
+// See http://crbug.com/341958
+TEST_F(OverlayEventFilterTest, CancelAtActivating) {
+  TestOverlayDelegate d1;
+  TestOverlayDelegate d2;
+
+  Shell::GetInstance()->overlay_filter()->Activate(&d1);
+  EXPECT_EQ(0, d1.GetCancelCountAndReset());
+  EXPECT_EQ(0, d2.GetCancelCountAndReset());
+
+  Shell::GetInstance()->overlay_filter()->Activate(&d2);
+  EXPECT_EQ(1, d1.GetCancelCountAndReset());
+  EXPECT_EQ(0, d2.GetCancelCountAndReset());
+
+  Shell::GetInstance()->overlay_filter()->Cancel();
+  EXPECT_EQ(0, d1.GetCancelCountAndReset());
+  EXPECT_EQ(1, d2.GetCancelCountAndReset());
+}
+
+}  // namespace test
+}  // namespace ash

ash/wm/partial_screenshot_view.cc

@@ -70,7 +70,7 @@ class PartialScreenshotView::OverlayDelegate
 
  private:
   virtual ~OverlayDelegate() {
-    Shell::GetInstance()->overlay_filter()->Deactivate();
+    Shell::GetInstance()->overlay_filter()->Deactivate(this);
   }
 
   std::vector<views::Widget*> widgets_;
@@ -83,6 +83,10 @@ std::vector<PartialScreenshotView*>
 PartialScreenshotView::StartPartialScreenshot(
     ScreenshotDelegate* screenshot_delegate) {
   std::vector<PartialScreenshotView*> views;
+
+  if (Shell::GetInstance()->overlay_filter()->IsActive())
+    return views;
+
   OverlayDelegate* overlay_delegate = new OverlayDelegate();
   aura::Window::Windows root_windows = Shell::GetAllRootWindows();
   for (aura::Window::Windows::iterator it = root_windows.begin();

ash/wm/partial_screenshot_view_unittest.cc

@@ -8,33 +8,51 @@
 #include "ash/shell.h"
 #include "ash/shell_window_ids.h"
 #include "ash/test/ash_test_base.h"
+#include "ash/test/test_overlay_delegate.h"
 #include "ash/test/test_screenshot_delegate.h"
 #include "ui/aura/window_event_dispatcher.h"
 #include "ui/events/test/event_generator.h"
+#include "ui/views/widget/widget.h"
+#include "ui/views/widget/widget_observer.h"
 
 namespace ash {
 
-class PartialScreenshotViewTest : public test::AshTestBase {
+class PartialScreenshotViewTest : public test::AshTestBase,
+                                  public views::WidgetObserver {
  public:
   PartialScreenshotViewTest() : view_(NULL) {}
-  virtual ~PartialScreenshotViewTest() {}
+  virtual ~PartialScreenshotViewTest() {
+    if (view_)
+      view_->GetWidget()->RemoveObserver(this);
+  }
 
-  virtual void SetUp() OVERRIDE {
-    test::AshTestBase::SetUp();
+  void StartPartialScreenshot() {
     std::vector<PartialScreenshotView*> views =
         PartialScreenshotView::StartPartialScreenshot(GetScreenshotDelegate());
-    ASSERT_EQ(1u, views.size());
-    view_ = views[0];
+    if (!views.empty()) {
+      view_ = views[0];
+      view_->GetWidget()->AddObserver(this);
+    }
   }
 
  protected:
   PartialScreenshotView* view_;
 
  private:
+  // views::WidgetObserver:
+  virtual void OnWidgetDestroying(views::Widget* widget) OVERRIDE {
+    if (view_ && view_->GetWidget() == widget)
+      view_ = NULL;
+    widget->RemoveObserver(this);
+  }
+
   DISALLOW_COPY_AND_ASSIGN(PartialScreenshotViewTest);
 };
 
 TEST_F(PartialScreenshotViewTest, BasicMouse) {
+  StartPartialScreenshot();
+  ASSERT_TRUE(view_);
+
   ui::test::EventGenerator generator(Shell::GetPrimaryRootWindow());
 
   generator.MoveMouseTo(100, 100);
@@ -53,6 +71,9 @@ TEST_F(PartialScreenshotViewTest, BasicMouse) {
 }
 
 TEST_F(PartialScreenshotViewTest, BasicTouch) {
+  StartPartialScreenshot();
+  ASSERT_TRUE(view_);
+
   ui::test::EventGenerator generator(Shell::GetPrimaryRootWindow());
 
   generator.set_current_location(gfx::Point(100,100));
@@ -74,4 +95,28 @@ TEST_F(PartialScreenshotViewTest, BasicTouch) {
   EXPECT_EQ("100,100 100x100", GetScreenshotDelegate()->last_rect().ToString());
 }
 
+// Partial screenshot session should not start when there is already another
+// overlay. See: http://crbug.com/341958
+TEST_F(PartialScreenshotViewTest, DontStartOverOverlay) {
+  OverlayEventFilter* overlay_filter = Shell::GetInstance()->overlay_filter();
+  test::TestOverlayDelegate delegate;
+  overlay_filter->Activate(&delegate);
+  EXPECT_EQ(&delegate, overlay_filter->delegate_);
+
+  StartPartialScreenshot();
+  EXPECT_TRUE(view_ == NULL);
+  EXPECT_EQ(&delegate, overlay_filter->delegate_);
+  overlay_filter->Deactivate(&delegate);
+
+  StartPartialScreenshot();
+  EXPECT_TRUE(view_ != NULL);
+
+  // Someone else attempts to activate the overlay session, which should cancel
+  // the current partial screenshot session.
+  overlay_filter->Activate(&delegate);
+  RunAllPendingInMessageLoop();
+  EXPECT_EQ(&delegate, overlay_filter->delegate_);
+  EXPECT_TRUE(view_ == NULL);
+}
+
 }  // namespace ash