Reland "Move URL to CriticalPersistedTabData"

This reverts commit a56b7ab. Reason for revert: Tests are now fixed in CL 2354454 Original change's description: > Revert "Move URL to CriticalPersistedTabData" > > This reverts commit ffd982c. > > Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=1115915 > Caused several tests to fail. > > Original change's description: > > Move URL to CriticalPersistedTabData > > > > This supports the effort of moving fields to UserData objects > > so Tab doesn't become too big and unwieldy. > > > > Bug: 1112357 > > Change-Id: I6c484d9586ca8ad7d1866fbbc9dac4735ecf1470 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2337673 > > Commit-Queue: David Maunder <davidjm@chromium.org> > > Reviewed-by: Tommy Nyquist <nyquist@chromium.org> > > Reviewed-by: David Trainor <dtrainor@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#797555} > > TBR=nyquist@chromium.org,dtrainor@chromium.org,jinsukkim@chromium.org,davidjm@chromium.org > > Change-Id: I084d6dcb0cef11311a8d9d5f8bc26bffe6309339 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 1112357 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2352813 > Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> > Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> > Cr-Commit-Position: refs/heads/master@{#797616} TBR=nyquist@chromium.org,dtrainor@chromium.org,jinsukkim@chromium.org,arthursonzogni@chromium.org,davidjm@chromium.org Bug: 1112357 Change-Id: I9a343866b11de1c438bddc375e62b532847a2e1d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2354672 Reviewed-by: David Maunder <davidjm@chromium.org> Reviewed-by: Tommy Nyquist <nyquist@chromium.org> Commit-Queue: David Maunder <davidjm@chromium.org> Cr-Commit-Position: refs/heads/master@{#797881}
FairyWorld · Aug 13, 2020 · b232a64 · b232a64
1 parent de85fa7
commit b232a64
Show file tree

Hide file tree

Showing 33 changed files with 306 additions and 153 deletions.
diff --git a/...hromium/chrome/browser/autofill_assistant/AutofillAssistantBackButtonIntegrationTest.java b/...hromium/chrome/browser/autofill_assistant/AutofillAssistantBackButtonIntegrationTest.java
@@ -167,14 +167,18 @@ public void backButtonWithSnackbarDestroysAutofillAssistantUi() throws Exception
         Espresso.pressBack();
         waitUntilViewMatchesCondition(withText(R.string.undo), isCompletelyDisplayed());
         onView(withId(R.id.autofill_assistant)).check(doesNotExist());
-        assertThat(mTestRule.getActivity().getActivityTab().getUrl().getSpec(),
+
+        assertThat(
+                ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab()).getSpec(),
                 is(getURL(TEST_PAGE_B)));
 
         // Third press on back button navigates back.
         Espresso.pressBack();
-        waitUntil(()
-                          -> mTestRule.getActivity().getActivityTab().getUrl().getSpec().equals(
-                                  getURL(TEST_PAGE_A)));
+        waitUntil(
+                ()
+                        -> ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab())
+                                   .getSpec()
+                                   .equals(getURL(TEST_PAGE_A)));
     }
 
     @Test
@@ -242,7 +246,8 @@ public void backButtonWithSettingsShowsErrorMessageAndDestroysAutofillAssistantU
         Espresso.pressBack();
         waitUntilViewMatchesCondition(withText("Back button pressed"), isCompletelyDisplayed());
         waitUntilViewMatchesCondition(withText("Undo"), isDisplayed());
-        assertThat(mTestRule.getActivity().getActivityTab().getUrl().getSpec(),
+        assertThat(
+                ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab()).getSpec(),
                 is(getURL(TEST_PAGE_B)));
 
         // Undo should get back to the prompt state.
@@ -253,15 +258,18 @@ public void backButtonWithSettingsShowsErrorMessageAndDestroysAutofillAssistantU
         Espresso.pressBack();
         waitUntilViewMatchesCondition(withText("Back button pressed"), isCompletelyDisplayed());
         waitUntilViewMatchesCondition(withText("Undo"), isDisplayed());
