Update inactivity tracker so that the pref is always stored on stop.

This also changes the behavior of onStart - it previously cleared the
timestamp too soon for other code to read the pref. Now the timestamp
is stored in memory in the ChromeInactivityTracker even after the pref
is cleared.

Bug: 951874

Change-Id: Ic2c128e899749aac055e3fd2487f6024fb1fbdba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1565278
Commit-Queue: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652193}

chrome/android/chrome_test_java_sources.gni

@@ -473,6 +473,7 @@ chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/tabmodel/TestTabModelDirectory.java",
   "javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java",
   "javatests/src/org/chromium/chrome/browser/tabmodel/document/MockDocumentTabModel.java",
+  "javatests/src/org/chromium/chrome/browser/tasks/ReturnToChromeTest.java",
   "javatests/src/org/chromium/chrome/browser/test/ChromeBrowserTestRule.java",
   "javatests/src/org/chromium/chrome/browser/test/ClearAppDataTestRule.java",
   "javatests/src/org/chromium/chrome/browser/test/CommandLineInitRule.java",

chrome/android/java/src/org/chromium/chrome/browser/ChromeInactivityTracker.java

@@ -11,13 +11,15 @@ import org.chromium.base.Log;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.init.ActivityLifecycleDispatcher;
 import org.chromium.chrome.browser.lifecycle.Destroyable;
+import org.chromium.chrome.browser.lifecycle.PauseResumeWithNativeObserver;
 import org.chromium.chrome.browser.lifecycle.StartStopWithNativeObserver;
 import org.chromium.content_public.browser.UiThreadTaskTraits;
 
 /**
  * Manages pref that can track the delay since the last stop of the tracked activity.
  */
-public class ChromeInactivityTracker implements StartStopWithNativeObserver, Destroyable {
+public class ChromeInactivityTracker
+        implements StartStopWithNativeObserver, PauseResumeWithNativeObserver, Destroyable {
     private static final String TAG = "InactivityTracker";
 
     private static final long UNKNOWN_LAST_BACKGROUNDED_TIME = -1;
@@ -90,12 +92,8 @@ public class ChromeInactivityTracker implements StartStopWithNativeObserver, Des
                 FEATURE_NAME, NTP_LAUNCH_DELAY_IN_MINS_PARAM, DEFAULT_LAUNCH_DELAY_IN_MINS);
 
         mIsEnabled = ChromeFeatureList.isEnabled(FEATURE_NAME);
-        if (mIsEnabled) {
-            mLifecycleDispatcher = lifecycleDispatcher;
-            mLifecycleDispatcher.register(this);
-        } else {
-            mLifecycleDispatcher = null;
-        }
+        mLifecycleDispatcher = lifecycleDispatcher;
+        mLifecycleDispatcher.register(this);
     }
 
     /**
@@ -107,7 +105,15 @@ public class ChromeInactivityTracker implements StartStopWithNativeObserver, Des
         ContextUtils.getAppSharedPreferences().edit().putLong(mPrefName, timeInMillis).apply();
     }
 
-    private long getLastBackgroundedTimeMs() {
+    /**
+     * Updates shared preferences such that the last backgrounded time is no
+     * longer valid. This will prevent multiple new intents from firing.
+     */
+    private void clearLastBackgroundedTimeInPrefs() {
+        setLastBackgroundedTimeInPrefs(UNKNOWN_LAST_BACKGROUNDED_TIME);
+    }
+
+    long getLastBackgroundedTimeMs() {
         return ContextUtils.getAppSharedPreferences().getLong(
                 mPrefName, UNKNOWN_LAST_BACKGROUNDED_TIME);
     }
@@ -150,12 +156,29 @@ public class ChromeInactivityTracker implements StartStopWithNativeObserver, Des
 
     @Override
     public void onStartWithNative() {
-        setLastBackgroundedTimeInPrefs(UNKNOWN_LAST_BACKGROUNDED_TIME);
         cancelCurrentTask();
     }
 
