Adaptively change duration to wait before showing offline indicator

Adaptively change duration to wait before showing offline indicator.
If the device has been recently online, then we wait a long time
before showing an offline indicator. This gives device
sufficient time to switch over to a different network.

If the device has not been recently online (e.g., at the app
startup or when app is brought to foreground), we wait shorter
time of 2 seconds before invoking the
offline indicator callback. This shorter duration is sufficient to
catch racing issues related to Android API callbacks, i.e., 2 seconds
is sufficient that we receive connection change callback
from Android when app is brought to foreground.

Having the 2 different timeouts ensures that we respond faster when
needed while reducing the false positives when the device is
taking too long to switch the network. This approach may
still lead to indicator being temporarily wrong in
some niche edge cases, but eventually (at most 10 seconds)
the snackbar would be correct.

Change-Id: I6ef350c86973d8449be54930d1e1972706eb5c92
Bug: 1084740
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2303983
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791112}

chrome/android/java/src/org/chromium/chrome/browser/offlinepages/indicator/OfflineDetector.java

@@ -6,6 +6,7 @@ package org.chromium.chrome.browser.offlinepages.indicator;
 
 import android.os.Handler;
 import android.os.SystemClock;
+import android.text.TextUtils;
 
 import androidx.annotation.VisibleForTesting;
 
@@ -13,7 +14,9 @@ import org.chromium.base.ApplicationState;
 import org.chromium.base.ApplicationStatus;
 import org.chromium.base.Callback;
 import org.chromium.base.supplier.Supplier;