-        assertThat(mTestRule.getActivity().getActivityTab().getUrl().getSpec(),
+        assertThat(
+                ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab()).getSpec(),
                 is(getURL(TEST_PAGE_B)));
 
         // Third press on back button destroys Autofill UI and navigates back.
         Espresso.pressBack();
         waitUntilViewAssertionTrue(withId(R.id.autofill_assistant), doesNotExist(), 3000L);
-        waitUntil(()
-                          -> mTestRule.getActivity().getActivityTab().getUrl().getSpec().equals(
-                                  getURL(TEST_PAGE_A)));
+        waitUntil(
+                ()
+                        -> ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab())
+                                   .getSpec()
+                                   .equals(getURL(TEST_PAGE_A)));
     }
 
     @Test
@@ -295,9 +303,11 @@ public void backButtonInStoppedAutofillAssistantState() {
                 withId(R.id.autofill_assistant), doesNotExist(), DEFAULT_MAX_TIME_TO_POLL);
         onView(withText("Shutdown")).check(doesNotExist());
         onView(withText(R.string.undo)).check(doesNotExist());
-        waitUntil(()
-                          -> mTestRule.getActivity().getActivityTab().getUrl().getSpec().equals(
-                                  getURL(TEST_PAGE_A)));
+        waitUntil(
+                ()
+                        -> ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab())
+                                   .getSpec()
+                                   .equals(getURL(TEST_PAGE_A)));
     }
 
     @Test
@@ -355,7 +365,8 @@ public void navigationAfterBackButtonDestroysAutofillAssistantUi() {
         Espresso.pressBack();
         waitUntilViewMatchesCondition(withText("Back button pressed"), isCompletelyDisplayed());
         waitUntilViewMatchesCondition(withText("Undo"), isDisplayed());
-        assertThat(mTestRule.getActivity().getActivityTab().getUrl().getSpec(),
+        assertThat(
+                ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab()).getSpec(),
                 is(getURL(TEST_PAGE_B)));
 
         // Navigation destroys the Autofill Assistant UI.
@@ -402,9 +413,11 @@ public void backButtonIsIgnoredInBrowseMode() {
 
         // Second press on back button navigates back, without removing the Autofill Assistannt UI.
         Espresso.pressBack();
-        waitUntil(()
-                          -> mTestRule.getActivity().getActivityTab().getUrl().getSpec().equals(
-                                  getURL(TEST_PAGE_A)));
+        waitUntil(
+                ()
+                        -> ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab())
+                                   .getSpec()
+                                   .equals(getURL(TEST_PAGE_A)));
         onView(withId(R.id.autofill_assistant)).check(matches(isDisplayed()));
         onView(withId(R.id.status_message)).check(matches(withText("Prompt")));
     }

diff --git a/...hromium/chrome/browser/autofill_assistant/AutofillAssistantNavigationIntegrationTest.java b/...hromium/chrome/browser/autofill_assistant/AutofillAssistantNavigationIntegrationTest.java
@@ -123,9 +123,11 @@ public void navigatingWithLocationBarShowsError() {
         onView(withId(org.chromium.chrome.R.id.url_bar))
                 .perform(click(), typeText(getURL(TEST_PAGE_B)), pressImeActionButton());
         waitUntilViewMatchesCondition(withText(containsString("Sorry")), isCompletelyDisplayed());
-        waitUntil(()
-                          -> mTestRule.getActivity().getActivityTab().getUrl().getSpec().equals(
-                                  getURL(TEST_PAGE_B)));
+        waitUntil(
+                ()
+                        -> ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab())
+                                   .getSpec()
+                                   .equals(getURL(TEST_PAGE_B)));
     }
 
     @Test
@@ -157,9 +159,11 @@ public void clickingLinkDoesNotCauseError() {
         startAutofillAssistantOnTab(TEST_PAGE_A);
 
         waitUntilViewMatchesCondition(withText("Prompt"), isCompletelyDisplayed());
-        waitUntil(()
-                          -> mTestRule.getActivity().getActivityTab().getUrl().getSpec().equals(
-                                  getURL(TEST_PAGE_B)));
+        waitUntil(
+                ()
+                        -> ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab())
+                                   .getSpec()
+                                   .equals(getURL(TEST_PAGE_B)));
     }
 
     @Test
