Store xsrf token received with psuggest results.

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index e4f35813feb91cacb699c4ff33e352afe0dd1845..4b6ac7d5f6af53eb35455675528625ca50c4ba70 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -45,6 +45,7 @@
 #include "chrome/common/net/url_fixer_upper.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
+#include "content/public/browser/user_metrics.h"
 #include "grit/generated_resources.h"
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
@@ -175,6 +176,65 @@ void SetAndClassifyMatchContents(const string16& query_string,
 }  // namespace
 
 
+// This class handles making requests to the server in order to delete
+// suggestions out of web history.

[Peter Kasting, 2013/11/27 21:58:19]

Nit: "suggestions out of web history" -> "personalized suggestions" ("web
history" is sort of a Google product phrase)

[Maria, 2013/11/28 00:17:03]

Done.

+class SuggestionDeletionHandler : public net::URLFetcherDelegate {
+ public:
+  SuggestionDeletionHandler();
+  virtual ~SuggestionDeletionHandler();
+
+  // Kicks off the deletion request to the server.
+  void StartRequest(

[Peter Kasting, 2013/11/27 21:58:19]

Given that our lone caller creates us and then immediately runs this function,
why is this a separate function at all?  It seems like the constructor should
take these args and perform this functionality.

[Maria, 2013/11/28 00:17:03]

I don't like putting work in constructor and generally the style guide agrees:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Doing_...

[Peter Kasting, 2013/11/28 00:44:17]

This code can neither fail nor perform virtual method calls on |this|, so it's
safe to perform at construction time.  Please put it in the constructor, it's
not only a simpler API, it's also safer and easier to reason about because we
know the relevant variables can never be unset.

+      const std::string& deletion_url,
+      Profile* profile,
+      const base::Callback<void(bool, SuggestionDeletionHandler *)>&

[Peter Kasting, 2013/11/27 21:58:19]

Nit: No space before '*'

This callback type should be a typedef.

[Maria, 2013/11/28 00:17:03]

Done.

+          callback);
+
+ private:
+  // net::URLFetcherDelegate:
+  virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
+
+  scoped_ptr<net::URLFetcher> deletion_fetcher_;
+  base::Callback<void(bool, SuggestionDeletionHandler *)> callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(SuggestionDeletionHandler);
+};
+
+
+// SuggestionDeletionHandler ---------------------------------

[Peter Kasting, 2013/11/27 21:58:19]

Nit: This goes above the declaration.  Extend the "-"s out to column 79.

[Maria, 2013/11/28 00:17:03]

Done.

+
+SuggestionDeletionHandler::SuggestionDeletionHandler() {
+};
+
+SuggestionDeletionHandler::~SuggestionDeletionHandler() {
+};
+
+void SuggestionDeletionHandler::StartRequest(
+    const std::string& deletion_url,
+    Profile* profile,
+    const base::Callback<void(bool, SuggestionDeletionHandler *)>& callback) {
+  callback_ = callback;
+  GURL url(deletion_url);
+  DCHECK(url.is_valid());
+
+  deletion_fetcher_.reset(net::URLFetcher::Create(
+      SearchProvider::kDeletionURLFetcherID,
+      url,
+      net::URLFetcher::GET,
+      this));
+  deletion_fetcher_->SetRequestContext(profile->GetRequestContext());
+  deletion_fetcher_->Start();
+};
+
+void SuggestionDeletionHandler::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  DCHECK(source == deletion_fetcher_.get());
+  callback_.Run(
+      source->GetStatus().is_success() && (source->GetResponseCode() == 200),
+      this);
+};
+
+
 // SearchProvider::Providers --------------------------------------------------
 
 SearchProvider::Providers::Providers(TemplateURLService* template_url_service)
