Skip to content

Commit

Permalink
Reland chromium#1 of [GURL] Migrate PseudoTab.
Browse files Browse the repository at this point in the history
Changes test to be more relaxed to account for fully uninitialized JNI.

> [GURL] Migrate PseudoTab.
>
> This includes a one-time migration for clients that have string URLs persisted as we convert them to serialized GURLs. Bots shouldn't see this (no persistence) but should cause a temporary blip (and then slow long-tail) reports of early GURL loads.
>
> Bug: 783819
>
> Change-Id: I4bade009cff4d698ac3441d1c416fe4b7d251349
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2945815
> Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
> Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
> Commit-Queue: Yaron Friedman <yfriedman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#893008}

Bug: 783819
Change-Id: Ief287405f56ec84a126c6f232626cb410b8648b9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2966016
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Owners-Override: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#893222}

Change-Id: Ifbe9dd55581d1992d748286409bdeba503748e36
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2969225
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Commit-Queue: Yaron Friedman <yfriedman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#894853}
  • Loading branch information
yfriedman authored and Chromium LUCI CQ committed Jun 22, 2021
1 parent b563ee6 commit ad1fa5c
Show file tree
Hide file tree
Showing 23 changed files with 149 additions and 100 deletions.
1 change: 1 addition & 0 deletions chrome/android/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1456,6 +1456,7 @@ android_library("chrome_test_java") {
"//ui/base/mojom:mojom_java",
"//url:gurl_java",
"//url:gurl_javatests",
"//url:gurl_junit_test_support",
"//url:origin_java",
"//url/mojom:url_mojom_gurl_java",
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1092,7 +1092,8 @@ private void startNewTabFromLauncherIcon() {
}

private void startAndWaitNativeInitialization() {
Assert.assertFalse(LibraryLoader.getInstance().isInitialized());
Assert.assertTrue(NativeLibraryLoadedStatus.getProviderForTesting() == null ||
!NativeLibraryLoadedStatus.getProviderForTesting().areMainDexNativeMethodsReady());

CommandLine.getInstance().removeSwitch(ChromeSwitches.DISABLE_NATIVE_INITIALIZATION);
TestThreadUtils.runOnUiThreadBlocking(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import org.chromium.content_public.browser.test.util.TestThreadUtils;
import org.chromium.content_public.browser.test.util.TestTouchUtils;
import org.chromium.url.GURL;
import org.chromium.url.JUnitTestGURLs;

import java.io.File;
import java.io.FileOutputStream;
Expand Down Expand Up @@ -360,14 +361,18 @@ public static FakeMostVisitedSites setMVTiles(SuggestionsDependenciesRule sugges
*/
public static List<SiteSuggestion> createFakeSiteSuggestions() {
List<SiteSuggestion> siteSuggestions = new ArrayList<>();
siteSuggestions.add(new SiteSuggestion("0 EXPLORE_SITES", new GURL("https://www.bar.com"),
"", TileTitleSource.UNKNOWN, TileSource.EXPLORE, TileSectionType.PERSONALIZED,
new Date()));
siteSuggestions.add(new SiteSuggestion("0 EXPLORE_SITES",
// Use pre-serialized GURL to avoid loading native.
JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL), "", TileTitleSource.UNKNOWN,
TileSource.EXPLORE, TileSectionType.PERSONALIZED, new Date()));

String urlTemplate = JUnitTestGURLs.getGURL(JUnitTestGURLs.URL_1_NUMERAL).serialize();
for (int i = 0; i < 7; i++) {
siteSuggestions.add(new SiteSuggestion(String.valueOf(i),
new GURL("https://www." + i + ".com"), "", TileTitleSource.TITLE_TAG,
TileSource.TOP_SITES, TileSectionType.PERSONALIZED, new Date()));
// Use pre-serialized GURL to avoid loading native.
GURL.deserialize(urlTemplate.replace("www.1.com", "www." + i + ".com")), "",
TileTitleSource.TITLE_TAG, TileSource.TOP_SITES, TileSectionType.PERSONALIZED,
new Date()));
}

return siteSuggestions;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,7 @@ void initWithNative() {

private void updateFavicon(Tab tab) {
assert mTabListFaviconProvider.isInitialized();
// TODO(crbug/783819): convert TabListFaviconProvider to GURL
mTabListFaviconProvider.getFaviconForUrlAsync(tab.getUrl().getSpec(), false,
mTabListFaviconProvider.getFaviconForUrlAsync(tab.getUrl(), false,
(Drawable favicon) -> { mPropertyModel.set(FAVICON, favicon); });
}

Expand Down Expand Up @@ -253,7 +252,7 @@ public boolean isDialogVisible() {

private void updateSelectedTab(Tab tab) {
mPropertyModel.set(TITLE, tab.getTitle());
mTabListFaviconProvider.getFaviconForUrlAsync(tab.getUrl().getSpec(), false,
mTabListFaviconProvider.getFaviconForUrlAsync(tab.getUrl(), false,
(Drawable favicon) -> { mPropertyModel.set(FAVICON, favicon); });
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.chromium.chrome.browser.tabpersistence.TabStateDirectory;
import org.chromium.chrome.browser.tasks.tab_management.TabUiFeatureUtilities;
import org.chromium.components.embedder_support.util.UrlUtilities;
import org.chromium.url.GURL;

import java.io.ByteArrayInputStream;
import java.io.DataInputStream;
Expand Down Expand Up @@ -179,10 +180,9 @@ public String getTitle() {
* Get the URL of the {@link PseudoTab}.
* @return The URL
*/
public String getUrl() {
// TODO(crbug/783819): Return the GURL directly.
public GURL getUrl() {
if (mTab != null && mTab.get() != null && mTab.get().isInitialized()) {
return mTab.get().getUrl() != null ? mTab.get().getUrl().getSpec() : null;
return mTab.get().getUrl();
}
assert mTabId != null;
return TabAttributeCache.getUrl(mTabId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@ public TabAttributeCache(TabModelSelector tabModelSelector) {
@Override
public void onUrlUpdated(Tab tab) {
if (tab.isIncognito()) return;
String url = tab.getUrl().getSpec();
cacheUrl(tab.getId(), url);
cacheUrl(tab.getId(), tab.getUrl());
}

@Override
Expand Down Expand Up @@ -107,6 +106,7 @@ public void tabClosureCommitted(Tab tab) {
int id = tab.getId();
getSharedPreferences()
.edit()
.remove(getDeprecatedUrlKey(id))
.remove(getUrlKey(id))
.remove(getTitleKey(id))
.remove(getRootIdKey(id))
Expand All @@ -126,7 +126,7 @@ public void onTabStateInitialized() {
mTabModelSelector.getTabModelFilterProvider().getTabModelFilter(false);
for (int i = 0; i < filter.getCount(); i++) {
Tab tab = filter.getTabAt(i);
cacheUrl(tab.getId(), tab.getUrl().getSpec());
cacheUrl(tab.getId(), tab.getUrl());
cacheTitle(tab.getId(), tab.getTitle());
cacheRootId(tab.getId(), CriticalPersistedTabData.from(tab).getRootId());
cacheTimestampMillis(
Expand Down Expand Up @@ -168,6 +168,11 @@ public static void setTitleForTesting(int id, String title) {
}

private static String getUrlKey(int id) {
return id + "_gurl";
}

// Legacy from when URL was serialized as raw string.
private static String getDeprecatedUrlKey(int id) {
return id + "_url";
}

Expand All @@ -176,21 +181,24 @@ private static String getUrlKey(int id) {
* @param id The ID of the {@link PseudoTab}.
* @return The URL
*/
public static String getUrl(int id) {
return getSharedPreferences().getString(getUrlKey(id), "");
public static GURL getUrl(int id) {
String url = getSharedPreferences().getString(getUrlKey(id), "");
if (!url.isEmpty()) {
return GURL.deserialize(url);
}
return new GURL(getSharedPreferences().getString(getDeprecatedUrlKey(id), ""));
}

private static void cacheUrl(int id, String url) {
// TODO(crbug/783819): Use GURL directly.
getSharedPreferences().edit().putString(getUrlKey(id), url).apply();
private static void cacheUrl(int id, GURL url) {
getSharedPreferences().edit().putString(getUrlKey(id), url.serialize()).apply();
}

/**
* Set the URL of a {@link PseudoTab}.
* @param id The ID of the {@link PseudoTab}.
* @param url The URL
*/
static void setUrlForTesting(int id, String url) {
static void setUrlForTesting(int id, GURL url) {
cacheUrl(id, url);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.chromium.chrome.browser.tasks.pseudotab.PseudoTab;
import org.chromium.chrome.tab_ui.R;
import org.chromium.content_public.browser.UiThreadTaskTraits;
import org.chromium.url.GURL;

import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -115,7 +116,7 @@ private void initializeAndStartFetching(PseudoTab tab) {
for (int i = 0; i < 4; i++) {
if (mTabs.get(i) != null) {
final int index = i;
final String url = mTabs.get(i).getUrl();
final GURL url = mTabs.get(i).getUrl();
final boolean isIncognito = mTabs.get(i).isIncognito();
// getTabThumbnailWithCallback() might call the callback up to twice,
// so use |lastFavicon| to avoid fetching the favicon the second time.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.chromium.chrome.tab_ui.R;
import org.chromium.components.embedder_support.util.UrlUtilities;
import org.chromium.ui.base.ViewUtils;
import org.chromium.url.GURL;

import java.util.List;

Expand Down Expand Up @@ -158,7 +159,7 @@ public Drawable getDefaultFaviconDrawable(boolean isIncognito) {
* @param faviconCallback The callback that requests for favicon.
*/
public void getFaviconForUrlAsync(
String url, boolean isIncognito, Callback<Drawable> faviconCallback) {
GURL url, boolean isIncognito, Callback<Drawable> faviconCallback) {
if (mFaviconHelper == null || UrlUtilities.isNTPUrl(url)) {
faviconCallback.onResult(getRoundedChromeDrawable(isIncognito));
} else {
Expand Down Expand Up @@ -189,7 +190,7 @@ public Drawable getFaviconForUrlSync(@NonNull Bitmap icon) {
* @param faviconCallback The callback that requests for the composed favicon.
*/
public void getComposedFaviconImageAsync(
List<String> urls, boolean isIncognito, Callback<Drawable> faviconCallback) {
List<GURL> urls, boolean isIncognito, Callback<Drawable> faviconCallback) {
assert urls != null && urls.size() > 1 && urls.size() <= 4;

mFaviconHelper.getComposedFaviconImage(mProfile, urls, mFaviconSize, (image, iconUrl) -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1616,11 +1616,11 @@ void updateFaviconForTab(PseudoTab pseudoTab, @Nullable Bitmap icon) {
}

// The order of the url list matches the multi-thumbnail.
List<String> urls = new ArrayList<>();
List<GURL> urls = new ArrayList<>();
urls.add(pseudoTab.getUrl());
for (int i = 0; urls.size() < 4 && i < relatedTabList.size(); i++) {
if (pseudoTab.getId() == relatedTabList.get(i).getId()) continue;
urls.add(relatedTabList.get(i).getUrl().getSpec());
urls.add(relatedTabList.get(i).getUrl());
}

// For tab group card in grid tab switcher, the favicon is the composed favicon.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.chromium.chrome.browser.tasks.tab_management.TabListFaviconProvider;
import org.chromium.chrome.browser.tasks.tab_management.TabSwitcher;
import org.chromium.ui.modelutil.PropertyModel;
import org.chromium.url.GURL;
import org.chromium.url.JUnitTestGURLs;

/** Tests for {@link SingleTabSwitcherMediator}. */
Expand All @@ -52,10 +53,10 @@
public class SingleTabSwitcherMediatorUnitTest {
private final int mTabId = 1;
private final String mTitle = "test";
private final String mUrlString = JUnitTestGURLs.URL_1;
private final GURL mUrl = JUnitTestGURLs.getGURL(JUnitTestGURLs.URL_1);
private final int mTabId2 = 2;
private final String mTitle2 = "test2";
private final String mUrlString2 = JUnitTestGURLs.URL_2;
private final GURL mUrl2 = JUnitTestGURLs.getGURL(JUnitTestGURLs.URL_2);
private SingleTabSwitcherMediator mMediator;
private PropertyModel mPropertyModel;

Expand Down Expand Up @@ -99,10 +100,10 @@ public void setUp() {
doReturn(0).when(mNormalTabModel).index();
doReturn(1).when(mNormalTabModel).getCount();
doReturn(false).when(mNormalTabModel).isIncognito();
doReturn(JUnitTestGURLs.getGURL(mUrlString)).when(mTab).getUrl();
doReturn(mUrl).when(mTab).getUrl();
doReturn(mTabId).when(mTab).getId();
doReturn(mTitle).when(mTab).getTitle();
doReturn(JUnitTestGURLs.getGURL(mUrlString2)).when(mTab2).getUrl();
doReturn(mUrl2).when(mTab2).getUrl();
doReturn(mTabId2).when(mTab2).getId();
doReturn(mTitle2).when(mTab2).getTitle();
doReturn(true).when(mIncognitoTabModel).isIncognito();
Expand Down Expand Up @@ -130,7 +131,7 @@ public void showAndHide() {
.addTabModelFilterObserver(mTabModelObserverCaptor.capture());
verify(mTabModelSelector).addObserver(mTabModelSelectorObserverCaptor.capture());
verify(mTabListFaviconProvider)
.getFaviconForUrlAsync(eq(mUrlString), eq(false), mFaviconCallbackCaptor.capture());
.getFaviconForUrlAsync(eq(mUrl), eq(false), mFaviconCallbackCaptor.capture());
assertTrue(mMediator.overviewVisible());
verify(mOverviewModeObserver).startedShowing();
verify(mOverviewModeObserver).finishedShowing();
Expand Down Expand Up @@ -160,7 +161,7 @@ public void selectTabAfterSwitchingTabModel() {
.addTabModelFilterObserver(mTabModelObserverCaptor.capture());
verify(mTabModelSelector).addObserver(mTabModelSelectorObserverCaptor.capture());
verify(mTabListFaviconProvider)
.getFaviconForUrlAsync(eq(mUrlString), eq(false), mFaviconCallbackCaptor.capture());
.getFaviconForUrlAsync(eq(mUrl), eq(false), mFaviconCallbackCaptor.capture());
assertTrue(mMediator.overviewVisible());
verify(mOverviewModeObserver).startedShowing();
verify(mOverviewModeObserver).finishedShowing();
Expand Down Expand Up @@ -199,7 +200,7 @@ public void selectNextTabAfterClosingTheSelectedTab() {
.addTabModelFilterObserver(mTabModelObserverCaptor.capture());
verify(mTabModelSelector).addObserver(mTabModelSelectorObserverCaptor.capture());
verify(mTabListFaviconProvider)
.getFaviconForUrlAsync(eq(mUrlString), eq(false), mFaviconCallbackCaptor.capture());
.getFaviconForUrlAsync(eq(mUrl), eq(false), mFaviconCallbackCaptor.capture());
assertTrue(mMediator.overviewVisible());
verify(mOverviewModeObserver).startedShowing();
verify(mOverviewModeObserver).finishedShowing();
Expand Down Expand Up @@ -228,7 +229,7 @@ public void selectTabAfterSwitchingTabModelAndReshown() {
.addTabModelFilterObserver(mTabModelObserverCaptor.capture());
verify(mTabModelSelector).addObserver(mTabModelSelectorObserverCaptor.capture());
verify(mTabListFaviconProvider)
.getFaviconForUrlAsync(eq(mUrlString), eq(false), mFaviconCallbackCaptor.capture());
.getFaviconForUrlAsync(eq(mUrl), eq(false), mFaviconCallbackCaptor.capture());
assertEquals(mPropertyModel.get(TITLE), mTitle);

mTabModelSelectorObserverCaptor.getValue().onTabModelSelected(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.chromium.chrome.test.util.browser.Features;
import org.chromium.chrome.test.util.browser.Features.DisableFeatures;
import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
import org.chromium.url.GURL;
import org.chromium.url.JUnitTestGURLs;

import java.util.ArrayList;
Expand Down Expand Up @@ -250,11 +251,11 @@ public void getTitle_cache() {

@Test
public void getUrl_real() {
String url = JUnitTestGURLs.EXAMPLE_URL;
doReturn(JUnitTestGURLs.getGURL(url)).when(mTab1).getUrl();
GURL url = JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL);
doReturn(url).when(mTab1).getUrl();

PseudoTab tab = PseudoTab.fromTabId(TAB1_ID);
Assert.assertEquals("", tab.getUrl());
Assert.assertEquals(GURL.emptyGURL(), tab.getUrl());

PseudoTab realTab = PseudoTab.fromTab(mTab1);
Assert.assertNotEquals(tab, realTab);
Expand All @@ -263,11 +264,11 @@ public void getUrl_real() {

@Test
public void getUrl_cache() {
String url = "url 1";
TabAttributeCache.setUrlForTesting(TAB1_ID, url);
String url = JUnitTestGURLs.URL_1;
TabAttributeCache.setUrlForTesting(TAB1_ID, JUnitTestGURLs.getGURL(url));

PseudoTab tab = PseudoTab.fromTabId(TAB1_ID);
Assert.assertEquals(url, tab.getUrl());
Assert.assertEquals(url, tab.getUrl().getSpec());

PseudoTab realTab = PseudoTab.fromTab(mTab1);
Assert.assertNotEquals(tab, realTab);
Expand Down Expand Up @@ -466,7 +467,7 @@ public void testTabDestroyedUrl() {
// Url was not set. Without the isInitialized() check,
// pseudoTab.getUrl() would crash here with
// UnsupportedOperationException
Assert.assertEquals("", pseudoTab.getUrl());
Assert.assertEquals("", pseudoTab.getUrl().getSpec());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,8 @@ public void tearDown() {

@Test
public void updateUrl() {
String url = JUnitTestGURLs.EXAMPLE_URL;
doReturn(JUnitTestGURLs.getGURL(url)).when(mTab1).getUrl();
GURL url = JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL);
doReturn(url).when(mTab1).getUrl();

Assert.assertNotEquals(url, TabAttributeCache.getUrl(TAB1_ID));

Expand Down Expand Up @@ -359,17 +359,17 @@ public void removeEscapedCodePoints() {

@Test
public void onTabStateInitialized() {
String url1 = JUnitTestGURLs.EXAMPLE_URL;
doReturn(JUnitTestGURLs.getGURL(url1)).when(mTab1).getUrl();
GURL url1 = JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL);
doReturn(url1).when(mTab1).getUrl();
String title1 = "title 1";
doReturn(title1).when(mTab1).getTitle();
int rootId1 = 1337;
doReturn(rootId1).when(mCriticalPersistedTabData1).getRootId();
long timestamp1 = 123456;
doReturn(timestamp1).when(mCriticalPersistedTabData1).getTimestampMillis();

String url2 = JUnitTestGURLs.URL_2;
doReturn(JUnitTestGURLs.getGURL(url2)).when(mTab2).getUrl();
GURL url2 = JUnitTestGURLs.getGURL(JUnitTestGURLs.URL_2);
doReturn(url2).when(mTab2).getUrl();
String title2 = "title 2";
doReturn(title2).when(mTab2).getTitle();
int rootId2 = 42;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.chromium.chrome.browser.tasks.tab_management.TabUiUnitTestUtils;
import org.chromium.chrome.test.util.browser.Features;
import org.chromium.testing.local.LocalRobolectricTestRunner;
import org.chromium.url.GURL;

import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -90,9 +91,9 @@ public void setUp() {

MockitoAnnotations.initMocks(this);

mTab1 = TabUiUnitTestUtils.prepareTab(TAB1_ID, TAB1_TITLE, "");
mTab2 = TabUiUnitTestUtils.prepareTab(TAB2_ID, TAB2_TITLE, "");
mTab3 = TabUiUnitTestUtils.prepareTab(TAB3_ID, TAB3_TITLE, "");
mTab1 = TabUiUnitTestUtils.prepareTab(TAB1_ID, TAB1_TITLE, GURL.emptyGURL());
mTab2 = TabUiUnitTestUtils.prepareTab(TAB2_ID, TAB2_TITLE, GURL.emptyGURL());
mTab3 = TabUiUnitTestUtils.prepareTab(TAB3_ID, TAB3_TITLE, GURL.emptyGURL());

doReturn(mTabModelFilterProvider).when(mTabModelSelector).getTabModelFilterProvider();
doReturn(mTabGroupModelFilter).when(mTabModelFilterProvider).getCurrentTabModelFilter();
Expand Down
Loading

0 comments on commit ad1fa5c

Please sign in to comment.