Fix a DCHECK failure in AutocompleteEditModel::OnPopupDataChanged()

BUG=70343
TEST=See bug reports.

Review URL: http://codereview.chromium.org/6340012

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

chrome/browser/autocomplete/autocomplete.cc

@@ -802,9 +802,11 @@ void AutocompleteController::DeleteMatch(const AutocompleteMatch& match) {
   DCHECK(match.deletable);
   match.provider->DeleteMatch(match);  // This may synchronously call back to
                                        // OnProviderUpdate().
+  DCHECK(updated_latest_result_);
   CommitResult(true);  // Ensure any new result gets committed immediately.  If
                        // it was committed already or hasn't been modified, this
-                       // is harmless.
+                       // is harmless. We need to notify the edit box, because
+                       // the default match may have been changed.
 }
 
 void AutocompleteController::CommitIfQueryHasNeverBeenCommitted() {

chrome/browser/autocomplete/autocomplete_edit.cc

@@ -486,13 +486,7 @@ bool AutocompleteEditModel::OnEscapeKeyPressed() {
     AutocompleteMatch match;
     popup_->InfoForCurrentSelection(&match, NULL);
     if (match.destination_url != original_url_) {
-      // The user typed something, then selected a different item.  Restore the
-      // text they typed and change back to the default item.
-      // NOTE: This purposefully does not reset paste_state_.
-      just_deleted_text_ = false;
-      has_temporary_text_ = false;
-      popup_->ResetToDefaultMatch();
-      view_->OnRevertTemporaryText();
+      RevertTemporaryText(true);
       return true;
     }
   }
@@ -599,20 +593,27 @@ void AutocompleteEditModel::OnPopupDataChanged(
     return;
   }
 
-  // All cases that can result in |has_temporary_text_| being set should have
-  // been handled by the conditional above.
-  DCHECK(!has_temporary_text_);
-
+  bool call_controller_onchanged = true;
   inline_autocomplete_text_ = text;
   if (view_->OnInlineAutocompleteTextMaybeChanged(
       DisplayTextFromUserText(user_text_ + inline_autocomplete_text_),
       DisplayTextFromUserText(user_text_).length()))
-    return;
+    call_controller_onchanged = false;
 
-  // All other code paths that return invoke OnChanged. We need to invoke
-  // OnChanged in case the destination url changed (as could happen when control
-  // is toggled).
-  controller_->OnChanged();
+  // If |has_temporary_text_| is true, then we previously had a manual selection
+  // but now don't (or |destination_for_temporary_text_change| would have been
+  // non-NULL). This can happen when deleting the selected item in the popup.
+  // In this case, we've already reverted the popup to the default match, so we
+  // need to revert ourselves as well.
+  if (has_temporary_text_) {
+    RevertTemporaryText(false);
+    call_controller_onchanged = false;
+  }
+
+  // We need to invoke OnChanged in case the destination url changed (as could
+  // happen when control is toggled).
+  if (call_controller_onchanged)
+    controller_->OnChanged();
 }
 
 bool AutocompleteEditModel::OnAfterPossibleChange(
@@ -663,11 +664,8 @@ bool AutocompleteEditModel::OnAfterPossibleChange(
   // Change to keyword mode if the user has typed a keyword name and is now
   // pressing space after the name. Accepting the keyword will update our
   // state, so in that case there's no need to also return true here.
-  if (text_differs && allow_keyword_ui_change && !just_deleted_text &&
-      MaybeAcceptKeywordBySpace(old_user_text, user_text_))
-    return false;
-
-  return true;
+  return !(text_differs && allow_keyword_ui_change && !just_deleted_text &&
+           MaybeAcceptKeywordBySpace(old_user_text, user_text_));
 }
 
 void AutocompleteEditModel::PopupBoundsChangedTo(const gfx::Rect& bounds) {
@@ -781,6 +779,17 @@ bool AutocompleteEditModel::GetURLForText(const string16& text,
   return true;
 }
 
+void AutocompleteEditModel::RevertTemporaryText(bool revert_popup) {
+  // The user typed something, then selected a different item.  Restore the
+  // text they typed and change back to the default item.
+  // NOTE: This purposefully does not reset paste_state_.
+  just_deleted_text_ = false;
+  has_temporary_text_ = false;
+  if (revert_popup)
+    popup_->ResetToDefaultMatch();
+  view_->OnRevertTemporaryText();
+}
+
 bool AutocompleteEditModel::MaybeAcceptKeywordBySpace(
     const string16& old_user_text,
     const string16& new_user_text) {

chrome/browser/autocomplete/autocomplete_edit.h

@@ -378,6 +378,10 @@ class AutocompleteEditModel : public NotificationObserver {
   // copy.
   bool GetURLForText(const string16& text, GURL* url) const;
 
+  // Reverts the edit box from a temporary text back to the original user text.
+  // If |revert_popup| is true then the popup will be reverted as well.
+  void RevertTemporaryText(bool revert_popup);
+
   // Accepts current keyword if the user only typed a space at the end of
   // |new_user_text| comparing to the |old_user_text|.
   // Returns true if the current keyword is accepted.

chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc

@@ -96,15 +96,15 @@ const struct TestHistoryEntry {
   int typed_count;
   bool starred;
 } kHistoryEntries[] = {
-  {"http://www.bar.com/1", "Page 1", kSearchText, 1, 1, false },
-  {"http://www.bar.com/2", "Page 2", kSearchText, 1, 1, false },
-  {"http://www.bar.com/3", "Page 3", kSearchText, 1, 1, false },
-  {"http://www.bar.com/4", "Page 4", kSearchText, 1, 1, false },
-  {"http://www.bar.com/5", "Page 5", kSearchText, 1, 1, false },
-  {"http://www.bar.com/6", "Page 6", kSearchText, 1, 1, false },
-  {"http://www.bar.com/7", "Page 7", kSearchText, 1, 1, false },
-  {"http://www.bar.com/8", "Page 8", kSearchText, 1, 1, false },
-  {"http://www.bar.com/9", "Page 9", kSearchText, 1, 1, false },
+  {"http://www.bar.com/1", "Page 1", kSearchText, 10, 10, false },
+  {"http://www.bar.com/2", "Page 2", kSearchText, 9, 9, false },
+  {"http://www.bar.com/3", "Page 3", kSearchText, 8, 8, false },
+  {"http://www.bar.com/4", "Page 4", kSearchText, 7, 7, false },
+  {"http://www.bar.com/5", "Page 5", kSearchText, 6, 6, false },
+  {"http://www.bar.com/6", "Page 6", kSearchText, 5, 5, false },
+  {"http://www.bar.com/7", "Page 7", kSearchText, 4, 4, false },
+  {"http://www.bar.com/8", "Page 8", kSearchText, 3, 3, false },
+  {"http://www.bar.com/9", "Page 9", kSearchText, 2, 2, false },
 
   // To trigger inline autocomplete.
   {"http://www.def.com", "Page def", kSearchText, 10000, 10000, true },
@@ -244,6 +244,13 @@ class AutocompleteEditViewTest : public InProcessBrowserTest,
     }
 
     ASSERT_TRUE(model->loaded());
+    // Remove built-in template urls, like google.com, bing.com etc., as they
+    // may appear as autocomplete suggests and interfere with our tests.
+    model->SetDefaultSearchProvider(NULL);
+    TemplateURLModel::TemplateURLVector builtins = model->GetTemplateURLs();
+    for (TemplateURLModel::TemplateURLVector::const_iterator
+         i = builtins.begin(); i != builtins.end(); ++i)
+      model->Remove(*i);
 
     TemplateURL* template_url = new TemplateURL();
     template_url->SetURL(kSearchURL, 0, 0);
@@ -841,6 +848,102 @@ class AutocompleteEditViewTest : public InProcessBrowserTest,
     ASSERT_EQ("http://abc.com/",
               popup_model->result().default_match()->destination_url.spec());
   }
+
+  void DeleteItemTest() {
+    // Disable the search provider, to make sure the popup contains only history
+    // items.
+    TemplateURLModel* model = browser()->profile()->GetTemplateURLModel();
+    model->SetDefaultSearchProvider(NULL);
+
+    ui_test_utils::NavigateToURL(browser(), GURL(chrome::kAboutBlankURL));
+    browser()->FocusLocationBar();
+
+    AutocompleteEditView* edit_view = NULL;
+    ASSERT_NO_FATAL_FAILURE(GetAutocompleteEditView(&edit_view));
+
+    AutocompletePopupModel* popup_model = edit_view->model()->popup_model();
+    ASSERT_TRUE(popup_model);
+
+    string16 old_text = edit_view->GetText();
+
+    // Input something that can match history items.
+    edit_view->SetUserText(ASCIIToUTF16("bar"));
+    ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
+    ASSERT_TRUE(popup_model->IsOpen());
+
+    // Delete the inline autocomplete part.
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_DELETE, false, false, false));
+    string16 user_text = edit_view->GetText();
+    ASSERT_EQ(ASCIIToUTF16("bar"), user_text);
+    edit_view->SelectAll(true);
+    ASSERT_TRUE(edit_view->IsSelectAll());
+
+    // The first item should be the default match.
+    size_t default_line = popup_model->selected_line();
+    std::string default_url =
+        popup_model->result().match_at(default_line).destination_url.spec();
+
+    // Move down.
+    edit_view->model()->OnUpOrDownKeyPressed(1);
+    ASSERT_EQ(default_line + 1, popup_model->selected_line());
+    string16 selected_text =
+        popup_model->result().match_at(default_line + 1).fill_into_edit;
+    // Temporary text is shown.
+    ASSERT_EQ(selected_text, edit_view->GetText());
+    ASSERT_FALSE(edit_view->IsSelectAll());
+
+    // Delete the item.
+    popup_model->TryDeletingCurrentItem();
+    ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
+    // The selected line shouldn't be changed, because we have more than two
+    // items.
+    ASSERT_EQ(default_line + 1, popup_model->selected_line());
+    // Make sure the item is really deleted.
+    ASSERT_NE(selected_text,
+              popup_model->result().match_at(default_line + 1).fill_into_edit);
+    selected_text =
+        popup_model->result().match_at(default_line + 1).fill_into_edit;
+    // New temporary text is shown.
+    ASSERT_EQ(selected_text, edit_view->GetText());
+
+    // Revert to the default match.
+    ASSERT_TRUE(edit_view->model()->OnEscapeKeyPressed());
+    ASSERT_EQ(default_line, popup_model->selected_line());
+    ASSERT_EQ(user_text, edit_view->GetText());
+    ASSERT_TRUE(edit_view->IsSelectAll());
+
+    // Move down and up to select the default match as temporary text.
+    edit_view->model()->OnUpOrDownKeyPressed(1);
+    ASSERT_EQ(default_line + 1, popup_model->selected_line());
+    edit_view->model()->OnUpOrDownKeyPressed(-1);
+    ASSERT_EQ(default_line, popup_model->selected_line());
+
+    selected_text = popup_model->result().match_at(default_line).fill_into_edit;
+    // New temporary text is shown.
+    ASSERT_EQ(selected_text, edit_view->GetText());
+    ASSERT_FALSE(edit_view->IsSelectAll());
+
+    // Delete the default item.
+    popup_model->TryDeletingCurrentItem();
+    ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
+    // The selected line shouldn't be changed, but the default item should have
+    // been changed.
+    ASSERT_EQ(default_line, popup_model->selected_line());
+    // Make sure the item is really deleted.
+    ASSERT_NE(selected_text,
+              popup_model->result().match_at(default_line).fill_into_edit);
+    selected_text =
+        popup_model->result().match_at(default_line).fill_into_edit;
+    // New temporary text is shown.
+    ASSERT_EQ(selected_text, edit_view->GetText());
+
+    // As the current selected item is the new default item, pressing Escape key
+    // should revert all directly.
+    ASSERT_TRUE(edit_view->model()->OnEscapeKeyPressed());
+    ASSERT_EQ(old_text, edit_view->GetText());
+    ASSERT_TRUE(edit_view->IsSelectAll());
+  }
+
 };
 
 // Test if ctrl-* accelerators are workable in omnibox.