@@ -213,6 +273,7 @@ SearchProvider::SuggestResult::SuggestResult(
     const string16& match_contents,
     const string16& annotation,
     const std::string& suggest_query_params,
+    const std::string& deletion_url,
     bool from_keyword_provider,
     int relevance,
     bool relevance_from_server,
@@ -222,6 +283,7 @@ SearchProvider::SuggestResult::SuggestResult(
       match_contents_(match_contents),
       annotation_(annotation),
       suggest_query_params_(suggest_query_params),
+      deletion_url_(deletion_url),
       should_prefetch_(should_prefetch) {
 }
 
@@ -326,10 +388,12 @@ bool SearchProvider::Results::HasServerProvidedScores() const {
 // static
 const int SearchProvider::kDefaultProviderURLFetcherID = 1;
 const int SearchProvider::kKeywordProviderURLFetcherID = 2;
+const int SearchProvider::kDeletionURLFetcherID = 3;
 int SearchProvider::kMinimumTimeBetweenSuggestQueriesMs = 100;
 const char SearchProvider::kRelevanceFromServerKey[] = "relevance_from_server";
 const char SearchProvider::kShouldPrefetchKey[] = "should_prefetch";
 const char SearchProvider::kSuggestMetadataKey[] = "suggest_metadata";
+const char SearchProvider::kDeletionUrlKey[] = "deletion_url";
 const char SearchProvider::kTrue[] = "true";
 const char SearchProvider::kFalse[] = "false";
 
@@ -434,6 +498,21 @@ void SearchProvider::AddProviderInfo(ProvidersInfo* provider_info) const {
   }
 }
 
+void SearchProvider::DeleteMatch(const AutocompleteMatch& match) {
+  // TODO(mariakhomenko): Add support for deleting search history suggestions.
+  DCHECK(match.deletable);
+
+  std::string deletion_url = match.GetAdditionalInfo(
+      SearchProvider::kDeletionUrlKey);

[Peter Kasting, 2013/11/27 21:58:19]

Nit: Just inline this below

[Maria, 2013/11/28 00:17:03]

Done.

+
+  deletion_handlers_.push_back(new SuggestionDeletionHandler());
+  deletion_handlers_.back()->StartRequest(deletion_url, profile_,
+      base::Bind(&SearchProvider::OnDeletionComplete, base::Unretained(this)));
+  // Immediately updates the list of matches to show the match was deleted,

[Peter Kasting, 2013/11/27 21:58:19]

Nit: updates -> update

I'd put a blank line above this as well.

[Maria, 2013/11/28 00:17:03]

Done.

+  // regardless of whether the server request actually succeeds.
+  DeleteMatchFromMatches(match);
+}
+
 void SearchProvider::ResetSession() {
   field_trial_triggered_in_session_ = false;
 }
@@ -638,6 +717,7 @@ void SearchProvider::OnURLFetchComplete(const net::URLFetcher* source) {
         source->GetResponseHeaders();
     std::string json_data;
     source->GetResponseAsString(&json_data);
+
     // JSON is supposed to be UTF-8, but some suggest service providers send
     // JSON files in non-UTF-8 encodings.  The actual encoding is usually
     // specified in the Content-Type header field.
@@ -678,6 +758,42 @@ void SearchProvider::OnURLFetchComplete(const net::URLFetcher* source) {
     listener_->OnProviderUpdate(results_updated);
 }
 
+void SearchProvider::OnDeletionComplete(bool success,
+                                        SuggestionDeletionHandler* handler) {
+  if (success) {
+    content::RecordAction(
+        content::UserMetricsAction("Omnibox.ServerSuggestDelete.Success"));
+  } else {
+    content::RecordAction(
+        content::UserMetricsAction("Omnibox.ServerSuggestDelete.Failure"));
+  }
+
+  ScopedVector<SuggestionDeletionHandler>::iterator it = std::find(
+      deletion_handlers_.begin(), deletion_handlers_.end(), handler);
+  if (it != deletion_handlers_.end()) {
+    deletion_handlers_.erase(it);
+  } else {
+    NOTREACHED() << "Deletion handler should be in the vector of all handlers.";

[Peter Kasting, 2013/11/27 21:58:19]

Nit: :Don't handle DCHECK failure" style rule means this should just be:

  DCHECK(it != deletion_handlers_.end());
  deletion_handlers_.erase(it);

[Maria, 2013/11/28 00:17:03]

Done.

+  }
+}
+
+void SearchProvider::DeleteMatchFromMatches(const AutocompleteMatch& match) {
+  bool found = false;
+  for (ACMatches::iterator i(matches_.begin()); i != matches_.end(); ++i) {
+    if (i->contents == match.contents && i->type == match.type) {
+      // This item is a match based on contents, this works correctly for
+      // server-provided personal suggestions, but would need to be carefully
+      // re-examined if you are adding other deletable types.

[Peter Kasting, 2013/11/27 21:58:19]

Nit: How about moving this above the conditional and changing to:

Find the desired match to delete by checking the type and contents.  We can't
check the destination URL, because the autocomplete controller may have
reformulated that.  Note that while checking for matching contents works for
personalized suggestions, if more match types gain deletion support, this
algorithm may need to be re-examined.

[Maria, 2013/11/28 00:17:03]

Done.

+      found = true;
+      matches_.erase(i);
+      break;
+    }
+  }
+  DCHECK(found) << "Asked to delete a search suggestion that isn't in our set "
+      << "of matches";

[Peter Kasting, 2013/11/27 21:58:19]

Nit: I don't think the DCHECK here is of enough value to warrant the existence
of |found|; I'd just remove all that

[Maria, 2013/11/28 00:17:03]

Done.

+  listener_->OnProviderUpdate(true);
+}
+
 void SearchProvider::Run() {
   // Start a new request with the current input.
   suggest_results_pending_ = 0;
@@ -935,69 +1051,6 @@ void SearchProvider::ApplyCalculatedNavigationRelevance(
   }
 }
 
-bool SearchProvider::CanSendURL(
-    const GURL& current_page_url,
-    const GURL& suggest_url,
-    const TemplateURL* template_url,
-    AutocompleteInput::PageClassification page_classification,
-    Profile* profile) {
-  if (!current_page_url.is_valid())
-    return false;
-
-  // TODO(hfung): Show Most Visited on NTP with appropriate verbatim
-  // description when the user actively focuses on the omnibox as discussed in
-  // crbug/305366 if Most Visited (or something similar) will launch.
-  if ((page_classification ==
-       AutocompleteInput::INSTANT_NTP_WITH_FAKEBOX_AS_STARTING_FOCUS) ||
-      (page_classification ==
-       AutocompleteInput::INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS))
-    return false;
-
-  // Only allow HTTP URLs or HTTPS URLs for the same domain as the search
-  // provider.
-  if ((current_page_url.scheme() != content::kHttpScheme) &&
-      ((current_page_url.scheme() != content::kHttpsScheme) ||
-       !net::registry_controlled_domains::SameDomainOrHost(
-           current_page_url, suggest_url,
-           net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES)))
-    return false;
-
-  // Make sure we are sending the suggest request through HTTPS to prevent
-  // exposing the current page URL to networks before the search provider.
-  if (!suggest_url.SchemeIs(content::kHttpsScheme))
-    return false;
-
-  // Don't run if there's no profile or in incognito mode.
-  if (profile == NULL || profile->IsOffTheRecord())
-    return false;
-
-  // Don't run if we can't get preferences or search suggest is not enabled.
-  PrefService* prefs = profile->GetPrefs();
-  if (!prefs->GetBoolean(prefs::kSearchSuggestEnabled))
-    return false;
-
-  // Only make the request if we know that the provider supports zero suggest
-  // (currently only the prepopulated Google provider).
-  if (template_url == NULL || !template_url->SupportsReplacement() ||
-      TemplateURLPrepopulateData::GetEngineType(*template_url) !=
-      SEARCH_ENGINE_GOOGLE)
-    return false;
-
-  // Check field trials and settings allow sending the URL on suggest requests.
-  ProfileSyncService* service =
-      ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile);
-  browser_sync::SyncPrefs sync_prefs(prefs);
-  if (!OmniboxFieldTrial::InZeroSuggestFieldTrial() ||
-      service == NULL ||
-      !service->IsSyncEnabledAndLoggedIn() ||
-      !sync_prefs.GetPreferredDataTypes(syncer::UserTypes()).Has(
-          syncer::PROXY_TABS) ||
-      service->GetEncryptedDataTypes().Has(syncer::SESSIONS))
-    return false;
-
-  return true;
-}
-
 net::URLFetcher* SearchProvider::CreateSuggestFetcher(
     int id,
     const TemplateURL* template_url,
@@ -1134,6 +1187,7 @@ bool SearchProvider::ParseSuggestResults(Value* root_val, bool is_keyword) {
       string16 disambiguating_query;
       string16 annotation;
       std::string suggest_query_params;
+      std::string deletion_url;
       if (suggestion_details && (type == "ENTITY") &&
           suggestion_details->GetDictionary(index, &suggestion_detail) &&
           suggestion_detail) {
@@ -1142,11 +1196,15 @@ bool SearchProvider::ParseSuggestResults(Value* root_val, bool is_keyword) {
             !disambiguating_query.empty())
           suggestion = disambiguating_query;
         suggestion_detail->GetString("q", &suggest_query_params);
+      } else if (suggestion_details && (type == "PERSONALIZED_QUERY") &&
+          suggestion_details->GetDictionary(index, &suggestion_detail) &&
+          suggestion_detail) {
+        suggestion_detail->GetString("du", &deletion_url);
       }
       // TODO(kochi): Improve calculator suggestion presentation.
       results->suggest_results.push_back(SuggestResult(
           suggestion, match_contents, annotation, suggest_query_params,
-          is_keyword, relevance, true, should_prefetch));
+          deletion_url, is_keyword, relevance, true, should_prefetch));
     }
   }
 
