Instant: Update the non-view popup's IsOpen() state correctly.

Repro steps:
(i) --enable-instant-extended-api.
(ii) Type "ree" into the omnibox.
(iii) Press the <Tab> key to navigate the dropdown.
(iv) Click on the OS desktop to cause the omnibox to lose focus.
(v) Switch back to the Chrome app.
(vi) Hit Escape.

Here's what's happening:
1. Step (iii) causes |has_temporary_text_| to be true.
2. Step (iv) causes StopAutocomplete(), leading to OnResultChanged().
   popup_->IsOpen() returns false right away (at the start of the method), so
   |was_open| is false. Since the popup is neither currently open, nor was
   open, |has_temporary_text_| isn't cleared. This is the bug.
3. Step (vi) causes InfoForCurrentSelection() to crash.

This CL fixes things so that |was_open| is set to true correctly, by ensuring
that IsOpen() is updated only when asked to (instead of live-updating). This is
the way it's done in the other omnibox popup implementations as well.

I've verified that the newly added Instant browsertest fails (crashes in the
same way as in the bug report) without the fix and passes with the fix.

BUG=222518
R=pkasting@chromium.org
TEST=See repro steps above.

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

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

3 files changed, 43 insertions, 2 deletions

diff --git a/chrome/browser/ui/omnibox/omnibox_popup_non_view.cc b/chrome/browser/ui/omnibox/omnibox_popup_non_view.cc
index 288ee11..4c22d7e 100644
--- a/chrome/browser/ui/omnibox/omnibox_popup_non_view.cc
+++ b/chrome/browser/ui/omnibox/omnibox_popup_non_view.cc
@@ -8,20 +8,22 @@
 #include "ui/gfx/rect.h"
 
 OmniboxPopupNonView::OmniboxPopupNonView(OmniboxEditModel* edit_model)
-  : model_(this, edit_model) {
+  : model_(this, edit_model),
+    is_open_(false) {
 }
 
 OmniboxPopupNonView::~OmniboxPopupNonView() {
 }
 
 bool OmniboxPopupNonView::IsOpen() const {
-  return !model_.result().empty();
+  return is_open_;
 }
 
 void OmniboxPopupNonView::InvalidateLine(size_t line) {
 }
 
 void OmniboxPopupNonView::UpdatePopupAppearance() {
+  is_open_ = !model_.result().empty();
 }
 
 gfx::Rect OmniboxPopupNonView::GetTargetBounds() {
diff --git a/chrome/browser/ui/omnibox/omnibox_popup_non_view.h b/chrome/browser/ui/omnibox/omnibox_popup_non_view.h
index 9d6a32a..5e9b24b 100644
--- a/chrome/browser/ui/omnibox/omnibox_popup_non_view.h
+++ b/chrome/browser/ui/omnibox/omnibox_popup_non_view.h
@@ -32,6 +32,14 @@ class OmniboxPopupNonView : public OmniboxPopupView {
  private:
   OmniboxPopupModel model_;
 
+  // |is_open_| reflects whether the OmniboxEditModel has a non-empty result
+  // set. However, we can't get rid of this variable and just use the model's
+  // state directly, as there's at least one situation when the model's result
+  // set has been emptied, but the popup should still be considered to be open
+  // until UpdatePopupAppearance() is called (cf: |was_open| in
+  // OmniboxEditModel::OnResultChanged()).
+  bool is_open_;
+
   DISALLOW_COPY_AND_ASSIGN(OmniboxPopupNonView);
 };
 
diff --git a/chrome/browser/ui/search/instant_extended_browsertest.cc b/chrome/browser/ui/search/instant_extended_browsertest.cc
index e72ef8d5..0ccf9eb 100644
--- a/chrome/browser/ui/search/instant_extended_browsertest.cc
+++ b/chrome/browser/ui/search/instant_extended_browsertest.cc
@@ -1586,3 +1586,34 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest, HasBookmarkProvider) {
 
   EXPECT_TRUE(found_bookmark_match);
 }
+
+// Test that the omnibox's temporary text is reset when the popup is closed.
+IN_PROC_BROWSER_TEST_F(InstantExtendedTest, TemporaryTextResetWhenPopupClosed) {
+  ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
+  FocusOmniboxAndWaitForInstantExtendedSupport();
+  EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
+
+  // Show the overlay and arrow-down to a suggestion (this sets temporary text).
+  SetOmniboxTextAndWaitForOverlayToShow("juju");
+  SendDownArrow();
+
+  EXPECT_TRUE(HasTemporaryText());
+  EXPECT_EQ("result 1", GetOmniboxText());
+
+  // Click outside the omnibox (but not on the overlay), to make the omnibox
+  // lose focus. Close the popup explicitly, to workaround test/toolkit issues.
+  ui_test_utils::ClickOnView(browser(), VIEW_ID_TOOLBAR);
+  omnibox()->CloseOmniboxPopup();
+
+  // The temporary text should've been accepted as the user text.
+  EXPECT_FALSE(HasTemporaryText());
+  EXPECT_EQ("result 1", GetOmniboxText());
+
+  // Now refocus the omnibox and hit Escape. This shouldn't crash.
+  FocusOmnibox();
+  SendEscape();
+
+  // The omnibox should've reverted to the underlying permanent URL.
+  EXPECT_FALSE(HasTemporaryText());
+  EXPECT_EQ(std::string(chrome::kAboutBlankURL), GetOmniboxText());
+}