Handle Search suggest subtypes for Shortcuts DB

chrome/browser/history/shortcuts_backend.cc

Index: chrome/browser/history/shortcuts_backend.cc
diff --git a/chrome/browser/history/shortcuts_backend.cc b/chrome/browser/history/shortcuts_backend.cc
index a5abb3fedfe95669d6d40076a76b536daef4a9a1..be90ad070c2e221d6596b2e586fc479134a08366 100644
--- a/chrome/browser/history/shortcuts_backend.cc
+++ b/chrome/browser/history/shortcuts_backend.cc
@@ -13,6 +13,7 @@
 #include "base/guid.h"
 #include "base/i18n/case_conversion.h"
 #include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/autocomplete/autocomplete_result.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ -21,6 +22,7 @@
 #include "chrome/browser/history/shortcuts_database.h"
 #include "chrome/browser/omnibox/omnibox_log.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/autocomplete_match_type.h"
 #include "chrome/common/chrome_constants.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
@@ -31,19 +33,52 @@ using content::BrowserThread;
 
 namespace {
 
+bool IsSearchSuggestType(AutocompleteMatchType::Type type) {
+  switch (type) {
+    case AutocompleteMatchType::SEARCH_SUGGEST_INFINITE:
+    case AutocompleteMatchType::SEARCH_SUGGEST_ENTITY:
+    case AutocompleteMatchType::SEARCH_SUGGEST_PERSONALIZED:
+    case AutocompleteMatchType::SEARCH_SUGGEST_PROFILE:
+      return true;
+
+    default:
+      return false;
+  }
+}
+
+const GURL GetDestinationURLForShortcut(const AutocompleteMatch& match) {
+  return IsSearchSuggestType(match.type) ?
+      match.stripped_destination_url : match.destination_url;

[Peter Kasting, 2014/02/14 05:35:25]

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.

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

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?

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

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]

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.
Cool. Done.

+}
+
+const base::string16 GetContentsForShortcut(const AutocompleteMatch& match) {
+  return IsSearchSuggestType(match.type) ?
+      match.fill_into_edit : match.contents;
+}
+
+const base::string16 GetDescriptionForShortcut(const AutocompleteMatch& match) {
+  return IsSearchSuggestType(match.type) ? base::string16() : match.description;

[Peter Kasting, 2014/02/14 05:35:25]

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]

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

+}
+
 // Takes Match classification vector and removes all matched positions,
 // compacting repetitions if necessary.
 ACMatchClassifications StripMatchMarkers(
-    const ACMatchClassifications& matches) {
+    const ACMatchClassifications& classifications) {
   ACMatchClassifications unmatched;
-  for (ACMatchClassifications::const_iterator i(matches.begin());
-       i != matches.end(); ++i) {
+  for (ACMatchClassifications::const_iterator i(classifications.begin());
+       i != classifications.end(); ++i) {
     AutocompleteMatch::AddLastClassificationIfNecessary(
         &unmatched, i->offset, i->style & ~ACMatchClassification::MATCH);
   }
   return unmatched;
 }
 
+ACMatchClassifications GetClassificationsForShortcut(
+    AutocompleteMatchType::Type type,
+    const ACMatchClassifications& classifications) {
+  return IsSearchSuggestType(type) ? ACMatchClassifications() : classifications;
+}
+
 // Normally shortcuts have the same match type as the original match they were
 // created from, but for certain match types, we should modify the shortcut's
 // type slightly to reflect that the origin of the shortcut is historical.
@@ -55,6 +90,10 @@ AutocompleteMatch::Type GetTypeForShortcut(AutocompleteMatch::Type type) {
 
     case AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED:
     case AutocompleteMatchType::SEARCH_SUGGEST:
+    case AutocompleteMatchType::SEARCH_SUGGEST_ENTITY:
+    case AutocompleteMatchType::SEARCH_SUGGEST_INFINITE:
+    case AutocompleteMatchType::SEARCH_SUGGEST_PERSONALIZED:
+    case AutocompleteMatchType::SEARCH_SUGGEST_PROFILE:
       return AutocompleteMatchType::SEARCH_HISTORY;
 
     default:
@@ -71,11 +110,13 @@ namespace history {
 ShortcutsBackend::Shortcut::MatchCore::MatchCore(
     const AutocompleteMatch& match)
     : fill_into_edit(match.fill_into_edit),
-      destination_url(match.destination_url),
-      contents(match.contents),
-      contents_class(StripMatchMarkers(match.contents_class)),
-      description(match.description),
-      description_class(StripMatchMarkers(match.description_class)),
+      destination_url(GetDestinationURLForShortcut(match)),
+      contents(GetContentsForShortcut(match)),
+      contents_class(
+          GetClassificationsForShortcut(match.type, match.contents_class)),
+      description(GetDescriptionForShortcut(match)),
+      description_class(
+          GetClassificationsForShortcut(match.type, match.description_class)),
       transition(match.transition),
       type(GetTypeForShortcut(match.type)),
       keyword(match.keyword) {