[Offline pages] Refactoring: Merge Connectivity Listener into TabObserver

chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java

Index: chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java
index 16c95854140136747d91bb5a29c339554ede0351..ea4a2af40e4a80270edaaf9169a4b6a0c57b420e 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java
@@ -7,13 +7,16 @@ package org.chromium.chrome.browser.offlinepages;
 import android.content.Context;
 
 import org.chromium.base.Log;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.snackbar.SnackbarManager;
 import org.chromium.chrome.browser.snackbar.SnackbarManager.SnackbarController;
 import org.chromium.chrome.browser.tab.EmptyTabObserver;
 import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tab.TabObserver;
+import org.chromium.net.NetworkChangeNotifier;
 
-import java.util.Map;
-import java.util.WeakHashMap;
+import java.util.HashSet;
+import java.util.Set;
 
 /**
  * A class that observes events for a tab which has an associated offline page.  This will be
@@ -21,115 +24,121 @@ import java.util.WeakHashMap;
  * to show a snackbar if the device connects).  It will be removed when the user navigates away
  * from the offline tab.
  */
-public class OfflinePageTabObserver extends EmptyTabObserver {
+public class OfflinePageTabObserver

[dewittj, 2016/03/28 20:17:59]

This name is not so accurate anymore, now that it observes multiple types of
things.

[fgorski, 2016/03/28 22:52:32]

Please suggest a better name.
Primary task of this class is observing the tab and it serves a purpose beyond
just reload snackbar, see onPageLoadStarted()

We will have opportunity to rename it in future, once we start moving reload
snackbar controller from offlinepageutils. Perhaps OPReloadSnackbarController is
a better name for this class ;)