+import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.offlinepages.indicator.ConnectivityDetector.ConnectionState;
+import org.chromium.components.variations.VariationsAssociatedData;
 
 /**
  * Class that detects if the network is offline. Waits for the network to stablize before notifying
@@ -21,8 +24,34 @@ import org.chromium.chrome.browser.offlinepages.indicator.ConnectivityDetector.C
  */
 class OfflineDetector
         implements ConnectivityDetector.Observer, ApplicationStatus.ApplicationStateListener {
+    // If the connection is online, then we report that immediately via |mCallback|.
+    // |STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS| and
+    // |mStatusIndicatorWaitOnSwitchOnlineToOfflineDurationMs| control the duration before
+    // we report the device as offline. It's important to wait a bit before reporting connection as
+    // offline since some devices may take time to establish the connection. In such cases,
+    // reporting connection as offline could cause confusion to the user. Setting this to a large
+    // value has a downside that if the device is actually offline, then it would take us long time
+    // to report the connection as offline.
+
+    // |STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS| is the duration before we wait before
+    // reporting the connection type as offline if the app has never been on an online connection.
+    // In this case, we need to wait a shorter time before
+    // invoking |mCallback| since the actual connection change to offline happened  much earlier
+    // than when the app received the notification. Any delays in app receiving the notification of
+    // connection change are only due to device's CPU constraints.
     static final long STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS = 2000;
 
+    // |mStatusIndicatorWaitOnSwitchOnlineToOfflineDurationMs| is the duration before we wait before
+    // reporting the connection type as offline if the app has been on an online connection before.
+    // In this case, we need to wait a longer time before invoking |mCallback| since the connection
+    // change is ongoing. Any delays in app receiving the notification of connection change are due
+    // to time taken by device in reestablishing the connection as well as device CPU constraints.
+    // Value of |mStatusIndicatorWaitOnSwitchOnlineToOfflineDurationMs| is set to
+    // |STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS| by default, but can
+    // be overridden using finch.
+    static final long STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS = 10000;
+    final long mStatusIndicatorWaitOnSwitchOnlineToOfflineDurationMs;
+
     private static ConnectivityDetector sMockConnectivityDetector;
     private static Supplier<Long> sMockElapsedTimeSupplier;
 
@@ -49,7 +78,14 @@ class OfflineDetector
 
     // Time when the connection was last reported as offline. |callback| is invoked only when the
     // connection has been in the ofline for |STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS|.
-    private long mTimeWhenLastOffline;
+    private long mTimeWhenLastOfflineNotificationReceived;
+
+    // True if the |mConnectivityDetector| has been initialized.
+    private boolean mConnectivityDetectorInitialized;
+
+    // Last time when the device was online. Updated when we detect that the device is switching
+    // from "online" to "offline" or when we are notified that the device is online" at the end.
+    private long mTimeWhenLastOnline;
 
     /**
      * Constructs the offline indicator.
@@ -59,6 +95,9 @@ class OfflineDetector
     OfflineDetector(Callback<Boolean> callback) {
         mCallback = callback;
         mHandler = new Handler();
+        mStatusIndicatorWaitOnSwitchOnlineToOfflineDurationMs = getIntParamValueOrDefault(
+                "STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS",
+                STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS);
 
         mUpdateOfflineStatusIndicatorDelayedRunnable = () -> {
             // |callback| is invoked only when the app is in foreground. If the app is in
@@ -98,12 +137,26 @@ class OfflineDetector
                 (connectionState != ConnectionState.VALIDATED);
         if (previousLastReportedStateByOfflineDetector
                 == mIsOfflineLastReportedByConnectivityDetector) {
+            mConnectivityDetectorInitialized = true;
             return;
         }
 
         if (mIsOfflineLastReportedByConnectivityDetector) {
-            mTimeWhenLastOffline = getElapsedTime();
+            mTimeWhenLastOfflineNotificationReceived = getElapsedTime();
+        }
+
+        // Verify that the connectivity detector is initialized before setting
+        // |mTimeWhenLastOnline|. By default, |mIsOfflineLastReportedByConnectivityDetector| is
+        // false, i.e., the device is assumed to be online. Tracking
+        // |mConnectivityDetectorInitialized| helps us distinguish whether the connection type has
+        // switched from "default online" to "offline" or "online" to "offline".
+        if ((mConnectivityDetectorInitialized && !previousLastReportedStateByOfflineDetector)
+                || !mIsOfflineLastReportedByConnectivityDetector) {
+            mTimeWhenLastOnline = getElapsedTime();
         }
+
+        mConnectivityDetectorInitialized = true;
+
         updateState();
     }
 
@@ -168,11 +221,20 @@ class OfflineDetector
         // Check time since the app was foregrounded and time since the offline notification was
         // received.
         final long timeSinceLastForeground = getElapsedTime() - mTimeWhenLastForegrounded;
-        final long timeSinceOffline = getElapsedTime() - mTimeWhenLastOffline;
+        final long timeSinceOfflineNotificationReceived =
+                getElapsedTime() - mTimeWhenLastOfflineNotificationReceived;
+        final long timeSinceLastOnline = getElapsedTime() - mTimeWhenLastOnline;
+
         final long timeNeededForForeground =
                 STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS - timeSinceLastForeground;
         final long timeNeededForOffline =
-                STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS - timeSinceOffline;
+                STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS - timeSinceOfflineNotificationReceived;
+
+        // If the device has been online before, then we wait up to
+        // |mStatusIndicatorWaitOnSwitchOnlineToOfflineDurationMs| duration.
+        final long timeNeededAfterConnectionChangeFromOnlineToOffline = mTimeWhenLastOnline > 0
+                ? mStatusIndicatorWaitOnSwitchOnlineToOfflineDurationMs - timeSinceLastOnline
+                : 0;
 
         assert mUpdateOfflineStatusIndicatorDelayedRunnable != null;
 
@@ -180,13 +242,45 @@ class OfflineDetector
         // been in foreground and connection has been offline for sufficient time, then report the
         // state immediately.
         if (!mIsOfflineLastReportedByConnectivityDetector
-                || (timeNeededForForeground <= 0 && timeNeededForOffline <= 0)) {
+                || (timeNeededForForeground <= 0 && timeNeededForOffline <= 0
+                        && timeNeededAfterConnectionChangeFromOnlineToOffline <= 0)) {
             mUpdateOfflineStatusIndicatorDelayedRunnable.run();
             return;
         }
 
         // Wait before calling |mUpdateOfflineStatusIndicatorDelayedRunnable|.
         mHandler.postDelayed(mUpdateOfflineStatusIndicatorDelayedRunnable,
-                Math.max(timeNeededForForeground, timeNeededForOffline));
+                Math.max(Math.max(timeNeededForForeground, timeNeededForOffline),
+                        timeNeededAfterConnectionChangeFromOnlineToOffline));
+    }
+
+    /**
+     * Returns the value for a Finch parameter, or the default value if no parameter
+     * exists in the current configuration.
+     * @param paramName The name of the Finch parameter (or command-line switch) to get a value
+     *                  for.
+     * @param defaultValue The default value to return when there's no param or switch.
+     * @return The value -- either the param or the default.
+     */
+    private static long getIntParamValueOrDefault(String paramName, long defaultValue) {
+        String value;
+
+        // May throw exception in tests.
+        try {
+            value = VariationsAssociatedData.getVariationParamValue(
+                    ChromeFeatureList.OFFLINE_INDICATOR_V2, paramName);
+        } catch (java.lang.UnsupportedOperationException e) {
+            return defaultValue;
+        }
+
+        if (!TextUtils.isEmpty(value)) {
+            try {
+                return Integer.parseInt(value);
+            } catch (NumberFormatException e) {
+                return defaultValue;
+            }
+        }
+
+        return defaultValue;
     }
 }

chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/indicator/OfflineDetectorUnitTest.java

@@ -11,6 +11,7 @@ import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.verify;
 
 import static org.chromium.chrome.browser.offlinepages.indicator.OfflineDetector.STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS;
+import static org.chromium.chrome.browser.offlinepages.indicator.OfflineDetector.STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS;
 import static org.chromium.chrome.browser.offlinepages.indicator.OfflineDetector.setMockElapsedTimeSupplier;
 
 import android.os.Handler;
@@ -85,9 +86,9 @@ public class OfflineDetectorUnitTest {
         assertFalse("Notification received immediately after connection changed to offline",
                 mLastNotificationReceivedIsOffline);
         final ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
-        verify(mHandler).postDelayed(
-                captor.capture(), eq(STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS));
-        advanceTimeByMs(STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS);
+        verify(mHandler).postDelayed(captor.capture(),
+                eq(STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS));
+        advanceTimeByMs(STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS);
         captor.getValue().run();
 
         assertEquals("Notification count not updated after connection changed to offline", 1,
@@ -233,9 +234,9 @@ public class OfflineDetectorUnitTest {
 
         // Report the connection as offline and then app as backgrounded.
         changeConnectionState(true);
-        verify(mHandler).postDelayed(
-                captor.capture(), eq(STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS));
-        advanceTimeByMs(STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS);
+        verify(mHandler).postDelayed(captor.capture(),
+                eq(STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS));
+        advanceTimeByMs(STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS);
         captor.getValue().run();
         assertEquals("Notification not received even though connection is now offline", 1,
                 mNotificationReceivedByObserver);
@@ -282,9 +283,10 @@ public class OfflineDetectorUnitTest {
         assertFalse("Notification received immediately after connection changed to offline.",
                 mLastNotificationReceivedIsOffline);
         final ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
-        verify(mHandler).postDelayed(
-                captor.capture(), eq(STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS));
-        advanceTimeByMs(STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS - 1000L);
+        verify(mHandler).postDelayed(captor.capture(),
+                eq(STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS));
+        advanceTimeByMs(
+                STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS - 1000L);
 
         assertEquals("Notification received soon after connection changed to offline", 0,
                 mNotificationReceivedByObserver);
@@ -309,6 +311,101 @@ public class OfflineDetectorUnitTest {
                 mLastNotificationReceivedIsOffline);
     }
 
+    /**
+     * Waits |STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS| before
+     * reporting the device as offline if the app has been on an online connection before.
+     */
+    @Test
+    public void testCallbackNotInvokedOfflineInForeground() {
+        changeApplicationStateToBackground(false);
+        // Change to online.
+        changeConnectionState(false);
+        assertEquals(0, mNotificationReceivedByObserver);
+        assertFalse(mLastNotificationReceivedIsOffline);
+
+        assertEquals("Notification received immediately after connection changed to online", 0,
+                mNotificationReceivedByObserver);
+        assertFalse("Notification received immediately after connection changed to online.",
+                mLastNotificationReceivedIsOffline);
+        final ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
+
+        // Advance time by a long duration (10 minutes).
+        advanceTimeByMs(10 * 60 * 1000);
+
+        // The offline state should not be notified after
+        // |STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS| since the device
+        // has been in the foreground for a long time.
+        changeConnectionState(true);
+
+        verify(mHandler).postDelayed(captor.capture(),
+                eq(STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS));
+
+        assertEquals("Extra notification received even though device just changed to offline", 0,
+                mNotificationReceivedByObserver);
+        assertFalse("Extra notification received even though device just changed to offline",
+                mLastNotificationReceivedIsOffline);
+
+        // Advance time after which the offline state should be notified.
+        advanceTimeByMs(STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS
+                - STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS);
+        captor.getValue().run();
+        assertEquals("Expected notification when app has been offline for long", 1,
+                mNotificationReceivedByObserver);
+        assertTrue("Expected notification when app has been offline for long",
+                mLastNotificationReceivedIsOffline);
+    }
+
+    /**
+     * Waits |STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS| before
+     * reporting the device as offline if the app has been on an online connection before. If the
+     * device is reported as online in the meantime, then the posted task is cancelled and the
+     * callback is not invoked.
+     */
+    @Test
+    public void testCallbackNotInvokedOfflineInForegroundCancelled() {
+        changeApplicationStateToBackground(false);
+        // Change to online.
+        changeConnectionState(false);
+        assertEquals(0, mNotificationReceivedByObserver);
+        assertFalse(mLastNotificationReceivedIsOffline);
+
+        assertEquals("Notification received immediately after connection changed to online", 0,
+                mNotificationReceivedByObserver);
+        assertFalse("Notification received immediately after connection changed to online.",
+                mLastNotificationReceivedIsOffline);
+        final ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
+
+        // Advance time by a long duration (10 minutes).
+        advanceTimeByMs(10 * 60 * 1000);
+
+        // The offline state should not be notified after
+        // |STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS| since the device
+        // has been in the foreground for a long time.
+        changeConnectionState(true);
+
+        verify(mHandler).postDelayed(captor.capture(),
+                eq(STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS));
+
+        assertEquals("Extra notification received even though device just changed to offline", 0,
+                mNotificationReceivedByObserver);
+        assertFalse("Extra notification received even though device just changed to offline",
+                mLastNotificationReceivedIsOffline);
+
+        // Change to online before
+        // |STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS| duration is over.
+        // This should cancel the callbacks.
+        changeConnectionState(false);
+
+        // Advance time after which the offline state should be notified.
+        advanceTimeByMs(STATUS_INDICATOR_WAIT_ON_SWITCH_ONLINE_TO_OFFLINE_DEFAULT_DURATION_MS
+                - STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS);
+        captor.getValue().run();
+        assertEquals("Extra notification received even though device is now online", 0,
+                mNotificationReceivedByObserver);
+        assertFalse("Extra notification received even though device is now online",
+                mLastNotificationReceivedIsOffline);
+    }
+
     private void changeConnectionState(boolean offline) {
         final int state = offline ? ConnectionState.NO_INTERNET : ConnectionState.VALIDATED;
         mOfflineDetector.onConnectionStateChanged(state);