Fix a DCHECK failure in AutocompleteEditModel::OnPopupDataChanged()

chrome/browser/autocomplete/autocomplete_edit.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_edit.h"
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/metrics/histogram.h"
@@ skipping 587 matching lines @@
       // user's selection gets screwy; and if we don't update the popup, and the
       // user reverts, then the selected item will be as if control is still
       // pressed, even though maybe it isn't any more.  There is no obvious
       // right answer here :(
     }
     view_->OnTemporaryTextMaybeChanged(DisplayTextFromUserText(text),
                                        save_original_selection);
     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;
 
-  // 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();
+  // |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;
+  }
+
+  // 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(
     const std::wstring& new_text,
     bool selection_differs,
     bool text_differs,
     bool just_deleted_text,
     bool allow_keyword_ui_change) {
   // Update the paste state as appropriate: if we're just finishing a paste
   // that replaced all the text, preserve that information; otherwise, if we've
@@ skipping 161 matching lines @@
 //  static
 bool AutocompleteEditModel::IsSpaceCharForAcceptingKeyword(wchar_t c) {
   switch (c) {
     case 0x0020:  // Space
     case 0x3000:  // Ideographic Space
       return true;
     default:
       return false;
   }
 }