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 104 matching lines @@
         match.is_history_what_you_typed_match;
   }
 
   if (line == selected_line_)
     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);
-  GURL current_destination(result.match_at(selected_line_).destination_url);
-  view_->InvalidateLine(selected_line_);
+  // |selected_line_| could be kNoMatch only when the previously selected line
+  // has just been deleted. In this case, the |edit_model_| has already been
+  // reverted to the default match, so we need to use the default match for
+  // |current_destination|, to make sure |edit_model_| can record it properly.
+  // This step is necessary because the default match may have been changed if
+  // the deleted item was the default match.
+  GURL current_destination;
+  if (selected_line_ != kNoMatch) {
+    current_destination = result.match_at(selected_line_).destination_url;
+    view_->InvalidateLine(selected_line_);
+  } else {
+    // We must have a default match if we get here.
+    CHECK(result.default_match() != result.end());
+    current_destination = result.default_match()->destination_url;
+  }
   selected_line_ = line;
   view_->InvalidateLine(selected_line_);
 
   // Update the edit with the new data for this match.
   // TODO(pkasting): If |selected_line_| moves to the controller, this can be
   // eliminated and just become a call to the observer on the edit.
   std::wstring keyword;
   const bool is_keyword_hint = GetKeywordForMatch(match, &keyword);
   if (reset_to_default) {
     std::wstring inline_autocomplete_text;
@@ skipping 139 matching lines @@
   // 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 AutocompleteResult& result = controller_->result();
     if (!result.empty()) {

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

If I'm not mistaken, we can change this conditional to be more restrictive:

    if (!result.empty() && (selected_line_ != selected_line)) {

...because if the two are equal, then we're in one of two cases:

(1) Deleting the match didn't cause any change to our results at all, so the
controller never notified anyone of any changes.  In this case, nothing has
changed so it's safe to take no further action.

(2) We deleted the match and called the various notifications and thus have
switched to the new result set's default match.  It turns out this match is the
one we want to select anyway, so since we already have the right match selected,
we can safely do nothing further.

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

It has an issue for the deleting default match case. If we don't do the update
here, when the default match gets deleted, the new default match may be selected
as if it's reverted from a temporary text, i.e. the user text will be reverted
to the original user typed text, rather than being selected as a new temporary
text. I think we should stay in temporary text mode in this case, as the new
default match is actually selected manually.

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

But your code has an opposite problem: If we don't have a manually selected
match already, and calling DeleteMatch() does nothing to the result set, then
the user's text is changed to the temporary text (as if we'd arrowed down and
back up).

Perhaps we should make AutocompleteController::DeleteMatch() return a bool to
indicate whether it did anything, and use that to avoid this case.  It can do
this by storing the value of |updated_latest_result_| before it calls
CommitResult().

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

Isn't that guaranteed by above if (match.deletable) block? I'll do this change
if it's possible that deleting a deletable item may fail.

[Peter Kasting, 2011/01/25 04:23:20]

Why don't we do this: Add a DCHECK(updated_latest_result_) into DeleteMatch()
before the controller calls CommitResult() and don't bother adding a return
value.

Semantically, providers are _supposed_ to do something to force an update when
they delete a match, but right now we don't enforce that, so that's basically
what the DCHECK asserts.

[James Su, 2011/01/25 05:30:11]

I just checked the source code, and yes it's probably necessary to add a
DCHECK(updated_latest_result_) to avoid potential problem. But the history
provider may update the match without deleting it, so in such case your previous
statement makes sense that we shouldn't forcibly select this match as temporary
text. 

+      // Set |selected_line_| to kNoMatch to forcely select the line, even if

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

I think if we change the conditional as I note above, then we don't need this
change or any of the modifications in SetSelectedLine().

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

See above.

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

OK, but I still think this can be done more cleanly.  Instead of this statement
or any of your current modifications to SetSelectedLine(), you can simply add a
third arg, "bool force_update_even_if_same_line", set it true here and false
elsewhere, and change "if (line == selected_line_)" in SetSelectedLine() to "if
(!force_update_even_if_same_line && (line == selected_line_))".  This should
work because Observe() has ensures that |selected_line_| is set correctly to the
new default match and thus we don't need special-case code in SetSelectedLine()
to get the right destination for this case.

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

Ok I'll do it. It's actually my original approach. I'm just hesitated to add
another bool parameter to this method :)

+      // the value is not changed at all.
+      selected_line_ = kNoMatch;
       // 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);
     }
   }
 }
 
 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());
+  // We need to clear |manually_selected_match_|, as |selected_line_| has been
+  // reverted to the default match.

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

Nit: I don't think this comment is necessary; it's clear we need to keep these
two variables in sync.

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

Done.

+  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);
 
   view_->UpdatePopupAppearance();
   edit_model_->PopupBoundsChangedTo(view_->GetTargetBounds());
 }
 
 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());
 }