Initial implementation of dedupping search provider's URLs.

chrome/browser/autocomplete/autocomplete_match.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/autocomplete/autocomplete_match.h"
 
 #include "base/i18n/time_formatting.h"
 #include "base/logging.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
@@ skipping 338 matching lines @@
 }
 
 // static
 bool AutocompleteMatch::IsSearchType(Type type) {
   return type == SEARCH_WHAT_YOU_TYPED ||
          type == SEARCH_HISTORY ||
          type == SEARCH_SUGGEST ||
          type == SEARCH_OTHER_ENGINE;
 }
 
-void AutocompleteMatch::ComputeStrippedDestinationURL() {
+void AutocompleteMatch::ComputeStrippedDestinationURL(
+    TemplateURL* search_provider_url) {
   stripped_destination_url = destination_url;
   if (!stripped_destination_url.is_valid())
     return;
 
+  // Attempt to normalize search URLs by removing non-essential substitutions
+  // like aqs/oq/aq.

[beaudoin, 2012/10/19 14:25:34]

I would not be opposed to adding a NormalizeURL to TemplateURL to perform that
operation. Wonder what pkasting would think.

+  if (search_provider_url != NULL) {
+    string16 search_terms;
+    if (search_provider_url->ExtractSearchTermsFromURL(stripped_destination_url,
+                                                       &search_terms)) {
+      // Rewrite the URL by using the extracted search terms and ignoring other
+      // arguments (it's OK, because it doesn't affect the destination URL and
+      // is sufficient from de-dupping perspective).

[beaudoin, 2012/10/19 14:25:34]

This may confuse an "image search" with a regular search, since these only
differ in their query parameters. I don't know if it's an issue here.

[Peter Kasting, 2012/10/19 22:49:33]

Uuuuugggghhh.  This is the sort of reason why I get really unhappy about the
idea of having all these totally different result sets (images, news, maps,
whatever) just be query params on a single host + path.

I don't know how to solve this problem.  The only thing I can think of is to
pass a "strict" bool arg to the TemplateURL param extraction code, and then
somehow instruct the system on which params can be allowed to differ in "strict"
mode and which can't.  (E.g. we let aqs and oq differ in strict mode and say
that anything else is important.)

This is ugly, but I don't know how else to do this.  Note that for parsing
queries out to place in the local search history, we still want non-strict
matching.

[Bart N., 2012/10/22 18:20:24]

I agree this is a problem here. Even with your proposal, we may not be safe
(what if oq/aqs are actually important for image search?).

I'd be inclined into fixing/modifying ExtractSearchTermsFromURL so it is aware
of non-search properties, like Image Search or simply by adding another output
param which could indicate what corpora it extracted query terms for (or simply
bool* is_search). Then, we would be safe to de-dup iff we are on Search.

[beaudoin, 2012/10/22 18:43:23]

The problem is that ExtractSearchTermsFromURL is "Google-agnostic". That is, it
uses only information from the TemplateURL. I guess we could add some fields in
that TemplateURL to specify "important" query parameters but the exact way to do
this and the associated logic are relatively complex if we want to keep it
generic. I think this is where PK is headed with his "strict" bool.
More generally, I wonder if we should extend TemplateURL to allow search engines
to provide a "URL specification", mapping important ref and query parameters to
some canonical field known to Chrome. For example:

"google": {
   "name": "Google",
   "keyword": "google.com",
   "favicon": "http://www.google.com/favicon.ico",
   ...
   "params": {
     "query": [ {"loc": "ref", "param": "q"}, {"loc": "query", "param": "q"} ],
     "corpus": [ {"loc": "query", "param": "tbm"} ],
     "language": [ {"loc": "query", "param": "hl"} ]
   }
}

Then we could have a TemplateURL::ParseURL(const GURL& url) returning a
Dictionary value with the known fields filled-in. This in turn could be used to
de-dupe (instead of the current approach of reconstructing a URL). In some sense
it would be a generalized ExtractSearchTermsFromURL that would recognize that
the query is made up of more than the search term.

[Bart N., 2012/10/22 19:19:42]

Yes, totally makes sense. I didn't mean to make ExtractSearchTermsFromURL Google
specific; I was more thinking in line with your proposal (register CGI params
such as it's clear for TemplateURL whether it's OK to drop given param for
de-dupping/destination. For instance, we could indicate that a given param needs
to be updated before serving the URL again.

I like your proposal above. I wonder what Peter thinks. Anyway, it sounds like a
bigger refactoring though.

[beaudoin, 2012/10/22 21:36:48]

Totally. It would probably also have to wait after the move of
template_url_prepopulate_data to JSON which I'm working on.

For the moment, I would be inclined to say dedupping-that-confuses-corpuses is
better than no-dedupping-at-all and would recommend accepting that CL with the
current design, which is probably the best we can do without a major
refactoring.

+      stripped_destination_url =
+          GURL(search_provider_url->url_ref().ReplaceSearchTerms(
+              TemplateURLRef::SearchTermsArgs(search_terms)));

[Peter Kasting, 2012/10/19 22:49:33]

In the original bug we proposed replacing the actual destination URL, not just
the one used for de-duping, with the idea that this would prevent us from
repeatedly visiting some history URL that had embedded garbage in the aqs/oq
params if that were the highest-scoring match.

[Bart N., 2012/10/22 18:20:24]

Yup, I'm totally aware of that, in fact it could have been me who proposed that,
but I don't remember.

However, I intentionally didn't implement it in first shot, because it seems
more risky to me (e.g. look at the image search example). Would you be OK if I
added it as a follow-up item rather than do it in this CL?

[beaudoin, 2012/10/22 18:43:23]

With the approach above we could maybe even reconstruct the cleaned-up URL from
the extracted structure.

[Bart N., 2012/10/22 19:19:42]

Agreed. It would be clean and Google-agnostic.

+    }
+  }
+
   // |replacements| keeps all the substitions we're going to make to
   // from {destination_url} to {stripped_destination_url}.  |need_replacement|
   // is a helper variable that helps us keep track of whether we need
   // to apply the replacement.
   bool needs_replacement = false;
   GURL::Replacements replacements;
 
   // Remove the www. prefix from the host.
   static const char prefix[] = "www.";
   static const size_t prefix_len = arraysize(prefix) - 1;
-  std::string host = destination_url.host();
+  std::string host = stripped_destination_url.host();
   if (host.compare(0, prefix_len, prefix) == 0) {
     host = host.substr(prefix_len);
     replacements.SetHostStr(host);
     needs_replacement = true;
   }
 
   // Replace https protocol with http protocol.
   if (stripped_destination_url.SchemeIs(chrome::kHttpsScheme)) {
     replacements.SetScheme(
         chrome::kHttpScheme,
         url_parse::Component(0, strlen(chrome::kHttpScheme)));
     needs_replacement = true;
   }
 
   if (needs_replacement)
-    stripped_destination_url = destination_url.ReplaceComponents(replacements);
+    stripped_destination_url = stripped_destination_url.ReplaceComponents(
+        replacements);
 }
 
 void AutocompleteMatch::GetKeywordUIState(Profile* profile,
                                           string16* keyword,
                                           bool* is_keyword_hint) const {
   *is_keyword_hint = associated_keyword.get() != NULL;
   keyword->assign(*is_keyword_hint ? associated_keyword->keyword :
       GetSubstitutingExplicitlyInvokedKeyword(profile));
 }
 
@@ skipping 61 matching lines @@
         << " is unsorted in relation to last offset of " << last_offset
         << ". Provider: " << provider_name << ".";
     DCHECK_LT(i->offset, text.length())
         << " Classification of [" << i->offset << "," << text.length()
         << "] is out of bounds for \"" << text << "\". Provider: "
         << provider_name << ".";
     last_offset = i->offset;
   }
 }
 #endif