Only show Last N Pages in the UI when the corresponding tab is visible.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
index dc67d57487982bb5856f7b4c23bea943bf651931..4be92b3e1f54925b7d4151d9148c88ccb1a945fa 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
@@ -31,7 +31,9 @@ import org.chromium.chrome.browser.snackbar.Snackbar;
 import org.chromium.chrome.browser.snackbar.SnackbarManager;
 import org.chromium.chrome.browser.snackbar.SnackbarManager.SnackbarController;
 import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tabmodel.TabModel;
 import org.chromium.chrome.browser.tabmodel.TabModelSelector;
+import org.chromium.chrome.browser.tabmodel.TabModelSelectorTabModelObserver;
 import org.chromium.components.bookmarks.BookmarkId;
 import org.chromium.components.offlinepages.SavePageResult;
 import org.chromium.content_public.browser.LoadUrlParams;
@@ -45,8 +47,11 @@ import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.nio.channels.FileChannel;
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
+import java.util.WeakHashMap;
 import java.util.concurrent.TimeUnit;
 
 /**
@@ -703,6 +708,73 @@ public class OfflinePageUtils {
         return ConnectionType.CONNECTION_UNKNOWN;
     }
 
+    /**
+     * Tracks tab creation and closure for the Recent Tabs feature.  UI needs to stop showing
+     * recent offline pages as soon as the tab is closed.  The TabModel is used to get profile
+     * information because Tab's profile is tied to the native WebContents, which may not exist at
+     * tab adding or tab closing time.
+     */
+    private static class RecentTabTracker extends TabModelSelectorTabModelObserver {
+        private TabModelSelector mTabModelSelector;
+
+        public RecentTabTracker(TabModelSelector selector) {
+            super(selector);
+            mTabModelSelector = selector;
+        }
+
+        @Override
+        public void didAddTab(Tab tab, TabModel.TabLaunchType type) {
+            Profile profile = mTabModelSelector.getModel(tab.isIncognito()).getProfile();
+            OfflinePageBridge bridge = OfflinePageBridge.getForProfile(profile);
+            if (bridge == null) return;
+            bridge.registerRecentTab(tab.getId());
+        }
+
+        @Override
+        public void didCloseTab(int tabId, boolean incognito) {
+            Profile profile = mTabModelSelector.getModel(incognito).getProfile();
+            OfflinePageBridge bridge = OfflinePageBridge.getForProfile(profile);
+            if (bridge == null) return;
+
+            // First, unregister the tab with the UI.
+            bridge.unregisterRecentTab(tabId);
+
+            // Then, delete any "Last N" offline pages as well.  This is an optimization because
+            // the UI will no longer show the page, and the page would also be cleaned up by GC
+            // given enough time.
+            ClientId clientId =
+                    new ClientId(OfflinePageBridge.LAST_N_NAMESPACE, Integer.toString(tabId));
+            List<ClientId> clientIds = new ArrayList<>();
+            clientIds.add(clientId);
+
+            bridge.deletePagesByClientId(clientIds, new Callback<Integer>() {
+                @Override
+                public void onResult(Integer result) {
+                    // Result is ignored.
+                }
+            });
+        }
+    }
+
+    /**
+     * Tracks the observers of ChromeActivity's TabModelSelectors. This is weak so the activity can
+     * be garbage collected without worrying about this map.  The RecentTabTracker is held here so
+     * that it can be destroyed when the ChromeActivity gets a new TabModelSelector.
+     */
+    private static WeakHashMap<ChromeActivity, RecentTabTracker> sTabModelObservers =
+            new WeakHashMap<>();
+
+    /**
+     * Starts tracking the tab models in the given selector for tab addition and closure,
+     * destroying obsolete observers as necessary.
+     */
+    public static void observeTabModelSelector(
+            ChromeActivity activity, TabModelSelector tabModelSelector) {
+        RecentTabTracker previousObserver =

[Ted C, 2017/02/22 23:28:04]

I think you can get rid of the weakhashmap (and make it just a hashmap) and just
listen for activity changes.  When they get destroyed, remove the entry from the
map and destroy the observer.

You can look at ApplicationStatus to listen for state changes about the activity
state changes.

[carlosk, 2017/02/23 03:46:47]

Looking at the related code it seems that TabWindowManager and
TabModelSelectorUma both keep strong references to the Activity (the latter,
through ApplicationStatus.registerStateListenerForActivity). But also both of
them do listen for the activity lifecycle and eliminate that reference when it's
destroyed (see [1] and [2], respectively).

What might still be a problem though is that RecentTabTracker::destroy would not
be called but maybe that could be resolved by calling it from a finalize()
implementation?

However, even with all comments above, it seems that all other cases where
TabModelSelectorTabModelObserver is extended they do listen to the activity
being destroyed and to cleanup after themselves "manually".

In conclusion: even though it seems it should work as is I am implementing the
scheme you suggested to play it safe, but wanted to also share my findings to
ask you WDYT?


[1]
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

[2]
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...

+                sTabModelObservers.put(activity, new RecentTabTracker(tabModelSelector));
+        if (previousObserver != null) previousObserver.destroy();
+    }
+
     @VisibleForTesting
     static void setInstanceForTesting(OfflinePageUtils instance) {
         sInstance = instance;