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 2e79b08f3841cd8c1562bf669ea5ce13ea10a12b..d95ebd56d059ad1ceda733cfbfa9dd74492043ee 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -213,6 +213,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 +223,7 @@ SearchProvider::SuggestResult::SuggestResult(
       match_contents_(match_contents),
       annotation_(annotation),
       suggest_query_params_(suggest_query_params),
+      deletion_url_(deletion_url),
       should_prefetch_(should_prefetch) {
 }
 
@@ -321,15 +323,51 @@ bool SearchProvider::Results::HasServerProvidedScores() const {
 }
 
 
+// SearchProvider::SuggestionDeletionHandler ---------------------------------
+SearchProvider::SuggestionDeletionHandler::SuggestionDeletionHandler() {

[Peter Kasting, 2013/11/23 00:08:43]

Nit: Blank line above this

[Maria, 2013/11/26 02:36:27]

Done.

+};
+
+SearchProvider::SuggestionDeletionHandler::~SuggestionDeletionHandler() {

[Peter Kasting, 2013/11/23 00:08:43]

Nit: Function definition order should match the class' declaration order

[Maria, 2013/11/26 02:36:27]

Done.

Quick question: that doesn't seem currently true for search_provider.cc in
general. I grouped the functions together by usage -- should I rearrange them to
declaration order in .h file instead?

[Peter Kasting, 2013/11/26 02:46:22]

All class definitions in all files should always match the class declaration
order.

If an existing file doesn't conform to this, at least put your new functions in
some sort-of-compliant spot, e.g. just below the definition of whatever the
immediate prior function declaration was.  Better yet, make the file compliant,
either in this change or a separate one.

[Maria, 2013/11/27 02:18:06]

Done.

+};
+
+void SearchProvider::SuggestionDeletionHandler::StartRequest(
+    const std::string& deletion_url, Profile* profile,

[Peter Kasting, 2013/11/23 00:08:43]

Nit: One arg per line

[Maria, 2013/11/26 02:36:27]

Done.

+    const base::Callback<void(bool)>& callback) {
+  callback_ = callback;
+  GURL url = GURL(deletion_url);

[Peter Kasting, 2013/11/23 00:08:43]

Nit: GURL url(deletion_url);

[Maria, 2013/11/26 02:36:27]

Done. Sorry, Java habits die hard.

+  if (url.is_valid()) {

[Peter Kasting, 2013/11/23 00:08:43]

Perhaps we should check GURL validity in AddMatchToMap().  Then we could avoid
marking matches deletable which had an invalid URL, which will be a win once the
"deletable" state is exposed to users in the UI.

At that point this could be a DCHECK.

[Maria, 2013/11/26 02:36:27]

Done.

+    deletion_fetcher_.reset(net::URLFetcher::Create(
+        SearchProvider::kDeletionURLFetcherID,
+        url,
+        net::URLFetcher::GET,
+        this));
+    deletion_fetcher_->SetRequestContext(profile->GetRequestContext());
+    deletion_fetcher_->Start();
+  } else {
+    callback_.Run(false);
+  }
+};
+
+void SearchProvider::SuggestionDeletionHandler::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  DCHECK(source == deletion_fetcher_.get());
+  const bool was_deletion_success = source->GetStatus().is_success() &&
+      source->GetResponseCode() == 200;

[Peter Kasting, 2013/11/23 00:08:43]

Nit: Parens around binary subexprs

Optional: Inline into next statement (meaning is pretty clear)

[Maria, 2013/11/26 02:36:27]

Done.

+  callback_.Run(was_deletion_success);
+  delete this;
+};
+

[Peter Kasting, 2013/11/23 00:08:43]

Nit: One more blank line here

[Maria, 2013/11/26 02:36:27]

Done.

 // SearchProvider -------------------------------------------------------------
 
 // 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";
 
@@ -638,6 +676,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.
@@ -1134,6 +1173,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 +1182,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 +1250,7 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
                   input_.text(),
                   did_not_accept_default_suggestion,
                   std::string(),
+                  std::string(),
                   &map);
   }
   if (!keyword_input_.text().empty()) {
@@ -1234,6 +1279,7 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
                       keyword_input_.text(),
                       did_not_accept_keyword_suggestion,
                       std::string(),
+                      std::string(),
                       &map);
       }
     }
@@ -1508,6 +1554,7 @@ void SearchProvider::AddHistoryResultsToMap(const HistoryResults& results,
                   i->suggestion(),
                   did_not_accept_suggestion,
                   std::string(),
+                  std::string(),
                   map);
   }
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.AddHistoryResultsTime",
@@ -1558,7 +1605,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
@@ -1597,6 +1644,7 @@ void SearchProvider::AddSuggestResultsToMap(const SuggestResults& results,
                   results[i].suggestion(),
                   i,
                   results[i].suggest_query_params(),
+                  results[i].deletion_url(),
                   map);
   }
 }
@@ -1709,6 +1757,43 @@ int SearchProvider::CalculateRelevanceForHistory(
   return std::max(0, base_score - score_discount);
 }
 
+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);
+  // Allocate on the heap, class will delete itself when the request is done.

[Peter Kasting, 2013/11/23 00:08:43]

I think the SearchProvider should probably own this object (e.g. stick it on a
ScopedVector).  This way, during program shutdown, any still-in-progress fetches
will be auto-canceled.

