Generating autocomplete results with and without word breaks in the Omnibox.

components/omnibox/browser/url_index_private_data.h

Index: components/omnibox/browser/url_index_private_data.h
diff --git a/components/omnibox/browser/url_index_private_data.h b/components/omnibox/browser/url_index_private_data.h
index 100692cd7b44e63de9bfbc2b01c3501a6e2bbd95..94564f15bd4a9ac516132bd6a9c29ea07bc07b65 100644
--- a/components/omnibox/browser/url_index_private_data.h
+++ b/components/omnibox/browser/url_index_private_data.h
@@ -50,6 +50,21 @@ class URLIndexPrivateData
  public:
   URLIndexPrivateData();
 
+  bool TrimCandidatePool (

[Mark P, 2016/08/10 17:46:20]

All of these function should be private.  They're helper function that no one
external to this class needs to know about.

Also, when you move their declarations / make them private, remember to correct
the .cc.  Our style guide requires that the functions in the .cc are implemented
in the same order as they are declared in the header file.

You may to do this when the review is almost over; otherwise the "diff" between
you change and the original code will look large.

[Mark P, 2016/08/10 17:46:20]

This function and the one below should be commented.

[Mark P, 2016/08/10 17:46:20]

Here and throughout the change, you introduce a number of style violations. 
(Indeed, at times the only thing you did on some lines is to change the spacing
to a way that's not consistent with chromium style.)  Can you run "git cl
format" and use the changes it suggests for the parts of the code that you touch
or at least look over your code and change the spacing back?

[Lavar Askew, 2016/08/18 03:07:52]

Done.

[Lavar Askew, 2016/08/18 03:07:52]

"git cl format" did not make any suggestions for me. 

+      HistoryIDSet history_id_set);
+
+  ScoredHistoryMatches GetScoredItemsForSearchString(

[Mark P, 2016/08/10 17:46:19]

Most of this class calls this parameter a "term_string"; please be consistent
with naming the parameter and teh function.

[Lavar Askew, 2016/08/18 03:07:52]

search_string was the name of the variable before I started working on it.  It
made things clear to me in this method.  If you insist that I do that then I
will.

+      base::string16 search_string,
+      HistoryIDSet history_id_set,
+      size_t max_matches,
+      bookmarks::BookmarkModel* bookmark_model,
+      TemplateURLService* template_url_service);
+
+  // Given a base::string16 in |term_string|, scans the history index and
+  // returns the set of history item IDs.
+  HistoryIDSet HistoryItemsForWords(

[Mark P, 2016/08/10 17:46:19]

This function is confusingly named.  It sounds a like like
"HistoryItemsForTerms" but
(i) function HistoryItemsForTerms returns ScoredHistoryMatches; this returns
HistoryIDSets.  Having them both called HistoryItems... is confusing.
(ii) this function takes a term string; not words.  (Indeed, in the middle of
the function a comment explains the difference in terminology.  This function
should not be claiming to take "words".)  Having this function called "ForWords"
is misleading.

[Lavar Askew, 2016/08/18 03:07:52]

Done.

+      base::string16 search_string);
+
   // Given a base::string16 in |term_string|, scans the history index and
   // returns a vector with all scored, matching history items. The
   // |term_string| is broken down into individual terms (words), each of which