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 340 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(Profile* profile) {
   stripped_destination_url = destination_url;
   if (!stripped_destination_url.is_valid())
     return;
 
+  // Attempt to normalize URL for de-dupping purpose by removing non-essential
+  // term substitutions like aqs/oq/aq.

[Peter Kasting, 2012/10/26 22:13:49]

Nit: URL -> the URL, purpose -> purposes.

Don't refer to "non-essential term substitutions" (or specific params) since
this code removes _everything_ parametrized.

[Bart N., 2012/10/26 23:36:55]

Done.

+  // We try to find a template URL using this match's keyword.  If not found,
+  // we pick a template that matches the destination URL's host.

[Peter Kasting, 2012/10/26 22:13:49]

Nit: This part can be omitted as it's documented in the GetTemplateURL()
declaration.  Maybe just: "If the destination URL looks like it was generated
from a TemplateURL, remove all substitutions other than the search terms.  This
allows us to eliminate cases like past search URLs from history that differ only
by some obscure query param from each other or from the search/keyword provider
matches."

[Bart N., 2012/10/26 23:36:55]

Done.

+  // Finally, we check if the template supports substitutions and if so,
+  // we use it to generate a stripped destination URL.
+  TemplateURL* template_url = GetTemplateURL(profile, true);
+  if (template_url != NULL && template_url->SupportsReplacement()) {
+    string16 search_terms;
+    if (template_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).

[Peter Kasting, 2012/10/26 22:13:49]

Nit: I'd remove this comment entirely; the suggested comment I gave above makes
this clear enough IMO.

[Bart N., 2012/10/26 23:36:55]

Done.

+      stripped_destination_url =
+          GURL(template_url->url_ref().ReplaceSearchTerms(
+              TemplateURLRef::SearchTermsArgs(search_terms)));
+    }
+  }
+
   // |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));
 }
 
 string16 AutocompleteMatch::GetSubstitutingExplicitlyInvokedKeyword(
     Profile* profile) const {
   if (transition != content::PAGE_TRANSITION_KEYWORD)
     return string16();
   const TemplateURL* t_url = GetTemplateURL(profile);
   return (t_url && t_url->SupportsReplacement()) ? keyword : string16();
 }
 
 TemplateURL* AutocompleteMatch::GetTemplateURL(Profile* profile) const {
+  return GetTemplateURL(profile, false);
+}
+
+TemplateURL* AutocompleteMatch::GetTemplateURL(Profile* profile,
+                                               bool use_host) const {
   DCHECK(profile);
-  return keyword.empty() ? NULL :
-      TemplateURLServiceFactory::GetForProfile(profile)->
-          GetTemplateURLForKeyword(keyword);
+  TemplateURLService* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(profile);
+  TemplateURL* template_url = keyword.empty() ? NULL :
+      template_url_service->GetTemplateURLForKeyword(keyword);
+  if (use_host && template_url == NULL) {
+    template_url = template_url_service->GetTemplateURLForHost(
+        destination_url.host());
+  }
+  return template_url;
 }
 
 void AutocompleteMatch::RecordAdditionalInfo(const std::string& property,
                                              const std::string& value) {
   DCHECK(property.size());
   DCHECK(value.size());
   additional_info[property] = value;
 }
 
 void AutocompleteMatch::RecordAdditionalInfo(const std::string& property,
@@ skipping 38 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