Skip to content

Commit

Permalink
Stop sharing BaseSearchProvider::OnURLFetchComplete between 2 providers
Browse files Browse the repository at this point in the history 
Sharing BaseSearchProvider::OnURLFetchComplete() between SearchProvider and ZeroSuggestProvider requires both providers to inherit a lot of virtual methods to customize the method's behavior.
This makes it difficult to track the code path to understand what is happening in OnURLFetchComplete with each provider.

Since SearchSuggestionParser was introduced and OnURLFetchComplete became slimmer, 
it's simpler to have OnURLFetchComplete() implementations in each provider and remove virtual protected methods.

BUG=None

Review URL: https://codereview.chromium.org/436833002

Cr-Commit-Position: refs/heads/master@{#288675}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@288675 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
hashimoto@chromium.org committed Aug 11, 2014
1 parent 8e61bcf commit 776ee59
Show file tree
Hide file tree
Showing 6 changed files with 180 additions and 224 deletions.
54 changes: 4 additions & 50 deletions chrome/browser/autocomplete/base_search_provider.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,17 +94,14 @@ const int BaseSearchProvider::kDefaultProviderURLFetcherID = 1;
const int BaseSearchProvider::kKeywordProviderURLFetcherID = 2;
const int BaseSearchProvider::kDeletionURLFetcherID = 3;

BaseSearchProvider::BaseSearchProvider(AutocompleteProviderListener* listener,
TemplateURLService* template_url_service,
BaseSearchProvider::BaseSearchProvider(TemplateURLService* template_url_service,
Profile* profile,
AutocompleteProvider::Type type)
: AutocompleteProvider(type),
listener_(listener),
template_url_service_(template_url_service),
profile_(profile),
field_trial_triggered_(false),
field_trial_triggered_in_session_(false),
suggest_results_pending_(0) {
field_trial_triggered_in_session_(false) {
}

// static
Expand Down Expand Up @@ -368,38 +365,6 @@ bool BaseSearchProvider::CanSendURL(
return true;
}

void BaseSearchProvider::OnURLFetchComplete(const net::URLFetcher* source) {
DCHECK(!done_);
suggest_results_pending_--;
DCHECK_GE(suggest_results_pending_, 0); // Should never go negative.

const bool is_keyword = IsKeywordFetcher(source);

// Ensure the request succeeded and that the provider used is still available.
// A verbatim match cannot be generated without this provider, causing errors.
const bool request_succeeded =
source->GetStatus().is_success() && (source->GetResponseCode() == 200) &&
GetTemplateURL(is_keyword);

LogFetchComplete(request_succeeded, is_keyword);

bool results_updated = false;
if (request_succeeded) {
std::string json_data = SearchSuggestionParser::ExtractJsonData(source);
scoped_ptr<base::Value> data(
SearchSuggestionParser::DeserializeJsonData(json_data));
if (data && StoreSuggestionResponse(json_data, *data.get()))
return;

results_updated = data.get() && ParseSuggestResults(
*data.get(), is_keyword, GetResultsToFill(is_keyword));
}

UpdateMatches();
if (done_ || results_updated)
listener_->OnProviderUpdate(results_updated);
}

void BaseSearchProvider::AddMatchToMap(
const SearchSuggestionParser::SuggestResult& result,
const std::string& metadata,
Expand Down Expand Up @@ -479,12 +444,12 @@ void BaseSearchProvider::AddMatchToMap(

bool BaseSearchProvider::ParseSuggestResults(
const base::Value& root_val,
int default_result_relevance,
bool is_keyword_result,
SearchSuggestionParser::Results* results) {
if (!SearchSuggestionParser::ParseSuggestResults(
root_val, GetInput(is_keyword_result),
ChromeAutocompleteSchemeClassifier(profile_),
GetDefaultResultRelevance(),
ChromeAutocompleteSchemeClassifier(profile_), default_result_relevance,
profile_->GetPrefs()->GetString(prefs::kAcceptLanguages),
is_keyword_result, results))
return false;
Expand All @@ -499,20 +464,9 @@ bool BaseSearchProvider::ParseSuggestResults(

field_trial_triggered_ |= results->field_trial_triggered;
field_trial_triggered_in_session_ |= results->field_trial_triggered;
SortResults(is_keyword_result, results);
return true;
}

void BaseSearchProvider::SortResults(bool is_keyword,
SearchSuggestionParser::Results* results) {
}

bool BaseSearchProvider::StoreSuggestionResponse(
const std::string& json_data,
const base::Value& parsed_data) {
return false;
}

void BaseSearchProvider::ModifyProviderInfo(
metrics::OmniboxEventProto_ProviderInfo* provider_info) const {
}
Expand Down
50 changes: 7 additions & 43 deletions chrome/browser/autocomplete/base_search_provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@
#include "components/omnibox/autocomplete_match.h"
#include "components/omnibox/autocomplete_provider.h"
#include "components/omnibox/search_suggestion_parser.h"
#include "net/url_request/url_fetcher_delegate.h"

class AutocompleteProviderListener;
class GURL;
class Profile;
class SearchTermsData;
Expand All @@ -40,8 +38,7 @@ class Value;
// Base functionality for receiving suggestions from a search engine.
// This class is abstract and should only be used as a base for other
// autocomplete providers utilizing its functionality.
class BaseSearchProvider : public AutocompleteProvider,
public net::URLFetcherDelegate {
class BaseSearchProvider : public AutocompleteProvider {
public:
// ID used in creating URLFetcher for default provider's suggest results.
static const int kDefaultProviderURLFetcherID;
Expand All @@ -52,8 +49,7 @@ class BaseSearchProvider : public AutocompleteProvider,
// ID used in creating URLFetcher for deleting suggestion results.
static const int kDeletionURLFetcherID;

BaseSearchProvider(AutocompleteProviderListener* listener,
TemplateURLService* template_url_service,
BaseSearchProvider(TemplateURLService* template_url_service,
Profile* profile,
AutocompleteProvider::Type type);

Expand Down Expand Up @@ -172,9 +168,6 @@ class BaseSearchProvider : public AutocompleteProvider,
const SearchTermsData& search_terms_data,
Profile* profile);

// net::URLFetcherDelegate:
virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;

// If the |deletion_url| is valid, then set |match.deletable| to true and
// save the |deletion_url| into the |match|'s additional info under
// the key |kDeletionUrlKey|.
Expand All @@ -195,23 +188,16 @@ class BaseSearchProvider : public AutocompleteProvider,
MatchMap* map);

// Parses results from the suggest server and updates the appropriate suggest
// and navigation result lists in |results|. |is_keyword_result| indicates
// whether the response was received from the keyword provider.
// and navigation result lists in |results|. |default_result_relevance| is
// the relevance to use if it was not explicitly set by the server.
// |is_keyword_result| indicates whether the response was received from the
// keyword provider.
// Returns whether the appropriate result list members were updated.
bool ParseSuggestResults(const base::Value& root_val,
int default_result_relevance,
bool is_keyword_result,
SearchSuggestionParser::Results* results);

// Called at the end of ParseSuggestResults to rank the |results|.
virtual void SortResults(bool is_keyword,
SearchSuggestionParser::Results* results);

// Optionally, cache the received |json_data| and return true if we want
// to stop processing results at this point. The |parsed_data| is the parsed
// version of |json_data| used to determine if we received an empty result.
virtual bool StoreSuggestionResponse(const std::string& json_data,
const base::Value& parsed_data);

// Returns the TemplateURL corresponding to the keyword or default
// provider based on the value of |is_keyword|.
virtual const TemplateURL* GetTemplateURL(bool is_keyword) const = 0;
Expand All @@ -220,10 +206,6 @@ class BaseSearchProvider : public AutocompleteProvider,
// based on the value of |is_keyword|.
virtual const AutocompleteInput GetInput(bool is_keyword) const = 0;

// Returns a pointer to a Results object, which will hold suggest results.
virtual SearchSuggestionParser::Results* GetResultsToFill(
bool is_keyword) = 0;

// Returns whether the destination URL corresponding to the given |result|
// should contain command-line-specified query params.
virtual bool ShouldAppendExtraParams(
Expand All @@ -236,27 +218,13 @@ class BaseSearchProvider : public AutocompleteProvider,
// Clears the current results.
virtual void ClearAllResults() = 0;

// Returns the relevance to use if it was not explicitly set by the server.
virtual int GetDefaultResultRelevance() const = 0;

// Records in UMA whether the deletion request resulted in success.
virtual void RecordDeletionResult(bool success) = 0;

// Records UMA statistics about a suggest server response.
virtual void LogFetchComplete(bool succeeded, bool is_keyword) = 0;

// Modify provider-specific UMA statistics.
virtual void ModifyProviderInfo(
metrics::OmniboxEventProto_ProviderInfo* provider_info) const;

// Returns whether the |fetcher| is for the keyword provider.
virtual bool IsKeywordFetcher(const net::URLFetcher* fetcher) const = 0;

// Updates |matches_| from the latest results; applies calculated relevances
// if suggested relevances cause undesirable behavior. Updates |done_|.
virtual void UpdateMatches() = 0;

AutocompleteProviderListener* listener_;
TemplateURLService* template_url_service_;
Profile* profile_;

Expand All @@ -270,10 +238,6 @@ class BaseSearchProvider : public AutocompleteProvider,
// session.
bool field_trial_triggered_in_session_;

// The number of suggest results that haven't yet arrived. If it's greater
// than 0, it indicates that one of the URLFetchers is still running.
int suggest_results_pending_;

private:
friend class SearchProviderTest;
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, TestDeleteMatch);
Expand Down
Loading

0 comments on commit 776ee59

Please sign in to comment.