Store xsrf token received with psuggest results.

chrome/browser/autocomplete/search_provider.cc

 // Copyright 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/search_provider.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/callback.h"
@@ skipping 195 matching lines @@
 }
 
 
 // SearchProvider::SuggestResult ----------------------------------------------
 
 SearchProvider::SuggestResult::SuggestResult(
     const string16& suggestion,
     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,
     bool should_prefetch)
     : Result(from_keyword_provider, relevance, relevance_from_server),
       suggestion_(suggestion),
       match_contents_(match_contents),
       annotation_(annotation),
       suggest_query_params_(suggest_query_params),
+      deletion_url_(deletion_url),
       should_prefetch_(should_prefetch) {
 }
 
 SearchProvider::SuggestResult::~SuggestResult() {
 }
 
 bool SearchProvider::SuggestResult::IsInlineable(const string16& input) const {
   return StartsWith(suggestion_, input, false);
 }
 
@@ skipping 79 matching lines @@
   for (NavigationResults::const_iterator i(navigation_results.begin());
        i != navigation_results.end(); ++i) {
     if (i->relevance_from_server())
       return true;
   }
 
   return false;
 }
 
 
+// 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";
 
 SearchProvider::SearchProvider(AutocompleteProviderListener* listener,
                                Profile* profile)
     : AutocompleteProvider(listener, profile,
           AutocompleteProvider::TYPE_SEARCH),
       providers_(TemplateURLServiceFactory::GetForProfile(profile)),
       suggest_results_pending_(0),
       field_trial_triggered_(false),
