Fix a DCHECK failure in AutocompleteEditModel::OnPopupDataChanged()

chrome/browser/autocomplete/autocomplete_edit.cc

Index: chrome/browser/autocomplete/autocomplete_edit.cc
diff --git a/chrome/browser/autocomplete/autocomplete_edit.cc b/chrome/browser/autocomplete/autocomplete_edit.cc
index 1ad993eeb638778287a9157989000b414948c009..13a6f158be7193871ef4e72f078e3decde952458 100644
--- a/chrome/browser/autocomplete/autocomplete_edit.cc
+++ b/chrome/browser/autocomplete/autocomplete_edit.cc
@@ -605,20 +605,29 @@ 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;
 

[Peter Kasting, 2011/01/24 23:25:52]

Nit: Unnecessary blank line

[James Su, 2011/01/25 02:24:55]

Done.

   inline_autocomplete_text_ = text;
   if (view_->OnInlineAutocompleteTextMaybeChanged(
       DisplayTextFromUserText(user_text_ + inline_autocomplete_text_),
       DisplayTextFromUserText(user_text_).length()))
-    return;
+    call_controller_onchanged = false;
+
+  // |has_temporary_text_| is true here means we have been reverted to the

[Peter Kasting, 2011/01/24 23:25:52]

Nit: How about:

  // 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.

[James Su, 2011/01/25 02:24:55]

Done.

+  // default match, so we need to ask view to revert as well. We need to do it
+  // after updating the display text, to make sure the selection can be reverted
+  // correctly.
+  // This route can only be triggered when deleting the selected item in popup.
+  if (has_temporary_text_) {
+    has_temporary_text_ = false;

[Peter Kasting, 2011/01/24 23:25:52]

Let's refactor a little:

[private] void RevertTemporaryText() {
  just_deleted_text_ = false;
  has_temporary_text_ = false;
  view_->OnRevertTemporaryText();
}

Then call this function here as well as in OnEscapeKeyPressed() (which will
still also need to call popup_->ResetToDefaultMatch()).

[James Su, 2011/01/25 02:24:55]

It's actually not possible. In OnEscapeKeyPressed(), we need to call
popup_->ResetToDefaultMatch() between has_temporary_text_ = false and
view_->OnRevertTemporaryText().

[Peter Kasting, 2011/01/25 02:36:20]

In that case, let's refactor and make that call a conditional part of the
refactored function.

I am very keen to have places that are doing "the same thing" actually run
through the same code.

[James Su, 2011/01/25 02:51:51]

Ok, I'll update it asap.

+    view_->OnRevertTemporaryText();
+    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();
+  // 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(