@@ -898,6 +1001,10 @@ IN_PROC_BROWSER_TEST_F(AutocompleteEditViewTest, NonSubstitutingKeywordTest) {
   NonSubstitutingKeywordTest();
 }
 
+IN_PROC_BROWSER_TEST_F(AutocompleteEditViewTest, DeleteItem) {
+  DeleteItemTest();
+}
+
 #if defined(OS_LINUX)
 // TODO(oshima): enable these tests for views-implmentation when
 // these featuers are supported.
@@ -1079,4 +1186,8 @@ IN_PROC_BROWSER_TEST_F(AutocompleteEditViewViewsTest,
   NonSubstitutingKeywordTest();
 }
 
+IN_PROC_BROWSER_TEST_F(AutocompleteEditViewViewsTest, DeleteItem) {
+  DeleteItemTest();
+}
+
 #endif

chrome/browser/autocomplete/autocomplete_popup_model.cc

@@ -90,7 +90,8 @@ void AutocompletePopupModel::SetHoveredLine(size_t line) {
 }
 
 void AutocompletePopupModel::SetSelectedLine(size_t line,
-                                             bool reset_to_default) {
+                                             bool reset_to_default,
+                                             bool force) {
   // We should at least be dealing with the results of the current query.  Note
   // that even if |line| was valid on entry, this may make it invalid.  We clamp
   // it below.
@@ -115,7 +116,7 @@ void AutocompletePopupModel::SetSelectedLine(size_t line,
         match.is_history_what_you_typed_match;
   }
 
-  if (line == selected_line_)
+  if (line == selected_line_ && !force)
     return;  // Nothing else to do.
 
   // We need to update |selected_line_| before calling OnPopupDataChanged(), so
@@ -158,7 +159,7 @@ void AutocompletePopupModel::SetSelectedLine(size_t line,
 void AutocompletePopupModel::ResetToDefaultMatch() {
   const AutocompleteResult& result = controller_->result();
   CHECK(!result.empty());
-  SetSelectedLine(result.default_match() - result.begin(), true);
+  SetSelectedLine(result.default_match() - result.begin(), true, false);
   view_->OnDragCanceled();
 }
 
@@ -267,7 +268,7 @@ void AutocompletePopupModel::Move(int count) {
   // Clamp the new line to [0, result_.count() - 1].
   const size_t new_line = selected_line_ + count;
   SetSelectedLine(((count < 0) && (new_line >= selected_line_)) ? 0 : new_line,
-                  false);
+                  false, false);
 }
 
 void AutocompletePopupModel::TryDeletingCurrentItem() {
@@ -284,17 +285,21 @@ void AutocompletePopupModel::TryDeletingCurrentItem() {
       controller_->result().match_at(selected_line_);
   if (match.deletable) {
     const size_t selected_line = selected_line_;
-    controller_->DeleteMatch(match);  // This may synchronously notify us that
-                                      // the results have changed.
+    const bool was_temporary_text = !manually_selected_match_.empty();
+
+    // This will synchronously notify both the edit and us that the results
+    // have changed, causing both to revert to the default match.
+    controller_->DeleteMatch(match);
     const AutocompleteResult& result = controller_->result();
-    if (!result.empty()) {
+    if (!result.empty() &&
+        (was_temporary_text || selected_line != selected_line_)) {
       // Move the selection to the next choice after the deleted one.
       // SetSelectedLine() will clamp to take care of the case where we deleted
       // the last item.
       // TODO(pkasting): Eventually the controller should take care of this
       // before notifying us, reducing flicker.  At that point the check for
       // deletability can move there too.
-      SetSelectedLine(selected_line, false);
+      SetSelectedLine(selected_line, false, true);
     }
   }
 }
@@ -311,6 +316,7 @@ void AutocompletePopupModel::Observe(NotificationType type,
       kNoMatch : static_cast<size_t>(result->default_match() - result->begin());
   // There had better not be a nonempty result set with no default match.
   CHECK((selected_line_ != kNoMatch) || result->empty());
+  manually_selected_match_.Clear();
   // If we're going to trim the window size to no longer include the hovered
   // line, turn hover off.  Practically, this shouldn't happen, but it
   // doesn't hurt to be defensive.

chrome/browser/autocomplete/autocomplete_popup_model.h

@@ -70,10 +70,12 @@ class AutocompletePopupModel : public NotificationObserver {
   // new temporary text.  |line| will be clamped to the range of valid lines.
   // |reset_to_default| is true when the selection is being reset back to the
   // default match, and thus there is no temporary text (and no
-  // |manually_selected_match_|).
+  // |manually_selected_match_|). If |force| is true then the selected line will
+  // be updated forcibly even if the |line| is same as the current selected
+  // line.
   // NOTE: This assumes the popup is open, and thus both old and new values for
   // the selected line should not be kNoMatch.
-  void SetSelectedLine(size_t line, bool reset_to_default);
+  void SetSelectedLine(size_t line, bool reset_to_default, bool force);
 
   // Called when the user hits escape after arrowing around the popup.  This
   // will change the selected line back to the default match and redraw.

chrome/browser/autocomplete/autocomplete_popup_view_gtk.cc

@@ -513,7 +513,7 @@ gboolean AutocompletePopupViewGtk::HandleMotion(GtkWidget* widget,
   model_->SetHoveredLine(line);
   // Select the line if the user has the left mouse button down.
   if (!ignore_mouse_drag_ && (event->state & GDK_BUTTON1_MASK))
-    model_->SetSelectedLine(line, false);
+    model_->SetSelectedLine(line, false, false);
   return TRUE;
 }
 
@@ -524,7 +524,7 @@ gboolean AutocompletePopupViewGtk::HandleButtonPress(GtkWidget* widget,
   size_t line = LineFromY(static_cast<int>(event->y));
   model_->SetHoveredLine(line);
   if (event->button == 1)
-    model_->SetSelectedLine(line, false);
+    model_->SetSelectedLine(line, false, false);
   return TRUE;
 }
 

chrome/browser/autocomplete/autocomplete_popup_view_mac.mm

@@ -518,7 +518,7 @@ gfx::Rect AutocompletePopupViewMac::GetTargetBounds() {
 }
 
 void AutocompletePopupViewMac::SetSelectedLine(size_t line) {
-  model_->SetSelectedLine(line, false);
+  model_->SetSelectedLine(line, false, false);
 }
 
 // This is only called by model in SetSelectedLine() after updating

chrome/browser/search_engines/template_url_model.h

@@ -59,6 +59,7 @@ class TemplateURLModel : public WebDataServiceConsumer,
                          public NotificationObserver {
  public:
   typedef std::map<std::string, std::string> QueryTerms;
+  typedef std::vector<const TemplateURL*> TemplateURLVector;
 
   // Struct used for initializing the data store with fake data.
   // Each initializer is mapped to a TemplateURL.
@@ -157,7 +158,7 @@ class TemplateURLModel : public WebDataServiceConsumer,
 
   // Returns the set of URLs describing the keywords. The elements are owned
   // by TemplateURLModel and should not be deleted.
-  std::vector<const TemplateURL*> GetTemplateURLs() const;
+  TemplateURLVector GetTemplateURLs() const;
 
   // Increment the usage count of a keyword.
   // Called when a URL is loaded that was generated from a keyword.
@@ -258,7 +259,6 @@ class TemplateURLModel : public WebDataServiceConsumer,
   friend class TemplateURLModelTestUtil;
 
   typedef std::map<string16, const TemplateURL*> KeywordToTemplateMap;
-  typedef std::vector<const TemplateURL*> TemplateURLVector;
 
   // Helper functor for FindMatchingKeywords(), for finding the range of
   // keywords which begin with a prefix.

chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc

@@ -1030,7 +1030,7 @@ bool AutocompletePopupContentsView::OnMousePressed(
     size_t index = GetIndexForPoint(event.location());
     model_->SetHoveredLine(index);
     if (HasMatchAt(index) && event.IsLeftMouseButton())
-      model_->SetSelectedLine(index, false);
+      model_->SetSelectedLine(index, false, false);
   }
   return true;
 }
@@ -1056,7 +1056,7 @@ bool AutocompletePopupContentsView::OnMouseDragged(
     size_t index = GetIndexForPoint(event.location());
     model_->SetHoveredLine(index);
     if (!ignore_mouse_drag_ && HasMatchAt(index) && event.IsLeftMouseButton())
-      model_->SetSelectedLine(index, false);
+      model_->SetSelectedLine(index, false, false);
   }
   return true;
 }