Implement Bookmark Autocomplete Provider

chrome/browser/autocomplete/bookmark_provider.h

+#ifndef CHROME_BROWSER_AUTOCOMPLETE_BOOKMARK_PROVIDER_H_
+#define CHROME_BROWSER_AUTOCOMPLETE_BOOKMARK_PROVIDER_H_
+
+#include "chrome/browser/autocomplete/autocomplete_input.h"
+#include "chrome/browser/autocomplete/autocomplete_match.h"
+#include "chrome/browser/autocomplete/autocomplete_provider.h"
+#include "chrome/browser/bookmarks/bookmark_utils.h"
+#include "chrome/browser/history/snippet.h"
+
+class BookmarkModel;
+class Profile;
+
+// This class is an autocomplete provider which quickly (and synchronously)
+// provides autocomplete suggestions based on matches between the search terms
+// entered by the user into the omnibox and the titles of bookmarks. Page
+// titles and URLs of the bookmarks are not matched.
+class BookmarkProvider : public AutocompleteProvider {
+ public:
+  BookmarkProvider(AutocompleteProviderListener* listener, Profile* profile);
+
+  // AutocompleteProvider. |minimal_changes| is ignored since there is no
+  // asynch completion performed.
+  virtual void Start(const AutocompleteInput& input,
+                     bool minimal_changes) OVERRIDE;
+
+  // Sets the BookmarkModel for unit tests.
+  void set_bookmark_model_for_testing(BookmarkModel* bookmark_model) {

[Mark P, 2012/09/28 00:55:13]

Should this be private and a friend to your test class?

[mrossetti, 2012/10/02 00:44:16]

Not necessarily. There is no style guidance for this and, in this where there is
a very limited number of for_testing functions I'd prefer to not have a friend.

[Mark P, 2012/10/03 20:40:14]

You can make a function a friend without having to make the whole class friends.
 Limited friendship I suppose it can be called.

If you still decline, that's okay.

+    bookmark_model_ = bookmark_model;
+  }
+
+ private:
+  virtual ~BookmarkProvider();
+
+  // Performs the actual matching of |input| over the bookmarks and fills in
+  // matches. If |best_match| then only return the single best match, otherwise

[Mark P, 2012/09/28 00:55:13]

matches -> |matches_|

[mrossetti, 2012/10/02 00:44:16]

Done.

+  // return the top |kMaxMatches| matches.

[Mark P, 2012/09/28 00:55:13]

Using return (both times) in the above sentence is odd because this function
doesn't return anything.

[mrossetti, 2012/10/02 00:44:16]

Done.

+  void DoAutocomplete(const AutocompleteInput& input, bool best_match);
+
+  // Converts |title_match| to an AutocompleteMatch and returns the converted
+  // match.
+  AutocompleteMatch TitleMatchToACMatch(

[Mark P, 2012/09/28 00:55:13]

Can this be static or at least const?

[mrossetti, 2012/10/02 00:44:16]

Unfortunately, neither. Can't be static because it needs access to |languages_|.
Can't be const because the AutocompleteMatch created has a non-const pointer to
the provider. (And I don't want to go down the const rabbit hole trying to make
everything happy with a const pointer.)

+      const bookmark_utils::TitleMatch& title_match);
+
+  // Converts |positions| into ACMatchClassifications and returns the
+  // classifications. |text_length| is used to determine the need for a
+  // terminating classification.

[Mark P, 2012/09/28 00:55:13]

I don't understand the last sentence.  (What is a terminating classification and
why might you need one?)

[mrossetti, 2012/10/02 00:44:16]

I've updated the comment a bit, but for your edification what this means is that
if the last substring match doesn't extend to the end of the source string then
we need to add one final classification to turn off the match highlighting so
that the tail of the source string is not highlighted when it shouldn't be.

+  static ACMatchClassifications SpansFromMatch(
+      const Snippet::MatchPositions& positions,
+      size_t text_length);
+
+  BookmarkModel* bookmark_model_;
+
+  // Languages used during the URL formatting.
+  std::string languages_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkProvider);
+};
+
+#endif  // CHROME_BROWSER_AUTOCOMPLETE_BOOKMARK_PROVIDER_H_