Commit 0e7dda3d authored by Patti's avatar Patti Committed by Commit Bot

Settings: All sites can now be sorted by most visited.

The default sort order of the All Sites page is by the amount of site engagement
each site has. Retrieve the site engagement score for each origin in All Sites
and sort the list by the max site engagement per domain (eTLD+1) group.

Bug: 835712
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I421c41f3307a4e5b3ea35683bc4455dfc69ecd96
Reviewed-on: https://chromium-review.googlesource.com/1141667
Commit-Queue: Patti <patricialor@chromium.org>
Reviewed-by: default avatarSteven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579302}
parent 45875204
......@@ -2677,6 +2677,9 @@
<message name="IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SORT" desc="Label on a selection menu to choose the sorting method to display the All Sites list by.">
Sort by
</message>
<message name="IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SORT_METHOD_MOST_VISITED" desc="A selection menu option to sort the All Sites list by the most visited sites.">
Most visited
</message>
<message name="IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SORT_METHOD_NAME" desc="A selection menu option to sort the All Sites list by the name of the site, which is derived from its URL.">
Name
</message>
......
......@@ -40,8 +40,11 @@
<label id="sortLabel">$i18n{siteSettingsAllSitesSort}</label>
<select id="sortMethod" class="md-select" aria-labelledby="sortLabel"
on-change="onSortMethodChanged_">
<!-- TODO(https://crbug.com/835712): Implement remaining two sort
methods. -->
<option value="[[sortMethods_.mostVisited]]">
$i18n{siteSettingsAllSitesSortMethodMostVisited}
</option>
<!-- TODO(https://crbug.com/835712): Implement remaining sort
method. -->
<option value="[[sortMethods_.name]]">
$i18n{siteSettingsAllSitesSortMethodName}
</option>
......
......@@ -139,10 +139,22 @@ Polymer({
list.forEach(storageSiteGroup => {
if (this.siteGroupMap.has(storageSiteGroup.etldPlus1)) {
const siteGroup = this.siteGroupMap.get(storageSiteGroup.etldPlus1);
storageSiteGroup.origins.forEach(origin => {
if (!siteGroup.origins.includes(origin))
siteGroup.origins.push(origin);
const storageOriginInfoMap = new Map();
storageSiteGroup.origins.forEach(
originInfo =>
storageOriginInfoMap.set(originInfo.origin, originInfo));
// If there is an overlapping origin, update the original
// |originInfo|.
siteGroup.origins.forEach(originInfo => {
if (!storageOriginInfoMap.has(originInfo.origin))
return;
Object.apply(originInfo, storageOriginInfoMap.get(originInfo.origin));
storageOriginInfoMap.delete(originInfo.origin);
});
// Otherwise, add it to the list.
storageOriginInfoMap.forEach(
originInfo => siteGroup.origins.push(originInfo));
} else {
this.siteGroupMap.set(storageSiteGroup.etldPlus1, storageSiteGroup);
}
......@@ -160,8 +172,10 @@ Polymer({
filterPopulatedList_: function(siteGroupMap, searchQuery) {
const result = [];
for (const [etldPlus1, siteGroup] of siteGroupMap) {
if (siteGroup.origins.find(origin => origin.includes(searchQuery)))
if (siteGroup.origins.find(
originInfo => originInfo.origin.includes(searchQuery))) {
result.push(siteGroup);
}
}
return this.sortSiteGroupList_(result);
},
......@@ -174,11 +188,41 @@ Polymer({
*/
sortSiteGroupList_: function(siteGroupList) {
const sortMethod = this.$.sortMethod.value;
if (this.sortMethods_ && sortMethod == this.sortMethods_.name)
if (!this.sortMethods_)
return siteGroupList;
if (sortMethod == this.sortMethods_.mostVisited) {
siteGroupList.sort(this.mostVisitedComparator_);
} else if (sortMethod == this.sortMethods_.name) {
siteGroupList.sort(this.nameComparator_);
}
return siteGroupList;
},
/**
* Comparator used to sort SiteGroups by the amount of engagement the user has
* with the origins listed inside it. Note only the maximum engagement is used
* for each SiteGroup (as opposed to the sum) in order to prevent domains with
* higher numbers of origins from always floating to the top of the list.
* @param {!SiteGroup} siteGroup1
* @param {!SiteGroup} siteGroup2
* @private
*/
mostVisitedComparator_: function(siteGroup1, siteGroup2) {
const getMaxEngagement = (max, originInfo) => {
return (max > originInfo.engagement) ? max : originInfo.engagement;
};
const score1 = siteGroup1.origins.reduce(getMaxEngagement, 0);
const score2 = siteGroup2.origins.reduce(getMaxEngagement, 0);
return score2 - score1;
},
/**
* Comparator used to sort SiteGroups by their eTLD+1 name (domain).
* @param {!SiteGroup} siteGroup1
* @param {!SiteGroup} siteGroup2
* @private
*/
nameComparator_: function(siteGroup1, siteGroup2) {
return siteGroup1.etldPlus1.localeCompare(siteGroup2.etldPlus1);
},
......@@ -198,7 +242,8 @@ Polymer({
* @private
*/
onSortMethodChanged_: function() {
this.siteGroupList = this.sortSiteGroupList_(this.siteGroupList);
this.$.allSitesList.items =
this.sortSiteGroupList_(this.$.allSitesList.items);
// Force the iron-list to rerender its items, as the order has changed.
this.$.allSitesList.fire('iron-resize');
},
......@@ -239,14 +284,14 @@ Polymer({
const onNavigatedTo = () => {
this.async(() => {
if (this.selectedItem_ == null || this.siteGroupList.length == 0)
if (this.selectedItem_ == null || this.siteGroupMap.size == 0)
return;
// Focus the site-entry to ensure the iron-list renders it, otherwise
// the query selector will not be able to find it. Note the index is
// used here instead of the item, in case the item was already removed.
const index = Math.max(
0, Math.min(this.selectedItem_.index, this.siteGroupList.length));
0, Math.min(this.selectedItem_.index, this.siteGroupMap.size));
this.$.allSitesList.focusItem(index);
this.selectedItem_ = null;
});
......
......@@ -77,7 +77,8 @@
<template is="dom-repeat" items="[[siteGroup.origins]]">
<div class="settings-box list-item" on-click="onOriginTap_"
actionable>
<div class="favicon-image" style$="[[computeSiteIcon(item)]]">
<div class="favicon-image"
style$="[[computeSiteIcon(item.origin)]]">
</div>
<div class="site-representation middle text-elide">
<span class="url-directionality">
......
......@@ -93,7 +93,7 @@ Polymer({
// was computed.
}
originIndex = this.getIndexBoundToOriginList_(siteGroup, originIndex);
const url = this.toUrl(siteGroup.origins[originIndex]);
const url = this.toUrl(siteGroup.origins[originIndex].origin);
return url.host;
},
......@@ -143,7 +143,7 @@ Polymer({
return '';
originIndex = this.getIndexBoundToOriginList_(siteGroup, originIndex);
const url = this.toUrl(siteGroup.origins[originIndex]);
const url = this.toUrl(siteGroup.origins[originIndex].origin);
const scheme = url.protocol.replace(new RegExp(':*$'), '');
/** @type{string} */ const HTTPS_SCHEME = 'https';
if (scheme == HTTPS_SCHEME)
......@@ -161,7 +161,7 @@ Polymer({
getSiteGroupIcon_: function(siteGroup) {
// TODO(https://crbug.com/835712): Implement heuristic for finding a good
// favicon.
return this.computeSiteIcon(siteGroup.origins[0]);
return this.computeSiteIcon(siteGroup.origins[0].origin);
},
/**
......@@ -184,7 +184,7 @@ Polymer({
* @private
*/
onOriginTap_: function(e) {
this.navigateToSiteDetails_(this.siteGroup.origins[e.model.index]);
this.navigateToSiteDetails_(this.siteGroup.origins[e.model.index].origin);
},
/**
......@@ -195,7 +195,7 @@ Polymer({
onSiteEntryTap_: function() {
// Individual origins don't expand - just go straight to Site Details.
if (!this.grouped_(this.siteGroup)) {
this.navigateToSiteDetails_(this.siteGroup.origins[0]);
this.navigateToSiteDetails_(this.siteGroup.origins[0].origin);
return;
}
this.toggleCollapsible_();
......@@ -252,7 +252,7 @@ Polymer({
onResetSettings_: function(e) {
const contentSettingsTypes = this.getCategoryList();
for (let i = 0; i < this.siteGroup.origins.length; ++i) {
const origin = this.siteGroup.origins[i];
const origin = this.siteGroup.origins[i].origin;
this.browserProxy.setOriginPermissions(
origin, contentSettingsTypes, settings.ContentSetting.DEFAULT);
if (contentSettingsTypes.includes(settings.ContentSettingsTypes.PLUGINS))
......
......@@ -18,13 +18,20 @@ const ContentSettingProvider = {
PREFERENCE: 'preference',
};
/**
* Stores origin information.
* @typedef {{origin: string,
* engagement: number}}
*/
let OriginInfo;
/**
* Represents a list of sites, grouped under the same eTLD+1. For example, an
* origin "https://www.example.com" would be grouped together with
* "https://login.example.com" and "http://example.com" under a common eTLD+1 of
* "example.com".
* @typedef {{etldPlus1: string,
* origins: Array<string>}}
* origins: Array<OriginInfo>}}
*/
let SiteGroup;
......
......@@ -2211,6 +2211,8 @@ void AddSiteSettingsStrings(content::WebUIDataSource* html_source,
IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_DESCRIPTION},
{"siteSettingsAllSitesSearch", IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SEARCH},
{"siteSettingsAllSitesSort", IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SORT},
{"siteSettingsAllSitesSortMethodMostVisited",
IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SORT_METHOD_MOST_VISITED},
{"siteSettingsAllSitesSortMethodName",
IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SORT_METHOD_NAME},
{"siteSettingsSiteRepresentationSeparator",
......
......@@ -20,6 +20,7 @@
#include "chrome/browser/chrome_notification_types.h"
#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
#include "chrome/browser/content_settings/web_site_settings_uma_util.h"
#include "chrome/browser/engagement/site_engagement_service.h"
#include "chrome/browser/infobars/infobar_service.h"
#include "chrome/browser/permissions/chooser_context_base.h"
#include "chrome/browser/permissions/permission_decision_auto_blocker.h"
......@@ -148,19 +149,30 @@ void CreateOrAppendSiteGroupEntry(
}
}
// Converts a given |site_group_map| to a list of base::DictionaryValues.
// Converts a given |site_group_map| to a list of base::DictionaryValues, adding
// the site engagement score for each origin.
void ConvertSiteGroupMapToListValue(
const std::map<std::string, std::set<std::string>>& site_group_map,
base::Value* list_value) {
base::Value* list_value,
Profile* profile) {
DCHECK_EQ(base::Value::Type::LIST, list_value->type());
DCHECK(profile);
SiteEngagementService* engagement_service =
SiteEngagementService::Get(profile);
for (const auto& entry : site_group_map) {
// eTLD+1 is the effective top level domain + 1.
base::Value site_group(base::Value::Type::DICTIONARY);
site_group.SetKey(kEffectiveTopLevelDomainPlus1Name,
base::Value(entry.first));
base::Value origin_list(base::Value::Type::LIST);
for (const std::string& origin : entry.second)
origin_list.GetList().emplace_back(base::Value(origin));
for (const std::string& origin : entry.second) {
base::Value origin_object(base::Value::Type::DICTIONARY);
origin_object.SetKey("origin", base::Value(origin));
origin_object.SetKey(
"engagement",
base::Value(engagement_service->GetScore(GURL(origin))));
origin_list.GetList().emplace_back(std::move(origin_object));
}
site_group.SetKey(kOriginList, std::move(origin_list));
list_value->GetList().push_back(std::move(site_group));
}
......@@ -611,7 +623,7 @@ void SiteSettingsHandler::HandleGetAllSites(const base::ListValue* args) {
}
base::Value result(base::Value::Type::LIST);
ConvertSiteGroupMapToListValue(all_sites_map, &result);
ConvertSiteGroupMapToListValue(all_sites_map, &result, profile);
ResolveJavascriptCallback(*callback_id, result);
}
......@@ -624,8 +636,8 @@ void SiteSettingsHandler::OnLocalStorageFetched(
CreateOrAppendSiteGroupEntry(&all_sites_map, info.origin_url);
}
base::Value result(base::Value::Type::LIST);
ConvertSiteGroupMapToListValue(all_sites_map, &result);
FireWebUIListener("onLocalStorageListFetched", std::move(result));
ConvertSiteGroupMapToListValue(all_sites_map, &result, profile_);
FireWebUIListener("onLocalStorageListFetched", result);
}
void SiteSettingsHandler::HandleGetExceptionList(const base::ListValue* args) {
......
......@@ -408,8 +408,10 @@ TEST_F(SiteSettingsHandlerTest, GetAllSites) {
site_group.FindKey("origins")->GetList();
EXPECT_EQ("example.com", etld_plus1_string);
EXPECT_EQ(2UL, origin_list.size());
EXPECT_EQ(url1.spec(), origin_list[0].GetString());
EXPECT_EQ(url2.spec(), origin_list[1].GetString());
EXPECT_EQ(url1.spec(), origin_list[0].FindKey("origin")->GetString());
EXPECT_EQ(0, origin_list[0].FindKey("engagement")->GetDouble());
EXPECT_EQ(url2.spec(), origin_list[1].FindKey("origin")->GetString());
EXPECT_EQ(0, origin_list[1].FindKey("engagement")->GetDouble());
}
}
......@@ -435,7 +437,7 @@ TEST_F(SiteSettingsHandlerTest, GetAllSites) {
site_group.FindKey("origins")->GetList();
if (etld_plus1_string == "example2.net") {
EXPECT_EQ(1UL, origin_list.size());
EXPECT_EQ(url3.spec(), origin_list[0].GetString());
EXPECT_EQ(url3.spec(), origin_list[0].FindKey("origin")->GetString());
} else {
EXPECT_EQ("example.com", etld_plus1_string);
}
......@@ -598,9 +600,10 @@ TEST_F(SiteSettingsHandlerTest, GetAllSitesLocalStorage) {
ASSERT_TRUE(site_group->GetList("origins", &origin_list));
EXPECT_EQ(1U, origin_list->GetSize());
std::string actual_origin;
ASSERT_TRUE(origin_list->GetString(0, &actual_origin));
ASSERT_EQ(origin.spec(), actual_origin);
const base::DictionaryValue* origin_info;
ASSERT_TRUE(origin_list->GetDictionary(0, &origin_info));
EXPECT_EQ(origin.spec(), origin_info->FindKey("origin")->GetString());
EXPECT_EQ(0, origin_info->FindKey("engagement")->GetDouble());
}
TEST_F(SiteSettingsHandlerTest, Origins) {
......
......@@ -131,20 +131,68 @@ suite('AllSites', function() {
});
});
test('can be sorted by most visited', function() {
setUpCategory(prefsVarious);
testElement.populateList_();
return browserProxy.whenCalled('getAllSites').then(() => {
// Add additional origins and artificially insert fake engagement scores
// to sort.
assertEquals(3, testElement.siteGroupMap.size);
const fooSiteGroup = testElement.siteGroupMap.get('foo.com');
fooSiteGroup.origins.push(test_util.createOriginInfo(
'https://login.foo.com', {engagement: 20}));
assertEquals(2, fooSiteGroup.origins.length);
fooSiteGroup.origins[0].engagement = 50.4;
const googleSiteGroup = testElement.siteGroupMap.get('google.com');
assertEquals(1, googleSiteGroup.origins.length);
googleSiteGroup.origins[0].engagement = 55.1261;
const barSiteGroup = testElement.siteGroupMap.get('bar.com');
assertEquals(1, barSiteGroup.origins.length);
barSiteGroup.origins[0].engagement = 0.5235;
// 'Most visited' is the default sort method, so sort by a different
// method first to ensure changing to 'Most visited' works.
testElement.root.querySelector('select').value = 'name';
testElement.onSortMethodChanged_();
Polymer.dom.flush();
let siteEntries =
testElement.$.listContainer.querySelectorAll('site-entry');
assertEquals('bar.com', siteEntries[0].$.displayName.innerText.trim());
assertEquals('foo.com', siteEntries[1].$.displayName.innerText.trim());
assertEquals('google.com', siteEntries[2].$.displayName.innerText.trim());
testElement.root.querySelector('select').value = 'most-visited';
testElement.onSortMethodChanged_();
Polymer.dom.flush();
siteEntries = testElement.$.listContainer.querySelectorAll('site-entry');
// Each site entry is sorted by its maximum engagement, so expect
// 'foo.com' to come after 'google.com'.
assertEquals('google.com', siteEntries[0].$.displayName.innerText.trim());
assertEquals('foo.com', siteEntries[1].$.displayName.innerText.trim());
assertEquals('bar.com', siteEntries[2].$.displayName.innerText.trim());
});
});
test('can be sorted by name', function() {
setUpCategory(prefsVarious);
testElement.populateList_();
return browserProxy.whenCalled('getAllSites').then(() => {
Polymer.dom.flush();
const siteEntries =
let siteEntries =
testElement.$.listContainer.querySelectorAll('site-entry');
// TODO(https://crbug.com/835712): When there is more than one sort
// method, check that the all sites list can be sorted by name from an
// existing all sites list that is out of order. Currently this is not
// possible as the all sites list will always initially be sorted by the
// default sort method, and the default sort method is by name.
// Verify all sites is not sorted by name.
assertEquals(3, siteEntries.length);
assertEquals('foo.com', siteEntries[0].$.displayName.innerText.trim());
assertEquals('bar.com', siteEntries[1].$.displayName.innerText.trim());
assertEquals('google.com', siteEntries[2].$.displayName.innerText.trim());
// Change the sort method, then verify all sites is now sorted by name.
testElement.root.querySelector('select').value = 'name';
testElement.onSortMethodChanged_();
Polymer.dom.flush();
siteEntries = testElement.$.listContainer.querySelectorAll('site-entry');
assertEquals('bar.com', siteEntries[0].$.displayName.innerText.trim());
assertEquals('foo.com', siteEntries[1].$.displayName.innerText.trim());
assertEquals('google.com', siteEntries[2].$.displayName.innerText.trim());
......@@ -169,12 +217,15 @@ suite('AllSites', function() {
{
// Test merging an existing site works, with overlapping origin lists.
'etldPlus1': fooEtldPlus1,
'origins': [fooOrigin, 'https://foo.com'],
'origins': [
test_util.createOriginInfo(fooOrigin),
test_util.createOriginInfo('https://foo.com'),
],
},
{
// Test adding a new site entry works.
'etldPlus1': addEtldPlus1,
'origins': [addOrigin]
'origins': [test_util.createOriginInfo(addOrigin)],
}
]);
testElement.onLocalStorageListFetched(LOCAL_STORAGE_SITE_GROUP_LIST);
......@@ -183,14 +234,15 @@ suite('AllSites', function() {
siteEntries = testElement.$.listContainer.querySelectorAll('site-entry');
assertEquals(4, siteEntries.length);
assertEquals(addEtldPlus1, siteEntries[0].siteGroup.etldPlus1);
assertEquals(1, siteEntries[0].siteGroup.origins.length);
assertEquals(addOrigin, siteEntries[0].siteGroup.origins[0]);
assertEquals(fooEtldPlus1, siteEntries[0].siteGroup.etldPlus1);
assertEquals(2, siteEntries[0].siteGroup.origins.length);
assertEquals(
'https://foo.com', siteEntries[0].siteGroup.origins[0].origin);
assertEquals(fooOrigin, siteEntries[0].siteGroup.origins[1].origin);
assertEquals(fooEtldPlus1, siteEntries[2].siteGroup.etldPlus1);
assertEquals(2, siteEntries[2].siteGroup.origins.length);
assertTrue(siteEntries[2].siteGroup.origins.includes(fooOrigin));
assertTrue(siteEntries[2].siteGroup.origins.includes('https://foo.com'));
assertEquals(addEtldPlus1, siteEntries[3].siteGroup.etldPlus1);
assertEquals(1, siteEntries[3].siteGroup.origins.length);
assertEquals(addOrigin, siteEntries[3].siteGroup.origins[0].origin);
});
});
});
......@@ -130,7 +130,7 @@ suite('SiteEntry', function() {
settings.routes.SITE_SETTINGS_SITE_DETAILS.path,
settings.getCurrentRoute().path);
assertEquals(
TEST_MULTIPLE_SITE_GROUP.origins[1],
TEST_MULTIPLE_SITE_GROUP.origins[1].origin,
settings.getQueryParameters().get('site'));
});
......
......@@ -203,15 +203,25 @@ class TestSiteSettingsPrefsBrowserProxy extends TestBrowserProxy {
const origins_array = [...origins_set];
let result = [];
origins_array.forEach((origin, index) => {
let entry = {};
// Functionality to get the eTLD+1 from an origin exists only on the
// C++ side, so just do an (incorrect) approximate extraction here.
const host = new URL(origin).host;
let urlParts = host.split('.');
urlParts = urlParts.slice(Math.max(urlParts.length - 2, 0));
entry['etldPlus1'] = urlParts.join('.');
entry['origins'] = [origin];
result.push(entry);
const etldPlus1Name = urlParts.join('.');
let existing = result.find(siteGroup => {
return siteGroup.etldPlus1 == etldPlus1Name;
});
if (existing) {
existing.origins.push(test_util.createOriginInfo(origin));
} else {
let entry = {};
entry['etldPlus1'] = etldPlus1Name;
entry['origins'] = [test_util.createOriginInfo(origin)];
result.push(entry);
}
});
return Promise.resolve(result);
......
......@@ -192,12 +192,24 @@ cr.define('test_util', function() {
* @return {SiteGroup}
*/
function createSiteGroup(eTLDPlus1Name, originList) {
const originInfoList = originList.map(origin => createOriginInfo(origin));
return {
etldPlus1: eTLDPlus1Name,
origins: originList,
origins: originInfoList,
};
}
function createOriginInfo(origin, override) {
if (override === undefined)
override = {};
return Object.assign(
{
origin: origin,
engagement: 0,
},
override);
}
return {
eventToPromise: eventToPromise,
fakeDataBind: fakeDataBind,
......@@ -207,6 +219,7 @@ cr.define('test_util', function() {
createRawSiteException: createRawSiteException,
createSiteSettingsPrefs: createSiteSettingsPrefs,
createSiteGroup: createSiteGroup,
createOriginInfo: createOriginInfo,
};
});
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