[Maria, 2013/11/26 02:36:27]

Done.

+  SuggestionDeletionHandler* deletionHandler = new SuggestionDeletionHandler();
+  deletionHandler->StartRequest(deletion_url, profile_,
+      base::Bind(&SearchProvider::OnDeletionComplete, base::Unretained(this)));
+  // Immediately updates the list of matches to show the match was deleted,
+  // regardless of whether the server request actually succeeds.
+  DeleteMatchFromMatches(match);
+}
+
+void SearchProvider::OnDeletionComplete(bool success) {

[Peter Kasting, 2013/11/23 00:08:43]

If we check URL validity in AddMatchToMap(), then I think this can only be
called back after the fetch completes; at which point this function can be
inlined into the deletion handler.

[Maria, 2013/11/26 02:36:27]

True, but I thought this may be a more flexible approach. It's possible we may
need to add to this function in the future to call something to notify the UI
that the deletion was successful (since some of the UI mock variations have a
confirmation message for deletion). It would be helpful if the function was run
in the SearchProvider class and had access to search provider members. Does that
sound reasonable?

[Peter Kasting, 2013/11/26 02:46:22]

I would say, let's add that capability when it becomes necessary, unless you are
already in the process of adding it in a followup patch.

In general, my rule is, always make the code as simple as possible for the
current capabilities, and explicitly ignore any design possibilities that aren't
100% guaranteed.

[Maria, 2013/11/27 02:18:06]

Now this callback has another use which is to delete the deletion handler that
finished (which we have to do from search_provider class since it owns the
handlers). I think it makes sense to keep -- this is also a common pattern used
by other url fetchers.

+  if (success) {
+    UMA_HISTOGRAM_COUNTS("Omnibox.ServerSuggestDelete.Success", 1);

[Peter Kasting, 2013/11/23 00:08:43]

Nit: Is it possible to use ?: to simplify this to a single
UMA_HISTOGRAM_COUNTS() invocation?  Or do we have a utility that scans the
source code for explicit strings that follow this macro?  (I seem to remember
some such thing for some kind of stat collection, but I can't recall the
details.)

Also, shouldn't we be defining this in histograms.xml?

[Maria, 2013/11/26 02:36:27]

I read a bit about this and figured we should be using UserMetricsAction here
instead. I am adding an update to tools/metrics/actions/chromeactions.txt for
that and yes, it cannot use :? type logic because there's some processing done
on the source to extract the action -- and the call along with the constant
string need to appear on the same line for it to work correctly.

+  } else {
+    UMA_HISTOGRAM_COUNTS("Omnibox.ServerSuggestDelete.Failure", 1);
+  }
+}
+
+void SearchProvider::DeleteMatchFromMatches(const AutocompleteMatch& match) {

[Peter Kasting, 2013/11/23 00:08:43]

The body of this function is akin to the identically-named function in
history_provider.h.

Perhaps we should define a function in AutocompleteProvider, e.g.
FindMatchForDeletion(), that returns an iterator to the appropriate match (and
NOTREACHEDs on failure).  Then the caller can decide how to handle this.  This
would centralized the comparison portion of this function, so if we decide to
change it at some point, all providers will be updated simultaneously.

[Maria, 2013/11/26 02:36:27]

It is a similar function, but it's not identical. In the other implementation
they check the URL of the match is identical rather than the content. This
doesn't work here because the URL gets modified with aqs= parameter and has some
other subtle parameter differences even if it should be considered a match, so I
am not sure it's possible to unify this code.

[Peter Kasting, 2013/11/26 02:46:22]

Hmm.  That actually worries me.  With things like entity suggestions, I think
you can possibly have multiple matches whose |contents| and |type| match.  So
this deletion code may delete the wrong thing.

I am not really sure how to deal with this.  We may need to also check the
additional recorded data on the matches or something.

[Maria, 2013/11/27 02:18:06]

Right now the only deletable suggestions in search_provider come from psuggest.
Since those are queries you typed, I don't think it can have multiple ones with
the same content that are in fact different. Entity suggestions are not
deletable. I will add a comment here for the future to note this assumption by
the code.

+  bool found = false;
+  for (ACMatches::iterator i(matches_.begin()); i != matches_.end(); ++i) {
+    if (i->contents == match.contents && i->type == match.type) {
+      found = true;
+      matches_.erase(i);
+      break;
+    }
+  }
+  DCHECK(found) << "Asked to delete a search suggestion that isn't in our set "
+      << "of matches";
+  listener_->OnProviderUpdate(true);
+}
+
 void SearchProvider::AddMatchToMap(const string16& input_text,
                                    int relevance,
                                    bool relevance_from_server,
@@ -1721,6 +1806,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.
@@ -1753,6 +1839,11 @@ void SearchProvider::AddMatchToMap(const string16& input_text,
   match.RecordAdditionalInfo(kShouldPrefetchKey,
                              should_prefetch ? kTrue : kFalse);
 
+  if (!deletion_url.empty()) {
+    match.RecordAdditionalInfo(kDeletionUrlKey, deletion_url);
+    match.deletable = true;
+  }
+
   // Metadata is needed only for prefetching queries.
   if (should_prefetch)
     match.RecordAdditionalInfo(kSuggestMetadataKey, metadata);