Handle Search suggest subtypes for Shortcuts DB

Handle SEARCH_SUGGEST_.+ types for Shortcuts DB

- I think fill_into_edit should work just fine for Search Suggest types.

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

[Anuj, 2014-02-14 00:01:58]

PTAL

[Peter Kasting, 2014-02-14 05:35:24]

https://codereview.chromium.org/165163002/diff/30001/chrome/browser/history/s...
File chrome/browser/history/shortcuts_backend.cc (right):

https://codereview.chromium.org/165163002/diff/30001/chrome/browser/history/s...
chrome/browser/history/shortcuts_backend.cc:51: match.stripped_destination_url :
match.destination_url;
If we needed to do this, I'd expect us to need this on all search matches, not
just search suggestions.  But normally I'd expect that any sort of param
stripping should happen later.  the |stripped_destination_url| doesn't seem
right in any case, that's not necessarily guaranteed to be navigable -- it will
change the scheme and hostname, not just remove parameters.

https://codereview.chromium.org/165163002/diff/30001/chrome/browser/history/s...
chrome/browser/history/shortcuts_backend.cc:60: return
IsSearchSuggestType(match.type) ? base::string16() : match.description;
Hmm, should we be flattening the description here?  For matches where this is
e.g. "Google Search" we should be flattening at a later stage (after the matches
are retrieved from history).  If the description contains data for entity
suggestions, though, we would want to flatten that here.

[Anuj, 2014-02-14 20:18:07]

I am not sure about stripping the additional query parameters. Please see
comments below.

I will update the patch set after discussing that issue.

https://codereview.chromium.org/165163002/diff/30001/chrome/browser/history/s...
File chrome/browser/history/shortcuts_backend.cc (right):

https://codereview.chromium.org/165163002/diff/30001/chrome/browser/history/s...
chrome/browser/history/shortcuts_backend.cc:51: match.stripped_destination_url :
match.destination_url;
On 2014/02/14 05:35:25, Peter Kasting wrote:
> If we needed to do this, I'd expect us to need this on all search matches, not
> just search suggestions.  But normally I'd expect that any sort of param
> stripping should happen later.  the |stripped_destination_url| doesn't seem
> right in any case, that's not necessarily guaranteed to be navigable -- it
will
> change the scheme and hostname, not just remove parameters.

So the idea was to remove the additional query parameters which are set as
suggest_query_params on SearchTermsArgs.
But now that I think of it, they won't be stripped from the search history, then
why should we remove them from shortcuts DB?

https://codereview.chromium.org/165163002/diff/30001/chrome/browser/history/s...
chrome/browser/history/shortcuts_backend.cc:60: return
IsSearchSuggestType(match.type) ? base::string16() : match.description;
On 2014/02/14 05:35:25, Peter Kasting wrote:
> Hmm, should we be flattening the description here?  For matches where this is
> e.g. "Google Search" we should be flattening at a later stage (after the
matches
> are retrieved from history).  If the description contains data for entity
> suggestions, though, we would want to flatten that here.

So the description is present for Entity and Profile suggestion. I will change
the conditional.

[Peter Kasting, 2014-02-14 21:40:21]

https://codereview.chromium.org/165163002/diff/30001/chrome/browser/history/s...
File chrome/browser/history/shortcuts_backend.cc (right):

https://codereview.chromium.org/165163002/diff/30001/chrome/browser/history/s...
chrome/browser/history/shortcuts_backend.cc:51: match.stripped_destination_url :
match.destination_url;
On 2014/02/14 20:18:07, Anuj wrote:
> On 2014/02/14 05:35:25, Peter Kasting wrote:
> > If we needed to do this, I'd expect us to need this on all search matches,
not
> > just search suggestions.  But normally I'd expect that any sort of param
> > stripping should happen later.  the |stripped_destination_url| doesn't seem
> > right in any case, that's not necessarily guaranteed to be navigable -- it
> will
> > change the scheme and hostname, not just remove parameters.
> 
> So the idea was to remove the additional query parameters which are set as
> suggest_query_params on SearchTermsArgs.
> But now that I think of it, they won't be stripped from the search history,
then
> why should we remove them from shortcuts DB?

