Omnibox: Log Elapsed Time Since Last Time The Default Match Changed

chrome/browser/autocomplete/autocomplete_controller.cc

 // Copyright (c) 2012 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_controller.h"
 
 #include <set>
 #include <string>
 
 #include "base/command_line.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/string_number_conversions.h"
 #include "base/stringprintf.h"
 #include "base/time.h"
+#include "base/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_controller_delegate.h"
 #include "chrome/browser/autocomplete/bookmark_provider.h"
 #include "chrome/browser/autocomplete/builtin_provider.h"
 #include "chrome/browser/autocomplete/extension_app_provider.h"
 #include "chrome/browser/autocomplete/history_contents_provider.h"
 #include "chrome/browser/autocomplete/history_quick_provider.h"
 #include "chrome/browser/autocomplete/history_url_provider.h"
 #include "chrome/browser/autocomplete/keyword_provider.h"
 #include "chrome/browser/autocomplete/search_provider.h"
 #include "chrome/browser/autocomplete/shortcuts_provider.h"
@@ skipping 179 matching lines @@
   if (matches_requested == AutocompleteInput::ALL_MATCHES &&
       (text.length() < 6)) {
     base::TimeTicks end_time = base::TimeTicks::Now();
     std::string name = "Omnibox.QueryTime." + base::IntToString(text.length());
     base::Histogram* counter = base::Histogram::FactoryGet(
         name, 1, 1000, 50, base::Histogram::kUmaTargetedHistogramFlag);
     counter->Add(static_cast<int>((end_time - start_time).InMilliseconds()));
   }
   in_start_ = false;
   CheckIfDone();
-  UpdateResult(true);
+  UpdateResult(false, true);  // force saying the default match has changed

[Peter Kasting, 2012/11/16 23:57:18]

Nit: Comment adds nothing over the function declaration.  Say why we want to
force notification here.  (Something like what I said in my email to you
yesterday.)

[Mark P, 2012/11/19 22:21:45]

Expanded greatly, new with useful information (largely pasted from your e-mail).

 
   if (!done_)
     StartExpireTimer();
 }
 
 void AutocompleteController::Stop(bool clear_result) {
   for (ACProviders::const_iterator i(providers_.begin()); i != providers_.end();
        ++i) {
     (*i)->Stop(clear_result);
   }
@@ skipping 29 matching lines @@
   DCHECK(match.deletable);
   match.provider->DeleteMatch(match);  // This may synchronously call back to
                                        // OnProviderUpdate().
   // If DeleteMatch resulted in a callback to OnProviderUpdate and we're
   // not done, we might attempt to redisplay the deleted match. Make sure
   // we aren't displaying it by removing any old entries.
   ExpireCopiedEntries();
 }
 
 void AutocompleteController::ExpireCopiedEntries() {
-  // Clear out the results. This ensures no results from the previous result set
-  // are copied over.
-  result_.Reset();
-  // We allow matches from the previous result set to starve out matches from
-  // the new result set. This means in order to expire matches we have to query
-  // the providers again.
-  UpdateResult(false);
+  // The first true makes UpdateResult() clear out the results.  This
+  // ensures no results from the previous result set are copied over.
+  // (We allow matches from the previous result set to starve out
+  // matches from the new result set. This means in order to expire
+  // matches we have to query the providers again.)

[Peter Kasting, 2012/11/16 23:57:18]

I'm not sure this parenthetical comment really belongs here.

I think maybe we want this on the UpdateResult() declaration, or maybe in the
definition.  Basically we want to note that any existing result may contain (a)
stale matches from the previous result set and (b) these results may have
actually outranked legitimate matches from the current result set, so to get
just the current matches, it's easiest to simply throw everything out and then
reconstruct a result set from the providers' current data.

[Mark P, 2012/11/19 22:21:45]

Moved to the UpdateResults declaration, largely rewritten per your suggestion.

+  // The second true says to pretend the default match changed (even
+  // if it did not).

[Peter Kasting, 2012/11/16 23:57:18]

Again, this just says what we're doing; we need to instead say why.

(I'm not actually 100% sure we need to force-notify in this case.  I know we
used to but that may have been unnecessary.)

[Mark P, 2012/11/19 22:21:45]

Added somewhat of an explanation, largely culled from your e-mail.

+  UpdateResult(true, true);
 }
 
 void AutocompleteController::OnProviderUpdate(bool updated_matches) {
   if (in_zero_suggest_) {
     // We got ZeroSuggest results before Start(). Show only those results,
     // because results from other providers are stale.
     result_.Reset();
     result_.AppendMatches(zero_suggest_provider_->matches());
     result_.SortAndCull(input_, profile_);
     NotifyChanged(true);
   } else {
     CheckIfDone();
-    // Multiple providers may provide synchronous results, so we only update the
-    // results if we're not in Start().
+    // Multiple providers may provide synchronous results, so we only
+    // update the results if we're not in Start().

[Peter Kasting, 2012/11/16 23:57:18]

Nit: Line wrapping change unnecessary

[Mark P, 2012/11/19 22:21:45]

Fine, put back.

     if (!in_start_ && (updated_matches || done_))
-      UpdateResult(false);
+      UpdateResult(false, false);
   }
 }
 
 void AutocompleteController::AddProvidersInfo(
     ProvidersInfo* provider_info) const {
   provider_info->clear();
   for (ACProviders::const_iterator i(providers_.begin()); i != providers_.end();
        ++i) {
     // Add per-provider info, if any.
     (*i)->AddProviderInfo(provider_info);
 
     // This is also a good place to put code to add info that you want to
     // add for every provider.
   }
 }
 
