Avoid inconsistent state (leading to checkfailures) due to Windows minimize/restore functionality hiding and showing the autocomplete dropdown outside of the awareness of the controller.

Instead, this switches to the simpler model of just creating a new HWND when we want to show the dropdown, and destroying it when we want to close it.  Happily, this also seems to save quite a bit of code.

BUG=20511
TEST=Type a character in the omnibox, minimize the window, and restore it.  The dropdown should have disappeared.
Review URL: http://codereview.chromium.org/391011

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

7 files changed, 49 insertions, 167 deletions

diff --git a/chrome/browser/autocomplete/autocomplete_popup_model.cc b/chrome/browser/autocomplete/autocomplete_popup_model.cc
index c3147c1..8b0c9d1 100644
--- a/chrome/browser/autocomplete/autocomplete_popup_model.cc
+++ b/chrome/browser/autocomplete/autocomplete_popup_model.cc
@@ -175,12 +175,7 @@ GURL AutocompletePopupModel::URLsForCurrentSelection(
     // If there are no results, the popup should be closed (so we should have
     // failed the CHECK above), and URLsForDefaultMatch() should have been
     // called instead.
-    if (result->empty()) {
-      // We're going to checkfail, but first see whether
-      // controller_->latest_result() is actually in sync with |result|.
-      CHECK(controller_->latest_result().empty());
-      CHECK(false);
-    }
+    CHECK(!result->empty());
     CHECK(selected_line_ < result->size());
     match = result->begin() + selected_line_;
   }
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
index c3b93db..5ed809e 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
@@ -596,17 +596,11 @@ AutocompletePopupContentsView::AutocompletePopupContentsView(
     AutocompleteEditModel* edit_model,
     Profile* profile,
     const BubblePositioner* bubble_positioner)
-#if defined(OS_WIN)
-    : popup_(new AutocompletePopupWin(this)),
-#else
-    : popup_(new AutocompletePopupGtk(this)),
-#endif
-      model_(new AutocompletePopupModel(this, edit_model, profile)),
+    : model_(new AutocompletePopupModel(this, edit_model, profile)),
       edit_view_(edit_view),
       bubble_positioner_(bubble_positioner),
       result_font_(font.DeriveFont(kEditFontAdjust)),
