Fix a DCHECK failure in AutocompleteEditModel::OnPopupDataChanged()

chrome/browser/autocomplete/autocomplete_popup_model.cc

 // Copyright (c) 2011 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/autocomplete/autocomplete_popup_model.h"
 
 #include <algorithm>
 
 #include "unicode/ubidi.h"
 
@@ skipping 72 matching lines @@
   if ((hovered_line_ != kNoMatch) && (hovered_line_ != selected_line_))
     view_->InvalidateLine(hovered_line_);
 
   // Change the hover to the new line.
   hovered_line_ = line;
   if (!is_disabling && (hovered_line_ != selected_line_))
     view_->InvalidateLine(hovered_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.
   controller_->CommitIfQueryHasNeverBeenCommitted();
 
   const AutocompleteResult& result = controller_->result();
   if (result.empty())
     return;
 
   // Cancel the query so the matches don't change on the user.
   controller_->Stop(false);
 
   line = std::min(line, result.size() - 1);
   const AutocompleteMatch& match = result.match_at(line);
   if (reset_to_default) {
     manually_selected_match_.Clear();
   } else {
     // Track the user's selection until they cancel it.
     manually_selected_match_.destination_url = match.destination_url;
     manually_selected_match_.provider_affinity = match.provider;
     manually_selected_match_.is_history_what_you_typed_match =
         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
   // that when the edit notifies its controller that something has changed, the
   // controller can get the correct updated data.
   //
   // NOTE: We should never reach here with no selected line; the same code that
   // opened the popup and made it possible to get here should have also set a
   // selected line.
   CHECK(selected_line_ != kNoMatch);
@@ skipping 22 matching lines @@
   }
 
   // Repaint old and new selected lines immediately, so that the edit doesn't
   // appear to update [much] faster than the popup.
   view_->PaintUpdatesNow();
 }
 
 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();
 }
 
 void AutocompletePopupModel::InfoForCurrentSelection(
     AutocompleteMatch* match,
     GURL* alternate_nav_url) const {
   DCHECK(match != NULL);
   const AutocompleteResult* result;
   if (!controller_->done()) {
     // NOTE: Using latest_result() is important here since not only could it
@@ skipping 88 matching lines @@
   if (result.empty())
     return;
 
   // The user is using the keyboard to change the selection, so stop tracking
   // hover.
   SetHoveredLine(kNoMatch);
 
   // 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() {
   // We could use InfoForCurrentSelection() here, but it seems better to try
   // and shift-delete the actual selection, rather than any "in progress, not
   // yet visible" one.
   if (selected_line_ == kNoMatch)
     return;
 
   // Cancel the query so the matches don't change on the user.
   controller_->Stop(false);
 
   const AutocompleteMatch& match =
       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();
+    GURL old_url;
+
+    // If the user tries to delete a match, which is not manually selected,
+    // then we should not re-select it if it's not deleted at all.
+    // Otherwise it will be selected as temporary text unexpectedly.
+    // We can check the url of the default match before and after calling
+    // |controller->DeleteMatch()| to see if it's actually deleted or not.
+    // But if the match is manually selected, then we should always re-select
+    // the selected line manually no matter if the match is actually deleted
+    // or not, to make sure we stay in the temporary text mode.
+    if (!was_temporary_text)
+      old_url = controller_->result().match_at(selected_line).destination_url;
+
+    // This may synchronously notify us that the results have changed. And may

[Peter Kasting, 2011/01/25 21:56:44]

Nit: Given the DCHECK in DeleteMatch(), it would probably be more accurate to
say:

    // This will synchronously notify both the edit and us that the results have
    // changed, causing both to revert to the default match.

[James Su, 2011/01/25 22:32:37]

Done.

+    // cause edit model to revert the temporary text automatically.
+    controller_->DeleteMatch(match);
     const AutocompleteResult& result = controller_->result();
-    if (!result.empty()) {
+    if (!result.empty() &&
+        (was_temporary_text || selected_line != selected_line_ ||
+         old_url != result.match_at(selected_line).destination_url)) {
       // 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);
     }
   }
 }
 
 void AutocompletePopupModel::Observe(NotificationType type,
                                      const NotificationSource& source,
                                      const NotificationDetails& details) {
   DCHECK_EQ(NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED,
             type.value);
 
   const AutocompleteResult* result =
       Details<const AutocompleteResult>(details).ptr();
   selected_line_ = result->default_match() == result->end() ?
       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.
   if ((hovered_line_ != kNoMatch) && (result->size() <= hovered_line_))
     SetHoveredLine(kNoMatch);
 
   const bool was_open = view_->IsOpen();
   view_->UpdatePopupAppearance();
   if (view_->IsOpen())
     edit_model_->PopupBoundsChangedTo(view_->GetTargetBounds());
   else if (was_open)
     edit_model_->OnPopupClosed();
 }
 
 const SkBitmap* AutocompletePopupModel::GetSpecialIconForMatch(
     const AutocompleteMatch& match) const {
   if (!match.template_url || !match.template_url->IsExtensionKeyword())
     return NULL;
 
   return &profile_->GetExtensionService()->GetOmniboxPopupIcon(
       match.template_url->GetExtensionId());
 }