Skip to content

Commit

Permalink
Migrate NavigationEntry from String url to GURL
Browse files Browse the repository at this point in the history
BUG=783819

Change-Id: I45975dff6acc8a753aec5ac20ad658c96ad5e24c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587459
Commit-Queue: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837151}
  • Loading branch information
yfriedman authored and Chromium LUCI CQ committed Dec 15, 2020
1 parent 8ed8125 commit 09b9c59
Show file tree
Hide file tree
Showing 33 changed files with 229 additions and 178 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,16 @@
import android.webkit.WebHistoryItem;

import org.chromium.content_public.browser.NavigationEntry;
import org.chromium.url.GURL;

/**
* WebView Chromium implementation of WebHistoryItem. Simple immutable wrapper
* around NavigationEntry
*/
@SuppressWarnings("deprecation")
public class WebHistoryItemChromium extends WebHistoryItem {
private final String mUrl;
private final String mOriginalUrl;
private final GURL mUrl;
private final GURL mOriginalUrl;
private final String mTitle;
private final Bitmap mFavicon;

Expand All @@ -41,15 +42,15 @@ public int getId() {
*/
@Override
public String getUrl() {
return mUrl;
return mUrl.getSpec();
}

/**
* See {@link android.webkit.WebHistoryItem#getOriginalUrl}.
*/
@Override
public String getOriginalUrl() {
return mOriginalUrl;
return mOriginalUrl.getSpec();
}

/**
Expand All @@ -69,7 +70,7 @@ public Bitmap getFavicon() {
}

// Clone constructor.
private WebHistoryItemChromium(String url, String originalUrl, String title, Bitmap favicon) {
private WebHistoryItemChromium(GURL url, GURL originalUrl, String title, Bitmap favicon) {
mUrl = url;
mOriginalUrl = originalUrl;
mTitle = title;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2538,7 +2538,7 @@ public String getOriginalUrl() {
NavigationHistory history = mNavigationController.getNavigationHistory();
int currentIndex = history.getCurrentEntryIndex();
if (currentIndex >= 0 && currentIndex < history.getEntryCount()) {
return history.getEntryAtIndex(currentIndex).getOriginalUrl();
return history.getEntryAtIndex(currentIndex).getOriginalUrl().getSpec();
}
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -862,7 +862,7 @@ public boolean shouldOverrideUrlLoading(AwContentsClient.AwWebResourceRequest re
NavigationHistory navHistory = mAwContents.getNavigationHistory();
Assert.assertEquals(2, navHistory.getEntryCount());
Assert.assertEquals(1, navHistory.getCurrentEntryIndex());
Assert.assertEquals(linkUrl, navHistory.getEntryAtIndex(1).getUrl());
Assert.assertEquals(linkUrl, navHistory.getEntryAtIndex(1).getUrl().getSpec());

pageFinishedCount = onPageFinishedHelper.getCallCount();
InstrumentationRegistry.getInstrumentation().runOnMainSync(() -> mAwContents.goBack());
Expand All @@ -873,7 +873,7 @@ public boolean shouldOverrideUrlLoading(AwContentsClient.AwWebResourceRequest re
navHistory = mAwContents.getNavigationHistory();
Assert.assertEquals(2, navHistory.getEntryCount());
Assert.assertEquals(0, navHistory.getCurrentEntryIndex());
Assert.assertEquals(firstUrl, navHistory.getEntryAtIndex(0).getUrl());
Assert.assertEquals(firstUrl, navHistory.getEntryAtIndex(0).getUrl().getSpec());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ private NavigationHistory getNavigationHistory(final AwContents awContents)

private void checkHistoryItem(NavigationEntry item, String url, String originalUrl,
String title, boolean faviconNull) {
Assert.assertEquals(url, item.getUrl());
Assert.assertEquals(originalUrl, item.getOriginalUrl());
Assert.assertEquals(url, item.getUrl().getSpec());
Assert.assertEquals(originalUrl, item.getOriginalUrl().getSpec());
Assert.assertEquals(title, item.getTitle());
if (faviconNull) {
Assert.assertNull(item.getFavicon());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ private void checkHistoryItemList(TestVars vars) throws Throwable {
// but is only meant to test enough to make sure state is restored.
// See NavigationHistoryTest for more thorough tests.
for (int i = 0; i < NUM_NAVIGATIONS; ++i) {
Assert.assertEquals(mUrls[i], history.getEntryAtIndex(i).getOriginalUrl());
Assert.assertEquals(mUrls[i], history.getEntryAtIndex(i).getUrl());
Assert.assertEquals(mUrls[i], history.getEntryAtIndex(i).getOriginalUrl().getSpec());
Assert.assertEquals(mUrls[i], history.getEntryAtIndex(i).getUrl().getSpec());
Assert.assertEquals(TITLES[i], history.getEntryAtIndex(i).getTitle());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.chromium.content_public.browser.NavigationController;
import org.chromium.content_public.browser.NavigationHandle;
import org.chromium.content_public.browser.NavigationHistory;
import org.chromium.url.GURL;

/**
* Cache for attributes of {@link PseudoTab} to be available before native is ready.
Expand Down Expand Up @@ -248,13 +249,13 @@ private static void cacheLastSearchTerm(int id, String searchTerm) {
NavigationHistory history = controller.getNavigationHistory();

if (!TextUtils.isEmpty(
TemplateUrlServiceFactory.get().getSearchQueryForUrl(tab.getUrlString()))) {
TemplateUrlServiceFactory.get().getSearchQueryForUrl(tab.getUrl()))) {
// If we are already at a search result page, do not show the last search term.
return null;
}

for (int i = history.getCurrentEntryIndex() - 1; i >= 0; i--) {
String url = history.getEntryAtIndex(i).getOriginalUrl();
GURL url = history.getEntryAtIndex(i).getOriginalUrl();
String query = TemplateUrlServiceFactory.get().getSearchQueryForUrl(url);
if (!TextUtils.isEmpty(query)) {
return removeEscapedCodePoints(query);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1639,11 +1639,11 @@ static void navigateToLastSearchQuery(Tab tab) {
int offset = i - history.getCurrentEntryIndex();
if (!controller.canGoToOffset(offset)) continue;

String url = history.getEntryAtIndex(i).getOriginalUrl();
GURL url = history.getEntryAtIndex(i).getOriginalUrl();
String query = TemplateUrlServiceFactory.get().getSearchQueryForUrl(url);
if (TextUtils.isEmpty(query)) continue;

tab.loadUrl(new LoadUrlParams(url, PageTransition.KEYWORD_GENERATED));
tab.loadUrl(new LoadUrlParams(url.getSpec(), PageTransition.KEYWORD_GENERATED));
return;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ private static String getReferrerUrlFromTab(Tab tab) {
if (lastCommittedEntry == null) {
return null;
}
return lastCommittedEntry.getReferrerUrl();
return lastCommittedEntry.getReferrerUrl().getSpec();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
import org.chromium.content_public.browser.NavigationHandle;
import org.chromium.content_public.browser.NavigationHistory;
import org.chromium.content_public.browser.WebContents;
import org.chromium.url.GURL;
import org.chromium.url.JUnitTestGURLs;

import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -256,10 +258,10 @@ public void updateLastSearchTerm_incognito() {

@Test
public void findLastSearchTerm() {
String otherUrl = "https://example.com";
String searchUrl = "https://www.google.com/search?q=test";
GURL otherUrl = JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL);
GURL searchUrl = JUnitTestGURLs.getGURL(JUnitTestGURLs.SEARCH_URL);
String searchTerm = "test";
String searchUrl2 = "https://www.google.com/search?q=query";
GURL searchUrl2 = JUnitTestGURLs.getGURL(JUnitTestGURLs.SEARCH_2_URL);
String searchTerm2 = "query";

TemplateUrlService service = Mockito.mock(TemplateUrlService.class);
Expand All @@ -279,7 +281,7 @@ public void findLastSearchTerm() {
NavigationEntry navigationEntry0 = mock(NavigationEntry.class);
doReturn(navigationEntry1).when(navigationHistory).getEntryAtIndex(1);
doReturn(navigationEntry0).when(navigationHistory).getEntryAtIndex(0);
doReturn(otherUrl).when(mTab1).getUrlString();
doReturn(otherUrl).when(mTab1).getUrl();

// No searches.
doReturn(otherUrl).when(navigationEntry1).getOriginalUrl();
Expand Down Expand Up @@ -307,11 +309,11 @@ public void findLastSearchTerm() {

// Skip if the SRP is showing.
doReturn(2).when(navigationHistory).getCurrentEntryIndex();
doReturn(searchUrl).when(mTab1).getUrlString();
doReturn(searchUrl).when(mTab1).getUrl();
Assert.assertNull(TabAttributeCache.findLastSearchTerm(mTab1));

// Reset current SRP.
doReturn(otherUrl).when(mTab1).getUrlString();
doReturn(otherUrl).when(mTab1).getUrl();
Assert.assertEquals(searchTerm, TabAttributeCache.findLastSearchTerm(mTab1));

verify(navigationHistory, never()).getEntryAtIndex(eq(2));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@
import org.chromium.ui.base.PageTransition;
import org.chromium.ui.modelutil.PropertyModel;
import org.chromium.ui.modelutil.SimpleRecyclerViewAdapter;
import org.chromium.url.GURL;
import org.chromium.url.JUnitTestGURLs;

import java.lang.annotation.Retention;
Expand Down Expand Up @@ -1949,10 +1950,10 @@ public void testSearchTermProperty_TabGroups_Strip() {
public void navigateToLastSearchQuery() {
initAndAssertAllProperties();

String otherUrl = "https://example.com";
String searchUrl = "https://www.google.com/search?q=test";
GURL otherUrl = JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL);
GURL searchUrl = JUnitTestGURLs.getGURL(JUnitTestGURLs.SEARCH_URL);
String searchTerm = "test";
String searchUrl2 = "https://www.google.com/search?q=query";
GURL searchUrl2 = JUnitTestGURLs.getGURL(JUnitTestGURLs.SEARCH_2_URL);
String searchTerm2 = "query";
TemplateUrlService service = Mockito.mock(TemplateUrlService.class);
doReturn(null).when(service).getSearchQueryForUrl(otherUrl);
Expand Down Expand Up @@ -1986,47 +1987,47 @@ public void navigateToLastSearchQuery() {
doReturn(otherUrl).when(navigationEntry0).getOriginalUrl();
TabListMediator.SearchTermChipUtils.navigateToLastSearchQuery(mTab1);
inOrder.verify(mTab1).loadUrl(
refEq(new LoadUrlParams(searchUrl, PageTransition.KEYWORD_GENERATED)));
refEq(new LoadUrlParams(searchUrl.getSpec(), PageTransition.KEYWORD_GENERATED)));

// Has earlier SRP.
doReturn(otherUrl).when(navigationEntry1).getOriginalUrl();
doReturn(searchUrl2).when(navigationEntry0).getOriginalUrl();
TabListMediator.SearchTermChipUtils.navigateToLastSearchQuery(mTab1);
inOrder.verify(mTab1).loadUrl(
refEq(new LoadUrlParams(searchUrl2, PageTransition.KEYWORD_GENERATED)));
refEq(new LoadUrlParams(searchUrl2.getSpec(), PageTransition.KEYWORD_GENERATED)));

// Latest one wins.
doReturn(searchUrl).when(navigationEntry1).getOriginalUrl();
doReturn(searchUrl2).when(navigationEntry0).getOriginalUrl();
TabListMediator.SearchTermChipUtils.navigateToLastSearchQuery(mTab1);
inOrder.verify(mTab1).loadUrl(
refEq(new LoadUrlParams(searchUrl, PageTransition.KEYWORD_GENERATED)));
refEq(new LoadUrlParams(searchUrl.getSpec(), PageTransition.KEYWORD_GENERATED)));

// Rejected by canGoToOffset().
doReturn(false).when(navigationController).canGoToOffset(eq(-1));
TabListMediator.SearchTermChipUtils.navigateToLastSearchQuery(mTab1);
inOrder.verify(mTab1).loadUrl(
refEq(new LoadUrlParams(searchUrl2, PageTransition.KEYWORD_GENERATED)));
refEq(new LoadUrlParams(searchUrl2.getSpec(), PageTransition.KEYWORD_GENERATED)));

// Reset canGoToOffset().
doReturn(true).when(navigationController).canGoToOffset(anyInt());
TabListMediator.SearchTermChipUtils.navigateToLastSearchQuery(mTab1);
inOrder.verify(mTab1).loadUrl(
refEq(new LoadUrlParams(searchUrl, PageTransition.KEYWORD_GENERATED)));
refEq(new LoadUrlParams(searchUrl.getSpec(), PageTransition.KEYWORD_GENERATED)));

// Only care about previous ones.
doReturn(1).when(navigationHistory).getCurrentEntryIndex();
TabListMediator.SearchTermChipUtils.navigateToLastSearchQuery(mTab1);
inOrder.verify(mTab1).loadUrl(
refEq(new LoadUrlParams(searchUrl2, PageTransition.KEYWORD_GENERATED)));
refEq(new LoadUrlParams(searchUrl2.getSpec(), PageTransition.KEYWORD_GENERATED)));
}

@Test
public void searchListener() {
initAndAssertAllProperties();

String otherUrl = "https://example.com";
String searchUrl = "https://www.google.com/search?q=test";
GURL otherUrl = JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL);
GURL searchUrl = JUnitTestGURLs.getGURL(JUnitTestGURLs.SEARCH_URL);
String searchTerm = "test";
TemplateUrlService service = Mockito.mock(TemplateUrlService.class);
doReturn(null).when(service).getSearchQueryForUrl(otherUrl);
Expand Down Expand Up @@ -2055,14 +2056,14 @@ public void searchListener() {
verify(mGridCardOnClickListenerProvider)
.onTabSelecting(mModel.get(0).model.get(TabProperties.TAB_ID));
verify(mTab1).loadUrl(
refEq(new LoadUrlParams(searchUrl, PageTransition.KEYWORD_GENERATED)));
refEq(new LoadUrlParams(searchUrl.getSpec(), PageTransition.KEYWORD_GENERATED)));
}

@Test
public void searchListener_frozenTab() {
initAndAssertAllProperties();

String searchUrl = JUnitTestGURLs.SEARCH_URL;
GURL searchUrl = JUnitTestGURLs.getGURL(JUnitTestGURLs.SEARCH_URL);
String searchTerm = "test";
TemplateUrlService service = Mockito.mock(TemplateUrlService.class);
doReturn(searchTerm).when(service).getSearchQueryForUrl(searchUrl);
Expand All @@ -2088,9 +2089,9 @@ public void searchListener_frozenTab() {
verify(navigationController, never()).goToOffset(0);

doReturn(webContents).when(mTab1).getWebContents();
mTabObserverCaptor.getValue().onPageLoadStarted(mTab1, JUnitTestGURLs.getGURL(searchUrl));
mTabObserverCaptor.getValue().onPageLoadStarted(mTab1, searchUrl);
verify(mTab1).loadUrl(
refEq(new LoadUrlParams(searchUrl, PageTransition.KEYWORD_GENERATED)));
refEq(new LoadUrlParams(searchUrl.getSpec(), PageTransition.KEYWORD_GENERATED)));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import static org.mockito.Mockito.mock;

import org.json.JSONException;
import org.json.JSONObject;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
Expand All @@ -35,6 +36,7 @@
import org.chromium.content_public.browser.WebContents;
import org.chromium.testing.local.LocalRobolectricTestRunner;
import org.chromium.url.GURL;
import org.chromium.url.JUnitTestGURLs;

import java.util.Arrays;
import java.util.List;
Expand All @@ -54,7 +56,7 @@ public class TabContextTest {
private static final int LAST_COMMITTED_INDEX = 1;
private static final String TAB_CONTEXT_TAB_0_JSON = "[{\"id\":0,\"url\":"
+ "\"mock_url_tab_0\",\"title\":\"mock_title_tab_0\",\"timestamp\":100,"
+ "\"referrer\":\"mock_referrer_url_tab_0\"}]";
+ "\"referrer\":" + JSONObject.quote(JUnitTestGURLs.EXAMPLE_URL + "/") + "}]";

@Rule
public TestRule mProcessor = new Features.JUnitProcessor();
Expand All @@ -75,13 +77,13 @@ public class TabContextTest {
private TabModelFilter mTabModelFilter;

private Tab mTab0 = mockTab(TAB_0_ID, 6, "mock_title_tab_0", "mock_url_tab_0",
"mock_original_url_tab_0", "mock_referrer_url_tab_0", 100);
private Tab mRelatedTab0 =
mockTab(RELATED_TAB_0_ID, 6, "mock_title_related_tab_0", "mock_url_related_tab_0",
"mock_original_url_related_tab_0", "mock_referrer_url_related_tab_0", 200);
private Tab mRelatedTab1 =
mockTab(RELATED_TAB_1_ID, 6, "mock_title_related_tab_1", "mock_url_related_tab_1",
"mock_original_url_related_tab_1", "mock_referrer_url_related_tab_1", 300);
"mock_original_url_tab_0", JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL), 100);
private Tab mRelatedTab0 = mockTab(RELATED_TAB_0_ID, 6, "mock_title_related_tab_0",
"mock_url_related_tab_0", "mock_original_url_related_tab_0",
JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL), 200);
private Tab mRelatedTab1 = mockTab(RELATED_TAB_1_ID, 6, "mock_title_related_tab_1",
"mock_url_related_tab_1", "mock_original_url_related_tab_1",
JUnitTestGURLs.getGURL(JUnitTestGURLs.EXAMPLE_URL), 300);

@Before
public void setUp() {
Expand All @@ -91,8 +93,9 @@ public void setUp() {
doReturn(mTabModelFilter).when(mTabModelFilterProvider).getCurrentTabModelFilter();
}

// TODO(yfriedman): All of these should be GURLs.
private static TabImpl mockTab(int id, int rootId, String title, String url, String originalUrl,
String referrerUrl, long timestampMillis) {
GURL referrerUrl, long timestampMillis) {
TabImpl tab = mock(TabImpl.class);
doReturn(id).when(tab).getId();
UserDataHost userDataHost = new UserDataHost();
Expand All @@ -104,9 +107,7 @@ private static TabImpl mockTab(int id, int rootId, String title, String url, Str
doReturn(url).when(tab).getUrlString();
doReturn(originalUrl).when(tab).getOriginalUrl();
WebContents webContents = mock(WebContents.class);
GURL gurl = mock(GURL.class);
doReturn("").when(gurl).getSpec();
doReturn(gurl).when(webContents).getVisibleUrl();
doReturn(GURL.emptyGURL()).when(webContents).getVisibleUrl();
doReturn(webContents).when(tab).getWebContents();
NavigationController navigationController = mock(NavigationController.class);
doReturn(navigationController).when(webContents).getNavigationController();
Expand Down Expand Up @@ -160,8 +161,8 @@ public void testFindNoRelatedTabs() {

@Test
public void testExcludeClosingTabs() {
Tab newTab1 = mockTab(NEW_TAB_1_ID, NEW_TAB_1_ID, "", "", "", "", 0);
Tab newTab2 = mockTab(NEW_TAB_2_ID, NEW_TAB_2_ID, "", "", "", "", 0);
Tab newTab1 = mockTab(NEW_TAB_1_ID, NEW_TAB_1_ID, "", "", "", GURL.emptyGURL(), 0);
Tab newTab2 = mockTab(NEW_TAB_2_ID, NEW_TAB_2_ID, "", "", "", GURL.emptyGURL(), 0);
doReturn(mTab0).when(mTabModelFilter).getTabAt(eq(TAB_0_ID));
doReturn(newTab1).when(mTabModelFilter).getTabAt(eq(TAB_0_ID + 1));
doReturn(newTab2).when(mTabModelFilter).getTabAt(eq(TAB_0_ID + 2));
Expand Down Expand Up @@ -216,6 +217,6 @@ public void testTabContextJsonDeserialization() throws JSONException {
mTab0.getWebContents()
.getNavigationController()
.getLastCommittedEntryIndex());
Assert.assertEquals(lastCommittedEntry.getReferrerUrl(), tabInfo.referrerUrl);
Assert.assertEquals(tabInfo.referrerUrl, lastCommittedEntry.getReferrerUrl().getSpec());
}
}
Loading

0 comments on commit 09b9c59

Please sign in to comment.