I think the additional query parameters are definitely worth worrying about, but
we almost certainly want to worry about them at the time we pull out URLs to
provide as matches in the future, rather than when we populate the database; for
example, the set of query parameters we trim might change from when we insert to
when we later retrieve.

I don't know what sort of later sanitization we do.  It's probably worth
investigation.  However, I think you can ignore it in this CL.

[Anuj, 2014-02-14 22:09:55]

PTAL

https://codereview.chromium.org/165163002/diff/30001/chrome/browser/history/s...
File chrome/browser/history/shortcuts_backend.cc (right):

https://codereview.chromium.org/165163002/diff/30001/chrome/browser/history/s...
chrome/browser/history/shortcuts_backend.cc:51: match.stripped_destination_url :
match.destination_url;
On 2014/02/14 21:40:21, Peter Kasting wrote:
> On 2014/02/14 20:18:07, Anuj wrote:
> > On 2014/02/14 05:35:25, Peter Kasting wrote:
> > > If we needed to do this, I'd expect us to need this on all search matches,
> not
> > > just search suggestions.  But normally I'd expect that any sort of param
> > > stripping should happen later.  the |stripped_destination_url| doesn't
seem
> > > right in any case, that's not necessarily guaranteed to be navigable -- it
> > will
> > > change the scheme and hostname, not just remove parameters.
> > 
> > So the idea was to remove the additional query parameters which are set as
> > suggest_query_params on SearchTermsArgs.
> > But now that I think of it, they won't be stripped from the search history,
> then
> > why should we remove them from shortcuts DB?
> 
> I think the additional query parameters are definitely worth worrying about,
but
> we almost certainly want to worry about them at the time we pull out URLs to
> provide as matches in the future, rather than when we populate the database;
for
> example, the set of query parameters we trim might change from when we insert
to
> when we later retrieve.

I think we should strip out these parameters at the time of adding to database
precisely for the reason you mentioned. When adding a shortcut to DB, we have
the precise info of which parameters were added extra. At the time of retrieval,
we will need to hunt for a whitelist of the parameters to retain. It will be
unmaintainable to keep track of what parameters may need to be scrubbed.

> I don't know what sort of later sanitization we do.  It's probably worth
> investigation.  However, I think you can ignore it in this CL.

Cool. Done.

[Peter Kasting, 2014-02-14 22:16:36]

You know, now that I think about it, I realize there's a safer way to do all
this than trying to sanitize each field individually.

Instead, we should just make use of BaseSearchProvider::CreateSearchSuggestion()
to create a new, clean search result match which would do a search for the
match's fill_into_edit.

You may need to make this function public, but I think the data you need to pass
to it should already be available, or at least constructable, from the incoming
match.

This should effectively sanitize everything including the destination URL.

Sorry for not thinking of this sooner :P

[Anuj, 2014-02-14 22:25:35]

On 2014/02/14 22:16:36, Peter Kasting wrote:
> You know, now that I think about it, I realize there's a safer way to do all
> this than trying to sanitize each field individually.
> 
> Instead, we should just make use of
BaseSearchProvider::CreateSearchSuggestion()
> to create a new, clean search result match which would do a search for the
> match's fill_into_edit.
> 
> You may need to make this function public, but I think the data you need to
pass
> to it should already be available, or at least constructable, from the
incoming
> match.
> 
> This should effectively sanitize everything including the destination URL.
> 
> Sorry for not thinking of this sooner :P

How about 
- make a copy of the given AutocompleteMatch
- reset the SearchTermsArgs with search terms set to fill_into_edit
- Recompute Destination URL
Leave others (contents, description) as they are in the current patch set.

