Commit b07599ca authored by Cathy Li's avatar Cathy Li Committed by Commit Bot

[Explore sites]: Ensure that we have full rows of sites for each category at the start.

We limit the maximum number of rows shown by how many sites each category had to begin with (before any blacklisting occurs).

Bug: 911362
Change-Id: I35ac39870422248ae830050bb6ccf4f405960f4d
Reviewed-on: https://chromium-review.googlesource.com/c/1359694
Commit-Queue: Cathy Li <chili@chromium.org>
Reviewed-by: default avatarJustin DeWitt <dewittj@chromium.org>
Reviewed-by: default avatarPeter Williamson <petewil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615455}
parent a3a87516
......@@ -26,6 +26,8 @@ public class ExploreSitesCategory {
// The ID to use when creating the More button, that should not scroll the ESP when clicked.
public static final int MORE_BUTTON_ID = -1;
static final int MAX_COLUMNS = 4;
// This enum must match the numbering for ExploreSites.CategoryClick in histograms.xml. Do not
// reorder or remove items, only add new items before COUNT.
@Retention(RetentionPolicy.SOURCE)
......@@ -69,6 +71,7 @@ public class ExploreSitesCategory {
private Drawable mDrawable;
// Populated only for ESP.
private List<ExploreSitesSite> mSites;
private int mNumBlacklisted;
/**
* Creates an explore sites category data structure.
......@@ -113,17 +116,36 @@ public class ExploreSitesCategory {
public void addSite(ExploreSitesSite site) {
mSites.add(site);
if (site.getModel().get(ExploreSitesSite.BLACKLISTED_KEY)) {
mNumBlacklisted++;
}
}
public int getNumDisplayed() {
return mSites.size() - mNumBlacklisted;
}
public int getMaxRows() {
return mSites.size() / MAX_COLUMNS;
}
public boolean removeSite(int siteIndex) {
if (siteIndex > mSites.size() || siteIndex < 0) return false;
mSites.remove(siteIndex);
mSites.get(siteIndex).getModel().set(ExploreSitesSite.BLACKLISTED_KEY, true);
// Reset the tile indices to account for removed tile.
int tileIndex = mSites.get(siteIndex).getModel().get(ExploreSitesSite.TILE_INDEX_KEY);
mSites.get(siteIndex).getModel().set(
ExploreSitesSite.TILE_INDEX_KEY, ExploreSitesSite.DEFAULT_TILE_INDEX);
// Reset the tile indices to account for removed tiles.
for (int i = siteIndex; i < mSites.size(); ++i) {
ExploreSitesSite site = mSites.get(i);
site.getModel().set(ExploreSitesSite.TILE_INDEX_KEY, i);
if (!mSites.get(siteIndex).getModel().get(ExploreSitesSite.BLACKLISTED_KEY)) {
site.getModel().set(ExploreSitesSite.TILE_INDEX_KEY, tileIndex);
tileIndex++;
}
}
mNumBlacklisted++;
return true;
}
......
......@@ -38,7 +38,6 @@ import java.util.List;
public class ExploreSitesCategoryCardView extends LinearLayout {
private static final String TAG = "ExploreSitesCategoryCardView";
private static final int MAX_TILE_COUNT = 8;
private static final int MAX_COLUMNS = 4;
private static final int MAX_ROWS = 2;
private TextView mTitleView;
......@@ -92,7 +91,8 @@ public class ExploreSitesCategoryCardView extends LinearLayout {
// Update the view This may add any sites that we didn't have room for before. It
// should reset the tile indexeds for views we keep.
updateTileViews(mCategory.getSites());
updateTileViews(
mCategory.getSites(), mCategory.getNumDisplayed(), mCategory.getMaxRows());
}
@Override
public String getUrl() {
......@@ -146,8 +146,7 @@ public class ExploreSitesCategoryCardView extends LinearLayout {
super.onFinishInflate();
mTitleView = findViewById(R.id.category_title);
mTileView = findViewById(R.id.category_sites);
mTileView.setMaxColumns(MAX_COLUMNS);
mTileView.setMaxRows(MAX_ROWS);
mTileView.setMaxColumns(ExploreSitesCategory.MAX_COLUMNS);
}
public void setCategory(ExploreSitesCategory category, int categoryCardIndex,
......@@ -161,14 +160,14 @@ public class ExploreSitesCategoryCardView extends LinearLayout {
mCategory = category;
updateTitle(category.getTitle());
updateTileViews(category.getSites());
updateTileViews(category.getSites(), category.getNumDisplayed(), category.getMaxRows());
}
public void updateTitle(String categoryTitle) {
mTitleView.setText(categoryTitle);
}
public void updateTileViews(List<ExploreSitesSite> sites) {
public void updateTileViews(List<ExploreSitesSite> sites, int numSitesToShow, int maxRows) {
// Clear observers.
for (PropertyModelChangeProcessor<PropertyModel, ExploreSitesTileView, PropertyKey>
observer : mModelChangeProcessors) {
......@@ -176,14 +175,19 @@ public class ExploreSitesCategoryCardView extends LinearLayout {
}
mModelChangeProcessors.clear();
// Only show rows that would be fully populated by original list of sites. This is
// calculated within the category.
mTileView.setMaxRows(maxRows);
// Maximum number of sites that can be shown, defined as min of
// numSitesToShow and maxRows * maxCols.
int tileMax = Math.min(maxRows * ExploreSitesCategory.MAX_COLUMNS, numSitesToShow);
// Remove extra tiles if too many.
if (mTileView.getChildCount() > sites.size()) {
mTileView.removeViews(sites.size(), mTileView.getChildCount() - sites.size());
if (mTileView.getChildCount() > tileMax) {
mTileView.removeViews(tileMax, mTileView.getChildCount() - tileMax);
}
// Maximum number of sites to show.
int tileMax = Math.min(MAX_TILE_COUNT, sites.size());
// Add tiles if too few
if (mTileView.getChildCount() < tileMax) {
for (int i = mTileView.getChildCount(); i < tileMax; i++) {
......@@ -194,19 +198,27 @@ public class ExploreSitesCategoryCardView extends LinearLayout {
}
// Initialize all the non-empty tiles again to update.
for (int i = 0; i < tileMax; i++) {
ExploreSitesTileView tileView = (ExploreSitesTileView) mTileView.getChildAt(i);
final PropertyModel site = sites.get(i).getModel();
int tileIndex = 0;
for (ExploreSitesSite site : sites) {
if (tileIndex >= tileMax) break;
final PropertyModel siteModel = site.getModel();
// Skip blacklisted sites.
if (siteModel.get(ExploreSitesSite.BLACKLISTED_KEY)) continue;
ExploreSitesTileView tileView = (ExploreSitesTileView) mTileView.getChildAt(tileIndex);
tileView.initialize(mIconGenerator);
siteModel.set(ExploreSitesSite.TILE_INDEX_KEY, tileIndex);
mModelChangeProcessors.add(PropertyModelChangeProcessor.create(
site, tileView, new ExploreSitesSiteViewBinder()));
siteModel, tileView, new ExploreSitesSiteViewBinder()));
// Fetch icon if not present already.
if (site.get(ExploreSitesSite.ICON_KEY) == null) {
ExploreSitesBridge.getSiteImage(mProfile, site.get(ExploreSitesSite.ID_KEY),
(Bitmap icon) -> site.set(ExploreSitesSite.ICON_KEY, icon));
if (siteModel.get(ExploreSitesSite.ICON_KEY) == null) {
ExploreSitesBridge.getSiteImage(mProfile, siteModel.get(ExploreSitesSite.ID_KEY),
(Bitmap icon) -> siteModel.set(ExploreSitesSite.ICON_KEY, icon));
}
tileIndex++;
}
}
......
......@@ -170,7 +170,14 @@ public class ExploreSitesPage extends BasicNativePage {
mModel.set(STATUS_KEY, CatalogLoadingState.SUCCESS);
ListModel<ExploreSitesCategory> categoryListModel = mModel.get(CATEGORY_LIST_KEY);
categoryListModel.set(categoryList);
// Filter empty categories and categories with fewer sites originally than would fill a row.
for (ExploreSitesCategory category : categoryList) {
if ((category.getNumDisplayed() > 0) && (category.getMaxRows() > 0)) {
categoryListModel.add(category);
}
}
Parcelable savedScrollPosition = getLayoutManagerStateFromNavigationEntry();
if (savedScrollPosition != null) {
mLayoutManager.onRestoreInstanceState(savedScrollPosition);
......
......@@ -179,6 +179,8 @@ public class ExploreSitesSection {
int tileCount = 0;
for (final ExploreSitesCategory category : categoryList) {
if (tileCount >= MAX_CATEGORIES) break;
// Skip empty categories from being shown on NTP.
if (category.getNumDisplayed() == 0) continue;
createTileView(tileCount, category);
tileCount++;
}
......
......@@ -13,6 +13,7 @@ import org.chromium.chrome.browser.modelutil.PropertyModel;
* An object encapsulating info for a website.
*/
public class ExploreSitesSite {
static final int DEFAULT_TILE_INDEX = -1;
static final PropertyModel.ReadableIntPropertyKey ID_KEY =
new PropertyModel.ReadableIntPropertyKey();
static final PropertyModel.WritableIntPropertyKey TILE_INDEX_KEY =
......@@ -23,15 +24,20 @@ public class ExploreSitesSite {
new PropertyModel.ReadableObjectPropertyKey<>();
static final PropertyModel.WritableObjectPropertyKey<Bitmap> ICON_KEY =
new PropertyModel.WritableObjectPropertyKey<>();
static final PropertyModel.WritableBooleanPropertyKey BLACKLISTED_KEY =
new PropertyModel.WritableBooleanPropertyKey();
private PropertyModel mModel;
public ExploreSitesSite(int id, int tileIndex, String title, String url) {
mModel = new PropertyModel.Builder(ID_KEY, TILE_INDEX_KEY, TITLE_KEY, URL_KEY, ICON_KEY)
public ExploreSitesSite(int id, String title, String url, boolean isBlacklisted) {
mModel = new PropertyModel
.Builder(ID_KEY, TILE_INDEX_KEY, TITLE_KEY, URL_KEY, ICON_KEY,
BLACKLISTED_KEY)
.with(ID_KEY, id)
.with(TILE_INDEX_KEY, tileIndex)
.with(TITLE_KEY, title)
.with(URL_KEY, url)
.with(BLACKLISTED_KEY, isBlacklisted)
.with(TILE_INDEX_KEY, DEFAULT_TILE_INDEX)
.build();
}
......@@ -40,10 +46,9 @@ public class ExploreSitesSite {
}
@CalledByNative
private static void createSiteInCategory(
int siteId, String title, String url, ExploreSitesCategory category) {
ExploreSitesSite site =
new ExploreSitesSite(siteId, category.getSites().size(), title, url);
private static void createSiteInCategory(int siteId, String title, String url,
boolean isBlacklisted, ExploreSitesCategory category) {
ExploreSitesSite site = new ExploreSitesSite(siteId, title, url, isBlacklisted);
category.addSite(site);
}
}
......@@ -54,7 +54,7 @@ import java.util.ArrayList;
new ExploreSitesCategory(i, i, "Category #" + Integer.toString(i));
for (int j = 0; j < 8; j++) {
ExploreSitesSite site = new ExploreSitesSite(
i * 8 + j, j, "Site #" + Integer.toString(j), "https://example.com/");
i * 8 + j, "Site #" + Integer.toString(j), "https://example.com/", false);
category.addSite(site);
}
categoryList.add(category);
......
......@@ -5,6 +5,8 @@
package org.chromium.chrome.browser.explore_sites;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import org.junit.Test;
import org.junit.runner.RunWith;
......@@ -19,7 +21,6 @@ public class ExploreSitesCategoryUnitTest {
@Test
public void testAddSite() {
final int id = 1;
final int tileIndex = 3;
@ExploreSitesCategory.CategoryType
final int type = ExploreSitesCategory.CategoryType.SCIENCE;
final int siteId = 100;
......@@ -28,15 +29,17 @@ public class ExploreSitesCategoryUnitTest {
final String categoryTitle = "Movies";
ExploreSitesCategory category = new ExploreSitesCategory(id, type, categoryTitle);
category.addSite(new ExploreSitesSite(siteId, tileIndex, title, url));
category.addSite(new ExploreSitesSite(siteId, title, url, true)); // blacklisted
category.addSite(new ExploreSitesSite(siteId, title, url, false)); // not blacklisted
assertEquals(id, category.getId());
assertEquals(type, category.getType());
assertEquals(1, category.getSites().size());
assertEquals(2, category.getSites().size());
assertEquals(1, category.getNumDisplayed());
assertEquals(siteId, category.getSites().get(0).getModel().get(ExploreSitesSite.ID_KEY));
assertEquals(tileIndex,
category.getSites().get(0).getModel().get(ExploreSitesSite.TILE_INDEX_KEY));
assertEquals(title, category.getSites().get(0).getModel().get(ExploreSitesSite.TITLE_KEY));
assertEquals(url, category.getSites().get(0).getModel().get(ExploreSitesSite.URL_KEY));
assertTrue(category.getSites().get(0).getModel().get(ExploreSitesSite.BLACKLISTED_KEY));
assertFalse(category.getSites().get(1).getModel().get(ExploreSitesSite.BLACKLISTED_KEY));
}
}
......@@ -57,7 +57,8 @@ void CatalogReady(ScopedJavaGlobalRef<jobject>(j_result_obj),
for (auto& site : category.sites) {
Java_ExploreSitesSite_createSiteInCategory(
env, site.site_id, ConvertUTF8ToJavaString(env, site.title),
ConvertUTF8ToJavaString(env, site.url.spec()), j_category);
ConvertUTF8ToJavaString(env, site.url.spec()), site.is_blacklisted,
j_category);
}
}
base::android::RunObjectCallbackAndroid(j_callback_obj, j_result_obj);
......
......@@ -539,7 +539,9 @@ TEST_F(ExploreSitesServiceImplTest, BlacklistNonCanonicalUrls) {
&ExploreSitesServiceImplTest::CatalogCallback, base::Unretained(this)));
PumpLoop();
EXPECT_EQ(1U, database_categories()->at(0).sites.size());
EXPECT_EQ(2U, database_categories()->at(0).sites.size());
EXPECT_TRUE(database_categories()->at(0).sites.at(0).is_blacklisted);
EXPECT_FALSE(database_categories()->at(0).sites.at(1).is_blacklisted);
}
} // namespace explore_sites
......@@ -9,8 +9,13 @@ namespace explore_sites {
ExploreSitesSite::ExploreSitesSite(int site_id,
int category_id,
GURL url,
std::string title)
: site_id(site_id), category_id(category_id), url(url), title(title) {}
std::string title,
bool is_blacklisted)
: site_id(site_id),
category_id(category_id),
url(url),
title(title),
is_blacklisted(is_blacklisted) {}
ExploreSitesSite::ExploreSitesSite(ExploreSitesSite&& other) = default;
......
......@@ -22,7 +22,11 @@ constexpr int kFaviconsPerCategoryImage = 4;
// Image data is not represented here because it is requested separately from
// the UI layer.
struct ExploreSitesSite {
ExploreSitesSite(int site_id, int category_id, GURL url, std::string title);
ExploreSitesSite(int site_id,
int category_id,
GURL url,
std::string title,
bool is_blacklisted);
ExploreSitesSite(ExploreSitesSite&& other);
virtual ~ExploreSitesSite();
......@@ -30,6 +34,7 @@ struct ExploreSitesSite {
int category_id;
GURL url;
std::string title;
bool is_blacklisted;
DISALLOW_COPY_AND_ASSIGN(ExploreSitesSite);
};
......
......@@ -18,10 +18,12 @@ FROM categories
WHERE version_token = ?
ORDER BY category_id ASC;)";
static const char kSelectSiteSql[] = R"(SELECT site_id, sites.url, title
static const char kSelectSiteSql[] =
R"(SELECT site_id, sites.url, title,
site_blacklist.url IS NOT NULL as blacklisted
FROM sites
LEFT JOIN site_blacklist ON (sites.url = site_blacklist.url)
WHERE category_id = ? AND site_blacklist.url IS NULL;)";
WHERE category_id = ? ;)";
const char kDeleteSiteSql[] = R"(DELETE FROM sites
WHERE category_id NOT IN
......@@ -134,10 +136,12 @@ GetCatalogSync(bool update_current, sql::Database* db) {
site_statement.BindInt64(0, category.category_id);
while (site_statement.Step()) {
category.sites.emplace_back(site_statement.ColumnInt(0), // site_id
category.category_id,
GURL(site_statement.ColumnString(1)), // url
site_statement.ColumnString(2)); // title
category.sites.emplace_back(
site_statement.ColumnInt(0), // site_id
category.category_id,
GURL(site_statement.ColumnString(1)), // url
site_statement.ColumnString(2), // title
site_statement.ColumnBool(3)); // is_blacklisted
}
if (!site_statement.Succeeded())
return std::make_pair(GetCatalogStatus::kFailed, nullptr);
......
......@@ -29,7 +29,7 @@ void ValidateTestingCatalog(GetCatalogTask::CategoryList* catalog) {
EXPECT_EQ(2U, catalog->size());
ExploreSitesCategory* cat = &catalog->at(0);
EXPECT_EQ(3, cat->category_id);
EXPECT_EQ(4, cat->category_id);
EXPECT_EQ("5678", cat->version_token);
EXPECT_EQ(1, cat->category_type);
EXPECT_EQ("label_1", cat->label);
......@@ -37,11 +37,12 @@ void ValidateTestingCatalog(GetCatalogTask::CategoryList* catalog) {
EXPECT_EQ(1U, cat->sites.size());
ExploreSitesSite* site = &cat->sites[0];
EXPECT_EQ("https://www.example.com/1", site->url.spec());
EXPECT_EQ(3, site->category_id);
EXPECT_EQ(4, site->category_id);
EXPECT_EQ("example_1", site->title);
EXPECT_FALSE(site->is_blacklisted);
cat = &catalog->at(1);
EXPECT_EQ(4, cat->category_id);
EXPECT_EQ(5, cat->category_id);
EXPECT_EQ("5678", cat->version_token);
EXPECT_EQ(2, cat->category_type);
EXPECT_EQ("label_2", cat->label);
......@@ -49,17 +50,18 @@ void ValidateTestingCatalog(GetCatalogTask::CategoryList* catalog) {
EXPECT_EQ(1U, cat->sites.size());
site = &cat->sites[0];
EXPECT_EQ("https://www.example.com/2", site->url.spec());
EXPECT_EQ(4, site->category_id);
EXPECT_EQ(5, site->category_id);
EXPECT_EQ("example_2", site->title);
EXPECT_FALSE(site->is_blacklisted);
}
// Same as above, categories with no sites left after blacklisting are removed.
// Same as above, sites blacklisted are clearly marked.
void ValidateBlacklistTestingCatalog(GetCatalogTask::CategoryList* catalog) {
EXPECT_FALSE(catalog == nullptr);
EXPECT_EQ(1U, catalog->size());
EXPECT_EQ(2U, catalog->size());
ExploreSitesCategory* cat = &catalog->at(0);
EXPECT_EQ(3, cat->category_id);
EXPECT_EQ(4, cat->category_id);
EXPECT_EQ("5678", cat->version_token);
EXPECT_EQ(1, cat->category_type);
EXPECT_EQ("label_1", cat->label);
......@@ -67,8 +69,22 @@ void ValidateBlacklistTestingCatalog(GetCatalogTask::CategoryList* catalog) {
EXPECT_EQ(1U, cat->sites.size());
ExploreSitesSite* site = &cat->sites[0];
EXPECT_EQ("https://www.example.com/1", site->url.spec());
EXPECT_EQ(3, site->category_id);
EXPECT_EQ(4, site->category_id);
EXPECT_EQ("example_1", site->title);
EXPECT_FALSE(site->is_blacklisted);
cat = &catalog->at(1);
EXPECT_EQ(5, cat->category_id);
EXPECT_EQ("5678", cat->version_token);
EXPECT_EQ(2, cat->category_type);
EXPECT_EQ("label_2", cat->label);
EXPECT_EQ(1U, cat->sites.size());
site = &cat->sites[0];
EXPECT_EQ("https://www.example.com/2", site->url.spec());
EXPECT_EQ(5, site->category_id);
EXPECT_EQ("example_2", site->title);
EXPECT_TRUE(site->is_blacklisted);
}
void ExpectSuccessGetCatalogResult(
......@@ -135,9 +151,9 @@ class ExploreSitesGetCatalogTaskTest : public TaskTestBase {
};
void ExploreSitesGetCatalogTaskTest::PopulateTestingCatalog() {
// Populate a catalog with test data. There are two out-dated categories and
// two current categories. Each category (of the 4) has a site, but only the
// current categories should be returned.
// Populate a catalog with test data. There are three out-dated categories
// and three current categories. Each category (of the 6) has a site except
// two, but only the current categories should be returned.
ExecuteSync(base::BindLambdaForTesting([](sql::Database* db) {
sql::MetaTable meta_table;
ExploreSitesSchema::InitMetaTable(db, &meta_table);
......@@ -149,8 +165,10 @@ INSERT INTO categories
VALUES
(1, "1234", 1, "label_1"), -- older catalog
(2, "1234", 2, "label_2"), -- older catalog
(3, "5678", 1, "label_1"), -- current catalog
(4, "5678", 2, "label_2"); -- current catalog)"));
(3, "1234", 3, "label_3"), -- older catalog
(4, "5678", 1, "label_1"), -- current catalog
(5, "5678", 2, "label_2"), -- current catalog
(6, "5678", 3, "label_3"); -- current catalog)"));
if (!insert.Run())
return false;
......@@ -160,8 +178,8 @@ INSERT INTO sites
VALUES
(1, "https://www.example.com/1", 1, "example_old_1"),
(2, "https://www.example.com/2", 2, "example_old_2"),
(3, "https://www.example.com/1", 3, "example_1"),
(4, "https://www.example.com/2", 4, "example_2");
(3, "https://www.example.com/1", 4, "example_1"),
(4, "https://www.example.com/2", 5, "example_2");
)"));
return insert_sites.Run();
}));
......@@ -257,13 +275,13 @@ TEST_F(ExploreSitesGetCatalogTaskTest, SimpleCatalog) {
RunTask(&task);
// Since |update_current| is false, we should not have changed any rows in the
// DB.
EXPECT_EQ(4, GetNumberOfCategoriesInDB());
EXPECT_EQ(6, GetNumberOfCategoriesInDB());
EXPECT_EQ(4, GetNumberOfSitesInDB());
}
// This tests that sites on the blacklist do not show up when we do a get
// catalog task.
TEST_F(ExploreSitesGetCatalogTaskTest, BlasklistedSitesDoNotAppear) {
TEST_F(ExploreSitesGetCatalogTaskTest, BlasklistedSitesMarkedBlacklisted) {
BlacklistSite("https://www.example.com/2");
PopulateTestingCatalog();
GetCatalogTask task(store(), false,
......@@ -283,7 +301,7 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithVersionUpdate) {
EXPECT_EQ(std::make_pair(std::string("5678"), std::string()),
GetCurrentAndDownloadingVersion());
// The task should have pruned the database.
EXPECT_EQ(2, GetNumberOfCategoriesInDB());
EXPECT_EQ(3, GetNumberOfCategoriesInDB());
EXPECT_EQ(2, GetNumberOfSitesInDB());
}
......@@ -298,7 +316,7 @@ TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithoutVersionUpdate) {
EXPECT_EQ(std::make_pair(std::string("5678"), std::string("1234")),
GetCurrentAndDownloadingVersion());
EXPECT_EQ(4, GetNumberOfCategoriesInDB());
EXPECT_EQ(6, GetNumberOfCategoriesInDB());
EXPECT_EQ(4, GetNumberOfSitesInDB());
}
......
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