@@ -192,9 +196,11 @@ public void javaScriptNavigationDoesNotCauseError() {
         startAutofillAssistantOnTab(TEST_PAGE_A);
 
         waitUntilViewMatchesCondition(withText("Prompt"), isCompletelyDisplayed());
-        waitUntil(()
-                          -> mTestRule.getActivity().getActivityTab().getUrl().getSpec().equals(
-                                  getURL(TEST_PAGE_B)));
+        waitUntil(
+                ()
+                        -> ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab())
+                                   .getSpec()
+                                   .equals(getURL(TEST_PAGE_B)));
     }
 
     @Test
@@ -257,21 +263,28 @@ public void navigateActionDoesNotCauseError() {
         onView(withText("Navigate")).perform(click());
 
         waitUntilViewMatchesCondition(withText("Page A"), isCompletelyDisplayed());
-        waitUntil(()
-                          -> mTestRule.getActivity().getActivityTab().getUrl().getSpec().equals(
-                                  getURL(TEST_PAGE_A)));
+
+        waitUntil(
+                ()
+                        -> ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab())
+                                   .getSpec()
+                                   .equals(getURL(TEST_PAGE_A)));
         onView(withText("Go back")).perform(click());
 
         waitUntilViewMatchesCondition(withText("Page B"), isCompletelyDisplayed());
-        waitUntil(()
-                          -> mTestRule.getActivity().getActivityTab().getUrl().getSpec().equals(
-                                  getURL(TEST_PAGE_B)));
+        waitUntil(
+                ()
+                        -> ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab())
+                                   .getSpec()
+                                   .equals(getURL(TEST_PAGE_B)));
         onView(withText("Go forward")).perform(click());
 
         waitUntilViewMatchesCondition(withText("Page A"), isCompletelyDisplayed());
-        waitUntil(()
-                          -> mTestRule.getActivity().getActivityTab().getUrl().getSpec().equals(
-                                  getURL(TEST_PAGE_A)));
+        waitUntil(
+                ()
+                        -> ChromeTabUtils.getUrlOnUiThread(mTestRule.getActivity().getActivityTab())
+                                   .getSpec()
+                                   .equals(getURL(TEST_PAGE_A)));
     }
 
     @Test

