Commit e97d0ce5 authored by Min Qin's avatar Min Qin Committed by Chromium LUCI CQ

Fix a bug Chrome is showing 2 rows of most visited tiles when query tiles are enabled

On small screen device, only 1 row of most visited tile should show when
query tiles are enabled.
However, TileSection hard coded the number of rows to 2. So even though
Chrome reduced most visited tiles to 4, it could still end up showing in
2 rows if the screen can only accommodate 3 tiles in a row.

BUG=1166388

Change-Id: I10dabcd0db1f56fa650ed999a9d353eabbf6ce5c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2630732
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: default avatarTed Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844354}
parent b4bca0f9
...@@ -243,7 +243,13 @@ public class NewTabPageLayout extends LinearLayout implements TileGroup.Observer ...@@ -243,7 +243,13 @@ public class NewTabPageLayout extends LinearLayout implements TileGroup.Observer
mTileGroup = new TileGroup(tileRenderer, mManager, contextMenuManager, tileGroupDelegate, mTileGroup = new TileGroup(tileRenderer, mManager, contextMenuManager, tileGroupDelegate,
/* observer = */ this, offlinePageBridge); /* observer = */ this, offlinePageBridge);
mSiteSectionViewHolder = SiteSection.createViewHolder(getSiteSectionView(), mUiConfig); int maxRows = 2;
if (searchProviderIsGoogle && QueryTileUtils.isQueryTilesEnabledOnNTP()) {
maxRows = QueryTileSection.getMaxRowsForMostVisitedTiles(getContext());
}
mSiteSectionViewHolder =
SiteSection.createViewHolder(getSiteSectionView(), mUiConfig, maxRows);
mSiteSectionViewHolder.bindDataSource(mTileGroup, tileRenderer); mSiteSectionViewHolder.bindDataSource(mTileGroup, tileRenderer);
int variation = ExploreSitesBridge.getVariation(); int variation = ExploreSitesBridge.getVariation();
...@@ -275,8 +281,7 @@ public class NewTabPageLayout extends LinearLayout implements TileGroup.Observer ...@@ -275,8 +281,7 @@ public class NewTabPageLayout extends LinearLayout implements TileGroup.Observer
mSearchBoxCoordinator, profile, mManager::performSearchQuery); mSearchBoxCoordinator, profile, mManager::performSearchQuery);
} }
mTileGroup.startObserving( mTileGroup.startObserving(maxRows * getMaxColumnsForMostVisitedTiles());
getMaxRowsForMostVisitedTiles() * getMaxColumnsForMostVisitedTiles());
VrModuleProvider.registerVrModeObserver(this); VrModuleProvider.registerVrModeObserver(this);
if (VrModuleProvider.getDelegate().isInVr()) onEnterVr(); if (VrModuleProvider.getDelegate().isInVr()) onEnterVr();
...@@ -801,13 +806,6 @@ public class NewTabPageLayout extends LinearLayout implements TileGroup.Observer ...@@ -801,13 +806,6 @@ public class NewTabPageLayout extends LinearLayout implements TileGroup.Observer
} }
} }
private int getMaxRowsForMostVisitedTiles() {
Integer maxRows = mQueryTileSection == null
? null
: mQueryTileSection.getMaxRowsForMostVisitedTiles();
return maxRows == null ? 2 : maxRows.intValue();
}
/** /**
* Determines The maximum number of tiles to try and fit in a row. On smaller screens, there * Determines The maximum number of tiles to try and fit in a row. On smaller screens, there
* may not be enough space to fit all of them. * may not be enough space to fit all of them.
......
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
package org.chromium.chrome.browser.query_tiles; package org.chromium.chrome.browser.query_tiles;
import android.content.Context;
import android.graphics.Bitmap; import android.graphics.Bitmap;
import android.view.View; import android.view.View;
import android.view.ViewGroup; import android.view.ViewGroup;
...@@ -42,6 +43,7 @@ import java.util.List; ...@@ -42,6 +43,7 @@ import java.util.List;
* section. * section.
*/ */
public class QueryTileSection { public class QueryTileSection {
private static final String UMA_PREFIX = "QueryTiles.NTP";
private static final String MOST_VISITED_MAX_ROWS_SMALL_SCREEN = private static final String MOST_VISITED_MAX_ROWS_SMALL_SCREEN =
"most_visited_max_rows_small_screen"; "most_visited_max_rows_small_screen";
private static final String MOST_VISITED_MAX_ROWS_NORMAL_SCREEN = private static final String MOST_VISITED_MAX_ROWS_NORMAL_SCREEN =
...@@ -49,7 +51,6 @@ public class QueryTileSection { ...@@ -49,7 +51,6 @@ public class QueryTileSection {
private static final String VARIATION_SMALL_SCREEN_HEIGHT_THRESHOLD_DP = private static final String VARIATION_SMALL_SCREEN_HEIGHT_THRESHOLD_DP =
"small_screen_height_threshold_dp"; "small_screen_height_threshold_dp";
private static final int DEFAULT_SMALL_SCREEN_HEIGHT_THRESHOLD_DP = 700; private static final int DEFAULT_SMALL_SCREEN_HEIGHT_THRESHOLD_DP = 700;
private static final String UMA_PREFIX = "QueryTiles.NTP";
private final ViewGroup mQueryTileSectionView; private final ViewGroup mQueryTileSectionView;
private final SearchBoxCoordinator mSearchBoxCoordinator; private final SearchBoxCoordinator mSearchBoxCoordinator;
...@@ -204,12 +205,12 @@ public class QueryTileSection { ...@@ -204,12 +205,12 @@ public class QueryTileSection {
} }
/** /**
* @param context Context for display calculation.
* @return Max number of rows for most visited tiles. For smaller screens, the most visited * @return Max number of rows for most visited tiles. For smaller screens, the most visited
* tiles section on NTP is shortened so that feed is still visible above the fold. * tiles section on NTP is shortened so that feed is still visible above the fold.
*/ */
public Integer getMaxRowsForMostVisitedTiles() { public static int getMaxRowsForMostVisitedTiles(Context context) {
DisplayAndroid display = DisplayAndroid display = DisplayAndroid.getNonMultiDisplay(context);
DisplayAndroid.getNonMultiDisplay(mQueryTileSectionView.getContext());
int screenHeightDp = DisplayUtil.pxToDp(display, display.getDisplayHeight()); int screenHeightDp = DisplayUtil.pxToDp(display, display.getDisplayHeight());
int smallScreenHeightThresholdDp = ChromeFeatureList.getFieldTrialParamByFeatureAsInt( int smallScreenHeightThresholdDp = ChromeFeatureList.getFieldTrialParamByFeatureAsInt(
ChromeFeatureList.QUERY_TILES, VARIATION_SMALL_SCREEN_HEIGHT_THRESHOLD_DP, ChromeFeatureList.QUERY_TILES, VARIATION_SMALL_SCREEN_HEIGHT_THRESHOLD_DP,
......
...@@ -43,19 +43,20 @@ public class SiteSection extends OptionalLeaf implements TileGroup.Observer { ...@@ -43,19 +43,20 @@ public class SiteSection extends OptionalLeaf implements TileGroup.Observer {
.inflate(getLayout(), parent, false); .inflate(getLayout(), parent, false);
} }
public static SiteSectionViewHolder createViewHolder(ViewGroup view, UiConfig uiConfig) { public static SiteSectionViewHolder createViewHolder(
return new TileGridViewHolder(view, getMaxTileRows(), MAX_TILE_COLUMNS); ViewGroup view, UiConfig uiConfig, int maxRows) {
return new TileGridViewHolder(view, maxRows, MAX_TILE_COLUMNS);
} }
public SiteSection(SuggestionsUiDelegate uiDelegate, ContextMenuManager contextMenuManager, public SiteSection(SuggestionsUiDelegate uiDelegate, ContextMenuManager contextMenuManager,
TileGroup.Delegate tileGroupDelegate, OfflinePageBridge offlinePageBridge, TileGroup.Delegate tileGroupDelegate, OfflinePageBridge offlinePageBridge,
UiConfig uiConfig) { UiConfig uiConfig, int maxRows) {
mTileRenderer = new TileRenderer(ContextUtils.getApplicationContext(), mTileRenderer = new TileRenderer(ContextUtils.getApplicationContext(),
SuggestionsConfig.getTileStyle(uiConfig), TILE_TITLE_LINES, SuggestionsConfig.getTileStyle(uiConfig), TILE_TITLE_LINES,
uiDelegate.getImageFetcher()); uiDelegate.getImageFetcher());
mTileGroup = new TileGroup(mTileRenderer, uiDelegate, contextMenuManager, tileGroupDelegate, mTileGroup = new TileGroup(mTileRenderer, uiDelegate, contextMenuManager, tileGroupDelegate,
/* observer = */ this, offlinePageBridge); /* observer = */ this, offlinePageBridge);
mTileGroup.startObserving(MAX_TILE_COLUMNS * getMaxTileRows()); mTileGroup.startObserving(MAX_TILE_COLUMNS * maxRows);
} }
@Override @Override
...@@ -108,10 +109,6 @@ public class SiteSection extends OptionalLeaf implements TileGroup.Observer { ...@@ -108,10 +109,6 @@ public class SiteSection extends OptionalLeaf implements TileGroup.Observer {
return mTileGroup; return mTileGroup;
} }
private static int getMaxTileRows() {
return 2;
}
@LayoutRes @LayoutRes
private static int getLayout() { private static int getLayout() {
return R.layout.suggestions_site_tile_grid_modern; return R.layout.suggestions_site_tile_grid_modern;
......
...@@ -132,7 +132,7 @@ public class TileGridLayoutTest { ...@@ -132,7 +132,7 @@ public class TileGridLayoutTest {
@Feature({"NewTabPage", "RenderTest"}) @Feature({"NewTabPage", "RenderTest"})
@ParameterAnnotations.UseMethodParameter(NightModeTestUtils.NightModeParams.class) @ParameterAnnotations.UseMethodParameter(NightModeTestUtils.NightModeParams.class)
// TODO(https://crbug.com/906151): Add new goldens and enable ExploreSites. // TODO(https://crbug.com/906151): Add new goldens and enable ExploreSites.
@DisableFeatures(ChromeFeatureList.EXPLORE_SITES) @DisableFeatures({ChromeFeatureList.EXPLORE_SITES, ChromeFeatureList.QUERY_TILES})
public void testTileGridAppearance(boolean nightModeEnabled) throws Exception { public void testTileGridAppearance(boolean nightModeEnabled) throws Exception {
NewTabPage ntp = setUpFakeDataToShowOnNtp(FAKE_MOST_VISITED_URLS.length); NewTabPage ntp = setUpFakeDataToShowOnNtp(FAKE_MOST_VISITED_URLS.length);
mRenderTestRule.render(getTileGridLayout(ntp), "ntp_tile_grid_layout"); mRenderTestRule.render(getTileGridLayout(ntp), "ntp_tile_grid_layout");
...@@ -296,7 +296,7 @@ public class TileGridLayoutTest { ...@@ -296,7 +296,7 @@ public class TileGridLayoutTest {
activity.setContentView(contentView); activity.setContentView(contentView);
SiteSectionViewHolder viewHolder = SiteSection.createViewHolder( SiteSectionViewHolder viewHolder = SiteSection.createViewHolder(
SiteSection.inflateSiteSection(contentView), uiConfig); SiteSection.inflateSiteSection(contentView), uiConfig, 2);
uiConfig.updateDisplayStyle(); uiConfig.updateDisplayStyle();
...@@ -345,7 +345,7 @@ public class TileGridLayoutTest { ...@@ -345,7 +345,7 @@ public class TileGridLayoutTest {
}; };
SiteSection siteSection = SiteSection siteSection =
new SiteSection(uiDelegate, null, delegate, offlinePageBridge, uiConfig); new SiteSection(uiDelegate, null, delegate, offlinePageBridge, uiConfig, 2);
siteSection.addObserver(new ListObservable.ListObserver<PartialBindCallback>() { siteSection.addObserver(new ListObservable.ListObserver<PartialBindCallback>() {
@Override @Override
......
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