CreateSearchSuggestion takes parameters like AutocompleteInput etc, and it will
be a bit of abuse to twist various parameters to pass through any sanity checks.

Also do you agree that it is better to scrub parameters at the time of saving
than at the time of reading?

[Peter Kasting, 2014-02-14 22:28:33]

On 2014/02/14 22:25:35, Anuj wrote:
> On 2014/02/14 22:16:36, Peter Kasting wrote:
> > You know, now that I think about it, I realize there's a safer way to do all
> > this than trying to sanitize each field individually.
> > 
> > Instead, we should just make use of
> BaseSearchProvider::CreateSearchSuggestion()
> > to create a new, clean search result match which would do a search for the
> > match's fill_into_edit.
> > 
> > You may need to make this function public, but I think the data you need to
> pass
> > to it should already be available, or at least constructable, from the
> incoming
> > match.
> > 
> > This should effectively sanitize everything including the destination URL.
> > 
> > Sorry for not thinking of this sooner :P
> 
> How about 
> - make a copy of the given AutocompleteMatch
> - reset the SearchTermsArgs with search terms set to fill_into_edit
> - Recompute Destination URL
> Leave others (contents, description) as they are in the current patch set.
> 
> CreateSearchSuggestion takes parameters like AutocompleteInput etc, and it
will
> be a bit of abuse to twist various parameters to pass through any sanity
checks.

In what way?  We should be able to either get the current AutocompleteInput or
construct a new one from the desired fill_into_edit pretty easily; it just
depends which is easier and how much of the struct CreateSearchSuggestion()
reads.

> Also do you agree that it is better to scrub parameters at the time of saving
> than at the time of reading?

Some things should be scrubbed at save, some on retrieve.  I don't think we need
to agree on this to progress here so I'd rather not debate it.

[Anuj, 2014-02-15 01:20:05]

On 2014/02/14 22:28:33, Peter Kasting wrote:
> On 2014/02/14 22:25:35, Anuj wrote:
> > On 2014/02/14 22:16:36, Peter Kasting wrote:
> > > You know, now that I think about it, I realize there's a safer way to do
all
> > > this than trying to sanitize each field individually.
> > > 
> > > Instead, we should just make use of
> > BaseSearchProvider::CreateSearchSuggestion()
> > > to create a new, clean search result match which would do a search for the
> > > match's fill_into_edit.
> > > 
> > > You may need to make this function public, but I think the data you need
to
> > pass
> > > to it should already be available, or at least constructable, from the
> > incoming
> > > match.
> > > 
> > > This should effectively sanitize everything including the destination URL.
> > > 
> > > Sorry for not thinking of this sooner :P
> > 
> > How about 
> > - make a copy of the given AutocompleteMatch
> > - reset the SearchTermsArgs with search terms set to fill_into_edit
> > - Recompute Destination URL
> > Leave others (contents, description) as they are in the current patch set.
> > 
> > CreateSearchSuggestion takes parameters like AutocompleteInput etc, and it
> will
> > be a bit of abuse to twist various parameters to pass through any sanity
> checks.
> 
> In what way?  We should be able to either get the current AutocompleteInput or
> construct a new one from the desired fill_into_edit pretty easily; it just
> depends which is easier and how much of the struct CreateSearchSuggestion()
> reads.

In either approach, I will need access to profile to generate the new
destination_url
(see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...)

I tried something like following by exposing Profile through AutocompleteMatch
(match has a pointer to AutcompleteProvider which has a pointer to Profile)

const GURL GetDestinationURLForShortcut(const AutocompleteMatch& match) {
  if (IsSearchSuggestType(match.type)) {
    scoped_ptr<TemplateURLRef::SearchTermsArgs> search_terms_args(
        new TemplateURLRef::SearchTermsArgs(match.fill_into_edit));
    const TemplateURL* template_url =
        match.GetTemplateURL(match.profile, false);
    if (template_url) {
      return GURL(template_url->url_ref().ReplaceSearchTerms(
          *search_terms_args.get()));
    }
  }
  return GURL();
}