+    @Override
+    public void onResumeWithNative() {
+        // We clear the backgrounded time here, rather than in #onStartWithNative, to give
+        // handlers the chance to respond to inactivity during any onStartWithNative handler
+        // regardless of ordering. onResume is always called after onStart, and it should be fine to
+        // consider Chrome active if it reaches onResume.
+        clearLastBackgroundedTimeInPrefs();
+    }
+
+    @Override
+    public void onPauseWithNative() {}
+
     @Override
     public void onStopWithNative() {
+        // Always track the last backgrounded time in case others are using the pref.
+        long timeInMillis = System.currentTimeMillis();
+        setLastBackgroundedTimeInPrefs(timeInMillis);
+
+        if (!mIsEnabled) return;
+
         Log.i(TAG, "onStop, scheduling for " + mNtpLaunchDelayInMins + " minutes");
 
         cancelCurrentTask();
@@ -164,7 +187,6 @@ public class ChromeInactivityTracker implements StartStopWithNativeObserver, Des
             return;
         }
         mCurrentlyPostedInactiveCallback = new CancelableRunnableTask(mInactiveCallback);
-        setLastBackgroundedTimeInPrefs(System.currentTimeMillis());
         org.chromium.base.task.PostTask.postDelayedTask(UiThreadTaskTraits.DEFAULT,
                 mCurrentlyPostedInactiveCallback,
                 mNtpLaunchDelayInMins * DateUtils.MINUTE_IN_MILLIS);

chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java

@@ -922,8 +922,7 @@ public class ChromeTabbedActivity
         boolean isOverviewVisible = mOverviewModeController.overviewVisible();
 
         // Experiment: show tab switcher on return after {x} minutes (enable-tab-switcher-on-return}
