Revert of Do not clobber AutocompleteActionPredictor's DB when submit…

…ing search queries in the Omnibox (patchset Pissandshittium#8 id:140001 of https://codereview.chromium.org/1504803002/ ) Reason for revert: reviewers asked to revert Original issue's description: > Do not clobber AutocompleteActionPredictor's DB with misses when submiting search queries in the Omnibox > > BUG=565895 > > Committed: https://crrev.com/8c958e047d1f2f314cb6905068b83a8da704f2aa > Cr-Commit-Position: refs/heads/master@{#368561} TBR=cbentzel@chromium.org,pkasting@chromium.org,sky@chromium.org,mpearson@chromium.org,gabadie@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=565895 Review URL: https://codereview.chromium.org/1580403003 Cr-Commit-Position: refs/heads/master@{#369149}
qweri0p · Jan 13, 2016 · b188b19 · b188b19
1 parent 7a3cc3f
commit b188b19
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 124 deletions.
diff --git a/chrome/browser/android/omnibox/autocomplete_controller_android.cc b/chrome/browser/android/omnibox/autocomplete_controller_android.cc
@@ -246,13 +246,8 @@ void AutocompleteControllerAndroid::OnSuggestionSelected(
 
   OmniboxEventGlobalTracker::GetInstance()->OnURLOpened(&log);
 
-  const AutocompleteMatch& selected_match =
-      log.result.match_at(log.selected_index);
-  const bool update_database =
-      !AutocompleteMatch::IsSearchType(selected_match.type);
-
   predictors::AutocompleteActionPredictorFactory::GetForProfile(profile_)
-      ->OnOmniboxOpenedUrl(log, update_database);
+      ->OnOmniboxOpenedUrl(log);
 }
 
 void AutocompleteControllerAndroid::DeleteSuggestion(

diff --git a/chrome/browser/predictors/autocomplete_action_predictor.cc b/chrome/browser/predictors/autocomplete_action_predictor.cc
@@ -40,9 +40,9 @@
 
 namespace {
 
-const double kConfidenceCutoff[] = {
-  0.8,
-  0.5
+const float kConfidenceCutoff[] = {
+  0.8f,
+  0.5f
 };
 
 static_assert(arraysize(kConfidenceCutoff) ==
@@ -220,8 +220,7 @@ bool AutocompleteActionPredictor::IsPrerenderAbandonedForTesting() {
   return prerender_handle_ && prerender_handle_->IsAbandoned();
 }
 
-void AutocompleteActionPredictor::OnOmniboxOpenedUrl(const OmniboxLog& log,
-                                                     bool update_database) {
+void AutocompleteActionPredictor::OnOmniboxOpenedUrl(const OmniboxLog& log) {
   if (!initialized_)
     return;
 
@@ -256,27 +255,7 @@ void AutocompleteActionPredictor::OnOmniboxOpenedUrl(const OmniboxLog& log,
 
   const AutocompleteMatch& match = log.result.match_at(log.selected_index);
   const GURL& opened_url = match.destination_url;
-
-  if (update_database)
-    UpdateDatabase(log.text, opened_url);
-
-  ClearTransitionalMatches();
-
-  // Check against tracked urls and log accuracy for the confidence we
-  // predicted.
-  for (auto& it : tracked_urls_) {
-    if (opened_url == it.first) {
-      UMA_HISTOGRAM_COUNTS_100("AutocompleteActionPredictor.AccurateCount",
-                               it.second * 100);
-    }
-  }
-  tracked_urls_.clear();
-}
-
-void AutocompleteActionPredictor::UpdateDatabase(
-    const base::string16& user_text,
-    const GURL& opened_url) {
-  const base::string16 lower_user_text(base::i18n::ToLower(user_text));
+  const base::string16 lower_user_text(base::i18n::ToLower(log.text));
 
   // Traverse transitional matches for those that have a user_text that is a
   // prefix of |lower_user_text|.
@@ -320,6 +299,20 @@ void AutocompleteActionPredictor::UpdateDatabase(
   }
   if (rows_to_add.size() > 0 || rows_to_update.size() > 0)
     AddAndUpdateRows(rows_to_add, rows_to_update);
+
+  ClearTransitionalMatches();
+
+  // Check against tracked urls and log accuracy for the confidence we
+  // predicted.
+  for (std::vector<std::pair<GURL, double> >::const_iterator it =
+       tracked_urls_.begin(); it != tracked_urls_.end();
+       ++it) {
+    if (opened_url == it->first) {
+      UMA_HISTOGRAM_COUNTS_100("AutocompleteActionPredictor.AccurateCount",
+                               it->second * 100);
+    }
+  }
+  tracked_urls_.clear();
 }
 
 void AutocompleteActionPredictor::Observe(

diff --git a/chrome/browser/predictors/autocomplete_action_predictor.h b/chrome/browser/predictors/autocomplete_action_predictor.h
@@ -115,10 +115,8 @@ class AutocompleteActionPredictor
   // abandoned.
   bool IsPrerenderAbandonedForTesting();
 
-  // Outputs histograms, may update the database and clears the transitional
-  // matches. |update_database| must be set to false in cases when another
-  // service is in charge of the selected match (instant search for example).
-  void OnOmniboxOpenedUrl(const OmniboxLog& log, bool update_database);
+  // Should be called when a URL is opened from the omnibox.
+  void OnOmniboxOpenedUrl(const OmniboxLog& log);
 
  private:
   friend class AutocompleteActionPredictorTest;
@@ -187,9 +185,6 @@ class AutocompleteActionPredictor
   void CreateCaches(
       std::vector<AutocompleteActionPredictorTable::Row>* row_buffer);
 
-  // Updates database with hits and misses according to transitional matches.
-  void UpdateDatabase(const base::string16& user_text, const GURL& opened_url);
-
   // Attempts to call DeleteOldEntries if the in-memory database has been loaded
   // by |service|. Returns success as a boolean.
   bool TryDeleteOldEntries(history::HistoryService* service);

diff --git a/chrome/browser/predictors/autocomplete_action_predictor_unittest.cc b/chrome/browser/predictors/autocomplete_action_predictor_unittest.cc
@@ -23,8 +23,6 @@
 #include "components/history/core/browser/in_memory_database.h"
 #include "components/history/core/browser/url_database.h"
 #include "components/omnibox/browser/autocomplete_match.h"
-#include "components/omnibox/browser/autocomplete_result.h"
-#include "components/omnibox/browser/omnibox_log.h"
 #include "content/public/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -42,63 +40,43 @@ struct TestUrlInfo {
   int number_of_hits;
   int number_of_misses;
   AutocompleteActionPredictor::Action expected_action;
-  AutocompleteActionPredictor::Action expected_action_with_additional_hit;
 } test_url_db[] = {
   { GURL("http://www.testsite.com/a.html"),
     ASCIIToUTF16("Test - site - just a test"), 1,
     ASCIIToUTF16("j"), 5, 0,
-    AutocompleteActionPredictor::ACTION_PRERENDER,
     AutocompleteActionPredictor::ACTION_PRERENDER },
   { GURL("http://www.testsite.com/b.html"),
     ASCIIToUTF16("Test - site - just a test"), 1,
     ASCIIToUTF16("ju"), 3, 0,
-    AutocompleteActionPredictor::ACTION_PRERENDER,
     AutocompleteActionPredictor::ACTION_PRERENDER },
   { GURL("http://www.testsite.com/c.html"),
     ASCIIToUTF16("Test - site - just a test"), 5,
     ASCIIToUTF16("just"), 3, 1,
-    AutocompleteActionPredictor::ACTION_PRECONNECT,
-    AutocompleteActionPredictor::ACTION_PRERENDER },
+    AutocompleteActionPredictor::ACTION_PRECONNECT },
   { GURL("http://www.testsite.com/d.html"),
     ASCIIToUTF16("Test - site - just a test"), 5,
     ASCIIToUTF16("just"), 3, 0,
-    AutocompleteActionPredictor::ACTION_PRERENDER,
     AutocompleteActionPredictor::ACTION_PRERENDER },
   { GURL("http://www.testsite.com/e.html"),
     ASCIIToUTF16("Test - site - just a test"), 8,
     ASCIIToUTF16("just"), 3, 1,
-    AutocompleteActionPredictor::ACTION_PRECONNECT,
-    AutocompleteActionPredictor::ACTION_PRERENDER },
+    AutocompleteActionPredictor::ACTION_PRECONNECT },
   { GURL("http://www.testsite.com/f.html"),
     ASCIIToUTF16("Test - site - just a test"), 8,
     ASCIIToUTF16("just"), 3, 0,
-    AutocompleteActionPredictor::ACTION_PRERENDER,
     AutocompleteActionPredictor::ACTION_PRERENDER },
   { GURL("http://www.testsite.com/g.html"),
     ASCIIToUTF16("Test - site - just a test"), 12,
     base::string16(), 5, 0,
-    AutocompleteActionPredictor::ACTION_NONE,
     AutocompleteActionPredictor::ACTION_NONE },
   { GURL("http://www.testsite.com/h.html"),
     ASCIIToUTF16("Test - site - just a test"), 21,
     ASCIIToUTF16("just a test"), 2, 0,
-    AutocompleteActionPredictor::ACTION_NONE,
-    AutocompleteActionPredictor::ACTION_PRERENDER },
+    AutocompleteActionPredictor::ACTION_NONE },
   { GURL("http://www.testsite.com/i.html"),
     ASCIIToUTF16("Test - site - just a test"), 28,
     ASCIIToUTF16("just a test"), 2, 0,
-    AutocompleteActionPredictor::ACTION_NONE,
-    AutocompleteActionPredictor::ACTION_PRERENDER },
-  { GURL("http://www.testsite.com/j.html"),
-    ASCIIToUTF16("Test - site - just a test"), 28,
-    ASCIIToUTF16("just a test"), 0, 0,
-    AutocompleteActionPredictor::ACTION_NONE,
-    AutocompleteActionPredictor::ACTION_NONE },
-  { GURL("http://www.testsite.com/k.html"),
-    ASCIIToUTF16("Test - site - just a test"), 5,
-    ASCIIToUTF16("just"), 3, 4,
-    AutocompleteActionPredictor::ACTION_NONE,
-    AutocompleteActionPredictor::ACTION_PRECONNECT },
+    AutocompleteActionPredictor::ACTION_NONE }
 };
 
 }  // end namespace
@@ -407,56 +385,4 @@ TEST_F(AutocompleteActionPredictorTest, RecommendActionSearch) {
   }
 }
 
-TEST_F(AutocompleteActionPredictorTest, UpdateDatabase) {
-  ASSERT_NO_FATAL_FAILURE(AddAllRows());
-
-  for (size_t i = 0; i < arraysize(test_url_db); ++i) {
-    const base::string16& user_text = test_url_db[i].user_text;
-
-    AutocompleteMatch match;
-    match.type = AutocompleteMatchType::HISTORY_URL;
-    match.destination_url = GURL(test_url_db[i].url);
-    ACMatches matches = {match};
-    AutocompleteResult result;
-    result.AppendMatches(AutocompleteInput(), matches);
-    OmniboxLog log(
-        /* text = */ user_text,
-        /* just_deleted_text = */ false,
-        /* input_type = */ metrics::OmniboxInputType::URL,
-        /* is_popup_open = */ true,
-        /* selected_index = */ 0,
-        /* is_paste_and_go = */ false,
-        /* tab_id = */ 0,
-        /* current_page_classification = */
-            metrics::OmniboxEventProto_PageClassification_BLANK,
-        /* elapsed_time_since_user_first_modified_omnibox = */
-            base::TimeDelta(),
-        /* completed_length = */ 0,
-        /* elapsed_time_since_last_change_to_default_match = */
-            base::TimeDelta(),
-        /* result = */ result);
-
-    // Simulate opened URL but without updating the database that should lead to
-    // an unchanged recommendation afterwards.
-    predictor()->RegisterTransitionalMatches(user_text, result);
-    predictor()->OnOmniboxOpenedUrl(log, false);
-    EXPECT_EQ(test_url_db[i].expected_action,
-            predictor()->RecommendAction(user_text, match))
-        << "Unexpected action for " << match.destination_url;
-
-    // Simulate opened URL with updating the database to that could change the
-    // recommended action afterwards. Updating the database twice to avoid test
-    // failures because of floating point precision when computing the
-    // AutocompleteActionPredictor's DB entry's confidence that determine the
-    // recommendation.
-    predictor()->RegisterTransitionalMatches(user_text, result);
-    predictor()->OnOmniboxOpenedUrl(log, true);
-    predictor()->RegisterTransitionalMatches(user_text, result);
-    predictor()->OnOmniboxOpenedUrl(log, true);
-    EXPECT_EQ(test_url_db[i].expected_action_with_additional_hit,
-            predictor()->RecommendAction(user_text, match))
-        << "Unexpected action for " << match.destination_url;
-  }
-}
-
 }  // namespace predictors
diff --git a/chrome/browser/ui/omnibox/chrome_omnibox_client.cc b/chrome/browser/ui/omnibox/chrome_omnibox_client.cc
@@ -40,7 +40,6 @@
 #include "components/favicon/content/content_favicon_driver.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/autocomplete_result.h"
-#include "components/omnibox/browser/omnibox_log.h"
 #include "components/omnibox/browser/search_provider.h"
 #include "components/search/search.h"
 #include "components/search_engines/template_url_service.h"
@@ -404,13 +403,8 @@ void ChromeOmniboxClient::OnRevert() {
 }
 
 void ChromeOmniboxClient::OnURLOpenedFromOmnibox(OmniboxLog* log) {
-  const AutocompleteMatch& selected_match =
-      log->result.match_at(log->selected_index);
-  const bool update_database =
-      !AutocompleteMatch::IsSearchType(selected_match.type);
-
   predictors::AutocompleteActionPredictorFactory::GetForProfile(profile_)
-      ->OnOmniboxOpenedUrl(*log, update_database);
+      ->OnOmniboxOpenedUrl(*log);
 }
 
 void ChromeOmniboxClient::OnBookmarkLaunched() {