[views,gtk] Error bubble closed if GlobalError is removed from profile while bubble is displayed.

BUG=109728
TEST=browser_tests:GlobalErrorServiceBrowserTest.*

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

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

9 files changed, 150 insertions, 22 deletions

diff --git a/chrome/browser/ui/global_error.h b/chrome/browser/ui/global_error.h
index ffac75e..6352037 100644
--- a/chrome/browser/ui/global_error.h
+++ b/chrome/browser/ui/global_error.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -50,7 +50,8 @@ class GlobalError {
   // Returns the cancel button label for the bubble view. To hide the cancel
   // button return an empty string.
   virtual string16 GetBubbleViewCancelButtonLabel() = 0;
-  // Called when the bubble view is closed.
+  // Called when the bubble view is closed unless the bubble is closed as the
+  // result of |this| being removed from GlobalErrorService.
   virtual void BubbleViewDidClose() = 0;
   // Called when the user clicks on the accept button.
   virtual void BubbleViewAcceptButtonPressed() = 0;
diff --git a/chrome/browser/ui/global_error_service.cc b/chrome/browser/ui/global_error_service.cc
index 45ecf11..ed24f47 100644
--- a/chrome/browser/ui/global_error_service.cc
+++ b/chrome/browser/ui/global_error_service.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -31,7 +31,7 @@ void GlobalErrorService::RemoveGlobalError(GlobalError* error) {
 
 GlobalError* GlobalErrorService::GetGlobalErrorByMenuItemCommandID(
     int command_id) const {
-  for (std::vector<GlobalError*>::const_iterator
+  for (GlobalErrorList::const_iterator
        it = errors_.begin(); it != errors_.end(); ++it) {
     GlobalError* error = *it;
     if (error->HasMenuItem() && command_id == error->MenuItemCommandID())
@@ -41,7 +41,7 @@ GlobalError* GlobalErrorService::GetGlobalErrorByMenuItemCommandID(
 }
 
 int GlobalErrorService::GetFirstBadgeResourceID() const {
-  for (std::vector<GlobalError*>::const_iterator
+  for (GlobalErrorList::const_iterator
        it = errors_.begin(); it != errors_.end(); ++it) {
     GlobalError* error = *it;
     if (error->HasBadge())
@@ -51,7 +51,7 @@ int GlobalErrorService::GetFirstBadgeResourceID() const {
 }
 
 GlobalError* GlobalErrorService::GetFirstGlobalErrorWithBubbleView() const {
-  for (std::vector<GlobalError*>::const_iterator
+  for (GlobalErrorList::const_iterator
        it = errors_.begin(); it != errors_.end(); ++it) {
     GlobalError* error = *it;
     if (error->HasBubbleView() && !error->HasShownBubbleView())
diff --git a/chrome/browser/ui/global_error_service.h b/chrome/browser/ui/global_error_service.h
index 5ee29c6..0c189017 100644
--- a/chrome/browser/ui/global_error_service.h
+++ b/chrome/browser/ui/global_error_service.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -21,6 +21,9 @@ class Profile;
 //   - a sync error occurred
 class GlobalErrorService : public ProfileKeyedService {
  public:
+  // Type used to represent the list of currently active errors.
+  typedef std::vector<GlobalError*> GlobalErrorList;
+
   // Constructs a GlobalErrorService object for the given profile. The profile
   // maybe NULL for tests.
   explicit GlobalErrorService(Profile* profile);
@@ -49,7 +52,7 @@ class GlobalErrorService : public ProfileKeyedService {
   GlobalError* GetFirstGlobalErrorWithBubbleView() const;
 
   // Gets all errors.
-  const std::vector<GlobalError*>& errors() { return errors_; }
+  const GlobalErrorList& errors() { return errors_; }
 
   // Post a notification that a global error has changed and that the error UI
   // should update it self. Pass NULL for the given error to mean all error UIs
@@ -57,7 +60,7 @@ class GlobalErrorService : public ProfileKeyedService {
   void NotifyErrorsChanged(GlobalError* error);
 
  private:
-  std::vector<GlobalError*> errors_;
+  GlobalErrorList errors_;
   Profile* profile_;
 
   DISALLOW_COPY_AND_ASSIGN(GlobalErrorService);
diff --git a/chrome/browser/ui/global_error_service_browsertest.cc b/chrome/browser/ui/global_error_service_browsertest.cc
index ae97f0c..37932cb 100644
--- a/chrome/browser/ui/global_error_service_browsertest.cc
+++ b/chrome/browser/ui/global_error_service_browsertest.cc
@@ -1,13 +1,15 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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 "chrome/browser/ui/global_error_service.h"
 
+#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/global_error.h"
 #include "chrome/browser/ui/global_error_service_factory.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
 
 namespace {
 
@@ -52,7 +54,10 @@ class BubbleViewError : public GlobalError {
   virtual string16 GetBubbleViewCancelButtonLabel() OVERRIDE {
     return string16();
   }
-  virtual void BubbleViewDidClose() OVERRIDE {}
+  virtual void BubbleViewDidClose() OVERRIDE {
+    ++bubble_view_close_count_;
+  }
+
   virtual void BubbleViewAcceptButtonPressed() OVERRIDE {}
   virtual void BubbleViewCancelButtonPressed() OVERRIDE {}
 
@@ -86,3 +91,29 @@ IN_PROC_BROWSER_TEST_F(GlobalErrorServiceBrowserTest, ShowBubbleView) {
   EXPECT_TRUE(error->HasShownBubbleView());
   EXPECT_EQ(0, error->bubble_view_close_count());
 }
+
+// TODO(sail): enable on Mac once http://crbug.com/109728 is fixed.
+#if !defined(OS_MACOSX)
+// Test that bubble is silently dismissed if it is showing when the GlobalError
+// instance is removed from the profile.
+IN_PROC_BROWSER_TEST_F(GlobalErrorServiceBrowserTest,
+                       BubbleViewDismissedOnRemove) {
+  scoped_ptr<BubbleViewError> error(new BubbleViewError);
+
+  GlobalErrorService* service =
+      GlobalErrorServiceFactory::GetForProfile(browser()->profile());
+  service->AddGlobalError(error.get());
+
+  EXPECT_EQ(error.get(), service->GetFirstGlobalErrorWithBubbleView());
+  error->ShowBubbleView(browser());
+  ui_test_utils::RunAllPendingInMessageLoop();
+  EXPECT_TRUE(error->HasShownBubbleView());
+  EXPECT_EQ(0, error->bubble_view_close_count());
+
+  // Removing |error| from profile should dismiss the bubble view without
+  // calling |error->BubbleViewDidClose|.
+  service->RemoveGlobalError(error.get());
+  EXPECT_EQ(0, error->bubble_view_close_count());
+  // |error| is no longer owned by service and will be deleted.
+}
+#endif
diff --git a/chrome/browser/ui/gtk/global_error_bubble.cc b/chrome/browser/ui/gtk/global_error_bubble.cc
index 43604d8..2e64fa0 100644
--- a/chrome/browser/ui/gtk/global_error_bubble.cc
+++ b/chrome/browser/ui/gtk/global_error_bubble.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -10,10 +10,14 @@
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/global_error.h"
+#include "chrome/browser/ui/global_error_service.h"
+#include "chrome/browser/ui/global_error_service_factory.h"
 #include "chrome/browser/ui/gtk/browser_toolbar_gtk.h"
 #include "chrome/browser/ui/gtk/browser_window_gtk.h"
 #include "chrome/browser/ui/gtk/gtk_theme_service.h"
 #include "chrome/browser/ui/gtk/gtk_util.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "content/public/browser/notification_service.h"
 #include "ui/base/gtk/gtk_hig_constants.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/gtk_util.h"
@@ -100,6 +104,9 @@ GlobalErrorBubble::GlobalErrorBubble(Profile* profile,
                      G_CALLBACK(OnCancelButtonThunk), this);
   }
 
+  registrar_.Add(this, chrome::NOTIFICATION_GLOBAL_ERRORS_CHANGED,
+                 content::Source<Profile>(profile));
+
   BubbleGtk::ArrowLocationGtk arrow_location =
       base::i18n::IsRTL() ?
       BubbleGtk::ARROW_LOCATION_TOP_LEFT :
@@ -119,7 +126,12 @@ GlobalErrorBubble::~GlobalErrorBubble() {
 
 void GlobalErrorBubble::BubbleClosing(BubbleGtk* bubble,
                                       bool closed_by_escape) {
-  error_->BubbleViewDidClose();
+  // We unsubscribe from removal notifications here and below because any of
+  // GlobalError callbacks may remove it from the profile. Here it is needed
+  // also because |this| will be deleted later asynchronously in OnDestroy.
+  registrar_.RemoveAll();
+  if (error_)
+    error_->BubbleViewDidClose();
 }
 
 void GlobalErrorBubble::OnDestroy(GtkWidget* sender) {
@@ -127,15 +139,38 @@ void GlobalErrorBubble::OnDestroy(GtkWidget* sender) {
 }
 
 void GlobalErrorBubble::OnAcceptButton(GtkWidget* sender) {
+  registrar_.RemoveAll();
   error_->BubbleViewAcceptButtonPressed();
   bubble_->Close();
 }
 
 void GlobalErrorBubble::OnCancelButton(GtkWidget* sender) {
+  registrar_.RemoveAll();
   error_->BubbleViewCancelButtonPressed();
   bubble_->Close();
 }
 
+void GlobalErrorBubble::Observe(int type,
+                                const content::NotificationSource& source,
+                                const content::NotificationDetails& details) {
+  DCHECK(type == chrome::NOTIFICATION_GLOBAL_ERRORS_CHANGED);
+  DCHECK(error_);
+  if (content::Details<GlobalError>(details).ptr() == error_) {
+    Profile* profile = content::Source<Profile>(source).ptr();
+    const std::vector<GlobalError*>& errors =
+        GlobalErrorServiceFactory::GetForProfile(profile)->errors();
+    // This handles the case when a GlobalError instance is removed from profile
+    // not as a part of normal flow (i.e., after the bubble has been closed)
+    // but while the bubble is still showing. |error_| is no longer guaranteed
+    // to exist so we set it to |NULL| and dismiss the bubble.
+    if (std::find(errors.begin(), errors.end(), error_) == errors.end()) {
+      error_ = NULL;
+      registrar_.RemoveAll();
+      bubble_->Close();
+    }
+  }
+}
+
 void GlobalError::ShowBubbleView(Browser* browser, GlobalError* error) {
   BrowserWindowGtk* browser_window =
       BrowserWindowGtk::GetBrowserWindowForNativeWindow(
diff --git a/chrome/browser/ui/gtk/global_error_bubble.h b/chrome/browser/ui/gtk/global_error_bubble.h
index 037cc52..424dbc4 100644
--- a/chrome/browser/ui/gtk/global_error_bubble.h
+++ b/chrome/browser/ui/gtk/global_error_bubble.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -9,6 +9,8 @@
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "chrome/browser/ui/gtk/bubble/bubble_gtk.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
 #include "ui/base/gtk/gtk_signal.h"
 
 typedef struct _GtkWidget GtkWidget;
@@ -16,7 +18,8 @@ typedef struct _GtkWidget GtkWidget;
 class GlobalError;
 class Profile;
 
-class GlobalErrorBubble : public BubbleDelegateGtk {
+class GlobalErrorBubble : public BubbleDelegateGtk,
+                          public content::NotificationObserver {
  public:
   GlobalErrorBubble(Profile* profile, GlobalError* error, GtkWidget* anchor);
   virtual ~GlobalErrorBubble();
@@ -29,8 +32,17 @@ class GlobalErrorBubble : public BubbleDelegateGtk {
   CHROMEGTK_CALLBACK_0(GlobalErrorBubble, void, OnAcceptButton);
   CHROMEGTK_CALLBACK_0(GlobalErrorBubble, void, OnCancelButton);
 
+  // content::NotificationObserver overrides:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
   BubbleGtk* bubble_;
+  // Weak reference to the GlobalError instance the bubble is shown for. Is
+  // reset to |NULL| if instance is removed from GlobalErrorService while
+  // the bubble is still showing.
   GlobalError* error_;
+  content::NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(GlobalErrorBubble);
 };
diff --git a/chrome/browser/ui/toolbar/wrench_menu_model.cc b/chrome/browser/ui/toolbar/wrench_menu_model.cc
index 34a8afa..af05f08 100644
--- a/chrome/browser/ui/toolbar/wrench_menu_model.cc
+++ b/chrome/browser/ui/toolbar/wrench_menu_model.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -554,9 +554,9 @@ void WrenchMenuModel::AddGlobalErrorMenuItems() {
   // it won't show in the existing wrench menu. To fix this we need to some
   // how update the menu if new errors are added.
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
-  const std::vector<GlobalError*>& errors =
+  const GlobalErrorService::GlobalErrorList& errors =
       GlobalErrorServiceFactory::GetForProfile(browser_->profile())->errors();
-  for (std::vector<GlobalError*>::const_iterator
+  for (GlobalErrorService::GlobalErrorList::const_iterator
        it = errors.begin(); it != errors.end(); ++it) {
     GlobalError* error = *it;
     if (error->HasMenuItem()) {
diff --git a/chrome/browser/ui/views/global_error_bubble_view.cc b/chrome/browser/ui/views/global_error_bubble_view.cc
index 7f24036..9c368da 100644
--- a/chrome/browser/ui/views/global_error_bubble_view.cc
+++ b/chrome/browser/ui/views/global_error_bubble_view.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -6,9 +6,13 @@
 
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/ui/global_error.h"
+#include "chrome/browser/ui/global_error_service.h"
+#include "chrome/browser/ui/global_error_service_factory.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/toolbar_view.h"
 #include "chrome/browser/ui/views/window.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "content/public/browser/notification_service.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image.h"
 #include "ui/views/controls/button/text_button.h"
@@ -118,6 +122,9 @@ GlobalErrorBubbleView::GlobalErrorBubbleView(
 
   // Adjust the message label size in case buttons are too long.
   message_label->SizeToFit(layout->GetPreferredSize(this).width());
+
+  registrar_.Add(this, chrome::NOTIFICATION_GLOBAL_ERRORS_CHANGED,
+                 content::Source<Profile>(browser->profile()));
 }
 
 GlobalErrorBubbleView::~GlobalErrorBubbleView() {
@@ -131,6 +138,9 @@ gfx::Rect GlobalErrorBubbleView::GetAnchorRect() {
 
 void GlobalErrorBubbleView::ButtonPressed(views::Button* sender,
                                           const views::Event& event) {
+  // We unsubscribe from removal notifications here and below because any of
+  // GlobalError callbacks may remove it from the profile.
+  registrar_.RemoveAll();
   if (sender->tag() == TAG_ACCEPT_BUTTON)
     error_->BubbleViewAcceptButtonPressed();
   else if (sender->tag() == TAG_CANCEL_BUTTON)
@@ -141,7 +151,31 @@ void GlobalErrorBubbleView::ButtonPressed(views::Button* sender,
 }
 
 void GlobalErrorBubbleView::WindowClosing() {
-  error_->BubbleViewDidClose();
+  registrar_.RemoveAll();
+  if (error_)
+    error_->BubbleViewDidClose();
+}
+
+void GlobalErrorBubbleView::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(type == chrome::NOTIFICATION_GLOBAL_ERRORS_CHANGED);
+  DCHECK(error_);
+  if (content::Details<GlobalError>(details).ptr() == error_) {
+    Profile* profile = content::Source<Profile>(source).ptr();
+    const std::vector<GlobalError*>& errors =
+        GlobalErrorServiceFactory::GetForProfile(profile)->errors();
+    // This handles the case when a GlobalError instance is removed from profile
+    // not as a part of normal flow (i.e., after the bubble has been closed)
+    // but while the bubble is still showing. |error_| is no longer guaranteed
+    // to exist so we set it to |NULL| and dismiss the bubble.
+    if (std::find(errors.begin(), errors.end(), error_) == errors.end()) {
+      error_ = NULL;
+      registrar_.RemoveAll();
+      GetWidget()->Close();
+    }
+  }
 }
 
 void GlobalError::ShowBubbleView(Browser* browser, GlobalError* error) {
diff --git a/chrome/browser/ui/views/global_error_bubble_view.h b/chrome/browser/ui/views/global_error_bubble_view.h
index 868be51..666cd67 100644
--- a/chrome/browser/ui/views/global_error_bubble_view.h
+++ b/chrome/browser/ui/views/global_error_bubble_view.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -6,6 +6,8 @@
 #define CHROME_BROWSER_UI_VIEWS_GLOBAL_ERROR_BUBBLE_VIEW_H_
 #pragma once
 
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
 #include "ui/views/bubble/bubble_delegate.h"
 #include "ui/views/controls/button/button.h"
 
@@ -13,7 +15,8 @@ class Browser;
 class GlobalError;
 
 class GlobalErrorBubbleView : public views::ButtonListener,
-                              public views::BubbleDelegateView {
+                              public views::BubbleDelegateView,
+                              public content::NotificationObserver {
  public:
   GlobalErrorBubbleView(views::View* anchor_view,
                         views::BubbleBorder::ArrowLocation location,
@@ -31,9 +34,18 @@ class GlobalErrorBubbleView : public views::ButtonListener,
   // views::WidgetDelegate implementation.
   virtual void WindowClosing() OVERRIDE;
 
+  // content::NotificationObserver overrides:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
  private:
   Browser* browser_;
+  // Weak reference to the GlobalError instance the bubble is shown for. Is
+  // reset to |NULL| if instance is removed from GlobalErrorService while
+  // the bubble is still showing.
   GlobalError* error_;
+  content::NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(GlobalErrorBubbleView);
 };