Factor out AutocompleteMatch creation from BookmarkProvider

components/omnibox/browser/autocomplete_provider_utils.h

+// Copyright (c) 2016 The Chromium Authors. All rights reserved.

[Mark P, 2016/12/22 05:24:31]

This file seems awkwardly named for what's it doing.
Also, functions without a namespace I think are discouraged.

I'm not sure if I have a good suggestion for how to change this.  I will mention
that sometimes helper functions go as static member in AutocompleteProvider. 
We've also introduced intermediate subclasses, such as HistoryProvider (children
HistoryURL and HistoryQuick)  and BaseSearchProvider (children SearchProvider
and ZeroSuggest).  That I suppose is a possibility.

Anyway, please give this a bit of thought about how it can be organized /
structured better.

[Mark P, 2016/12/23 22:38:49]

Perhaps titled_url_match_utils, if nothing else seems better to you.

[mattreynolds, 2017/01/04 00:58:02]

I didn't want to add components/bookmarks dependencies in autocomplete_provider
and didn't want autocomplete_provider dependencies in titled_url_match. A
subclass seems like overkill for a single static function. I'll rename to
titled_url_match_utils.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OMNIBOX_BROWSER_AUTOCOMPLETE_PROVIDER_UTILS_H_
+#define COMPONENTS_OMNIBOX_BROWSER_AUTOCOMPLETE_PROVIDER_UTILS_H_
+
+#include "components/bookmarks/browser/titled_url_match.h"
+#include "components/omnibox/browser/autocomplete_match_type.h"

[Mark P, 2016/12/23 22:38:49]

Include something for string16?

[mattreynolds, 2017/01/04 00:58:02]

Done.

+
+class AutocompleteInput;
+class AutocompleteProvider;
+class AutocompleteSchemeClassifier;
+struct AutocompleteMatch;
+
+// Compose an AutocompleteMatch based on |match| that has the match's URL and
+// the page title and type |type|. |input| is used to compute the match's

[Mark P, 2016/12/23 22:38:49]

Please add a sentence about the variable |relevance| here (after the current
first sentence).

[mattreynolds, 2017/01/04 00:58:02]

Done.

+// inline_autocompletion. |fixed_up_input_text| is used in that way as well;

[Mark P, 2016/12/23 22:38:49]

By the way, I'm okay with the current comment that does not mention provider and
scheme_classifier.

[mattreynolds, 2017/01/04 00:58:02]

Acknowledged.

+// it's passed separately so this function doesn't have to compute it.
+AutocompleteMatch TitledUrlMatchToAutocompleteMatch(

[Mark P, 2016/12/23 22:38:49]

Given this (now-revised) description, I think match, type, and relevance should
be listed first in the parameter list.

[mattreynolds, 2017/01/04 00:58:02]

Done.

+    AutocompleteProvider* provider,
+    const AutocompleteSchemeClassifier& scheme_classifier,
+    const AutocompleteInput& input,
+    const base::string16& fixed_up_input_text,
+    const bookmarks::TitledUrlMatch& match,
+    AutocompleteMatchType::Type type,
+    int relevance);
+
+#endif  // COMPONENTS_OMNIBOX_BROWSER_AUTOCOMPLETE_PROVIDER_UTILS_H_