From 4872ba41d2be74386a21897636c7d64a698cc97b Mon Sep 17 00:00:00 2001 From: mgiuca Date: Mon, 11 May 2015 20:46:22 -0700 Subject: [PATCH] App Launcher: Webstore results are now ranked by title match. Previously, all Webstore results had an internal relevance score of 0.0. Now they are scored based on the query matched against the app's title, and scores of 0.0 are discarded. This fixes app results showing up because the word you typed appears in the app's web store description, but not its title. BUG=481837 Review URL: https://codereview.chromium.org/1110903002 Cr-Commit-Position: refs/heads/master@{#329343} --- .../app_list/search/webstore/webstore_provider.cc | 32 +++++++------ .../app_list/search/webstore/webstore_provider.h | 4 +- .../webstore/webstore_provider_browsertest.cc | 52 +++++++++++++--------- .../ui/app_list/search/webstore/webstore_result.cc | 13 +++--- .../ui/app_list/search/webstore/webstore_result.h | 1 + 5 files changed, 60 insertions(+), 42 deletions(-) diff --git a/chrome/browser/ui/app_list/search/webstore/webstore_provider.cc b/chrome/browser/ui/app_list/search/webstore/webstore_provider.cc index 0cb781f8e7ac..3d2fcc66a51c 100644 --- a/chrome/browser/ui/app_list/search/webstore/webstore_provider.cc +++ b/chrome/browser/ui/app_list/search/webstore/webstore_provider.cc @@ -18,6 +18,8 @@ #include "chrome/browser/ui/app_list/search/search_webstore_result.h" #include "chrome/browser/ui/app_list/search/webstore/webstore_result.h" #include "extensions/common/extension_urls.h" +#include "ui/app_list/search/tokenized_string.h" +#include "ui/app_list/search/tokenized_string_match.h" #include "url/gurl.h" namespace app_list { @@ -131,6 +133,7 @@ void WebstoreProvider::ProcessWebstoreSearchResults( } bool first_result = true; + TokenizedString query(base::UTF8ToUTF16(query_)); for (base::ListValue::const_iterator it = result_list->begin(); it != result_list->end(); ++it) { @@ -138,7 +141,7 @@ void WebstoreProvider::ProcessWebstoreSearchResults( if (!(*it)->GetAsDictionary(&dict)) continue; - scoped_ptr result(CreateResult(*dict)); + scoped_ptr result(CreateResult(query, *dict)); if (!result) continue; @@ -153,9 +156,8 @@ void WebstoreProvider::ProcessWebstoreSearchResults( } scoped_ptr WebstoreProvider::CreateResult( + const TokenizedString& query, const base::DictionaryValue& dict) { - scoped_ptr result; - std::string app_id; std::string localized_name; std::string icon_url_string; @@ -164,25 +166,29 @@ scoped_ptr WebstoreProvider::CreateResult( !dict.GetString(kKeyLocalizedName, &localized_name) || !dict.GetString(kKeyIconUrl, &icon_url_string) || !dict.GetBoolean(kKeyIsPaid, &is_paid)) { - return result.Pass(); + return scoped_ptr(); } GURL icon_url(icon_url_string); if (!icon_url.is_valid()) - return result.Pass(); + return scoped_ptr(); std::string item_type_string; dict.GetString(kKeyItemType, &item_type_string); extensions::Manifest::Type item_type = ParseItemType(item_type_string); - result.reset(new WebstoreResult(profile_, - app_id, - localized_name, - icon_url, - is_paid, - item_type, - controller_)); - return result.Pass(); + // Calculate the relevance score by matching the query with the title. Results + // with a match score of 0 are discarded. + // TODO(mgiuca): Set the tags to indicate the parts of the title that were + // matched. + TokenizedString title(base::UTF8ToUTF16(localized_name)); + TokenizedStringMatch match; + if (!match.Calculate(query, title)) + return scoped_ptr(); + + return make_scoped_ptr(new WebstoreResult(profile_, app_id, localized_name, + match.relevance(), icon_url, + is_paid, item_type, controller_)); } } // namespace app_list diff --git a/chrome/browser/ui/app_list/search/webstore/webstore_provider.h b/chrome/browser/ui/app_list/search/webstore/webstore_provider.h index 8dee4afb75e1..eb9570ff295c 100644 --- a/chrome/browser/ui/app_list/search/webstore/webstore_provider.h +++ b/chrome/browser/ui/app_list/search/webstore/webstore_provider.h @@ -24,6 +24,7 @@ class WebstoreProviderTest; class JSONResponseFetcher; class SearchResult; +class TokenizedString; // WebstoreProvider fetches search results from the web store server. // A "Search in web store" result will be returned if the server does not @@ -45,7 +46,8 @@ class WebstoreProvider : public WebserviceSearchProvider{ void OnWebstoreSearchFetched(scoped_ptr json); void ProcessWebstoreSearchResults(const base::DictionaryValue* json); - scoped_ptr CreateResult(const base::DictionaryValue& dict); + scoped_ptr CreateResult(const TokenizedString& query, + const base::DictionaryValue& dict); void set_webstore_search_fetched_callback(const base::Closure& callback) { webstore_search_fetched_callback_ = callback; diff --git a/chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc b/chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc index 594c1a191f96..f82341756d3d 100644 --- a/chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc +++ b/chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc @@ -53,21 +53,21 @@ const char kThreeResults[] = "\"results\":[" " {" " \"id\": \"app1_id\"," - " \"localized_name\": \"one\"," + " \"localized_name\": \"Mystery App\"," " \"icon_url\": \"http://host/icon1\"," " \"is_paid\": true," " \"item_type\": \"PLATFORM_APP\"" " }," " {" " \"id\": \"app2_id\"," - " \"localized_name\": \"two\"," + " \"localized_name\": \"App Mystère\"," " \"icon_url\": \"http://host/icon2\"," " \"is_paid\": false," " \"item_type\": \"HOSTED_APP\"" " }," " {" " \"id\": \"app3_id\"," - " \"localized_name\": \"three\"," + " \"localized_name\": \"Mistero App\"," " \"icon_url\": \"http://host/icon3\"," " \"is_paid\": false," " \"item_type\": \"LEGACY_PACKAGED_APP\"" @@ -91,19 +91,19 @@ ParsedSearchResult kParsedOneResult[] = {{"app1_id", 1}}; ParsedSearchResult kParsedThreeResults[] = {{"app1_id", - "one", + "Mystery App", "http://host/icon1", true, Manifest::TYPE_PLATFORM_APP, 1}, {"app2_id", - "two", + "App Mystère", "http://host/icon2", false, Manifest::TYPE_HOSTED_APP, 1}, {"app3_id", - "three", + "Mistero App", "http://host/icon3", false, Manifest::TYPE_LEGACY_PACKAGED_APP, @@ -130,7 +130,7 @@ class WebstoreProviderTest : public InProcessBrowserTest { switches::kEnableExperimentalAppList); webstore_provider_.reset(new WebstoreProvider( - ProfileManager::GetActiveUserProfile(), NULL)); + ProfileManager::GetActiveUserProfile(), nullptr)); webstore_provider_->set_webstore_search_fetched_callback( base::Bind(&WebstoreProviderTest::OnSearchResultsFetched, base::Unretained(this))); @@ -255,17 +255,29 @@ IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, Basic) { // |expected_result_titles| == |query| means we are expecting an error. // A search that returns 0 results. - {"synchronous", "", "synchronous", NULL, 0}, + {"synchronous", "", "synchronous", nullptr, 0}, // Getting an error response from the server (note: the responses // "ERROR_NOT_FOUND" and "ERROR_INTERNAL_SERVER_ERROR" are treated // specially by HandleResponse). - {"404", "ERROR_NOT_FOUND", "404", NULL, 0}, - {"500", "ERROR_INTERNAL_SERVER_ERROR", "500", NULL, 0}, + {"404", "ERROR_NOT_FOUND", "404", nullptr, 0}, + {"500", "ERROR_INTERNAL_SERVER_ERROR", "500", nullptr, 0}, // Getting bad JSON from the server. - {"bad json", "invalid json", "bad json", NULL, 0}, - // Good results. - {"1 result", kOneResult, "app1 name", kParsedOneResult, 1}, - {"3 result", kThreeResults, "one,two,three", kParsedThreeResults, 3}, + {"bad json", "invalid json", "bad json", nullptr, 0}, + // Good results. Note that the search term appears in all of the result + // titles. + {"app1", kOneResult, "app1 name", kParsedOneResult, 1}, + {"app", + kThreeResults, + "Mystery App,App Mystère,Mistero App", + kParsedThreeResults, + 3}, + // Search where one of the results does not include the query term. Only + // the results with a title matching the query should be selected. + {"myst", + kThreeResults, + "Mystery App,App Mystère", + kParsedThreeResults, + 2}, }; for (size_t i = 0; i < arraysize(kTestCases); ++i) { @@ -300,21 +312,21 @@ IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, NoSearchForSensitiveData) { }; for (size_t i = 0; i < arraysize(inputs); ++i) { - RunQueryAndVerify(inputs[i], kOneResult, NULL, 0); + RunQueryAndVerify(inputs[i], kOneResult, nullptr, 0); } } IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, NoSearchForShortQueries) { - RunQueryAndVerify("a", kOneResult, NULL, 0); - RunQueryAndVerify("ab", kOneResult, NULL, 0); - RunQueryAndVerify("abc", kOneResult, kParsedOneResult, 1); + RunQueryAndVerify("a", kOneResult, nullptr, 0); + RunQueryAndVerify("ap", kOneResult, nullptr, 0); + RunQueryAndVerify("app", kOneResult, kParsedOneResult, 1); } IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, SearchCache) { - RunQueryAndVerify("foo", kOneResult, kParsedOneResult, 1); + RunQueryAndVerify("app", kOneResult, kParsedOneResult, 1); // No result is provided but the provider gets the result from the cache. - RunQueryAndVerify("foo", "", kParsedOneResult, 1); + RunQueryAndVerify("app", "", kParsedOneResult, 1); } } // namespace test diff --git a/chrome/browser/ui/app_list/search/webstore/webstore_result.cc b/chrome/browser/ui/app_list/search/webstore/webstore_result.cc index 3fc90c8eafdd..303bfc505144 100644 --- a/chrome/browser/ui/app_list/search/webstore/webstore_result.cc +++ b/chrome/browser/ui/app_list/search/webstore/webstore_result.cc @@ -62,6 +62,7 @@ namespace app_list { WebstoreResult::WebstoreResult(Profile* profile, const std::string& app_id, const std::string& localized_name, + double relevance, const GURL& icon_url, bool is_paid, extensions::Manifest::Type item_type, @@ -77,7 +78,7 @@ WebstoreResult::WebstoreResult(Profile* profile, extension_registry_(NULL), weak_factory_(this) { set_id(extensions::Extension::GetBaseURLFromExtensionId(app_id_).spec()); - set_relevance(0.0); // What is the right value to use? + set_relevance(relevance); set_title(base::UTF8ToUTF16(localized_name_)); SetDefaultDetails(); @@ -127,13 +128,9 @@ void WebstoreResult::InvokeAction(int action_index, int event_flags) { } scoped_ptr WebstoreResult::Duplicate() const { - return scoped_ptr(new WebstoreResult(profile_, - app_id_, - localized_name_, - icon_url_, - is_paid_, - item_type_, - controller_)); + return scoped_ptr( + new WebstoreResult(profile_, app_id_, localized_name_, relevance(), + icon_url_, is_paid_, item_type_, controller_)); } void WebstoreResult::InitAndStartObserving() { diff --git a/chrome/browser/ui/app_list/search/webstore/webstore_result.h b/chrome/browser/ui/app_list/search/webstore/webstore_result.h index 426e2b3f7ac5..be7656cb26b1 100644 --- a/chrome/browser/ui/app_list/search/webstore/webstore_result.h +++ b/chrome/browser/ui/app_list/search/webstore/webstore_result.h @@ -33,6 +33,7 @@ class WebstoreResult : public SearchResult, WebstoreResult(Profile* profile, const std::string& app_id, const std::string& localized_name, + double relevance, const GURL& icon_url, bool is_paid, extensions::Manifest::Type item_type, -- 2.11.4.GIT