Skip to content

GitLab

T
tangled
Commit 7e14a87d authored by Filip Gorski's avatar Filip Gorski Committed by Commit Bot
Browse files
Options

[Zine] Old favicon code clean up 

Bug: 751628
Change-Id: Ief50f500266330c5ecceff718001f92bd7d4f467
Reviewed-on: https://chromium-review.googlesource.com/c/1291840Reviewed-by: default avatarTheresa <twellington@chromium.org>
Commit-Queue: Filip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610962}
parent 06ef9411
Hide whitespace changes
Inline Side-by-side
Showing with 52 additions and 207 deletions
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ImageFetcher.java
View file @ 7e14a87d
......@@ -4,7 +4,6 @@
package org.chromium.chrome.browser.suggestions;
import android.annotation.SuppressLint;
import android.graphics.Bitmap;
import android.os.SystemClock;
import android.support.annotation.NonNull;
......@@ -14,20 +13,12 @@ import org.chromium.base.Callback;
import org.chromium.base.DiscardableReferencePool;
import org.chromium.base.Promise;
import org.chromium.base.VisibleForTesting;
import org.chromium.chrome.browser.favicon.FaviconHelper;
import org.chromium.chrome.browser.favicon.LargeIconBridge;
import org.chromium.chrome.browser.native_page.NativePageHost;
import org.chromium.chrome.browser.ntp.cards.CardsVariationParameters;
import org.chromium.chrome.browser.ntp.snippets.FaviconFetchResult;
import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
import org.chromium.chrome.browser.ntp.snippets.SnippetsConfig;
import org.chromium.chrome.browser.ntp.snippets.SuggestionsSource;
import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.widget.ThumbnailProvider;
import java.net.URI;
import java.net.URISyntaxException;
/**
* Class responsible for fetching images for the views in the NewTabPage and Chrome Home.
* The images fetched by this class include:
......@@ -46,18 +37,11 @@ import java.net.URISyntaxException;
* in the callback. Otherwise, the callback will not be called.
*/
public class ImageFetcher {
/** Supported favicon sizes in pixels. */
private static final int[] FAVICON_SERVICE_SUPPORTED_SIZES = {16, 24, 32, 48, 64};
private static final String FAVICON_SERVICE_FORMAT =
"https://s2.googleusercontent.com/s2/favicons?domain=%s&src=chrome_newtab_mobile&sz=%d&alt=404";
/** Min size for site attribution: only 16px as many sites do not have any other small icon. */
private static final int PUBLISHER_FAVICON_MINIMUM_SIZE_PX = 16;
static final int PUBLISHER_FAVICON_MINIMUM_SIZE_PX = 16;
/** Desired size for site attribution: only 32px as larger icons are often too complex */
private static final int PUBLISHER_FAVICON_DESIRED_SIZE_PX = 32;
private final boolean mUseFaviconService;
private final NativePageHost mHost;
static final int PUBLISHER_FAVICON_DESIRED_SIZE_PX = 32;
private boolean mIsDestroyed;
......@@ -65,16 +49,13 @@ public class ImageFetcher {
private final Profile mProfile;
private final DiscardableReferencePool mReferencePool;
private ThumbnailProvider mThumbnailProvider;
private FaviconHelper mFaviconHelper;
private LargeIconBridge mLargeIconBridge;
public ImageFetcher(SuggestionsSource suggestionsSource, Profile profile,
DiscardableReferencePool referencePool, NativePageHost host) {
DiscardableReferencePool referencePool) {
mSuggestionsSource = suggestionsSource;
mProfile = profile;
mReferencePool = referencePool;
mUseFaviconService = CardsVariationParameters.isFaviconServiceEnabled();
mHost = host;
}
/**
......@@ -117,35 +98,15 @@ public class ImageFetcher {
* If there is an error while fetching the favicon, the callback will not be called.
*
* @param suggestion The suggestion whose URL needs a favicon.
* @param faviconSizePx The required size for the favicon.
* @param faviconCallback The callback where the bitmap will be returned when fetched.
*/
public void makeFaviconRequest(SnippetArticle suggestion, final int faviconSizePx,
final Callback<Bitmap> faviconCallback) {
public void makeFaviconRequest(
SnippetArticle suggestion, final Callback<Bitmap> faviconCallback) {
assert !mIsDestroyed;
if (!suggestion.isContextual() && !suggestion.isArticle()) return;
long faviconFetchStartTimeMs = SystemClock.elapsedRealtime();
URI pageUrl;
try {
pageUrl = new URI(suggestion.getUrl());
} catch (URISyntaxException e) {
assert false;
return;
}
if (suggestion.isContextual()
|| (suggestion.isArticle() && SnippetsConfig.isFaviconsFromNewServerEnabled())) {
// The new code path.
fetchFaviconFromLocalCacheOrGoogleServer(
suggestion, faviconFetchStartTimeMs, faviconCallback);
} else {
// The old code path. Currently, we have to use this for non-articles, due to privacy.
// TODO(jkrcal): Remove this path when we completely switch to the new code on Stable.
// https://crbug.com/751628
fetchFaviconFromLocalCache(pageUrl, true, faviconFetchStartTimeMs, faviconSizePx,
suggestion, faviconCallback);
}
fetchFaviconFromLocalCacheOrGoogleServer(
suggestion, SystemClock.elapsedRealtime(), faviconCallback);
}
/**
......@@ -165,11 +126,6 @@ public class ImageFetcher {
}
public void onDestroy() {
if (mFaviconHelper != null) {
mFaviconHelper.destroy();
mFaviconHelper = null;
}
if (mLargeIconBridge != null) {
mLargeIconBridge.destroy();
mLargeIconBridge = null;
......@@ -183,40 +139,6 @@ public class ImageFetcher {
mIsDestroyed = true;
}
public static String getSnippetDomain(URI snippetUri) {
return String.format("%s://%s", snippetUri.getScheme(), snippetUri.getHost());
}
private void fetchFaviconFromLocalCache(final URI snippetUri, final boolean fallbackToService,
final long faviconFetchStartTimeMs, final int faviconSizePx,
final SnippetArticle suggestion, final Callback<Bitmap> faviconCallback) {
getFaviconHelper().getLocalFaviconImageForURL(mProfile, getSnippetDomain(snippetUri),
faviconSizePx, new FaviconHelper.FaviconImageCallback() {
@Override
public void onFaviconAvailable(Bitmap image, String iconUrl) {
if (image != null) {
assert faviconCallback != null;
faviconCallback.onResult(image);
recordFaviconFetchHistograms(suggestion,
fallbackToService ? FaviconFetchResult.SUCCESS_CACHED
: FaviconFetchResult.SUCCESS_FETCHED,
SystemClock.elapsedRealtime() - faviconFetchStartTimeMs);
// Update the time when the icon was last requested therefore postponing
// the automatic eviction of the favicon from the favicon database.
getFaviconHelper().touchOnDemandFavicon(mProfile, iconUrl);
} else if (fallbackToService) {
if (!fetchFaviconFromService(suggestion, snippetUri,
faviconFetchStartTimeMs, faviconSizePx, faviconCallback)) {
recordFaviconFetchHistograms(suggestion, FaviconFetchResult.FAILURE,
SystemClock.elapsedRealtime() - faviconFetchStartTimeMs);
}
}
}
});
}
private void fetchFaviconFromLocalCacheOrGoogleServer(SnippetArticle suggestion,
final long faviconFetchStartTimeMs, final Callback<Bitmap> faviconCallback) {
// The bitmap will not be resized to desired size in c++, this only expresses preference
......@@ -233,72 +155,6 @@ public class ImageFetcher {
});
}
// TODO(crbug.com/635567): Fix this properly.
@SuppressLint("DefaultLocale")
private boolean fetchFaviconFromService(final SnippetArticle suggestion, final URI snippetUri,
final long faviconFetchStartTimeMs, final int faviconSizePx,
final Callback<Bitmap> faviconCallback) {
if (!mUseFaviconService) return false;
int sizePx = getFaviconServiceSupportedSize(faviconSizePx);
if (sizePx == 0) return false;
// Replace the default icon by another one from the service when it is fetched.
ensureIconIsAvailable(
getSnippetDomain(snippetUri), // Store to the cache for the whole domain.
String.format(FAVICON_SERVICE_FORMAT, snippetUri.getHost(), sizePx),
/* isLargeIcon = */ false, new FaviconHelper.IconAvailabilityCallback() {
@Override
public void onIconAvailabilityChecked(boolean newlyAvailable) {
if (!newlyAvailable) {
recordFaviconFetchHistograms(suggestion, FaviconFetchResult.FAILURE,
SystemClock.elapsedRealtime() - faviconFetchStartTimeMs);
return;
}
// The download succeeded, the favicon is in the cache; fetch it.
fetchFaviconFromLocalCache(snippetUri, /* fallbackToService = */ false,
faviconFetchStartTimeMs, faviconSizePx, suggestion,
faviconCallback);
}
});
return true;
}
/**
* Checks if an icon with the given URL is available. If not,
* downloads it and stores it as a favicon/large icon for the given {@code pageUrl}.
* @param pageUrl The URL of the site whose icon is being requested.
* @param iconUrl The URL of the favicon/large icon.
* @param isLargeIcon Whether the {@code iconUrl} represents a large icon or favicon.
* @param callback The callback to be notified when the favicon has been checked.
*/
private void ensureIconIsAvailable(String pageUrl, String iconUrl, boolean isLargeIcon,
FaviconHelper.IconAvailabilityCallback callback) {
if (mHost.getActiveTab() != null && mHost.getActiveTab().getWebContents() != null) {
getFaviconHelper().ensureIconIsAvailable(mProfile,
mHost.getActiveTab().getWebContents(), pageUrl, iconUrl, isLargeIcon, callback);
}
}
private int getFaviconServiceSupportedSize(int faviconSizePX) {
// Take the smallest size larger than mFaviconSizePx.
for (int size : FAVICON_SERVICE_SUPPORTED_SIZES) {
if (size > faviconSizePX) return size;
}
// Or at least the largest available size (unless too small).
int largestSize =
FAVICON_SERVICE_SUPPORTED_SIZES[FAVICON_SERVICE_SUPPORTED_SIZES.length - 1];
if (faviconSizePX <= largestSize * 1.5) return largestSize;
return 0;
}
private void recordFaviconFetchHistograms(
SnippetArticle suggestion, int result, long fetchTime) {
// Record the histogram for articles only to have a fair comparision.
if (!suggestion.isArticle()) return;
SuggestionsMetrics.recordArticleFaviconFetchResult(result);
SuggestionsMetrics.recordArticleFaviconFetchTime(fetchTime);
}
/**
* Utility method to lazily create the {@link ThumbnailProvider}, and avoid unnecessary native
* calls in tests.
......@@ -311,18 +167,6 @@ public class ImageFetcher {
return mThumbnailProvider;
}
/**
* Utility method to lazily create the {@link FaviconHelper}, and avoid unnecessary native
* calls in tests.
*/
@VisibleForTesting
protected FaviconHelper getFaviconHelper() {
if (mFaviconHelper == null) {
mFaviconHelper = SuggestionsDependencyFactory.getInstance().createFaviconHelper();
}
return mFaviconHelper;
}
/**
* Utility method to lazily create the {@link LargeIconBridge}, and avoid unnecessary native
* calls in tests.
......
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBinder.java
View file @ 7e14a87d
......@@ -206,7 +206,7 @@ public class SuggestionsBinder {
setFaviconOnView(drawable, publisherFaviconSizePx);
};
mImageFetcher.makeFaviconRequest(mSuggestion, publisherFaviconSizePx, faviconCallback);
mImageFetcher.makeFaviconRequest(mSuggestion, faviconCallback);
}
private void setThumbnail() {
......
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java
View file @ 7e14a87d
......@@ -41,7 +41,7 @@ public class SuggestionsUiDelegateImpl implements SuggestionsUiDelegate {
mSuggestionsRanker = new SuggestionsRanker();
mSuggestionsEventReporter = eventReporter;
mSuggestionsNavigationDelegate = navigationDelegate;
mImageFetcher = new ImageFetcher(suggestionsSource, profile, referencePool, host);
mImageFetcher = new ImageFetcher(suggestionsSource, profile, referencePool);
mSnackbarManager = snackbarManager;
mHost = host;
......
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java
View file @ 7e14a87d
......@@ -485,12 +485,12 @@ public class ArticleSnippetsTest {
private class MockImageFetcher extends ImageFetcher {
public MockImageFetcher(
SuggestionsSource suggestionsSource, DiscardableReferencePool referencePool) {
super(suggestionsSource, null, referencePool, null);
super(suggestionsSource, null, referencePool);
}
@Override
public void makeFaviconRequest(SnippetArticle suggestion, final int faviconSizePx,
final Callback<Bitmap> faviconCallback) {
public void makeFaviconRequest(
SnippetArticle suggestion, final Callback<Bitmap> faviconCallback) {
// Run the callback asynchronously in case the caller made that assumption.
ThreadUtils.postOnUiThread(() -> {
// Return an arbitrary drawable.
......
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/ImageFetcherTest.java
View file @ 7e14a87d
......@@ -4,10 +4,11 @@
package org.chromium.chrome.browser.suggestions;
import static org.junit.Assert.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.chromium.chrome.test.util.browser.suggestions.ContentSuggestionsTestUtils.createDummySuggestion;
......@@ -23,11 +24,8 @@ import org.robolectric.annotation.Config;
import org.chromium.base.Callback;
import org.chromium.base.DiscardableReferencePool;
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.chrome.browser.favicon.FaviconHelper;
import org.chromium.chrome.browser.favicon.FaviconHelper.FaviconImageCallback;
import org.chromium.chrome.browser.favicon.LargeIconBridge;
import org.chromium.chrome.browser.favicon.LargeIconBridge.LargeIconCallback;
import org.chromium.chrome.browser.native_page.NativePageHost;
import org.chromium.chrome.browser.ntp.cards.CardsVariationParameters;
import org.chromium.chrome.browser.ntp.snippets.KnownCategories;
import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
......@@ -38,8 +36,8 @@ import org.chromium.chrome.browser.widget.ThumbnailProvider;
import org.chromium.chrome.test.support.DisableHistogramsRule;
import org.chromium.chrome.test.util.browser.suggestions.SuggestionsDependenciesRule;
import java.net.URI;
import java.util.HashMap;
/**
* Unit tests for {@link ImageFetcher}.
*/
......@@ -56,8 +54,6 @@ public class ImageFetcherTest {
private DiscardableReferencePool mReferencePool = new DiscardableReferencePool();
@Mock
private FaviconHelper mFaviconHelper;
@Mock
private ThumbnailProvider mThumbnailProvider;
@Mock
......@@ -72,30 +68,46 @@ public class ImageFetcherTest {
mSuggestionsDeps.getFactory().largeIconBridge = mLargeIconBridge;
mSuggestionsDeps.getFactory().thumbnailProvider = mThumbnailProvider;
mSuggestionsDeps.getFactory().faviconHelper = mFaviconHelper;
mSuggestionsDeps.getFactory().suggestionsSource = mSuggestionsSource;
}
@SuppressWarnings("unchecked")
@Test
public void testFaviconFetch() {
ImageFetcher imageFetcher = new ImageFetcher(mSuggestionsSource, mock(Profile.class),
mReferencePool, mock(NativePageHost.class));
SnippetArticle suggestion = createDummySuggestion(KnownCategories.BOOKMARKS);
imageFetcher.makeFaviconRequest(suggestion, IMAGE_SIZE_PX, mock(Callback.class));
String expectedFaviconDomain =
ImageFetcher.getSnippetDomain(URI.create(suggestion.getUrl()));
verify(mFaviconHelper)
.getLocalFaviconImageForURL(any(Profile.class), eq(expectedFaviconDomain),
eq(IMAGE_SIZE_PX), any(FaviconImageCallback.class));
ImageFetcher imageFetcher =
new ImageFetcher(mSuggestionsSource, mock(Profile.class), mReferencePool);
Callback mockCallback = mock(Callback.class);
@KnownCategories
int[] categoriesWithIcon = new int[] {KnownCategories.ARTICLES, KnownCategories.CONTEXTUAL};
for (@KnownCategories int category : categoriesWithIcon) {
SnippetArticle suggestion = createDummySuggestion(category);
imageFetcher.makeFaviconRequest(suggestion, mockCallback);
verify(mSuggestionsSource)
.fetchSuggestionFavicon(eq(suggestion),
eq(ImageFetcher.PUBLISHER_FAVICON_MINIMUM_SIZE_PX),
eq(ImageFetcher.PUBLISHER_FAVICON_DESIRED_SIZE_PX),
any(Callback.class));
}
@KnownCategories
int[] categoriesThatDontFetch = new int[] {
KnownCategories.DOWNLOADS, KnownCategories.BOOKMARKS, KnownCategories.READING_LIST};
for (@KnownCategories int category : categoriesThatDontFetch) {
SnippetArticle suggestion = createDummySuggestion(category);
imageFetcher.makeFaviconRequest(suggestion, mockCallback);
verify(mSuggestionsSource, never())
.fetchSuggestionFavicon(
eq(suggestion), anyInt(), anyInt(), any(Callback.class));
}
}
@Test
public void testDownloadThumbnailFetch() {
ImageFetcher imageFetcher = new ImageFetcher(mSuggestionsSource, mock(Profile.class),
mReferencePool, mock(NativePageHost.class));
ImageFetcher imageFetcher =
new ImageFetcher(mSuggestionsSource, mock(Profile.class), mReferencePool);
SnippetArticle suggestion = createDummySuggestion(KnownCategories.DOWNLOADS);
......@@ -110,8 +122,8 @@ public class ImageFetcherTest {
@SuppressWarnings("unchecked")
@Test
public void testArticleThumbnailFetch() {
ImageFetcher imageFetcher = new ImageFetcher(mSuggestionsSource, mock(Profile.class),
mReferencePool, mock(NativePageHost.class));
ImageFetcher imageFetcher =
new ImageFetcher(mSuggestionsSource, mock(Profile.class), mReferencePool);
SnippetArticle suggestion = createDummySuggestion(KnownCategories.ARTICLES);
imageFetcher.makeArticleThumbnailRequest(suggestion, mock(Callback.class));
......@@ -121,24 +133,13 @@ public class ImageFetcherTest {
@Test
public void testLargeIconFetch() {
ImageFetcher imageFetcher = new ImageFetcher(mSuggestionsSource, mock(Profile.class),
mReferencePool, mock(NativePageHost.class));
ImageFetcher imageFetcher =
new ImageFetcher(mSuggestionsSource, mock(Profile.class), mReferencePool);
imageFetcher.makeLargeIconRequest(URL_STRING, IMAGE_SIZE_PX, mock(LargeIconCallback.class));
String expectedIconDomain = ImageFetcher.getSnippetDomain(URI.create(URL_STRING));
verify(mLargeIconBridge)
.getLargeIconForUrl(
eq(expectedIconDomain), eq(IMAGE_SIZE_PX), any(LargeIconCallback.class));
}
@Test
public void testSnippetDomainExtraction() {
URI uri = URI.create(URL_STRING + "/test");
String expected = URL_STRING;
String actual = ImageFetcher.getSnippetDomain(uri);
assertEquals(expected, actual);
eq(URL_STRING), eq(IMAGE_SIZE_PX), any(LargeIconCallback.class));
}
}
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupUnitTest.java
View file @ 7e14a87d
......@@ -479,7 +479,7 @@ public class TileGroupUnitTest {
private final List<LargeIconCallback> mCallbackList = new ArrayList<>();
public FakeImageFetcher() {
super(null, null, null, null);
super(null, null, null);
}
@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