Pass AutocompleteProvider::kMaxMatches to InMemoryURLIndex

Pass AutocompleteProvider::kMaxMatches to InMemoryURLIndex

The only client of InMemoryURLIndex::HistoryItemsForTerms is
//chrome/browser/autocomplete/history_quick_provider.cc which has access
to the constant. So pass the constant to the history component instead
of having the dependency.

BUG=374730
TBR=thakis  // For OWNERS at chrome/browser/autocomplete/history_quick_provider.cc

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273072

[blundell, 2014-05-22 15:23:25]

Sylvain,

Could you do an initial review?

[Jiang Jiang, 2014-05-22 15:29:41]

I wonder if we should get rid of the AutocompleteProvider::kMaxMatches in
in_memory_url_index_unittest.cc, otherwise how do we move it to //components?

Also, I guess //chrome/browser/history/DEPS can be updated to move the
chrome/browser/autocomplete/autocomplete_provider.h from include_rules to
specific_include_rules for '.*_[a-z]*test\.cc'?

[sdefresne, 2014-05-22 15:41:12]

https://codereview.chromium.org/296743009/diff/1/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/296743009/diff/1/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:452:
AutocompleteProvider::kMaxMatches);
Most of the tests don't depends on the value of
AutocompleteProvider::kMaxMatches, except InMemoryURLIndexTest.HugeResultSet
that check that the returned list is bounded.

So I'd recommend using another constant in that in_memory_url_index_unittest.cc
(it does not have to be in sync with the value in AutocompleteProvider), and
remove the include chrome/browser/autocomplete/autocomplete_provider.h (and
remove it from DEPS).

[Jiang Jiang, 2014-05-23 16:12:51]

https://codereview.chromium.org/296743009/diff/1/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/296743009/diff/1/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:452:
AutocompleteProvider::kMaxMatches);
On 2014/05/22 15:41:13, sdefresne wrote:
> Most of the tests don't depends on the value of
> AutocompleteProvider::kMaxMatches, except InMemoryURLIndexTest.HugeResultSet
> that check that the returned list is bounded.
> 
> So I'd recommend using another constant in that
in_memory_url_index_unittest.cc
> (it does not have to be in sync with the value in AutocompleteProvider), and
> remove the include chrome/browser/autocomplete/autocomplete_provider.h (and
> remove it from DEPS).

Done.

[blundell, 2014-05-26 07:20:24]

LGTM!

[Jiang Jiang, 2014-05-26 07:56:25]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-05-26 07:56:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/296743009/20001

[commit-bot: I haz the power, 2014-05-26 09:45:25]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-26 09:51:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-26 09:51:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Jiang Jiang, 2014-05-26 09:55:41]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-05-26 09:56:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/296743009/20001

[blundell, 2014-05-26 11:03:26]

Minor note: The "For OWNERS at
chrome/browser/autocomplete/history_quick_provider.cc" should be included in the
email you send adding the OWNER as a (TBR) reviewer, and doesn't need to be
included in the CL description itself.

Thanks,

Colin

[Jiang Jiang, 2014-05-26 20:08:48]

The CQ bit was unchecked by jiangj@opera.com

[Jiang Jiang, 2014-05-26 20:09:21]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-05-26 20:09:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/296743009/20001

[commit-bot: I haz the power, 2014-05-26 23:51:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-26 23:51:40]

Failed to apply patch for chrome/browser/history/url_index_private_data.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/history/url_index_private_data.cc
  Hunk #4 FAILED at 489.
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/history/url_index_private_data.cc.rej

Patch:       chrome/browser/history/url_index_private_data.cc
Index: chrome/browser/history/url_index_private_data.cc
diff --git a/chrome/browser/history/url_index_private_data.cc
b/chrome/browser/history/url_index_private_data.cc
index
6dbfae7de006f865f7423cf255ed2ffec64bdeac..47b9468dc40330e50d9fa0dc6a470c64af94955e
100644
--- a/chrome/browser/history/url_index_private_data.cc
+++ b/chrome/browser/history/url_index_private_data.cc
@@ -19,7 +19,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
-#include "chrome/browser/autocomplete/autocomplete_provider.h"
 #include "chrome/browser/history/history_database.h"
 #include "chrome/browser/history/history_db_task.h"
 #include "chrome/browser/history/history_service.h"
@@ -150,6 +149,7 @@ URLIndexPrivateData::URLIndexPrivateData()
 ScoredHistoryMatches URLIndexPrivateData::HistoryItemsForTerms(
     base::string16 search_string,
     size_t cursor_position,
+    size_t max_matches,
     const std::string& languages,
     BookmarkService* bookmark_service) {
   // If cursor position is set and useful (not at either end of the
@@ -246,14 +246,14 @@ ScoredHistoryMatches
URLIndexPrivateData::HistoryItemsForTerms(
       AddHistoryMatch(*this, languages, bookmark_service, lower_raw_string,
                       lower_raw_terms, base::Time::Now())).ScoredMatches();
 
-  // Select and sort only the top kMaxMatches results.
-  if (scored_items.size() > AutocompleteProvider::kMaxMatches) {
+  // Select and sort only the top |max_matches| results.
+  if (scored_items.size() > max_matches) {
     std::partial_sort(scored_items.begin(),
                       scored_items.begin() +
-                          AutocompleteProvider::kMaxMatches,
+                          max_matches,
                       scored_items.end(),
                       ScoredHistoryMatch::MatchScoreGreater);
-      scored_items.resize(AutocompleteProvider::kMaxMatches);
+      scored_items.resize(max_matches);
   } else {
     std::sort(scored_items.begin(), scored_items.end(),
               ScoredHistoryMatch::MatchScoreGreater);
@@ -489,7 +489,7 @@ scoped_refptr<URLIndexPrivateData>
URLIndexPrivateData::Duplicate() const {
   //    pre_filter_item_count_
   //    post_filter_item_count_
   //    post_scoring_item_count_
-};
+}
 
 bool URLIndexPrivateData::Empty() const {
   return history_info_map_.empty();

[Jiang Jiang, 2014-05-27 20:11:16]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-05-27 20:12:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/296743009/40001

[commit-bot: I haz the power, 2014-05-27 23:40:24]

Change committed as 273072