diff --git a/...rnal/javatests/src/org/chromium/chrome/features/start_surface/StartSurfaceLayoutTest.java b/...rnal/javatests/src/org/chromium/chrome/features/start_surface/StartSurfaceLayoutTest.java
@@ -1572,7 +1572,7 @@ public void testSearchTermChip_withChip() throws InterruptedException {
                 .check(waitForView(allOf(withText(expectedTerm), isDisplayed())));
 
         // Click the chip and check the tab navigates back to the search result page.
-        assertEquals(mUrl, currentTab.getUrlString());
+        assertEquals(mUrl, ChromeTabUtils.getUrlStringOnUiThread(currentTab));
         OverviewModeBehaviorWatcher hideWatcher = TabUiTestHelper.createOverviewHideWatcher(cta);
         onView(withId(R.id.search_button))
                 .check(waitForView(allOf(withText(expectedTerm), isDisplayed())));

diff --git a/...ab_ui/javatests/src/org/chromium/chrome/browser/tasks/tab_management/TabUiTestHelper.java b/...ab_ui/javatests/src/org/chromium/chrome/browser/tasks/tab_management/TabUiTestHelper.java
@@ -460,7 +460,7 @@ public static void prepareTabsWithThumbnail(ChromeTabbedActivityTestRule rule, i
 
     private static void verifyAllTabsHaveUrl(TabModel tabModel, String url) {
         for (int i = 0; i < tabModel.getCount(); i++) {
-            assertEquals(url, tabModel.getTabAt(i).getUrlString());
+            assertEquals(url, ChromeTabUtils.getUrlStringOnUiThread(tabModel.getTabAt(i)));
         }
     }
 

diff --git a/.../android/feed/core/javatests/src/org/chromium/chrome/browser/feed/FeedNewTabPageTest.java b/.../android/feed/core/javatests/src/org/chromium/chrome/browser/feed/FeedNewTabPageTest.java
@@ -291,7 +291,7 @@ public void testSignInPromo_AccountsNotReady() {
 
         mIsCachePopulatedInAccountManagerFacade = true;
         TestThreadUtils.runOnUiThreadBlocking(mTab::reload);
-        ChromeTabUtils.waitForTabPageLoaded(mTab, mTab.getUrlString());
+        ChromeTabUtils.waitForTabPageLoaded(mTab, ChromeTabUtils.getUrlStringOnUiThread(mTab));
 
         // Check that the sign-in promo is displayed this time.
         onView(instanceOf(RecyclerView.class))

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/tab/TabImpl.java b/chrome/android/java/src/org/chromium/chrome/browser/tab/TabImpl.java
@@ -136,11 +136,6 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
      */
     private LoadUrlParams mPendingLoadParams;
 
-    /**
-     * URL of the page currently loading. Used as a fall-back in case tab restore fails.
-     */
-    private GURL mUrl;
-
     /**
      * True while a page load is in progress.
      */
@@ -332,6 +327,7 @@ public int getId() {
         return mId;
     }
 
+    // TODO(crbug.com/1113249) move getUrl() and getUrlString() to CriticalPersistedTabData
     @Override
     public String getUrlString() {
         return getUrl().getSpec();
@@ -345,10 +341,12 @@ public GURL getUrl() {
         // If we have a ContentView, or a NativePage, or the url is not empty, we have a WebContents
         // so cache the WebContent's url. If not use the cached version.
         if (getWebContents() != null || isNativePage() || !url.getSpec().isEmpty()) {
-            mUrl = url;
+            CriticalPersistedTabData.from(this).setUrl(url);
         }
 
-        return mUrl != null ? mUrl : GURL.emptyGURL();
+        return CriticalPersistedTabData.from(this).getUrl() != null
+                ? CriticalPersistedTabData.from(this).getUrl()
+                : GURL.emptyGURL();
     }
 
     @Override
@@ -760,7 +758,9 @@ void initialize(Tab parent, @Nullable @TabCreationState Integer creationState,
             CriticalPersistedTabData.from(this).setLaunchTypeAtCreation(mLaunchType);
             mCreationState = creationState;
             mPendingLoadParams = loadUrlParams;
-            if (loadUrlParams != null) mUrl = new GURL(loadUrlParams.getUrl());
+            if (loadUrlParams != null) {
+                CriticalPersistedTabData.from(this).setUrl(new GURL(loadUrlParams.getUrl()));
+            }
 
             TabHelpers.initTabHelpers(this, parent);
 
@@ -822,7 +822,8 @@ void restoreFieldsFromState(TabState state) {
         assert state != null;
         CriticalPersistedTabData.from(this).setWebContentsState(state.contentsState);
         CriticalPersistedTabData.from(this).setTimestampMillis(state.timestampMillis);
-        mUrl = new GURL(state.contentsState.getVirtualUrlFromState());
+        CriticalPersistedTabData.from(this).setUrl(
+                new GURL(state.contentsState.getVirtualUrlFromState()));
         CriticalPersistedTabData.from(this).setTitle(
                 state.contentsState.getDisplayTitleFromState());
         CriticalPersistedTabData.from(this).setLaunchTypeAtCreation(state.tabLaunchTypeAtCreation);
@@ -1393,7 +1394,9 @@ private boolean unfreezeContents() {
             initWebContents(webContents);
 
             if (!restored) {
-                String url = mUrl.getSpec().isEmpty() ? UrlConstants.NTP_URL : mUrl.getSpec();
+                String url = CriticalPersistedTabData.from(this).getUrl().getSpec().isEmpty()
+                        ? UrlConstants.NTP_URL
+                        : CriticalPersistedTabData.from(this).getUrl().getSpec();
                 loadUrl(new LoadUrlParams(url, PageTransition.GENERATED));
             }
         } finally {

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java b/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java
@@ -73,6 +73,7 @@ public class TabWebContentsObserver extends TabWebContentsUserData {
     private final ObserverList<Callback<WebContents>> mInitObservers = new ObserverList<>();
     private final Handler mHandler = new Handler();
     private WebContentsObserver mObserver;
+    private String mLastUrl;
 
     public static TabWebContentsObserver from(Tab tab) {
         TabWebContentsObserver observer = get(tab);
@@ -294,6 +295,7 @@ public void didFinishNavigation(NavigationHandle navigation) {
                 recordErrorInPolicyAuditor(
                         navigation.getUrl(), navigation.errorDescription(), navigation.errorCode());
             }
+            mLastUrl = navigation.getUrl();
 
             if (!navigation.hasCommitted()) return;
 
@@ -353,7 +355,7 @@ public void viewportFitChanged(@WebContentsObserver.ViewportFitType int value) {
         @Override
         public void destroy() {
             MediaCaptureNotificationService.updateMediaNotificationForTab(
-                    ContextUtils.getApplicationContext(), mTab.getId(), null, mTab.getUrlString());
+                    ContextUtils.getApplicationContext(), mTab.getId(), null, mLastUrl);
             super.destroy();
         }
     }

diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/LauncherShortcutTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/LauncherShortcutTest.java
@@ -40,6 +40,7 @@
 import org.chromium.chrome.browser.tabmodel.TabModelSelectorObserver;
 import org.chromium.chrome.test.ChromeJUnit4RunnerDelegate;
 import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
+import org.chromium.chrome.test.util.ChromeTabUtils;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 
 import java.util.Arrays;
@@ -115,8 +116,10 @@ public void testLauncherShortcut(boolean incognito) throws Exception {
                 () -> mActivityTestRule.getActivity().getActivityTab());
         Assert.assertEquals("Incorrect tab launch type.", TabLaunchType.FROM_LAUNCHER_SHORTCUT,
                 activityTab.getLaunchType());
-        Assert.assertTrue("Tab should be an NTP. Tab url: " + activityTab.getUrl(),
-                NewTabPage.isNTPUrl(activityTab.getUrl()));
+
+        Assert.assertTrue(
+                "Tab should be an NTP. Tab url: " + ChromeTabUtils.getUrlOnUiThread(activityTab),
+                NewTabPage.isNTPUrl(ChromeTabUtils.getUrlOnUiThread(activityTab)));
 
         // Verify tab model.
         Assert.assertEquals("Incorrect tab model selected.", incognito,

diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/MainActivityWithURLTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/MainActivityWithURLTest.java
@@ -63,7 +63,8 @@ public void testLaunchActivityWithURL() {
     public void testLaunchActivity() {
         // Launch chrome
         mActivityTestRule.startMainActivityFromLauncher();
-        String currentUrl = mActivityTestRule.getActivity().getActivityTab().getUrlString();
+        String currentUrl = ChromeTabUtils.getUrlStringOnUiThread(
+                mActivityTestRule.getActivity().getActivityTab());
         Assert.assertNotNull(currentUrl);
         Assert.assertEquals(false, currentUrl.isEmpty());
     }
@@ -78,14 +79,17 @@ public void testLaunchActivity() {
     public void testNewTabPageLaunch() {
         // Launch chrome with NTP.
         mActivityTestRule.startMainActivityWithURL(UrlConstants.NTP_URL);
-        String currentUrl = mActivityTestRule.getActivity().getActivityTab().getUrlString();
+        String currentUrl = ChromeTabUtils.getUrlStringOnUiThread(
+                mActivityTestRule.getActivity().getActivityTab());
         Assert.assertNotNull(currentUrl);
         Assert.assertEquals(false, currentUrl.isEmpty());
 
         // Open NTP.
         ChromeTabUtils.newTabFromMenu(
                 InstrumentationRegistry.getInstrumentation(), mActivityTestRule.getActivity());
-        currentUrl = mActivityTestRule.getActivity().getActivityTab().getUrlString();
+
+        currentUrl = ChromeTabUtils.getUrlStringOnUiThread(
+                mActivityTestRule.getActivity().getActivityTab());
         Assert.assertNotNull(currentUrl);
         Assert.assertEquals(false, currentUrl.isEmpty());
     }