But the above change create a dependency error:
** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  chrome/browser/history/shortcuts_backend.cc
      Illegal include: "chrome/browser/search_engines/template_url.h"
    Because of "-chrome/browser" from chrome/browser/history's include_rules.

I can either
1. Move the above method in autocomplete_match.{h,cc} and see if that avoids the
deps requirement.
2. Do a simple removal of match.search_terms_args.suggest_query_params from
match.destination_url


> > Also do you agree that it is better to scrub parameters at the time of
saving
> > than at the time of reading?
> 
> Some things should be scrubbed at save, some on retrieve.  I don't think we
need
> to agree on this to progress here so I'd rather not debate it.

Aah. I just meant to ask if scrubbing is easier at the time of saving for this
specific case of saving search-suggest suggestions.

[Peter Kasting, 2014-02-19 20:32:23]

On 2014/02/15 01:20:05, Anuj wrote:
> In either approach, I will need access to profile to generate the new
> destination_url

There's one ShortcutsBackend per profile, and the Profile gets passed in to the
ShortcutsBackend constructor, so that class can certainly keep the pointer
around.  Then it could pass that profile to the MatchCore constructor as a
second arg.  Does that solve the problem?

[Anuj, 2014-02-22 01:16:54]

On 2014/02/19 20:32:23, Peter Kasting wrote:
> On 2014/02/15 01:20:05, Anuj wrote:
> > In either approach, I will need access to profile to generate the new
> > destination_url
> 
> There's one ShortcutsBackend per profile, and the Profile gets passed in to
the
> ShortcutsBackend constructor, so that class can certainly keep the pointer
> around.  Then it could pass that profile to the MatchCore constructor as a
> second arg.  Does that solve the problem?

The problem is not the need for profile. But the resulting dependency violation.

[Peter Kasting, 2014-02-22 01:40:11]

On 2014/02/22 01:16:54, Anuj wrote:
> On 2014/02/19 20:32:23, Peter Kasting wrote:
> > On 2014/02/15 01:20:05, Anuj wrote:
> > > In either approach, I will need access to profile to generate the new
> > > destination_url
> > 
> > There's one ShortcutsBackend per profile, and the Profile gets passed in to
> the
> > ShortcutsBackend constructor, so that class can certainly keep the pointer
> > around.  Then it could pass that profile to the MatchCore constructor as a
> > second arg.  Does that solve the problem?
> 
> The problem is not the need for profile. But the resulting dependency
violation.

Do you have to violate that rule?  It seems like shortcuts_provider.cc can call
CreateSearchSuggestion() and pass it the TemplateURL from the match without
having to #include template_url.h.

[Anuj, 2014-02-22 02:14:00]

On 2014/02/22 01:40:11, Peter Kasting wrote:
> On 2014/02/22 01:16:54, Anuj wrote:
> > The problem is not the need for profile. But the resulting dependency
> > violation.
> 
> Do you have to violate that rule?  It seems like shortcuts_provider.cc can
call
> CreateSearchSuggestion() and pass it the TemplateURL from the match without
> having to #include template_url.h.

Not sure what am I missing here. ShortcutsProvider does not come into picture
when saving the suggestion.
Or do you mean to say that we should save destination url as is, and reconstruct
it using fill_into_edit as search terms?
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...)

[Peter Kasting, 2014-02-22 02:15:09]

On 2014/02/22 02:14:00, Anuj wrote:
> On 2014/02/22 01:40:11, Peter Kasting wrote:
> > On 2014/02/22 01:16:54, Anuj wrote:
> > > The problem is not the need for profile. But the resulting dependency
> > > violation.
> > 
> > Do you have to violate that rule?  It seems like shortcuts_provider.cc can
> call
> > CreateSearchSuggestion() and pass it the TemplateURL from the match without
> > having to #include template_url.h.
> 
> Not sure what am I missing here. ShortcutsProvider does not come into picture
> when saving the suggestion.

