Commit 4872ba41 authored by mgiuca's avatar mgiuca Committed by Commit bot

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}
parent 6285aa89
...@@ -18,6 +18,8 @@ ...@@ -18,6 +18,8 @@
#include "chrome/browser/ui/app_list/search/search_webstore_result.h" #include "chrome/browser/ui/app_list/search/search_webstore_result.h"
#include "chrome/browser/ui/app_list/search/webstore/webstore_result.h" #include "chrome/browser/ui/app_list/search/webstore/webstore_result.h"
#include "extensions/common/extension_urls.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" #include "url/gurl.h"
namespace app_list { namespace app_list {
...@@ -131,6 +133,7 @@ void WebstoreProvider::ProcessWebstoreSearchResults( ...@@ -131,6 +133,7 @@ void WebstoreProvider::ProcessWebstoreSearchResults(
} }
bool first_result = true; bool first_result = true;
TokenizedString query(base::UTF8ToUTF16(query_));
for (base::ListValue::const_iterator it = result_list->begin(); for (base::ListValue::const_iterator it = result_list->begin();
it != result_list->end(); it != result_list->end();
++it) { ++it) {
...@@ -138,7 +141,7 @@ void WebstoreProvider::ProcessWebstoreSearchResults( ...@@ -138,7 +141,7 @@ void WebstoreProvider::ProcessWebstoreSearchResults(
if (!(*it)->GetAsDictionary(&dict)) if (!(*it)->GetAsDictionary(&dict))
continue; continue;
scoped_ptr<SearchResult> result(CreateResult(*dict)); scoped_ptr<SearchResult> result(CreateResult(query, *dict));
if (!result) if (!result)
continue; continue;
...@@ -153,9 +156,8 @@ void WebstoreProvider::ProcessWebstoreSearchResults( ...@@ -153,9 +156,8 @@ void WebstoreProvider::ProcessWebstoreSearchResults(
} }
scoped_ptr<SearchResult> WebstoreProvider::CreateResult( scoped_ptr<SearchResult> WebstoreProvider::CreateResult(
const TokenizedString& query,
const base::DictionaryValue& dict) { const base::DictionaryValue& dict) {
scoped_ptr<SearchResult> result;
std::string app_id; std::string app_id;
std::string localized_name; std::string localized_name;
std::string icon_url_string; std::string icon_url_string;
...@@ -164,25 +166,29 @@ scoped_ptr<SearchResult> WebstoreProvider::CreateResult( ...@@ -164,25 +166,29 @@ scoped_ptr<SearchResult> WebstoreProvider::CreateResult(
!dict.GetString(kKeyLocalizedName, &localized_name) || !dict.GetString(kKeyLocalizedName, &localized_name) ||
!dict.GetString(kKeyIconUrl, &icon_url_string) || !dict.GetString(kKeyIconUrl, &icon_url_string) ||
!dict.GetBoolean(kKeyIsPaid, &is_paid)) { !dict.GetBoolean(kKeyIsPaid, &is_paid)) {
return result.Pass(); return scoped_ptr<SearchResult>();
} }
GURL icon_url(icon_url_string); GURL icon_url(icon_url_string);
if (!icon_url.is_valid()) if (!icon_url.is_valid())
return result.Pass(); return scoped_ptr<SearchResult>();
std::string item_type_string; std::string item_type_string;
dict.GetString(kKeyItemType, &item_type_string); dict.GetString(kKeyItemType, &item_type_string);
extensions::Manifest::Type item_type = ParseItemType(item_type_string); extensions::Manifest::Type item_type = ParseItemType(item_type_string);
result.reset(new WebstoreResult(profile_, // Calculate the relevance score by matching the query with the title. Results
app_id, // with a match score of 0 are discarded.
localized_name, // TODO(mgiuca): Set the tags to indicate the parts of the title that were
icon_url, // matched.
is_paid, TokenizedString title(base::UTF8ToUTF16(localized_name));
item_type, TokenizedStringMatch match;
controller_)); if (!match.Calculate(query, title))
return result.Pass(); return scoped_ptr<SearchResult>();
return make_scoped_ptr(new WebstoreResult(profile_, app_id, localized_name,
match.relevance(), icon_url,
is_paid, item_type, controller_));
} }
} // namespace app_list } // namespace app_list
...@@ -24,6 +24,7 @@ class WebstoreProviderTest; ...@@ -24,6 +24,7 @@ class WebstoreProviderTest;
class JSONResponseFetcher; class JSONResponseFetcher;
class SearchResult; class SearchResult;
class TokenizedString;
// WebstoreProvider fetches search results from the web store server. // WebstoreProvider fetches search results from the web store server.
// A "Search in web store" result will be returned if the server does not // A "Search in web store" result will be returned if the server does not
...@@ -45,7 +46,8 @@ class WebstoreProvider : public WebserviceSearchProvider{ ...@@ -45,7 +46,8 @@ class WebstoreProvider : public WebserviceSearchProvider{
void OnWebstoreSearchFetched(scoped_ptr<base::DictionaryValue> json); void OnWebstoreSearchFetched(scoped_ptr<base::DictionaryValue> json);
void ProcessWebstoreSearchResults(const base::DictionaryValue* json); void ProcessWebstoreSearchResults(const base::DictionaryValue* json);
scoped_ptr<SearchResult> CreateResult(const base::DictionaryValue& dict); scoped_ptr<SearchResult> CreateResult(const TokenizedString& query,
const base::DictionaryValue& dict);
void set_webstore_search_fetched_callback(const base::Closure& callback) { void set_webstore_search_fetched_callback(const base::Closure& callback) {
webstore_search_fetched_callback_ = callback; webstore_search_fetched_callback_ = callback;
......
...@@ -53,21 +53,21 @@ const char kThreeResults[] = ...@@ -53,21 +53,21 @@ const char kThreeResults[] =
"\"results\":[" "\"results\":["
" {" " {"
" \"id\": \"app1_id\"," " \"id\": \"app1_id\","
" \"localized_name\": \"one\"," " \"localized_name\": \"Mystery App\","
" \"icon_url\": \"http://host/icon1\"," " \"icon_url\": \"http://host/icon1\","
" \"is_paid\": true," " \"is_paid\": true,"
" \"item_type\": \"PLATFORM_APP\"" " \"item_type\": \"PLATFORM_APP\""
" }," " },"
" {" " {"
" \"id\": \"app2_id\"," " \"id\": \"app2_id\","
" \"localized_name\": \"two\"," " \"localized_name\": \"App Mystère\","
" \"icon_url\": \"http://host/icon2\"," " \"icon_url\": \"http://host/icon2\","
" \"is_paid\": false," " \"is_paid\": false,"
" \"item_type\": \"HOSTED_APP\"" " \"item_type\": \"HOSTED_APP\""
" }," " },"
" {" " {"
" \"id\": \"app3_id\"," " \"id\": \"app3_id\","
" \"localized_name\": \"three\"," " \"localized_name\": \"Mistero App\","
" \"icon_url\": \"http://host/icon3\"," " \"icon_url\": \"http://host/icon3\","
" \"is_paid\": false," " \"is_paid\": false,"
" \"item_type\": \"LEGACY_PACKAGED_APP\"" " \"item_type\": \"LEGACY_PACKAGED_APP\""
...@@ -91,19 +91,19 @@ ParsedSearchResult kParsedOneResult[] = {{"app1_id", ...@@ -91,19 +91,19 @@ ParsedSearchResult kParsedOneResult[] = {{"app1_id",
1}}; 1}};
ParsedSearchResult kParsedThreeResults[] = {{"app1_id", ParsedSearchResult kParsedThreeResults[] = {{"app1_id",
"one", "Mystery App",
"http://host/icon1", "http://host/icon1",
true, true,
Manifest::TYPE_PLATFORM_APP, Manifest::TYPE_PLATFORM_APP,
1}, 1},
{"app2_id", {"app2_id",
"two", "App Mystère",
"http://host/icon2", "http://host/icon2",
false, false,
Manifest::TYPE_HOSTED_APP, Manifest::TYPE_HOSTED_APP,
1}, 1},
{"app3_id", {"app3_id",
"three", "Mistero App",
"http://host/icon3", "http://host/icon3",
false, false,
Manifest::TYPE_LEGACY_PACKAGED_APP, Manifest::TYPE_LEGACY_PACKAGED_APP,
...@@ -130,7 +130,7 @@ class WebstoreProviderTest : public InProcessBrowserTest { ...@@ -130,7 +130,7 @@ class WebstoreProviderTest : public InProcessBrowserTest {
switches::kEnableExperimentalAppList); switches::kEnableExperimentalAppList);
webstore_provider_.reset(new WebstoreProvider( webstore_provider_.reset(new WebstoreProvider(
ProfileManager::GetActiveUserProfile(), NULL)); ProfileManager::GetActiveUserProfile(), nullptr));
webstore_provider_->set_webstore_search_fetched_callback( webstore_provider_->set_webstore_search_fetched_callback(
base::Bind(&WebstoreProviderTest::OnSearchResultsFetched, base::Bind(&WebstoreProviderTest::OnSearchResultsFetched,
base::Unretained(this))); base::Unretained(this)));
...@@ -255,17 +255,29 @@ IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, Basic) { ...@@ -255,17 +255,29 @@ IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, Basic) {
// |expected_result_titles| == |query| means we are expecting an error. // |expected_result_titles| == |query| means we are expecting an error.
// A search that returns 0 results. // A search that returns 0 results.
{"synchronous", "", "synchronous", NULL, 0}, {"synchronous", "", "synchronous", nullptr, 0},
// Getting an error response from the server (note: the responses // Getting an error response from the server (note: the responses
// "ERROR_NOT_FOUND" and "ERROR_INTERNAL_SERVER_ERROR" are treated // "ERROR_NOT_FOUND" and "ERROR_INTERNAL_SERVER_ERROR" are treated
// specially by HandleResponse). // specially by HandleResponse).
{"404", "ERROR_NOT_FOUND", "404", NULL, 0}, {"404", "ERROR_NOT_FOUND", "404", nullptr, 0},
{"500", "ERROR_INTERNAL_SERVER_ERROR", "500", NULL, 0}, {"500", "ERROR_INTERNAL_SERVER_ERROR", "500", nullptr, 0},
// Getting bad JSON from the server. // Getting bad JSON from the server.
{"bad json", "invalid json", "bad json", NULL, 0}, {"bad json", "invalid json", "bad json", nullptr, 0},
// Good results. // Good results. Note that the search term appears in all of the result
{"1 result", kOneResult, "app1 name", kParsedOneResult, 1}, // titles.
{"3 result", kThreeResults, "one,two,three", kParsedThreeResults, 3}, {"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) { for (size_t i = 0; i < arraysize(kTestCases); ++i) {
...@@ -300,21 +312,21 @@ IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, NoSearchForSensitiveData) { ...@@ -300,21 +312,21 @@ IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, NoSearchForSensitiveData) {
}; };
for (size_t i = 0; i < arraysize(inputs); ++i) { 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) { IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, NoSearchForShortQueries) {
RunQueryAndVerify("a", kOneResult, NULL, 0); RunQueryAndVerify("a", kOneResult, nullptr, 0);
RunQueryAndVerify("ab", kOneResult, NULL, 0); RunQueryAndVerify("ap", kOneResult, nullptr, 0);
RunQueryAndVerify("abc", kOneResult, kParsedOneResult, 1); RunQueryAndVerify("app", kOneResult, kParsedOneResult, 1);
} }
IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, SearchCache) { 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. // No result is provided but the provider gets the result from the cache.
RunQueryAndVerify("foo", "", kParsedOneResult, 1); RunQueryAndVerify("app", "", kParsedOneResult, 1);
} }
} // namespace test } // namespace test
......
...@@ -62,6 +62,7 @@ namespace app_list { ...@@ -62,6 +62,7 @@ namespace app_list {
WebstoreResult::WebstoreResult(Profile* profile, WebstoreResult::WebstoreResult(Profile* profile,
const std::string& app_id, const std::string& app_id,
const std::string& localized_name, const std::string& localized_name,
double relevance,
const GURL& icon_url, const GURL& icon_url,
bool is_paid, bool is_paid,
extensions::Manifest::Type item_type, extensions::Manifest::Type item_type,
...@@ -77,7 +78,7 @@ WebstoreResult::WebstoreResult(Profile* profile, ...@@ -77,7 +78,7 @@ WebstoreResult::WebstoreResult(Profile* profile,
extension_registry_(NULL), extension_registry_(NULL),
weak_factory_(this) { weak_factory_(this) {
set_id(extensions::Extension::GetBaseURLFromExtensionId(app_id_).spec()); 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_)); set_title(base::UTF8ToUTF16(localized_name_));
SetDefaultDetails(); SetDefaultDetails();
...@@ -127,13 +128,9 @@ void WebstoreResult::InvokeAction(int action_index, int event_flags) { ...@@ -127,13 +128,9 @@ void WebstoreResult::InvokeAction(int action_index, int event_flags) {
} }
scoped_ptr<SearchResult> WebstoreResult::Duplicate() const { scoped_ptr<SearchResult> WebstoreResult::Duplicate() const {
return scoped_ptr<SearchResult>(new WebstoreResult(profile_, return scoped_ptr<SearchResult>(
app_id_, new WebstoreResult(profile_, app_id_, localized_name_, relevance(),
localized_name_, icon_url_, is_paid_, item_type_, controller_));
icon_url_,
is_paid_,
item_type_,
controller_));
} }
void WebstoreResult::InitAndStartObserving() { void WebstoreResult::InitAndStartObserving() {
......
...@@ -33,6 +33,7 @@ class WebstoreResult : public SearchResult, ...@@ -33,6 +33,7 @@ class WebstoreResult : public SearchResult,
WebstoreResult(Profile* profile, WebstoreResult(Profile* profile,
const std::string& app_id, const std::string& app_id,
const std::string& localized_name, const std::string& localized_name,
double relevance,
const GURL& icon_url, const GURL& icon_url,
bool is_paid, bool is_paid,
extensions::Manifest::Type item_type, extensions::Manifest::Type item_type,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment