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

chrome/browser/autocomplete/autocomplete_controller.cc

Index: chrome/browser/autocomplete/autocomplete_controller.cc
diff --git a/chrome/browser/autocomplete/autocomplete_controller.cc b/chrome/browser/autocomplete/autocomplete_controller.cc
index 9c2a4064a385d96adad1f9f255ccaafc2583947d..c00b7dedde0fa18627fc11698b33e5e80be5654d 100644
--- a/chrome/browser/autocomplete/autocomplete_controller.cc
+++ b/chrome/browser/autocomplete/autocomplete_controller.cc
@@ -14,6 +14,7 @@
 #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"
@@ -213,7 +214,7 @@ void AutocompleteController::Start(
   }
   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();
@@ -263,13 +264,14 @@ void AutocompleteController::DeleteMatch(const AutocompleteMatch& match) {
 }
 
 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) {
@@ -282,10 +284,10 @@ void AutocompleteController::OnProviderUpdate(bool updated_matches) {
     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);
   }
 }
 
@@ -302,7 +304,23 @@ void AutocompleteController::AddProvidersInfo(
   }
 }
 
-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_);
 
@@ -329,27 +347,30 @@ void AutocompleteController::UpdateResult(bool is_synchronous_pass) {
   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(