Sorry, I misspoke, I meant ShortcutsBackend.

[Anuj, 2014-02-28 00:47:48]

On 2014/02/22 02:15:09, Peter Kasting wrote:
> On 2014/02/22 02:14:00, Anuj wrote:
> > On 2014/02/22 01:40:11, Peter Kasting wrote:
> > > On 2014/02/22 01:16:54, Anuj wrote:
> > > > The problem is not the need for profile. But the resulting dependency
> > > > violation.
> > > 
> > > Do you have to violate that rule?  It seems like shortcuts_provider.cc can
> > call
> > > CreateSearchSuggestion() and pass it the TemplateURL from the match
without
> > > having to #include template_url.h.
> > 
> > Not sure what am I missing here. ShortcutsProvider does not come into
picture
> > when saving the suggestion.
> 
> Sorry, I misspoke, I meant ShortcutsBackend.

even CreateSearchSuggestion from BaseSearchProvider
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...)
will violate DEPS
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/his...

[Peter Kasting, 2014-02-28 00:58:39]

On 2014/02/28 00:47:48, Anuj wrote:
> On 2014/02/22 02:15:09, Peter Kasting wrote:
> > On 2014/02/22 02:14:00, Anuj wrote:
> > > On 2014/02/22 01:40:11, Peter Kasting wrote:
> > > > On 2014/02/22 01:16:54, Anuj wrote:
> > > > > The problem is not the need for profile. But the resulting dependency
> > > > > violation.
> > > > 
> > > > Do you have to violate that rule?  It seems like shortcuts_provider.cc
can
> > > call
> > > > CreateSearchSuggestion() and pass it the TemplateURL from the match
> without
> > > > having to #include template_url.h.
> > > 
> > > Not sure what am I missing here. ShortcutsProvider does not come into
> picture
> > > when saving the suggestion.
> > 
> > Sorry, I misspoke, I meant ShortcutsBackend.
> 
> even CreateSearchSuggestion from BaseSearchProvider
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...)
> will violate DEPS
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/his...

In the limit we may need to move the shortcuts backend into the autocomplete
directory so it can depend on other content there.  The only reason its existing
dependencies pass is because they're temporarily whitelisted, so it seems like
eventually this will have to happen anyway.

[Anuj, 2014-03-14 20:48:12]

I tried moving shortcuts_backend* stuff to chrome/browser/autocomplete. But the
ShortcutsBackend is in history namespace. Also, ShortcutsDatabase depends on
shortcuts_backend.h for definition of MatchCore.

On the whole, it seemed the refactoring will eclipse the original intent of the
CL.

Let me know if I should file a tracking bug for refactoring.

[Peter Kasting, 2014-03-14 21:00:58]

On 2014/03/14 20:48:12, Anuj wrote:
> I tried moving shortcuts_backend* stuff to chrome/browser/autocomplete. But
the
> ShortcutsBackend is in history namespace.

Yes, it should be un-namespaced.  That doesn't seem like it should require much
code change?  Or is there something major that this causes?

> Also, ShortcutsDatabase depends on
> shortcuts_backend.h for definition of MatchCore.

We can solve that by moving MatchCore into shortcuts_database.h, right?

> On the whole, it seemed the refactoring will eclipse the original intent of
the
> CL.

I guess it depends on the answers to the above questions.  If this is mostly
mechanical and doesn't break the world, we could just do it in another CL and
land it, then get back to this one.  If there are serious functional changes
that need to happen, we could postpone that.

[Anuj, 2014-03-14 21:05:29]

On 2014/03/14 21:00:58, Peter Kasting wrote:
> On 2014/03/14 20:48:12, Anuj wrote:
> > I tried moving shortcuts_backend* stuff to chrome/browser/autocomplete. But
> the
> > ShortcutsBackend is in history namespace.
> 
> Yes, it should be un-namespaced.  That doesn't seem like it should require
much
> code change?  Or is there something major that this causes?