@@ -1206,6 +1264,7 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
                   input_.text(),
                   did_not_accept_default_suggestion,
                   std::string(),
+                  std::string(),
                   &map);
   }
   if (!keyword_input_.text().empty()) {
@@ -1234,6 +1293,7 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
                       keyword_input_.text(),
                       did_not_accept_keyword_suggestion,
                       std::string(),
+                      std::string(),
                       &map);
       }
     }
@@ -1542,6 +1602,7 @@ void SearchProvider::AddHistoryResultsToMap(const HistoryResults& results,
                   i->suggestion(),
                   did_not_accept_suggestion,
                   std::string(),
+                  std::string(),
                   map);
   }
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.AddHistoryResultsTime",
@@ -1592,7 +1653,7 @@ SearchProvider::SuggestResults SearchProvider::ScoreHistoryResults(
         prevent_search_history_inlining);
     scored_results.push_back(
         SuggestResult(i->term, string16(), string16(), std::string(),
-                      is_keyword, relevance, false, false));
+                      std::string(), is_keyword, relevance, false, false));
   }
 
   // History returns results sorted for us.  However, we may have docked some
@@ -1631,6 +1692,7 @@ void SearchProvider::AddSuggestResultsToMap(const SuggestResults& results,
                   results[i].suggestion(),
                   i,
                   results[i].suggest_query_params(),
+                  results[i].deletion_url(),
                   map);
   }
 }
