Refactor Offline detector to a separate class.

Refactor detection of offline connections for the purpose of
offline indicator to a separate class.

This class also handles the logic of waiting for 2 seconds after the
connection is offline before notifying the offline indicator class.
If the connection is online, then the indicator class is notified
immediately.

In the next CL, this class would also handle the logic of
holding on to the notification when Chrome is brought to
the foreground (see crbug.com/1093574)

Change-Id: I3368fce894b1fa2dfda3efce3fef5c3764c7217b
Bug: 1084740
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2240899
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Jian Li <jianli@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: Sinan Sahin <sinansahin@google.com>
Cr-Commit-Position: refs/heads/master@{#779670}

chrome/android/chrome_java_sources.gni

@@ -1056,6 +1056,7 @@ chrome_java_sources = [
   "java/src/org/chromium/chrome/browser/offlinepages/TriggerConditions.java",
   "java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java",
   "java/src/org/chromium/chrome/browser/offlinepages/indicator/ConnectivityDetector.java",
+  "java/src/org/chromium/chrome/browser/offlinepages/indicator/OfflineDetector.java",
   "java/src/org/chromium/chrome/browser/offlinepages/indicator/OfflineIndicatorController.java",
   "java/src/org/chromium/chrome/browser/offlinepages/indicator/OfflineIndicatorControllerV2.java",
   "java/src/org/chromium/chrome/browser/offlinepages/indicator/TopSnackbarManager.java",

chrome/android/chrome_junit_test_java_sources.gni

@@ -150,6 +150,7 @@ chrome_junit_test_java_sources = [
   "junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java",
   "junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsUnitTest.java",
   "junit/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPackerTest.java",
+  "junit/src/org/chromium/chrome/browser/offlinepages/indicator/OfflineDetectorUnitTest.java",
   "junit/src/org/chromium/chrome/browser/offlinepages/indicator/OfflineIndicatorControllerV2UnitTest.java",
   "junit/src/org/chromium/chrome/browser/offlinepages/prefetch/OfflineNotificationBackgroundTaskUnitTest.java",
   "junit/src/org/chromium/chrome/browser/offlinepages/prefetch/PrefetchBackgroundTaskUnitTest.java",

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

+// Copyright 2020 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.offlinepages.indicator;
+
+import android.os.Handler;
+
+import androidx.annotation.VisibleForTesting;
+
+import org.chromium.base.Callback;
+import org.chromium.base.supplier.Supplier;
+import org.chromium.chrome.browser.offlinepages.indicator.ConnectivityDetector.ConnectionState;
+
+/**
+ * Class that detects if the network is offline. Waits for the network to stablize before notifying
+ * the observer.
+ */
+class OfflineDetector implements ConnectivityDetector.Observer {
+    static final long STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS = 2000;
+
+    private static ConnectivityDetector sMockConnectivityDetector;
+    private static Supplier<Long> sMockElapsedTimeSupplier;
+
+    private ConnectivityDetector mConnectivityDetector;
+
+    // Maintains if the connection is effectively offline.
+    // Effectively offline means that all checks have been passed and the
+    // |mCallback| has been invoked to notify the observers.
+    private boolean mIsEffectivelyOffline;
+
+    // True if the network is offline as detected by the connectivity detector.
+    private boolean mIsOfflineLastReportedByConnectivityDetector;
+
+    private Handler mHandler;
+    private Runnable mUpdateOfflineStatusIndicatorDelayedRunnable;
+    private final Callback<Boolean> mCallback;
+
+    /**
+     * Constructs the offline indicator.
+     * @param callback The {@link callback} is invoked when the connectivity status is stable and
+     *         has changed.
+     */
+    OfflineDetector(Callback<Boolean> callback) {
+        mCallback = callback;
+        mHandler = new Handler();
+
+        if (sMockConnectivityDetector != null) {
+            mConnectivityDetector = sMockConnectivityDetector;
+        } else {
+            mConnectivityDetector = new ConnectivityDetector(this);
+        }
+
+        mUpdateOfflineStatusIndicatorDelayedRunnable = () -> {
+            // Connection state has not changed since |mUpdateOfflineStatusIndicatorDelayedRunnable|
+            // was posted.
+            if (mIsOfflineLastReportedByConnectivityDetector != mIsEffectivelyOffline) {
+                mIsEffectivelyOffline = mIsOfflineLastReportedByConnectivityDetector;
+                mCallback.onResult(mIsEffectivelyOffline);
+            }
+        };
+    }
+
+    @Override
+    public void onConnectionStateChanged(int connectionState) {
+        boolean previousLastReportedStateByOfflineDetector =
+                mIsOfflineLastReportedByConnectivityDetector;
+        mIsOfflineLastReportedByConnectivityDetector =
+                (connectionState != ConnectionState.VALIDATED);
+        if (previousLastReportedStateByOfflineDetector
+                == mIsOfflineLastReportedByConnectivityDetector) {
+            return;
+        }
+
+        // Report the online state immediately if the connection is now online.
+        if (!mIsOfflineLastReportedByConnectivityDetector) {
+            mHandler.removeCallbacks(mUpdateOfflineStatusIndicatorDelayedRunnable);
+            // Run the callback and report the connection as online if the last time we reported
+            // connection as offline.
+            if (mIsEffectivelyOffline) {
+                mIsEffectivelyOffline = false;
+                mCallback.onResult(mIsEffectivelyOffline);
+            }
+            return;
+        }
+
+        // Post a task to invoke the mCallback after some time.
+        mHandler.postDelayed(mUpdateOfflineStatusIndicatorDelayedRunnable,
+                STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS);
+    }
+
+    /*
+     * Returns true if the connection is offline and the connection state has been stable.
+     */
+    boolean isConnectionStateOffline() {
+        return mIsEffectivelyOffline;
+    }
+
+    void destroy() {
+        if (mConnectivityDetector != null) {
+            mConnectivityDetector.destroy();
+            mConnectivityDetector = null;
+        }
+        mHandler.removeCallbacks(mUpdateOfflineStatusIndicatorDelayedRunnable);
+    }
+
+    @VisibleForTesting
+    static void setMockConnectivityDetector(ConnectivityDetector connectivityDetector) {
+        sMockConnectivityDetector = connectivityDetector;
+    }
+
+    @VisibleForTesting
+    static void setMockElapsedTimeSupplier(Supplier<Long> supplier) {
+        sMockElapsedTimeSupplier = supplier;
+    }
+
+    @VisibleForTesting
+    void setHandlerForTesting(Handler handler) {
+        mHandler = handler;
+    }
+}

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

@@ -20,7 +20,6 @@ import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.base.supplier.ObservableSupplier;
 import org.chromium.base.supplier.Supplier;
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.offlinepages.indicator.ConnectivityDetector.ConnectionState;
 import org.chromium.chrome.browser.status_indicator.StatusIndicatorCoordinator;
 
 import java.lang.annotation.Retention;
@@ -31,7 +30,7 @@ import java.util.concurrent.TimeUnit;
  * Class that controls visibility and content of {@link StatusIndicatorCoordinator} to relay
  * connectivity information.
  */
-public class OfflineIndicatorControllerV2 implements ConnectivityDetector.Observer {
+public class OfflineIndicatorControllerV2 {
     @IntDef({UmaEnum.CAN_ANIMATE_NATIVE_CONTROLS,
             UmaEnum.CAN_ANIMATE_NATIVE_CONTROLS_OMNIBOX_FOCUSED,
             UmaEnum.CANNOT_ANIMATE_NATIVE_CONTROLS,
@@ -47,15 +46,20 @@ public class OfflineIndicatorControllerV2 implements ConnectivityDetector.Observ
     }
 
     static final long STATUS_INDICATOR_WAIT_BEFORE_HIDE_DURATION_MS = 2000;
+
+    // TODO(tbansal): Consider moving the cooldown logic to OfflineDetector.java.
+    // The cooldown period was added to prevent showing/changing the indicator too frequently. In
+    // longer term, OfflineDetector should protect against sending signals to this class too
+    // frequently.
     static final long STATUS_INDICATOR_COOLDOWN_BEFORE_NEXT_ACTION_MS = 5000;
 
-    private static ConnectivityDetector sMockConnectivityDetector;
+    private static OfflineDetector sMockOfflineDetector;
     private static Supplier<Long> sMockElapsedTimeSupplier;
 
     private Context mContext;
     private StatusIndicatorCoordinator mStatusIndicator;
     private Handler mHandler;
-    private ConnectivityDetector mConnectivityDetector;
+    private OfflineDetector mOfflineDetector;
     private ObservableSupplier<Boolean> mIsUrlBarFocusedSupplier;
     private Supplier<Boolean> mCanAnimateBrowserControlsSupplier;
     private Callback<Boolean> mOnUrlBarFocusChanged;
@@ -91,6 +95,12 @@ public class OfflineIndicatorControllerV2 implements ConnectivityDetector.Observ
         // If we're offline at start-up, we should have a small enough last action time so that we
         // don't wait for the cool-down.
         mLastActionTime = getElapsedTime() - STATUS_INDICATOR_COOLDOWN_BEFORE_NEXT_ACTION_MS;
+        if (sMockOfflineDetector != null) {
+            mOfflineDetector = sMockOfflineDetector;
+        } else {
+            mOfflineDetector =
+                    new OfflineDetector((Boolean offline) -> onConnectionStateChanged(offline));
+        }
 
         mShowRunnable = () -> {
             RecordUserAction.record("OfflineIndicator.Shown");
@@ -148,23 +158,14 @@ public class OfflineIndicatorControllerV2 implements ConnectivityDetector.Observ
         mIsUrlBarFocusedSupplier.addObserver(mOnUrlBarFocusChanged);
 
         mUpdateStatusIndicatorDelayedRunnable = () -> {
-            final boolean offline =
-                    isConnectionStateOffline(mConnectivityDetector.getConnectionState());
+            final boolean offline = mOfflineDetector.isConnectionStateOffline();
             if (offline != mIsOffline) {
                 updateStatusIndicator(offline);
             }
         };
-
-        if (sMockConnectivityDetector != null) {
-            mConnectivityDetector = sMockConnectivityDetector;
-        } else {
-            mConnectivityDetector = new ConnectivityDetector(this);
-        }
     }
 
-    @Override
-    public void onConnectionStateChanged(int connectionState) {
-        final boolean offline = isConnectionStateOffline(connectionState);
+    public void onConnectionStateChanged(boolean offline) {
         if (mIsOffline == offline) {
             return;
         }
@@ -183,9 +184,9 @@ public class OfflineIndicatorControllerV2 implements ConnectivityDetector.Observ
     }
 
     public void destroy() {
-        if (mConnectivityDetector != null) {
-            mConnectivityDetector.destroy();
-            mConnectivityDetector = null;
+        if (mOfflineDetector != null) {
+            mOfflineDetector.destroy();
+            mOfflineDetector = null;
         }
 
         if (mIsUrlBarFocusedSupplier != null) {
@@ -238,13 +239,9 @@ public class OfflineIndicatorControllerV2 implements ConnectivityDetector.Observ
         mLastActionTime = getElapsedTime();
     }
 
-    private boolean isConnectionStateOffline(@ConnectionState int connectionState) {
-        return connectionState != ConnectionState.VALIDATED;
-    }
-
     @VisibleForTesting
-    static void setMockConnectivityDetector(ConnectivityDetector connectivityDetector) {
-        sMockConnectivityDetector = connectivityDetector;
+    static void setMockOfflineDetector(OfflineDetector offlineDetector) {
+        sMockOfflineDetector = offlineDetector;
     }
 
     @VisibleForTesting

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

+// Copyright 2020 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.offlinepages.indicator;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+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.setMockElapsedTimeSupplier;
+
+import android.os.Handler;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.chrome.browser.offlinepages.indicator.ConnectivityDetector.ConnectionState;
+
+/**
+ * Unit tests for {@link OfflineDetector}.
+ */
+@RunWith(BaseRobolectricTestRunner.class)
+public class OfflineDetectorUnitTest {
+    @Mock
+    private ConnectivityDetector mConnectivityDetector;
+    @Mock
+    private Handler mHandler;
+
+    private long mElapsedTimeMs;
+    private OfflineDetector mOfflineDetector;
+
+    private int mNotificationReceivedByObserver = 0;
+    private boolean mLastNotificationReceivedIsOffline;
+
+    @Before
+    public void setUp() {
+        MockitoAnnotations.initMocks(this);
+
+        mElapsedTimeMs = 0;
+        OfflineDetector.setMockElapsedTimeSupplier(() -> mElapsedTimeMs);
+
+        OfflineDetector.setMockConnectivityDetector(mConnectivityDetector);
+
+        mOfflineDetector =
+                new OfflineDetector((Boolean offline) -> onConnectionStateChanged(offline));
+        mOfflineDetector.setHandlerForTesting(mHandler);
+    }
+
+    @After
+    public void tearDown() {
+        OfflineDetector.setMockElapsedTimeSupplier(null);
+    }
+
+    /**
+     * Tests that the online notification is sent immediately when the device goes online.
+     * Also, verifies that the offline notification is sent after
+     * |STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS| time has elapsed.
+     */
+    @Test
+    public void testCallbackInvokedOnConnectionChange() {
+        // Change to online.
+        changeConnectionState(false);
+        assertEquals(0, mNotificationReceivedByObserver);
+        assertFalse(mLastNotificationReceivedIsOffline);
+
+        // Change to offline.
+        changeConnectionState(true);
+        assertEquals("Notification received immediately after connection changed to offline", 0,
+                mNotificationReceivedByObserver);
+        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);
+        captor.getValue().run();
+
+        assertEquals("Notification count not updated after connection changed to offline", 1,
+                mNotificationReceivedByObserver);
+        assertTrue("Notification not received after connection changed to offline",
+                mLastNotificationReceivedIsOffline);
+
+        // Change to online.
+        changeConnectionState(false);
+        assertEquals("Notification count not updated after connection changed to online", 2,
+                mNotificationReceivedByObserver);
+        assertFalse("Notification not received after connection changed to online",
+                mLastNotificationReceivedIsOffline);
+
+        // Change to online again. It should not trigger a notification.
+        changeConnectionState(false);
+        assertEquals(
+                "Extra notification received even though there is no change in connection state", 2,
+                mNotificationReceivedByObserver);
+        assertFalse(
+                "Extra notification received even though there is no change in connection state",
+                mLastNotificationReceivedIsOffline);
+    }
+
+    /**
+     * Tests that when the device switches immediately from offline to online, then the callback is
+     * not executed.
+     */
+    @Test
+    public void testCallbackNotInvokedOfflineToFastOnline() {
+        // Change to online.
+        changeConnectionState(false);
+        assertEquals(0, mNotificationReceivedByObserver);
+        assertFalse(mLastNotificationReceivedIsOffline);
+
+        // Change to offline.
+        changeConnectionState(true);
+        assertEquals("Notification received immediately after connection changed to offline", 0,
+                mNotificationReceivedByObserver);
+        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);
+
+        assertEquals("Notification received soon after connection changed to offline", 0,
+                mNotificationReceivedByObserver);
+        assertFalse("Notification received soon after connection changed to offline",
+                mLastNotificationReceivedIsOffline);
+
+        // Change to online.
+        changeConnectionState(false);
+        assertEquals("Extra notification received after connection changed to online", 0,
+                mNotificationReceivedByObserver);
+        assertFalse("Connection is reported as offline when it's online",
+                mLastNotificationReceivedIsOffline);
+
+        // Move clock forward by 1000ms so that the offline callback posts after
+        // |STATUS_INDICATOR_WAIT_ON_OFFLINE_DURATION_MS|. This should not trigger a notification
+        // since the connection is now online.
+        advanceTimeByMs(1000L);
+        captor.getValue().run();
+        assertEquals("Extra notification received even though connection is still online", 0,
+                mNotificationReceivedByObserver);
+        assertFalse("Connection is reported as offline when it's online",
+                mLastNotificationReceivedIsOffline);
+    }
+
+    private void changeConnectionState(boolean offline) {
+        final int state = offline ? ConnectionState.NO_INTERNET : ConnectionState.VALIDATED;
+        mOfflineDetector.onConnectionStateChanged(state);
+    }
+
+    private void advanceTimeByMs(long delta) {
+        mElapsedTimeMs += delta;
+        setMockElapsedTimeSupplier(() -> mElapsedTimeMs);
+    }
+
+    private void onConnectionStateChanged(boolean offline) {
+        mNotificationReceivedByObserver++;
+        mLastNotificationReceivedIsOffline = offline;
+    }
+}

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

@@ -51,6 +51,8 @@ public class OfflineIndicatorControllerV2UnitTest {
     @Mock
     private ConnectivityDetector mConnectivityDetector;
     @Mock
+    private OfflineDetector mOfflineDetector;
+    @Mock
     private Handler mHandler;
     @Mock
     private Supplier<Boolean> mCanAnimateNativeBrowserControls;
@@ -78,7 +80,8 @@ public class OfflineIndicatorControllerV2UnitTest {
         when(mTimeUtils.getTimeTicksNowUs()).thenReturn(0L);
 
         mIsUrlBarFocusedSupplier.set(false);
-        OfflineIndicatorControllerV2.setMockConnectivityDetector(mConnectivityDetector);
+        OfflineDetector.setMockConnectivityDetector(mConnectivityDetector);
+        OfflineIndicatorControllerV2.setMockOfflineDetector(mOfflineDetector);
         mElapsedTimeMs = 0;
         OfflineIndicatorControllerV2.setMockElapsedTimeSupplier(() -> mElapsedTimeMs);
         mController = new OfflineIndicatorControllerV2(mContext, mStatusIndicator,
@@ -164,6 +167,7 @@ public class OfflineIndicatorControllerV2UnitTest {
     public void testCoolDown_Show() {
         // First, show.
         changeConnectionState(true);
+        verify(mStatusIndicator, times(1)).show(eq("Offline"), any(), anyInt(), anyInt(), anyInt());
         // Advance time so we can hide.
         advanceTimeByMs(STATUS_INDICATOR_COOLDOWN_BEFORE_NEXT_ACTION_MS);
         // Now, hide.
@@ -289,8 +293,8 @@ public class OfflineIndicatorControllerV2UnitTest {
 
     private void changeConnectionState(boolean offline) {
         final int state = offline ? ConnectionState.NO_INTERNET : ConnectionState.VALIDATED;
-        when(mConnectivityDetector.getConnectionState()).thenReturn(state);
-        mController.onConnectionStateChanged(state);
+        when(mOfflineDetector.isConnectionStateOffline()).thenReturn(offline);
+        mController.onConnectionStateChanged(offline);
     }
 
     private void advanceTimeByMs(long delta) {