The ShortcutsProviderTest has been set in namespace history.

> > Also, ShortcutsDatabase depends on
> > shortcuts_backend.h for definition of MatchCore.
> 
> We can solve that by moving MatchCore into shortcuts_database.h, right?

Yes.
> 
> > On the whole, it seemed the refactoring will eclipse the original intent of
> the
> > CL.
> 
> I guess it depends on the answers to the above questions.  If this is mostly
> mechanical and doesn't break the world, we could just do it in another CL and
> land it, then get back to this one.  If there are serious functional changes
> that need to happen, we could postpone that.

I hope it is mostly mechanical. But I will prefer landing this before the
refactoring, as this solves a bug.

[Anuj, 2014-03-14 21:08:29]

On 2014/03/14 21:05:29, Anuj wrote:
> On 2014/03/14 21:00:58, Peter Kasting wrote:
> > On 2014/03/14 20:48:12, Anuj wrote:
> > > I tried moving shortcuts_backend* stuff to chrome/browser/autocomplete.
But
> > the
> > > ShortcutsBackend is in history namespace.
> > 
> > Yes, it should be un-namespaced.  That doesn't seem like it should require
> much
> > code change?  Or is there something major that this causes?
> 
> The ShortcutsProviderTest has been set in namespace history.
> 
> > > Also, ShortcutsDatabase depends on
> > > shortcuts_backend.h for definition of MatchCore.
> > 
> > We can solve that by moving MatchCore into shortcuts_database.h, right?
> 
> Yes.
> > 
> > > On the whole, it seemed the refactoring will eclipse the original intent
of
> > the
> > > CL.
> > 
> > I guess it depends on the answers to the above questions.  If this is mostly
> > mechanical and doesn't break the world, we could just do it in another CL
and
> > land it, then get back to this one.  If there are serious functional changes
> > that need to happen, we could postpone that.
> 
> I hope it is mostly mechanical. But I will prefer landing this before the
> refactoring, as this solves a bug.

Also, note that the nature of fix will not change much. As the translation of
destination url from AutocompleteMatch to MatchCore will then need to happen in
shortcuts_database.cc. But shortcuts_database.cc can not call the required
methods because of dependency constraints. So the destinaiton_url will need to
be computed outside - It will probably move from omnibox_navigation_observer to
(chrome/browser/autocomplete/)shortcuts_backend.cc.

[Anuj, 2014-03-17 17:17:56]

Reminder!

[Peter Kasting, 2014-03-18 01:04:54]

On 2014/03/17 17:17:56, Anuj wrote:
> Reminder!

I'm awaiting Brett's signoff on https://codereview.chromium.org/200493006/ ,
which moves the ShortcutsBackend over to the autocomplete/ directory to help
with the layering issues in this CL.  Once that lands, hopefully my suggestions
above become more feasible.

[Peter Kasting, 2014-03-19 00:14:11]

On 2014/03/18 01:04:54, Peter Kasting wrote:
> I'm awaiting Brett's signoff on https://codereview.chromium.org/200493006/ ,
> which moves the ShortcutsBackend over to the autocomplete/ directory to help
> with the layering issues in this CL.  Once that lands, hopefully my
suggestions
> above become more feasible.

This is now landed in r257826.

[Anuj, 2014-03-20 19:13:22]

PTAL