-void AutocompleteController::UpdateResult(bool is_synchronous_pass) {
+void AutocompleteController::UpdateResult(
+    bool regenerate_result, bool force_notify_default_match_changed) {

[Peter Kasting, 2012/11/16 23:57:18]

Nit: One arg per line (your style is legal, but doesn't match the autocomplete
code style)

[Mark P, 2012/11/19 22:21:45]

Okay, fixed.

+  const bool last_default_was_valid = result_.default_match() != result_.end();
+  // The following two variables are only set and used if
+  // |last_default_was_valid|.
+  string16 last_default_fill_into_edit(ASCIIToUTF16(""));

[Peter Kasting, 2012/11/16 23:57:18]

Nit: remove "(ASCIIToUTF16(""))" as it does nothing (3 places)

[Mark P, 2012/11/19 22:21:45]

Fixed all three.

+  string16 last_default_associated_keyword(ASCIIToUTF16(""));
+  if (last_default_was_valid) {
+    last_default_fill_into_edit = result_.default_match()->fill_into_edit;
+    if (result_.default_match()->associated_keyword != NULL)
+      last_default_associated_keyword =
+          result_.default_match()->associated_keyword->keyword;
+  }
+
+  if (regenerate_result)
+    result_.Reset();
+
   AutocompleteResult last_result;
   last_result.Swap(&result_);
 
   for (ACProviders::const_iterator i(providers_.begin()); i != providers_.end();
        ++i)
     result_.AppendMatches((*i)->matches());
 
   // Sort the matches and trim to a small number of "best" matches.
   result_.SortAndCull(input_, profile_);
 
   // Need to validate before invoking CopyOldMatches as the old matches are not
   // valid against the current input.
 #ifndef NDEBUG
   result_.Validate();
 #endif
 
   if (!done_) {
     // This conditional needs to match the conditional in Start that invokes
     // StartExpireTimer.
     result_.CopyOldMatches(input_, last_result, profile_);
   }
 
   UpdateKeywordDescriptions(&result_);
   UpdateAssociatedKeywords(&result_);
   UpdateAssistedQueryStats(&result_);
 
-  bool notify_default_match = is_synchronous_pass;
-  if (!is_synchronous_pass) {
-    const bool last_default_was_valid =
-        last_result.default_match() != last_result.end();
-    const bool default_is_valid = result_.default_match() != result_.end();
-    // We've gotten async results. Send notification that the default match
-    // updated if fill_into_edit differs or associated_keyword differ.  (The
-    // latter can change if we've just started Chrome and the keyword database
-    // finishes loading while processing this request.) We don't check the URL
-    // as that may change for the default match even though the fill into edit
-    // hasn't changed (see SearchProvider for one case of this).
-    notify_default_match =
-        (last_default_was_valid != default_is_valid) ||
-        (default_is_valid &&
-          ((result_.default_match()->fill_into_edit !=
-            last_result.default_match()->fill_into_edit) ||
-            (result_.default_match()->associated_keyword.get() !=
-              last_result.default_match()->associated_keyword.get())));
+  const bool default_is_valid = result_.default_match() != result_.end();
+  string16 default_associated_keyword(ASCIIToUTF16(""));
+  if (default_is_valid &&
+      (result_.default_match()->associated_keyword != NULL)) {
+    default_associated_keyword =
+        result_.default_match()->associated_keyword->keyword;
+  }
+  // We've gotten async results. Send notification that the default match
+  // updated if fill_into_edit differs or associated_keyword differ.  (The
+  // latter can change if we've just started Chrome and the keyword database
+  // finishes loading while processing this request.) We don't check the URL
+  // as that may change for the default match even though the fill into edit
+  // hasn't changed (see SearchProvider for one case of this).
+  const bool notify_default_match =
+      (last_default_was_valid != default_is_valid) ||
+      (default_is_valid && last_default_was_valid &&

[Peter Kasting, 2012/11/16 23:57:18]

Nit: Adding "last_default_was_valid &&" is unnecessary because it's guaranteed
to be true based on the previous portion of the conditional.

[Mark P, 2012/11/19 22:21:45]

I know.  However, I think it makes the logic cleaner especially because I said
the two string16 last_default_... strings are only valid if
|last_default_was_valid|.  I like making it obvious that
|last_default_was_valid| is true when I'm going to use those last_default_...
strings.

[Peter Kasting, 2012/11/20 06:49:19]

In that case what about removing the "default_is_valid" condition instead?

[Mark P, 2012/11/20 17:34:40]

I'm comfortable with that.  Done.

+       ((result_.default_match()->fill_into_edit !=
+          last_default_fill_into_edit) ||
+         (default_associated_keyword != last_default_associated_keyword)));
+  if (notify_default_match) {

[Peter Kasting, 2012/11/16 23:57:18]

Nit: {} unnecessary

[Mark P, 2012/11/19 22:21:45]

Removed.

+    last_time_default_match_changed_ = base::TimeTicks::Now();
   }
 
-  NotifyChanged(notify_default_match);
+  NotifyChanged(force_notify_default_match_changed || notify_default_match);
 }
 
 void AutocompleteController::UpdateAssociatedKeywords(
     AutocompleteResult* result) {
   if (!keyword_provider_)
     return;
 
   std::set<string16> keywords;
   for (ACMatches::iterator match(result->begin()); match != result->end();
        ++match) {
@@ skipping 108 matching lines @@
   }
   done_ = true;
 }
 
 void AutocompleteController::StartExpireTimer() {
   if (result_.HasCopiedMatches())
     expire_timer_.Start(FROM_HERE,
                         base::TimeDelta::FromMilliseconds(kExpireTimeMS),
                         this, &AutocompleteController::ExpireCopiedEntries);
 }