Skip reparenting new tab pages

When new tab pages are reparented, the view isn't loaded completely.
New tab pages have their own logic to restore scroll position, which
is the only outstanding state that would change between theme change.

Bug: 1052246,1055228
Change-Id: I5c7b42f6daecc30a6499bcdf4bc6ded97795afab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2067385
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746128}

chrome/android/java/src/org/chromium/chrome/browser/night_mode/NightModeReparentingController.java

@@ -25,7 +25,6 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
-// TODO(wylieb): Write unittests for this class.
 /** Controls the reparenting of tabs when the theme is swapped. */
 public class NightModeReparentingController implements NightModeStateProvider.Observer {
     /** Provides data to {@link NightModeReparentingController} facilitate reparenting tabs. */
@@ -35,6 +34,9 @@ public class NightModeReparentingController implements NightModeStateProvider.Ob
 
         /** Gets a {@link TabModelSelector} which is used to add the tab. */
         TabModelSelector getTabModelSelector();
+
+        /** @return Whether the given Url is an NTP url, exists solely to support unit testing. */
+        boolean isNTPUrl(String url);
     }
 
     private Delegate mDelegate;
@@ -93,7 +95,8 @@ public class NightModeReparentingController implements NightModeStateProvider.Ob
     private void reattachTab(ReparentingTask task, TabReparentingParams params, TabModel tabModel) {
         final Tab tab = params.getTabToReparent();
         task.finish(mReparentingDelegate, () -> {
-            tabModel.addTab(tab, params.getTabIndex(), TabLaunchType.FROM_REPARENTING,
+            int tabIndex = params.getTabIndex() > tabModel.getCount() ? -1 : params.getTabIndex();
+            tabModel.addTab(tab, tabIndex, TabLaunchType.FROM_REPARENTING,
                     TabCreationState.LIVE_IN_FOREGROUND);
             AsyncTabParamsManager.remove(tab.getId());
         });
@@ -108,18 +111,15 @@ public class NightModeReparentingController implements NightModeStateProvider.Ob
             List<Tab> tabs = tabsAndModels.get(model);
             for (int i = 0; i < tabs.size(); i++) {
                 Tab tab = tabs.get(i);
+
+                // Intentionally skip new tab pages and allow them to reload and restore scroll
+                // state themselves.
+                if (mDelegate.isNTPUrl(tab.getUrlString())) continue;
+
                 TabReparentingParams params = new TabReparentingParams(tab, null, null);
                 params.setFromNightModeReparenting(true);
-
-                // Only the first tab is considered foreground and that is the only one for which
-                // we need an index. It will be reattached at the end. All remaining tabs will be
-                // reattached in reverse order, therefore we can ignore the index.
-                if (tab == foregroundTab) {
-                    params.setIsForegroundTab(true);
-                    params.setTabIndex(i);
-                } else {
-                    params.setIsForegroundTab(false);
-                }
+                params.setIsForegroundTab(tab == foregroundTab);
+                params.setTabIndex(i);
 
                 AsyncTabParamsManager.add(tab.getId(), params);
                 ReparentingTask.from(tab).detach();

chrome/android/java/src/org/chromium/chrome/browser/tab_activity_glue/ReparentingDelegateFactory.java

@@ -7,6 +7,7 @@ package org.chromium.chrome.browser.tab_activity_glue;
 import org.chromium.chrome.browser.ActivityTabProvider;
 import org.chromium.chrome.browser.compositor.CompositorViewHolder;
 import org.chromium.chrome.browser.night_mode.NightModeReparentingController;
+import org.chromium.chrome.browser.ntp.NewTabPage;
 import org.chromium.chrome.browser.tab.TabDelegateFactory;
 import org.chromium.chrome.browser.tabmodel.TabModelSelector;
 import org.chromium.ui.base.WindowAndroid;
@@ -55,6 +56,11 @@ public class ReparentingDelegateFactory {
             public TabModelSelector getTabModelSelector() {
                 return tabModelSelector;
             }
+
+            @Override
+            public boolean isNTPUrl(String url) {
+                return NewTabPage.isNTPUrl(url);
+            }
         };
     }
 }

chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabReparentingParams.java

@@ -95,11 +95,6 @@ public class TabReparentingParams implements AsyncTabParams {
         return mTabIndex;
     }
 
-    /** @return Whether this holds a tab index. */
-    public boolean hasTabIndex() {
-        return mTabIndex != TAB_INDEX_NOT_SET;
-    }
-
     /** Set whether these params are from night mode reparenting. */
     public void setFromNightModeReparenting(boolean fromNightModeReparenting) {
         mIsFromNightModeReparenting = fromNightModeReparenting;

chrome/android/junit/src/org/chromium/chrome/browser/night_mode/NightModeReparentingControllerTest.java

@@ -33,6 +33,7 @@ import org.chromium.chrome.browser.tabmodel.AsyncTabParamsManager;
 import org.chromium.chrome.browser.tabmodel.TabModelSelector;
 import org.chromium.chrome.browser.tabmodel.TabReparentingParams;
 import org.chromium.chrome.test.util.browser.tabmodel.MockTabModel;
+import org.chromium.components.embedder_support.util.UrlConstants;
 
 import java.util.HashMap;
 import java.util.Map;
@@ -46,6 +47,7 @@ public class NightModeReparentingControllerTest {
     class FakeNightModeReparentingDelegate implements NightModeReparentingController.Delegate {
         ActivityTabProvider mActivityTabProvider;
         TabModelSelector mTabModelSelector;
+        boolean mIsNTPUrl;
 
         @Override
         public ActivityTabProvider getActivityTabProvider() {
@@ -69,6 +71,15 @@ public class NightModeReparentingControllerTest {
 
             return mTabModelSelector;
         }
+
+        @Override
+        public boolean isNTPUrl(String url) {
+            return mIsNTPUrl;
+        }
+
+        public void setIsNTPUrl(boolean isNTPUrl) {
+            mIsNTPUrl = isNTPUrl;
+        }
     }
 
     @Mock
@@ -82,7 +93,7 @@ public class NightModeReparentingControllerTest {
     Tab mForegroundTab;
 
     NightModeReparentingController mController;
-    NightModeReparentingController.Delegate mDelegate;
+    FakeNightModeReparentingDelegate mDelegate;
 
     @Before
     public void setUp() {
@@ -92,6 +103,7 @@ public class NightModeReparentingControllerTest {
         mIncognitoTabModel = new MockTabModel(true, null);
 
         mDelegate = new FakeNightModeReparentingDelegate();
+        mDelegate.setIsNTPUrl(false);
         mController = new NightModeReparentingController(mDelegate, mReparentingTaskDelegate);
     }
 
@@ -126,6 +138,15 @@ public class NightModeReparentingControllerTest {
         verify(mTask, times(1)).finish(anyObject(), anyObject());
     }
 
+    @Test
+    public void testReparenting_singleTab_NTP() {
+        mDelegate.setIsNTPUrl(true);
+        mForegroundTab = createAndAddMockTab(1, false, UrlConstants.NTP_URL);
+        mController.onNightModeStateChanged();
+
+        Assert.assertFalse(AsyncTabParamsManager.hasParamsWithTabToReparent());
+    }
+
     @Test
     public void testReparenting_singleTab_currentModelNullOnStart() {
         mForegroundTab = createAndAddMockTab(1, false);
@@ -139,7 +160,7 @@ public class NightModeReparentingControllerTest {
     }
 
     @Test
-    public void testReparenting_multipleTabs_onlyOneIsReparented() {
+    public void testReparenting_multipleTabs() {
         mForegroundTab = createAndAddMockTab(1, false);
         createAndAddMockTab(2, false);
         mController.onNightModeStateChanged();
@@ -157,8 +178,7 @@ public class NightModeReparentingControllerTest {
         Assert.assertEquals(1, tab.getId());
 
         trp = (TabReparentingParams) AsyncTabParamsManager.getAsyncTabParams().get(2);
-        Assert.assertFalse("The index of the background tabs stored shouldn't have a tab index.",
-                trp.hasTabIndex());
+        Assert.assertEquals(1, trp.getTabIndex());
         Assert.assertTrue(trp.isFromNightModeReparenting());
         Assert.assertFalse(trp.isForegroundTab());
 
@@ -183,9 +203,7 @@ public class NightModeReparentingControllerTest {
         Assert.assertTrue(params instanceof TabReparentingParams);
 
         TabReparentingParams trp = (TabReparentingParams) params;
-        Assert.assertEquals(
-                "The index of the first tab stored should match its index in the tab stack.", 1,
-                trp.getTabIndex());
+        Assert.assertEquals(1, trp.getTabIndex());
         Assert.assertTrue(trp.isFromNightModeReparenting());
         Assert.assertTrue(trp.isForegroundTab());
 
@@ -210,9 +228,7 @@ public class NightModeReparentingControllerTest {
         Assert.assertTrue(params instanceof TabReparentingParams);
 
         TabReparentingParams trp = (TabReparentingParams) params;
-        Assert.assertEquals(
-                "The index of the first tab stored should match its index in the tab stack.", 0,
-                trp.getTabIndex());
+        Assert.assertEquals(0, trp.getTabIndex());
         Assert.assertTrue(trp.isFromNightModeReparenting());
         Assert.assertTrue(trp.isForegroundTab());
 
@@ -236,9 +252,7 @@ public class NightModeReparentingControllerTest {
         // Check the foreground tab.
         TabReparentingParams trp =
                 (TabReparentingParams) AsyncTabParamsManager.getAsyncTabParams().get(2);
-        Assert.assertEquals(
-                "The index of the first tab stored should match its index in the tab stack.", 1,
-                trp.getTabIndex());
+        Assert.assertEquals(1, trp.getTabIndex());
         Assert.assertTrue(trp.isFromNightModeReparenting());
         Assert.assertTrue(trp.isForegroundTab());
 
@@ -248,13 +262,9 @@ public class NightModeReparentingControllerTest {
 
         // Check the background tabs.
         trp = (TabReparentingParams) AsyncTabParamsManager.getAsyncTabParams().get(1);
-        Assert.assertFalse("The index of the background tabs stored shouldn't have a tab index.",
-                trp.hasTabIndex());
         Assert.assertTrue(trp.isFromNightModeReparenting());
         Assert.assertFalse(trp.isForegroundTab());
         trp = (TabReparentingParams) AsyncTabParamsManager.getAsyncTabParams().get(3);
-        Assert.assertFalse("The index of the background tabs stored shouldn't have a tab index.",
-                trp.hasTabIndex());
         Assert.assertTrue(trp.isFromNightModeReparenting());
         Assert.assertFalse(trp.isForegroundTab());
 
@@ -282,10 +292,12 @@ public class NightModeReparentingControllerTest {
      *
      * @param id The id to give to the tab.
      * @param incognito Whether to add the tab to the incognito model or the regular model.
+     * @param url The url to set for the tab.
      * @return The tab that was added. Use the return value to set the foreground tab for tests.
      */
-    private Tab createAndAddMockTab(int id, boolean incognito) {
+    private Tab createAndAddMockTab(int id, boolean incognito, String url) {
         Tab tab = Mockito.mock(Tab.class);
+        doReturn(url).when(tab).getUrlString();
         UserDataHost udh = new UserDataHost();
         udh.setUserData(ReparentingTask.class, mTask);
         doReturn(udh).when(tab).getUserDataHost();
@@ -306,6 +318,10 @@ public class NightModeReparentingControllerTest {
         return tab;
     }
 
+    private Tab createAndAddMockTab(int id, boolean incognito) {
+        return createAndAddMockTab(id, incognito, "https://google.com");
+    }
+
     private Tab getForegroundTab() {
         return mForegroundTab;
     }