-      ALLOW_THIS_IN_INITIALIZER_LIST(size_animation_(this)),
-      is_open_(false) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(size_animation_(this)) {
   // The following little dance is required because set_border() requires a
   // pointer to a non-const object.
   BubbleBorder* bubble_border = new BubbleBorder;
@@ -634,9 +628,7 @@ gfx::Rect AutocompletePopupContentsView::GetPopupBounds() const {
 // AutocompletePopupContentsView, AutocompletePopupView overrides:
 
 bool AutocompletePopupContentsView::IsOpen() const {
-  const bool is_open = popup_->IsOpen();
-  CHECK(is_open == is_open_);
-  return is_open;
+  return (popup_ != NULL);
 }
 
 void AutocompletePopupContentsView::InvalidateLine(size_t line) {
@@ -646,11 +638,11 @@ void AutocompletePopupContentsView::InvalidateLine(size_t line) {
 void AutocompletePopupContentsView::UpdatePopupAppearance() {
   if (model_->result().empty()) {
     // No matches, close any existing popup.
-    if (popup_->IsCreated()) {
+    if (popup_ != NULL) {
       size_animation_.Stop();
-      popup_->Hide();
+      popup_->CloseNow();
+      popup_.reset();
     }
-    is_open_ = false;
     return;
   }
 
@@ -684,22 +676,19 @@ void AutocompletePopupContentsView::UpdatePopupAppearance() {
     size_animation_.Reset();
   target_bounds_ = new_target_bounds;
 
-  if (!popup_->IsCreated()) {
-    // If we've never been shown, we need to create the window.
-    popup_->Init(edit_view_, this);
+  if (popup_ == NULL) {
+    // If the popup is currently closed, we need to create it.
+    popup_.reset(new AutocompletePopupClass(edit_view_, this));
   } else {
-    // Animate the popup shrinking, but don't animate growing larger (or
-    // appearing for the first time) since that would make the popup feel less
-    // responsive.
+    // Animate the popup shrinking, but don't animate growing larger since that
+    // would make the popup feel less responsive.
     GetWidget()->GetBounds(&start_bounds_, true);
-    if (popup_->IsVisible() &&
-        (target_bounds_.height() < start_bounds_.height()))
+    if (target_bounds_.height() < start_bounds_.height())
       size_animation_.Show();
     else
       start_bounds_ = target_bounds_;
-    popup_->Show();
+    popup_->SetBounds(GetPopupBounds());
   }
-  is_open_ = true;
 
   SchedulePaint();
 }
@@ -757,8 +746,8 @@ void AutocompletePopupContentsView::SetSelectedLine(size_t index,
 void AutocompletePopupContentsView::AnimationProgressed(
     const Animation* animation) {
   // We should only be running the animation when the popup is already visible.
-  CHECK(IsOpen());
-  popup_->Show();  // Adjusts bounds.
+  DCHECK(popup_ != NULL);
+  popup_->SetBounds(GetPopupBounds());
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
index 056e653..2f991bc 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
@@ -87,6 +87,12 @@ class AutocompletePopupContentsView : public views::View,
   virtual void Layout();
 
  private:
+#if defined(OS_WIN)
+  typedef AutocompletePopupWin AutocompletePopupClass;
+#else
+  typedef AutocompletePopupGtk AutocompletePopupClass;
+#endif
+
   // Returns true if the model has a match at the specified index.
   bool HasMatchAt(size_t index) const;
 
@@ -103,12 +109,8 @@ class AutocompletePopupContentsView : public views::View,
   // Makes the contents of the canvas slightly transparent.
   void MakeCanvasTransparent(gfx::Canvas* canvas);
 
-#if defined(OS_WIN)
   // The popup that contains this view.
-  scoped_ptr<AutocompletePopupWin> popup_;
-#else
-  scoped_ptr<AutocompletePopupGtk> popup_;
-#endif
+  scoped_ptr<AutocompletePopupClass> popup_;
 
   // The provider of our result set.
   scoped_ptr<AutocompletePopupModel> model_;
@@ -132,8 +134,6 @@ class AutocompletePopupContentsView : public views::View,
   gfx::Rect start_bounds_;
   gfx::Rect target_bounds_;
 
-  bool is_open_;  // Used only for sanity-checking.
-
   DISALLOW_COPY_AND_ASSIGN(AutocompletePopupContentsView);
 };
 
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_gtk.cc b/chrome/browser/views/autocomplete/autocomplete_popup_gtk.cc
index 5ea5e39..f178bc2 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_gtk.cc
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_gtk.cc
@@ -14,60 +14,25 @@
 // AutocompletePopupGtk, public:
 
 AutocompletePopupGtk::AutocompletePopupGtk(
+    AutocompleteEditView* edit_view,
     AutocompletePopupContentsView* contents)
-    : WidgetGtk(WidgetGtk::TYPE_POPUP),
-      contents_(contents),
-      edit_view_(NULL),
-      is_open_(false) {
+    : WidgetGtk(WidgetGtk::TYPE_POPUP) {
+  // Create the popup.  // Owned by |contents|.
   set_delete_on_destroy(false);
-}
-
-AutocompletePopupGtk::~AutocompletePopupGtk() {
-}
-
-void AutocompletePopupGtk::Show() {
-  // Move the popup to the place appropriate for the window's current position -
-  // it may have been moved since it was last shown.
-  SetBounds(contents_->GetPopupBounds());
-  if (!IsVisible()) {
-    WidgetGtk::Show();
-    StackWindow();
-  }
-  is_open_ = true;
-}
-
-void AutocompletePopupGtk::Hide() {
-  WidgetGtk::Hide();
-  is_open_ = false;
-}
-
-void AutocompletePopupGtk::Init(AutocompleteEditView* edit_view,
-                                views::View* contents) {
   MakeTransparent();
-  // Create the popup
   WidgetGtk::Init(gtk_widget_get_parent(edit_view->GetNativeView()),
-                  contents_->GetPopupBounds());
+                  contents->GetPopupBounds());
   // The contents is owned by the LocationBarView.
-  contents_->set_parent_owned(false);
-  SetContentsView(contents_);
-
-  edit_view_ = edit_view;
+  contents->set_parent_owned(false);
+  SetContentsView(contents);
 
   Show();
-}
 
-bool AutocompletePopupGtk::IsOpen() const {
-  const bool is_open = IsCreated() && IsVisible();
-  CHECK(is_open == is_open_);
-  return is_open;
-}
-
-bool AutocompletePopupGtk::IsCreated() const {
-  return GTK_IS_WIDGET(GetNativeView());
-}
-
-void AutocompletePopupGtk::StackWindow() {
-  GtkWidget* toplevel = gtk_widget_get_toplevel(edit_view_->GetNativeView());
+  // Restack the popup window directly above the browser's toplevel window.
+  GtkWidget* toplevel = gtk_widget_get_toplevel(edit_view->GetNativeView());
   DCHECK(GTK_WIDGET_TOPLEVEL(toplevel));
   gtk_util::StackPopupWindow(GetNativeView(), toplevel);
 }
+
+AutocompletePopupGtk::~AutocompletePopupGtk() {
+}
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_gtk.h b/chrome/browser/views/autocomplete/autocomplete_popup_gtk.h
index f15bfe1..b2c680b 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_gtk.h
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_gtk.h
@@ -12,32 +12,12 @@ class AutocompletePopupContentsView;
 
 class AutocompletePopupGtk : public views::WidgetGtk {
  public:
-  explicit AutocompletePopupGtk(AutocompletePopupContentsView* contents);
+  // Creates the popup and shows it. |edit_view| is the edit that created us.
+  AutocompletePopupGtk(AutocompleteEditView* edit_view,
+                       AutocompletePopupContentsView* contents);
   virtual ~AutocompletePopupGtk();
 
-  // Overridden from WidgetWin:
-  virtual void Show();
-  virtual void Hide();
-
-  // Creates the popup and shows it for the first time. |edit_view| is the edit
-  // that created us.
-  void Init(AutocompleteEditView* edit_view, views::View* contents);
-
-  // Returns true if the popup is open.
-  bool IsOpen() const;
-
-  // Returns true if the popup has been created.
-  bool IsCreated() const;
-
  private:
-  // Restack the popup window directly above the browser's toplevel window.
-  void StackWindow();
-
-  AutocompletePopupContentsView* contents_;
-  AutocompleteEditView* edit_view_;
-
-  bool is_open_;  // Used only for sanity-checking.
-
   DISALLOW_COPY_AND_ASSIGN(AutocompletePopupGtk);
 };
 
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_win.cc b/chrome/browser/views/autocomplete/autocomplete_popup_win.cc
index f3737ae..df8d9dd 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_win.cc
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_win.cc
@@ -14,39 +14,17 @@
 // AutocompletePopupWin, public:
 
 AutocompletePopupWin::AutocompletePopupWin(
-    AutocompletePopupContentsView* contents)
-    : contents_(contents),
-      is_open_(false) {
-  set_delete_on_destroy(false);
+    AutocompleteEditView* edit_view,
+    AutocompletePopupContentsView* contents) {
+  // Create the popup.
+  set_delete_on_destroy(false);  // Owned by |contents|.
   set_window_style(WS_POPUP | WS_CLIPCHILDREN);
   set_window_ex_style(WS_EX_TOOLWINDOW | WS_EX_LAYERED);
-}
-
-AutocompletePopupWin::~AutocompletePopupWin() {
-}
-
-void AutocompletePopupWin::Show() {
-  // Move the popup to the place appropriate for the window's current position -
-  // it may have been moved since it was last shown.
-  SetBounds(contents_->GetPopupBounds());
-  if (!IsVisible())
-    WidgetWin::Show();
-  is_open_ = true;
-}
-
-void AutocompletePopupWin::Hide() {
-  WidgetWin::Hide();
-  is_open_ = false;
-}
-
-void AutocompletePopupWin::Init(AutocompleteEditView* edit_view,
-                                views::View* contents) {
-  // Create the popup
   WidgetWin::Init(GetAncestor(edit_view->GetNativeView(), GA_ROOT),
-                  contents_->GetPopupBounds());
+                  contents->GetPopupBounds());
   // The contents is owned by the LocationBarView.
-  contents_->set_parent_owned(false);
-  SetContentsView(contents_);
+  contents->set_parent_owned(false);
+  SetContentsView(contents);
 
   // When an IME is attached to the rich-edit control, retrieve its window
   // handle and show this popup window under the IME windows.
@@ -56,17 +34,9 @@ void AutocompletePopupWin::Init(AutocompleteEditView* edit_view,
   HWND ime_window = ImmGetDefaultIMEWnd(edit_view->GetNativeView());
   SetWindowPos(ime_window ? ime_window : HWND_NOTOPMOST, 0, 0, 0, 0,
                SWP_NOSIZE | SWP_NOMOVE | SWP_NOACTIVATE | SWP_SHOWWINDOW);
-  is_open_ = true;
 }
 
-bool AutocompletePopupWin::IsOpen() const {
-  const bool is_open = IsCreated() && IsVisible();
-  CHECK(is_open == is_open_);
-  return is_open;
-}
-
-bool AutocompletePopupWin::IsCreated() const {
-  return !!IsWindow();
+AutocompletePopupWin::~AutocompletePopupWin() {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_win.h b/chrome/browser/views/autocomplete/autocomplete_popup_win.h
index 6290b4d..734938e 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_win.h
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_win.h
@@ -12,34 +12,17 @@ class AutocompletePopupContentsView;
 
 class AutocompletePopupWin : public views::WidgetWin {
  public:
-  explicit AutocompletePopupWin(AutocompletePopupContentsView* contents);
+  // Creates the popup and shows it. |edit_view| is the edit that created us.
+  AutocompletePopupWin(AutocompleteEditView* edit_view,
+                       AutocompletePopupContentsView* contents);
   virtual ~AutocompletePopupWin();
 
-  // Overridden from WidgetWin:
-  virtual void Show();
-  virtual void Hide();
-
-  // Creates the popup and shows it for the first time. |edit_view| is the edit
-  // that created us.
-  void Init(AutocompleteEditView* edit_view, views::View* contents);
-
-  // Returns true if the popup is open.
-  bool IsOpen() const;
-
-  // Returns true if the popup has been created.
-  bool IsCreated() const;
-
- protected:
+ private:
   // Overridden from WidgetWin:
   virtual LRESULT OnMouseActivate(HWND window,
                                   UINT hit_test,
                                   UINT mouse_message);
 
- private:
-  AutocompletePopupContentsView* contents_;
-
-  bool is_open_;  // Used only for sanity-checking.
-
   DISALLOW_COPY_AND_ASSIGN(AutocompletePopupWin);
 };