[Peter Kasting, 2014-03-21 18:36:26]

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:345: type ==
AutocompleteMatchType::SEARCH_OTHER_ENGINE;
Nit: Can simplify by removing everything covered by IsSearchSuggestType(), and
then doing "|| IsSearchSuggestType();" at the end of the expression.

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.h:167: static bool
IsSearchSuggestType(Type type);
This sounds from the name like it should return true for SEARCH_SUGGEST, but it
doesn't.  Maybe IsSpecializedSuggestionType()?

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:35: const GURL
GetDestinationURLForShortcut(
Discussed briefly in person: Let's use CreateSearchSuggestion() instead of these
manual sanitization methods, but it's probably possible to refactor that a bit
to avoid constructing lots of huge and mostly-ignored temporary objects.

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend_unittest.cc (right):

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend_unittest.cc:191: { "0,1",    
"0,0",      AutocompleteMatchType::SEARCH_SUGGEST_ENTITY,
Nit: If you're going to indent these less, then also reduce the indenting above,
and just wrap the one long line like:

    { "0,1",     "0,0,11,2,15,0",
                             AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,

[Anuj, 2014-03-24 07:16:16]

PTAL

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:345: type ==
AutocompleteMatchType::SEARCH_OTHER_ENGINE;
On 2014/03/21 18:36:26, Peter Kasting wrote:
> Nit: Can simplify by removing everything covered by IsSearchSuggestType(), and
> then doing "|| IsSearchSuggestType();" at the end of the expression.

Done.

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.h:167: static bool
IsSearchSuggestType(Type type);
On 2014/03/21 18:36:26, Peter Kasting wrote:
> This sounds from the name like it should return true for SEARCH_SUGGEST, but
it
> doesn't.  Maybe IsSpecializedSuggestionType()?

Done.

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:35: const GURL
GetDestinationURLForShortcut(
On 2014/03/21 18:36:26, Peter Kasting wrote:
> Discussed briefly in person: Let's use CreateSearchSuggestion() instead of
these
> manual sanitization methods, but it's probably possible to refactor that a bit
> to avoid constructing lots of huge and mostly-ignored temporary objects.

The simple implementation doesn't seem too bad.

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend_unittest.cc (right):

https://codereview.chromium.org/165163002/diff/300001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend_unittest.cc:191: { "0,1",    
"0,0",      AutocompleteMatchType::SEARCH_SUGGEST_ENTITY,
On 2014/03/21 18:36:26, Peter Kasting wrote:
> Nit: If you're going to indent these less, then also reduce the indenting
above,
> and just wrap the one long line like:
> 
>     { "0,1",     "0,0,11,2,15,0",
>                              AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,

Done.

[Peter Kasting, 2014-03-24 18:59:18]

https://codereview.chromium.org/165163002/diff/340001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/165163002/diff/340001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:144: const
AutocompleteMatch::Type match_type = GetTypeForShortcut(match.type);
Nit: Can you inline this into the MatchCore() call below?  Placed here, it's
confusing that we calculate this, but then (purposefully) use match.type instead
of match_type in the next statement.

https://codereview.chromium.org/165163002/diff/340001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:151: suggestion,
Hmm.  4 levels of indenting, for many lines, is kinda sad.

What if we added a public CreateSearchSuggestion() method on BaseSearchProvider
that took only the args you need to pass?  This would result in less nesting in
both places, and would also avoid the need for a friend declaration.

https://codereview.chromium.org/165163002/diff/340001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:166: false) : match;
Nit: Break after ':' and unindent

[Anuj, 2014-03-24 21:45:49]

PTAL

https://codereview.chromium.org/165163002/diff/340001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/165163002/diff/340001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:144: const
AutocompleteMatch::Type match_type = GetTypeForShortcut(match.type);
On 2014/03/24 18:59:19, Peter Kasting wrote:
> Nit: Can you inline this into the MatchCore() call below?  Placed here, it's
> confusing that we calculate this, but then (purposefully) use match.type
instead
> of match_type in the next statement.

I ended up using match_type below.

https://codereview.chromium.org/165163002/diff/340001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:151: suggestion,
On 2014/03/24 18:59:19, Peter Kasting wrote:
> Hmm.  4 levels of indenting, for many lines, is kinda sad.
> 
> What if we added a public CreateSearchSuggestion() method on
BaseSearchProvider
> that took only the args you need to pass?  This would result in less nesting
in
> both places, and would also avoid the need for a friend declaration.

Done.

https://codereview.chromium.org/165163002/diff/340001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:166: false) : match;
On 2014/03/24 18:59:19, Peter Kasting wrote:
> Nit: Break after ':' and unindent

Done.

[Peter Kasting, 2014-03-24 21:53:06]

LGTM

https://codereview.chromium.org/165163002/diff/350001/chrome/browser/autocomp...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/165163002/diff/350001/chrome/browser/autocomp...
chrome/browser/autocomplete/base_search_provider.cc:130: const base::string16&
match_contents,
You don't seem to use this arg?

https://codereview.chromium.org/165163002/diff/350001/chrome/browser/autocomp...
chrome/browser/autocomplete/base_search_provider.cc:152: false);
Nit: Let's go ahead and omit the EOL comments and just do this like:

  return CreateSearchSuggestion(
      NULL, AutocompleteInput(), BaseSearchProvider::SuggestResult(
          suggestion, type, suggestion, base::string16(), base::string16(),
          std::string(), std::string(), from_keyword_provider, 0, false, false,
          base::string16()),
      template_url, 0, 0, false);

...just because this is so long otherwise, and almost all the args are really
"don't care" values rather than things where it's important what they mean.

[Anuj, 2014-03-25 06:32:04]

Done. submitting

https://codereview.chromium.org/165163002/diff/350001/chrome/browser/autocomp...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/165163002/diff/350001/chrome/browser/autocomp...
chrome/browser/autocomplete/base_search_provider.cc:130: const base::string16&
match_contents,
On 2014/03/24 21:53:07, Peter Kasting wrote:
> You don't seem to use this arg?

Done.

https://codereview.chromium.org/165163002/diff/350001/chrome/browser/autocomp...
chrome/browser/autocomplete/base_search_provider.cc:152: false);
On 2014/03/24 21:53:07, Peter Kasting wrote:
> Nit: Let's go ahead and omit the EOL comments and just do this like:
> 
>   return CreateSearchSuggestion(
>       NULL, AutocompleteInput(), BaseSearchProvider::SuggestResult(
>           suggestion, type, suggestion, base::string16(), base::string16(),
>           std::string(), std::string(), from_keyword_provider, 0, false,
false,
>           base::string16()),
>       template_url, 0, 0, false);
> 
> ...just because this is so long otherwise, and almost all the args are really
> "don't care" values rather than things where it's important what they mean.

Done.

[Anuj, 2014-03-25 06:32:09]

The CQ bit was checked by skanuj@chromium.org

[commit-bot: I haz the power, 2014-03-25 06:32:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/165163002/370001

[commit-bot: I haz the power, 2014-03-25 12:50:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/165163002/370001

[commit-bot: I haz the power, 2014-03-25 15:33:45]

Change committed as 259206

[erikchen, 2014-03-25 17:50:17]

A revert of this CL has been created in
https://codereview.chromium.org/211283005/ by erikchen@chromium.org.

The reason for reverting is:
http://build.chromium.org/p/chromium.win/builders/Interactive%20Tests%20%28db....

[Anuj, 2014-03-25 20:11:59]

https://codereview.chromium.org/165163002/diff/370001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/165163002/diff/370001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:143: const base::string16&
suggestion = match.search_terms_args->search_terms;
Accessing search terms without knowing if the match is of search type caused
problem here.

[Peter Kasting, 2014-03-25 20:40:52]

https://codereview.chromium.org/165163002/diff/370001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/165163002/diff/370001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:143: const base::string16&
suggestion = match.search_terms_args->search_terms;
On 2014/03/25 20:12:00, Anuj wrote:
> Accessing search terms without knowing if the match is of search type caused
> problem here.

Looks like this is fixable by just inlining that temp into the one use below,
now that the refactored CreateSearchSuggestion() removed the need to refer to
|suggestion| repeatedly here.