@@ skipping 288 matching lines @@
                           elapsed_time);
     }
   }
 
   bool results_updated = false;
   if (request_succeeded) {
     const net::HttpResponseHeaders* const response_headers =
         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.
     if (response_headers) {
       std::string charset;
       if (response_headers->GetCharset(&charset)) {
         string16 data_16;
         // TODO(jungshik): Switch to CodePageToUTF8 after it's added.
         if (base::CodepageToUTF16(json_data, charset.c_str(),
                                   base::OnStringConversionError::FAIL,
@@ skipping 476 matching lines @@
         results->navigation_results.push_back(NavigationResult(
             *this, url, title, is_keyword, relevance, true));
       }
     } else {
       bool should_prefetch = static_cast<int>(index) == prefetch_index;
       DictionaryValue* suggestion_detail = NULL;
       string16 match_contents = suggestion;
       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) {
         suggestion_detail->GetString("a", &annotation);
         if (suggestion_detail->GetString("dq", &disambiguating_query) &&
             !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));
     }
   }
 
   // Ignore suggested scores for non-keyword matches in keyword mode; if the
   // server is allowed to score these, it could interfere with the user's
   // ability to get good keyword results.
   const bool abandon_suggested_scores =
       !is_keyword && !providers_.keyword_provider().empty();
   // Apply calculated relevance scores to suggestions if a valid list was
   // not provided or we're abandoning suggested scores entirely.
@@ skipping 39 matching lines @@
                   relevance_from_server,
                   false,
                   std::string(),
                   AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
                   false,
                   input_.text(),
                   string16(),
                   input_.text(),
                   did_not_accept_default_suggestion,
                   std::string(),
+                  std::string(),
                   &map);
   }
   if (!keyword_input_.text().empty()) {
     const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
     // We only create the verbatim search query match for a keyword
     // if it's not an extension keyword.  Extension keywords are handled
     // in KeywordProvider::Start().  (Extensions are complicated...)
     // Note: in this provider, SEARCH_OTHER_ENGINE must correspond
     // to the keyword verbatim search query.  Do not create other matches
     // of type SEARCH_OTHER_ENGINE.
     if (keyword_url &&
         (keyword_url->GetType() != TemplateURL::OMNIBOX_API_EXTENSION)) {
       bool keyword_relevance_from_server;
       const int keyword_verbatim_relevance =
           GetKeywordVerbatimRelevance(&keyword_relevance_from_server);
       if (keyword_verbatim_relevance > 0) {
         AddMatchToMap(keyword_input_.text(),
                       keyword_verbatim_relevance,
                       keyword_relevance_from_server,
                       false,
                       std::string(),
                       AutocompleteMatchType::SEARCH_OTHER_ENGINE,
                       true,
                       keyword_input_.text(),
                       string16(),
                       keyword_input_.text(),
                       did_not_accept_keyword_suggestion,
                       std::string(),
+                      std::string(),
                       &map);
       }
     }
   }
   AddHistoryResultsToMap(keyword_history_results_, true,
                          did_not_accept_keyword_suggestion, &map);
   AddHistoryResultsToMap(default_history_results_, false,
                          did_not_accept_default_suggestion, &map);
 
   AddSuggestResultsToMap(keyword_results_.suggest_results,
@@ skipping 254 matching lines @@
                   false,
                   false,
                   std::string(),
                   AutocompleteMatchType::SEARCH_HISTORY,
                   is_keyword,
                   i->suggestion(),
                   string16(),
                   i->suggestion(),
                   did_not_accept_suggestion,
                   std::string(),
+                  std::string(),
                   map);
   }
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.AddHistoryResultsTime",
                       base::TimeTicks::Now() - start_time);
 }
 
 SearchProvider::SuggestResults SearchProvider::ScoreHistoryResults(
     const HistoryResults& results,
     bool base_prevent_inline_autocomplete,
     bool input_multiple_words,
@@ skipping 30 matching lines @@
       classifier->Classify(i->term, false, false, &match, NULL);
       prevent_inline_autocomplete =
           !AutocompleteMatch::IsSearchType(match.type);
     }
 
     int relevance = CalculateRelevanceForHistory(
         i->time, is_keyword, !prevent_inline_autocomplete,
         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
   // results' scores, so things are no longer in order.  Do a stable sort to get
   // things back in order without otherwise disturbing results with equal
   // scores, then force the scores to be unique, so that the order in which
   // they're shown is deterministic.
   std::stable_sort(scored_results.begin(), scored_results.end(),
                    CompareScoredResults());
   int last_relevance = 0;
@@ skipping 18 matching lines @@
                   results[i].relevance_from_server(),
                   results[i].should_prefetch(),
                   metadata,
                   AutocompleteMatchType::SEARCH_SUGGEST,
                   is_keyword,
                   results[i].match_contents(),
                   results[i].annotation(),
                   results[i].suggestion(),
                   i,
                   results[i].suggest_query_params(),
+                  results[i].deletion_url(),
                   map);
   }
 }
 
 int SearchProvider::GetVerbatimRelevance(bool* relevance_from_server) const {
   // Use the suggested verbatim relevance score if it is non-negative (valid),
   // if inline autocomplete isn't prevented (always show verbatim on backspace),
   // and if it won't suppress verbatim, leaving no default provider matches.
   // Otherwise, if the default provider returned no matches and was still able
   // to suppress verbatim, the user would have no search/nav matches and may be
@@ skipping 92 matching lines @@
   // Don't let scores go below 0.  Negative relevance scores are meaningful in
   // a different way.
   int base_score;
   if (is_primary_provider)
     base_score = (input_.type() == AutocompleteInput::URL) ? 750 : 1050;
   else
     base_score = 200;
   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,
                                    bool should_prefetch,
                                    const std::string& metadata,
                                    AutocompleteMatch::Type type,
                                    bool is_keyword,
                                    const string16& match_contents,
                                    const string16& annotation,
                                    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.
   int omnibox_start_margin = chrome::kDisableStartMargin;
 #if !defined(OS_ANDROID) && !defined(IOS)
   if (chrome::IsInstantExtendedAPIEnabled()) {
     Browser* browser =
         chrome::FindBrowserWithProfile(profile_, chrome::GetActiveDesktop());
     if (browser && browser->instant_controller() &&
         browser->instant_controller()->instant()) {
@@ skipping 12 matching lines @@
       !is_keyword || providers_.default_provider().empty());
   if (!match.destination_url.is_valid())
     return;
   match.search_terms_args->bookmark_bar_pinned =
       profile_->GetPrefs()->GetBoolean(prefs::kShowBookmarkBar);
   match.RecordAdditionalInfo(kRelevanceFromServerKey,
                              relevance_from_server ? kTrue : kFalse);
   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);
 
   // Try to add |match| to |map|.  If a match for |query_string| is already in
   // |map|, replace it if |match| is more relevant.
   // NOTE: Keep this ToLower() call in sync with url_database.cc.
   MatchKey match_key(
       std::make_pair(base::i18n::ToLower(query_string),
                      match.search_terms_args->suggest_query_params));
@@ skipping 141 matching lines @@
     it->set_relevance(max_query_relevance);
     it->set_relevance_from_server(relevance_from_server);
   }
 }
 
 void SearchProvider::UpdateDone() {
   // We're done when the timer isn't running, there are no suggest queries
   // pending, and we're not waiting on Instant.
   done_ = !timer_.IsRunning() && (suggest_results_pending_ == 0);
 }