@@ -1755,6 +1817,7 @@ void SearchProvider::AddMatchToMap(const string16& input_text,
                                    const string16& query_string,
                                    int accepted_suggestion,
                                    const std::string& suggest_query_params,
+                                   const std::string& deletion_url,
                                    MatchMap* map) {
   // On non-mobile, ask the instant controller for the appropriate start margin.
   // On mobile the start margin is unused, so leave the value as default there.
@@ -1787,6 +1850,11 @@ void SearchProvider::AddMatchToMap(const string16& input_text,
   match.RecordAdditionalInfo(kShouldPrefetchKey,
                              should_prefetch ? kTrue : kFalse);
 
+  if (!deletion_url.empty() && GURL(deletion_url).is_valid()) {
+    match.RecordAdditionalInfo(kDeletionUrlKey, deletion_url);
+    match.deletable = true;
+  }
+
   // Metadata is needed only for prefetching queries.
   if (should_prefetch)
     match.RecordAdditionalInfo(kSuggestMetadataKey, metadata);
@@ -1952,3 +2020,66 @@ void SearchProvider::UpdateDone() {
   // pending, and we're not waiting on Instant.
   done_ = !timer_.IsRunning() && (suggest_results_pending_ == 0);
 }
+
+bool SearchProvider::CanSendURL(
+    const GURL& current_page_url,
+    const GURL& suggest_url,
+    const TemplateURL* template_url,
+    AutocompleteInput::PageClassification page_classification,
+    Profile* profile) {
+  if (!current_page_url.is_valid())
+    return false;
+
+  // TODO(hfung): Show Most Visited on NTP with appropriate verbatim
+  // description when the user actively focuses on the omnibox as discussed in
+  // crbug/305366 if Most Visited (or something similar) will launch.
+  if ((page_classification ==
+       AutocompleteInput::INSTANT_NTP_WITH_FAKEBOX_AS_STARTING_FOCUS) ||
+      (page_classification ==
+       AutocompleteInput::INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS))
+    return false;
+
+  // Only allow HTTP URLs or HTTPS URLs for the same domain as the search
+  // provider.
+  if ((current_page_url.scheme() != content::kHttpScheme) &&
+      ((current_page_url.scheme() != content::kHttpsScheme) ||
+       !net::registry_controlled_domains::SameDomainOrHost(
+           current_page_url, suggest_url,
+           net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES)))
+    return false;
+
+  // Make sure we are sending the suggest request through HTTPS to prevent
+  // exposing the current page URL to networks before the search provider.
+  if (!suggest_url.SchemeIs(content::kHttpsScheme))
+    return false;
+
+  // Don't run if there's no profile or in incognito mode.
+  if (profile == NULL || profile->IsOffTheRecord())
+    return false;
+
+  // Don't run if we can't get preferences or search suggest is not enabled.
+  PrefService* prefs = profile->GetPrefs();
+  if (!prefs->GetBoolean(prefs::kSearchSuggestEnabled))
+    return false;
+
+  // Only make the request if we know that the provider supports zero suggest
+  // (currently only the prepopulated Google provider).
+  if (template_url == NULL || !template_url->SupportsReplacement() ||
+      TemplateURLPrepopulateData::GetEngineType(*template_url) !=
+      SEARCH_ENGINE_GOOGLE)
+    return false;
+
+  // Check field trials and settings allow sending the URL on suggest requests.
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile);
+  browser_sync::SyncPrefs sync_prefs(prefs);
+  if (!OmniboxFieldTrial::InZeroSuggestFieldTrial() ||
+      service == NULL ||
+      !service->IsSyncEnabledAndLoggedIn() ||
+      !sync_prefs.GetPreferredDataTypes(syncer::UserTypes()).Has(
+          syncer::PROXY_TABS) ||
+      service->GetEncryptedDataTypes().Has(syncer::SESSIONS))
+    return false;
+
+  return true;
+}