Fix a DCHECK failure in AutocompleteEditModel::OnPopupDataChanged()

chrome/browser/autocomplete/autocomplete_popup_model.cc

Index: chrome/browser/autocomplete/autocomplete_popup_model.cc
diff --git a/chrome/browser/autocomplete/autocomplete_popup_model.cc b/chrome/browser/autocomplete/autocomplete_popup_model.cc
index 089f603fcbb48a0fbd50c12bf3064c3dce989c0a..27db0fb12a2373d414f777e59925c572f4ad9405 100644
--- a/chrome/browser/autocomplete/autocomplete_popup_model.cc
+++ b/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,35 @@ void AutocompletePopupModel::TryDeletingCurrentItem() {
       controller_->result().match_at(selected_line_);
   if (match.deletable) {
     const size_t selected_line = selected_line_;
+    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.

[Peter Kasting, 2011/01/25 21:18:24]

I am confused.  The entire point of the DCHECK() added to
AutocompleteController::DeleteMatch() is to document the invariant that if you
ask a match to be deleted, the provider _must_ delete it and update the result
set.  Given that, AutocompletePopupModel::Observe() must get called, and this
business with |old_url| will never be necessary.

I think you should omit everything that refers to |old_url|.

[James Su, 2011/01/25 21:47:03]

According to HistoryProvider::DeleteMatch(), the match will not really be
deleted if it's a What-You-Typed or Bookmark match, though the match will be
updated to be looked like it has no backing data. And in this case the latest
result will still be updated to make sure the popup can be updated accordingly.
But we probably don't want to update the edit, as the match is actually still
there.

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

You're missing my point.  There are two possible cases where the user is trying
to delete (WLOG) the What You Typed match:

1) That match is the default and the user is deleting it without having made a
prior manual selection.
2) That match may or may not be the default and the user has made a manual
selection.

Because the result set is always updated, the controller will notify the popup
and edit of changes in both cases.  That means in both cases, after we call
DeleteMatch() here, the edit and popup will have been reset to the default
match, no temporary text.  In case (1), we want to stop there.  In case (2), we
want to force-select the same line to put us back in temporary text mode.

Thus, the check for (was_temporary_text || (selected_line != selected_line_)) is
sufficient to handle this correctly.  There's no need to store and check the
default match URL.

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

For case (1), my point is: if the match is really deleted then the default match
is actually changed, then we should convert into temporary text mode, because
this change is made by the user manually. But anyway I don't have strong opinion
here. I'm ok to change the code, if you insist that we should stay in normal
mode for this case.

+    // 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;
+
     controller_->DeleteMatch(match);  // This may synchronously notify us that
-                                      // the results have changed.
+                                      // the results have changed. And may

[Peter Kasting, 2011/01/25 21:18:24]

Nit: If you really want to add this second piece of the comment, I'd make it a
full-line comment and join it into one sentence, since it's getting long for an
EOL comment.  Otherwise, you can just leave it out.

[James Su, 2011/01/25 21:47:03]

Done.

+                                      // cause edit model to revert the
+                                      // temporary text automatically.
     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);
     }
   }
 }
@@ -311,6 +330,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.