-        long lastBackgroundedTimeMillis =
-                ContextUtils.getAppSharedPreferences().getLong(LAST_BACKGROUNDED_TIME_MS_PREF, -1);
+        long lastBackgroundedTimeMillis = mInactivityTracker.getLastBackgroundedTimeMs();
         if (ReturnToChromeExperimentsUtil.shouldShowTabSwitcher(lastBackgroundedTimeMillis)
                 && isMainIntentFromLauncher(getIntent()) && !isOverviewVisible) {
             toggleOverview();

chrome/android/java/src/org/chromium/chrome/browser/tasks/ReturnToChromeExperimentsUtil.java

@@ -4,13 +4,15 @@
 
 package org.chromium.chrome.browser.tasks;
 
+import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.ChromeFeatureList;
 
 /**
  * This is a utility class for managing experiments related to returning to Chrome.
  */
 public final class ReturnToChromeExperimentsUtil {
-    private static final String TAB_SWITCHER_ON_RETURN_MS = "tab_switcher_on_return_time_ms";
+    @VisibleForTesting
+    public static final String TAB_SWITCHER_ON_RETURN_MS = "tab_switcher_on_return_time_ms";
 
     private ReturnToChromeExperimentsUtil() {}
 

chrome/android/javatests/src/org/chromium/chrome/browser/tasks/ReturnToChromeTest.java

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.tasks;
+
+import static org.chromium.chrome.browser.tasks.ReturnToChromeExperimentsUtil.TAB_SWITCHER_ON_RETURN_MS;
+
+import android.app.Activity;
+import android.content.Context;
+import android.os.SystemClock;
+import android.support.test.InstrumentationRegistry;
+import android.support.test.filters.SmallTest;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.ActivityState;
+import org.chromium.base.ApplicationStatus;
+import org.chromium.base.test.util.CallbackHelper;
+import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.base.test.util.Feature;
+import org.chromium.chrome.browser.ChromeFeatureList;
+import org.chromium.chrome.browser.ChromeSwitches;
+import org.chromium.chrome.browser.ChromeTabbedActivity;
+import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
+
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Tests the functionality of return to chrome features that open overview mode if the timeout
+ * has passed.
+ */
+@RunWith(ChromeJUnit4ClassRunner.class)
+public class ReturnToChromeTest {
+    @Rule
+    public ChromeTabbedActivityTestRule mActivityTestRule = new ChromeTabbedActivityTestRule();
+
+    private ChromeTabbedActivity mActivity;
+
+    @Before
+    public void setUp() throws Exception {
+        Context context = InstrumentationRegistry.getInstrumentation().getTargetContext();
+
+        mActivityTestRule.startMainActivityFromLauncher();
+        mActivity = mActivityTestRule.getActivity();
+    }
+
+    /**
+     * Test that overview mode is not triggered if the delay is longer than the interval between
+     * stop and start.
+     */
+    @Test
+    @SmallTest
+    @Feature({"ReturnToChrome"})
+    @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
+            "enable-features=" + ChromeFeatureList.TAB_SWITCHER_ON_RETURN + "<FakeStudyName",
+            "force-fieldtrials=FakeStudyName/Enabled",
+            "force-fieldtrial-params=FakeStudyName.Enabled:" + TAB_SWITCHER_ON_RETURN_MS
+                    + "/100000"})
+    public void
+    testObserverModeNotTriggeredWithoutDelay() throws Exception {
+        finishActivityCompletely();
+
+        mActivityTestRule.startMainActivityFromLauncher();
+        mActivity = mActivityTestRule.getActivity();
+
+        Assert.assertFalse(mActivity.getLayoutManager().overviewVisible());
+    }
+
+    @Test
+    @SmallTest
+    @Feature({"ReturnToChrome"})
+    @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
+            "enable-features=" + ChromeFeatureList.TAB_SWITCHER_ON_RETURN + "<FakeStudyName",
+            "force-fieldtrials=FakeStudyName/Enabled",
+            "force-fieldtrial-params=FakeStudyName.Enabled:" + TAB_SWITCHER_ON_RETURN_MS + "/1"})
+    public void
+    testObserverModeTriggeredWithDelay() throws Exception {
+        finishActivityCompletely();
+
+        // Sleep past the timeout
+        SystemClock.sleep(30);
+
+        mActivityTestRule.startMainActivityFromLauncher();
+        mActivity = mActivityTestRule.getActivity();
+
+        Assert.assertTrue(mActivity.getLayoutManager().overviewVisible());
+    }
+
+    private void finishActivityCompletely() throws InterruptedException, TimeoutException {
+        final CallbackHelper activityCallback = new CallbackHelper();
+        ApplicationStatus.ActivityStateListener stateListener =
+                new ApplicationStatus.ActivityStateListener() {
+                    @Override
+                    public void onActivityStateChange(Activity activity, int newState) {
+                        if (newState == ActivityState.STOPPED) {
+                            activityCallback.notifyCalled();
+                            ApplicationStatus.unregisterActivityStateListener(this);
+                        }
+                    }
+                };
+
+        ApplicationStatus.registerStateListenerForAllActivities(stateListener);
+        try {
+            mActivity.finish();
+            activityCallback.waitForCallback("Activity did not stop as expected", 0);
+            mActivityTestRule.setActivity(null);
+        } finally {
+            ApplicationStatus.unregisterActivityStateListener(stateListener);
+        }
+    }
+}

chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java

@@ -435,7 +435,7 @@ public class ChromeActivityTestRule<T extends ChromeActivity> extends ActivityTe
 
         ChromeTabUtils.waitForTabPageLoaded(tab, (String) null);
 
-        if (tab != null && NewTabPage.isNTPUrl(tab.getUrl())) {
+        if (tab != null && NewTabPage.isNTPUrl(tab.getUrl()) && !getActivity().isInOverviewMode()) {
             NewTabPageTestUtils.waitForNtpLoaded(tab);
         }