+        extends EmptyTabObserver implements NetworkChangeNotifier.ConnectionTypeObserver {
     private static final String TAG = "OfflinePageTO";
     private Context mContext;
     private SnackbarManager mSnackbarManager;
-    private boolean mConnected;
     private SnackbarController mSnackbarController;
-    private boolean mWasHidden;
-    private OfflinePageConnectivityListener mListener;
+    private final Set<Integer> mObservedTabs = new HashSet<Integer>();
+    private boolean mWasSnackbarShown;
+    private Tab mTab;
+    private boolean mIsObservingNetworkChagnes;

[dewittj, 2016/03/28 20:17:59]

nit: fix spelling.

Also, this bit is completely unnecessary (except that you inspect it in test). 
Can we actually remove this field from production code, and keep the boolean
up-to-date in the tests using mocking code?

[fgorski, 2016/03/28 22:52:32]

Done.
185, 201

[dewittj, 2016/03/28 23:40:02]

Technically, StopObservingNetworkChanges can be called without actually checking
if we are observing network changes, since ObserverList#removeObserver bails
early if there is no such observer.  On second thought, that would mean we'd be
depending on this particular behavior of ObserverList which might not stay the
same forever, so I could see why you might want to keep track of the bit.

[fgorski, 2016/03/29 16:00:43]

Acknowledged.

 
-    private static final Map<Integer, OfflinePageTabObserver> sTabObservers =
-            new WeakHashMap<Integer, OfflinePageTabObserver>();
+    private static TabHelper sTabHelper;
+    private static OfflinePageTabObserver sInstance;
 
     /**
-     * Create and attach a tab observer if we don't already have one, otherwise update it.
-     * @param context Android context.
-     * @param snackbarManager The snackbar manager to show and dismiss snackbars.
-     * @param tab The tab we are adding an observer for.
-     * @param connected True if we were connected when this call was made.
-     * @param snackbarController The snackbar controller to control snackbar behavior.
+     * Helper class that allows mocking access to the tab methods, without having to have a tab,
+     * which makes things testable.
      */
-    public static void addObserverForTab(Context context, SnackbarManager snackbarManager, Tab tab,
-            boolean connected, SnackbarController snackbarController) {
-        // See if we already have an observer for this tab.
-        int tabId = tab.getId();
-        OfflinePageTabObserver observer = sTabObservers.get(tabId);
-
-        if (observer == null) {
-            // If we don't have an observer, build one and attach it to the tab.
-            observer = new OfflinePageTabObserver(
-                    context, snackbarManager, tab, connected, snackbarController);
-            sTabObservers.put(tabId, observer);
-            tab.addObserver(observer);
-        } else {
-            // If we already have an observer, update the connected state.
-            observer.setConnected(connected);
-            // If we already have an observer and addObserver was called, we should re-enable
-            // the connectivity listener in case we go offline again.  This typically happens
-            // if a background page comes back to the foreground.
-            // To make testing work properly, replace the snackbar controller.  Otherwise the test
-            // will use the release version of the snackbar controller instead of its mock.
-            if (!connected) {
-                observer.enableConnectivityListener(snackbarController);
+    static class TabHelper {

[dewittj, 2016/03/28 20:17:59]

any reason not to expose an interface, instead of a concrete class like this?

[fgorski, 2016/03/28 22:52:32]

There is no benefit to do so. We don't intend to expose it beyond this class and
the only override is provided by test mocks.

+        public int getTabId(Tab tab) {
+            if (tab == null) return Tab.INVALID_TAB_ID;
+            return tab.getId();
+        }
+
+        public boolean isOfflinePage(Tab tab) {
+            return tab != null && tab.isOfflinePage();
+        }
+
+        public boolean isTabShowing(Tab tab) {
+            return tab != null && !tab.isFrozen() && !tab.isHidden();
+        }
+
+        public void addObserver(Tab tab, TabObserver observer) {
+            if (tab != null && observer != null) {
+                tab.addObserver(observer);
+            }
+        }
+
+        public void removeObserver(Tab tab, TabObserver observer) {
+            if (tab != null && observer != null) {
+                tab.removeObserver(observer);
             }
         }
     }
 
+    static void setTabHelperForTesting(TabHelper tabHelper) {
+        sTabHelper = tabHelper;
+    }
+
+    static TabHelper getTabHelper() {
+        if (sTabHelper == null) {
+            sTabHelper = new TabHelper();
+        }
+        return sTabHelper;
+    }
+
+    static void init(Context context, SnackbarManager manager, SnackbarController controller) {
+        sInstance = new OfflinePageTabObserver(context, manager, controller);
+    }
+
+    static OfflinePageTabObserver getInstance() {
+        return sInstance;
+    }
+
+    @VisibleForTesting
+    static void setInstanceForTesting(OfflinePageTabObserver instance) {
+        sInstance = instance;
+    }
+
     /**
-     * Removes the observer for a tab with the specified tabId.
-     * @param tab The tab to remove an observer for.
+     * Create and attach a tab observer if we don't already have one, otherwise update it.
+     * @param tab The tab we are adding an observer for.
      */
-    private static void removeObserverForTab(Tab tab) {
-        OfflinePageTabObserver observer = sTabObservers.get(tab.getId());
-        if (observer != null) {
-            tab.removeObserver(observer);
-        }
-        sTabObservers.remove(tab.getId());
+    public static void addObserverForTab(Tab tab) {
+        assert getInstance() != null;
+        getInstance().startObservingTab(tab);
     }
 
     /**
      * Builds a new OfflinePageTabObserver.
      * @param context Android context.
      * @param snackbarManager The snackbar manager to show and dismiss snackbars.
-     * @param tab The tab we are adding an observer for.
-     * @param connected True if the phone is connected when the observer is created.
      * @param snackbarController Controller to use to build the snackbar.
      */
-    OfflinePageTabObserver(Context context, SnackbarManager snackbarManager, Tab tab,
-            boolean connected, SnackbarController snackbarController) {
+    OfflinePageTabObserver(Context context, SnackbarManager snackbarManager,
+            SnackbarController snackbarController) {
         mContext = context;
         mSnackbarManager = snackbarManager;
-        mConnected = connected;
-        // Remember if the tab was hidden when we started, so we can show the snackbar when
-        // the tab becomes visible.
-        mWasHidden = tab.isHidden();
-
-        mListener = new OfflinePageConnectivityListener(
-                context, snackbarManager, tab, snackbarController);
         mSnackbarController = snackbarController;
-    }
-
-    private void setConnected(boolean connected) {
-        mConnected = connected;
-    }
 
-    private void enableConnectivityListener(SnackbarController snackbarController) {
-        mListener.enable(snackbarController);
+        // The first time observer is created snackbar has net yet been shown.
+        mWasSnackbarShown = false;
+        mIsObservingNetworkChagnes = false;
     }
 
     // Methods from EmptyTabObserver
     @Override
-    public void onShown(Tab visibleTab) {
-        // If there is no bookmark on this tab, there is nothing to do.
-        long bookmarkNum = visibleTab.getBookmarkId();
-        if (bookmarkNum == Tab.INVALID_BOOKMARK_ID) return;
-
-        if (mWasHidden) {
-            if (mConnected) {
-                OfflinePageUtils.showReloadSnackbar(
-                        mContext, mSnackbarManager, mSnackbarController);
-            }
+    public void onShown(Tab tab) {
+        if (!getTabHelper().isOfflinePage(tab)) return;
 
-            mWasHidden = false;
+        mWasSnackbarShown = false;

[dewittj, 2016/03/28 20:17:59]

This could use a comment: why is this being reset?

[fgorski, 2016/03/28 22:52:32]

Done.

+        mTab = tab;
+        if (isConnected()) {
             Log.d(TAG, "onShown, showing 'delayed' snackbar");
+            showReloadSnackbar();
+            // TODO(fgorski): Move the variable assignment to the method above, once
+            // OfflinePageUtils can be mocked.
+            mWasSnackbarShown = true;
         }
     }
 
     @Override
     public void onHidden(Tab hiddenTab) {
-        mWasHidden = true;
+        mWasSnackbarShown = false;
+        mTab = null;
         // In case any snackbars are showing, dismiss them before we switch tabs.
         mSnackbarManager.dismissSnackbars(mSnackbarController);
     }
@@ -140,30 +149,114 @@ public class OfflinePageTabObserver extends EmptyTabObserver {
     }
 
     @Override
-    public void onDestroyed(Tab destroyedTab) {
-        // Unregister this tab for OS connectivity notifications.
-        mListener.disable();
-        destroyedTab.removeObserver(this);
-        sTabObservers.remove(destroyedTab.getId());
+    public void onDestroyed(Tab tab) {
         Log.d(TAG, "onDestroyed");
+        stopObservingTab(tab);
     }
 
     @Override
-    public void onUrlUpdated(Tab reloadingTab) {
-        // Unregister this tab for OS connectivity notifications.
-        mListener.disable();
+    public void onUrlUpdated(Tab tab) {
         Log.d(TAG, "onUrlUpdated");
-        removeObserverForTab(reloadingTab);
+        if (!getTabHelper().isOfflinePage(tab)) {
+            stopObservingTab(tab);
+        }
         // In case any snackbars are showing, dismiss them before we navigate away.
         mSnackbarManager.dismissSnackbars(mSnackbarController);
     }
 
+    void startObservingTab(Tab tab) {
+        int tabId = getTabHelper().getTabId(tab);
+
+        // If the tab does not contain an offline page, we don't care to track it.
+        // TODO(fgorski): Or do we actually care, because of onPageLoadStarted?

[Pete Williamson, 2016/03/28 19:52:38]

As log as we get a didFailPageLoad() event even when we are not observing a
page, it should be safe to track only offline pages.

[fgorski, 2016/03/28 22:52:32]

Done.

+        if (!getTabHelper().isOfflinePage(tab)) return;
+
+        // TODO(fgorski): check for one of 2 things:
+        // 1. can we presume that we start observing the current tab, always
+        // 2. will onShow happen right after and then we don't need the next 2 lines:
+        mTab = tab;
+        mWasSnackbarShown = false;
+
+        // If we are not observing the tab yet, let's.
+        if (!mObservedTabs.contains(tabId)) {
+            mObservedTabs.add(tabId);
+            getTabHelper().addObserver(tab, this);
+        }
+        if (!isObservingNetworkChanges()) {
+            startObservingNetworkChanges();
+            mIsObservingNetworkChagnes = true;
+        }
+    }
+
     /**
-     * Attaches a connectivity listener if needed by this observer.
-     * @param tabId The index of the tab that this listener is listening to.
-     * @param listener The listener itself.
+     * Removes the observer for a tab with the specified tabId.
+     * @param tabId ID of a tab that was observed.
      */
-    private void setConnectivityListener(int tabId, OfflinePageConnectivityListener listener) {
-        mListener = listener;
+    void stopObservingTab(Tab tab) {
+        int tabId = getTabHelper().getTabId(tab);
+        if (mObservedTabs.contains(tabId)) {
+            mObservedTabs.remove(tabId);
+            getTabHelper().removeObserver(tab, this);
+        }
+        if (mObservedTabs.isEmpty() && isObservingNetworkChanges()) {
+            stopObservingNetworkChanges();
+            mIsObservingNetworkChagnes = false;
+        }
+        mWasSnackbarShown = false;
+        mTab = null;
+    }
+

[dewittj, 2016/03/28 20:17:59]

nit: add line
// Methods from ConnectionTypeObserver

[fgorski, 2016/03/28 22:52:32]

Done.

+    @Override
+    public void onConnectionTypeChanged(int connectionType) {
+        Log.d(TAG, "Got connectivity event, connectionType: " + connectionType + ", controller "
+                        + mSnackbarController);
+
+        Log.d(TAG, "Connection changed, connected " + isConnected());
+        // Shows or hides the snackbar as needed.  This also adds some hysterisis - if we keep
+        // connecting and disconnecting, we don't want to flash the snackbar.  It will timeout after
+        // several seconds.
+        if (isConnected() && getTabHelper().isTabShowing(mTab) && !wasSnackbarShown()) {
+            Log.d(TAG, "Connection became available, show reload snackbar.");
+            showReloadSnackbar();
+            // TODO(fgorski): Move the variable assignment to the method above, once
+            // OfflinePageUtils can be mocked.
+            mWasSnackbarShown = true;
+        }
+    }
+
+    @VisibleForTesting
+    boolean isObservingTab(Tab tab) {

[dewittj, 2016/03/28 20:17:59]

Any way to test this by inspecting the tab itself?  Better to test using the
public api and observe external state changes.  TabHelperForTest could keep
track of which tabs were observed. 

[fgorski, 2016/03/28 22:52:32]

I want to track observed tabs internally. Changed the code to reflect that.

+        return mObservedTabs.contains(getTabHelper().getTabId(tab));
+    }
+
+    @VisibleForTesting
+    boolean isObservingNetworkChanges() {
+        return mIsObservingNetworkChagnes;
+    }
+
+    @VisibleForTesting
+    boolean isConnected() {
+        return OfflinePageUtils.isConnected();
+    }
+
+    @VisibleForTesting
+    boolean wasSnackbarShown() {
+        return mWasSnackbarShown;
+    }
+
+    @VisibleForTesting
+    void showReloadSnackbar() {
+        OfflinePageUtils.showReloadSnackbar(
+                mContext, mSnackbarManager, mSnackbarController, getTabHelper().getTabId(mTab));
+    }
+
+    @VisibleForTesting
+    void startObservingNetworkChanges() {
+        NetworkChangeNotifier.addConnectionTypeObserver(this);
+    }
+
+    @VisibleForTesting
+    void stopObservingNetworkChanges() {
+        NetworkChangeNotifier.removeConnectionTypeObserver(this);
     }
 }