Tie PersistedTabData#delete() to Tab#delete()

We should tie the deletion of PersistedTabData to the TabLifeCycle
rather than rely on the client of PersistedTabData to know where
they should call PersistedTabData#delete().

Bug: 1119454
Change-Id: I4bdc3d7b7e9a526647cc13f0974f1ef964b8bbd5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2473359
Commit-Queue: David Maunder <davidjm@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#831037}

chrome/android/java/src/org/chromium/chrome/browser/tab/TabImpl.java

@@ -24,6 +24,7 @@ import org.chromium.base.TraceEvent;
 import org.chromium.base.UserDataHost;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.NativeMethods;
+import org.chromium.base.supplier.ObservableSupplierImpl;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.WarmupManager;
 import org.chromium.chrome.browser.WebContentsFactory;
@@ -189,6 +190,8 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
     private final UserDataHost mUserDataHost = new UserDataHost();
 
     private boolean mIsDestroyed;
+    private ObservableSupplierImpl<Boolean> mIsTabSaveEnabledSupplier =
+            new ObservableSupplierImpl<>();
 
     /**
      * Creates an instance of a {@link TabImpl}.
@@ -205,6 +208,7 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
      */
     @SuppressLint("HandlerLeak")
     TabImpl(int id, Tab parent, boolean incognito, @Nullable @TabLaunchType Integer launchType) {
+        mIsTabSaveEnabledSupplier.set(false);
         mId = TabIdManager.getInstance().generateValidId(id);
         mIncognito = incognito;
         if (parent == null) {
@@ -757,6 +761,11 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
                 && TabImplJni.get().getHideFutureNavigations(mNativeTabAndroid);
     }
 
+    @Override
+    public void setIsTabSaveEnabled(boolean isTabSaveEnabled) {
+        mIsTabSaveEnabledSupplier.set(isTabSaveEnabled);
+    }
+
     // TabObscuringHandler.Observer
 
     @Override
@@ -851,6 +860,7 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
             if (CriticalPersistedTabData.from(this).getTimestampMillis() == INVALID_TIMESTAMP) {
                 CriticalPersistedTabData.from(this).setTimestampMillis(System.currentTimeMillis());
             }
+            registerTabSaving();
             String appId;
             Boolean hasThemeColor;
             int themeColor;
@@ -871,6 +881,12 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
         }
     }
 
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    public void registerTabSaving() {
+        CriticalPersistedTabData.from(this).registerIsTabSaveEnabledSupplier(
+                mIsTabSaveEnabledSupplier);
+    }
+
     private boolean useCriticalPersistedTabData() {
         return CachedFeatureFlags.isEnabled(ChromeFeatureList.CRITICAL_PERSISTED_TAB_DATA);
     }

chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java

@@ -662,6 +662,7 @@ public class TabModelImpl extends TabModelJniBridge {
     private void finalizeTabClosure(Tab tab, boolean notifyTabClosureCommitted) {
         if (mTabContentManager != null) mTabContentManager.removeTabThumbnail(tab.getId());
         mTabSaver.removeTabFromQueues(tab);
+        tab.setIsTabSaveEnabled(false);
 
         if (!isIncognito()) HistoricalTabSaver.createHistoricalTab(tab);
 

chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java

@@ -756,6 +756,7 @@ public class TabPersistentStore extends TabPersister {
             return;
         }
         mTabsToSave.addLast(tab);
+        tab.setIsTabSaveEnabled(isCriticalPersistedTabDataEnabled());
     }
 
     public void removeTabFromQueues(Tab tab) {
@@ -773,8 +774,6 @@ public class TabPersistentStore extends TabPersister {
             mSaveTabTask = null;
             saveNextTab();
         }
-        // TODO(crbug.com/1119454) hook delete() into Tab#destroy()
-        CriticalPersistedTabData.from(tab).delete();
         cleanupPersistentData(tab.getId(), tab.isIncognito());
     }
 

chrome/android/javatests/src/org/chromium/chrome/browser/tab/state/CriticalPersistedTabDataTest.java

@@ -4,6 +4,10 @@
 
 package org.chromium.chrome.browser.tab.state;
 
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
 import androidx.test.filters.SmallTest;
 
 import org.junit.Assert;
@@ -15,9 +19,12 @@ import org.mockito.MockitoAnnotations;
 
 import org.chromium.base.Callback;
 import org.chromium.base.ThreadUtils;
+import org.chromium.base.supplier.ObservableSupplierImpl;
 import org.chromium.base.test.BaseJUnit4ClassRunner;
+import org.chromium.base.test.UiThreadTest;
 import org.chromium.chrome.browser.tab.MockTab;
 import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tab.TabImpl;
 import org.chromium.chrome.browser.tab.WebContentsState;
 import org.chromium.chrome.test.ChromeBrowserTestRule;
 
@@ -50,11 +57,12 @@ public class CriticalPersistedTabDataTest {
     }
 
     private CriticalPersistedTabData mCriticalPersistedTabData;
-
     private MockPersistedTabDataStorage mStorage;
 
     private static Tab mockTab(int id, boolean isEncrypted) {
-        return new MockTab(id, isEncrypted);
+        Tab tab = MockTab.createAndInitialize(id, isEncrypted);
+        tab.setIsTabSaveEnabled(true);
+        return tab;
     }
 
     @Before
@@ -96,6 +104,9 @@ public class CriticalPersistedTabDataTest {
                             ROOT_ID, TIMESTAMP, WEB_CONTENTS_STATE, CONTENT_STATE_VERSION,
                             OPENER_APP_ID, THEME_COLOR, LAUNCH_TYPE_AT_CREATION);
             mStorage.setSemaphore(saveSemaphore);
+            ObservableSupplierImpl<Boolean> supplier = new ObservableSupplierImpl<>();
+            supplier.set(true);
+            criticalPersistedTabData.registerIsTabSaveEnabledSupplier(supplier);
             criticalPersistedTabData.saveForTesting();
             acquireSemaphore(saveSemaphore);
             CriticalPersistedTabData.from(mockTab(TAB_ID, isEncrypted), callback);
@@ -140,4 +151,27 @@ public class CriticalPersistedTabDataTest {
             throw new RuntimeException(e);
         }
     }
+
+    @Test
+    @SmallTest
+    @UiThreadTest
+    public void testTabSaving() throws Throwable {
+        TabImpl tab = new MockTab(1, false);
+        CriticalPersistedTabData spyCriticalPersistedTabData =
+                spy(CriticalPersistedTabData.from(tab));
+        tab = MockTab.initializeWithCriticalPersistedTabData(tab, spyCriticalPersistedTabData);
+        tab.registerTabSaving();
+
+        tab.setIsTabSaveEnabled(true);
+        verify(spyCriticalPersistedTabData, times(1)).save();
+        verify(spyCriticalPersistedTabData, times(0)).delete();
+
+        tab.setIsTabSaveEnabled(false);
+        verify(spyCriticalPersistedTabData, times(1)).save();
+        verify(spyCriticalPersistedTabData, times(1)).delete();
+
+        tab.setIsTabSaveEnabled(true);
+        verify(spyCriticalPersistedTabData, times(2)).save();
+        verify(spyCriticalPersistedTabData, times(1)).delete();
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/tab/state/PersistedTabDataTest.java

@@ -12,6 +12,7 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.MockitoAnnotations;
 
+import org.chromium.base.supplier.ObservableSupplierImpl;
 import org.chromium.base.test.BaseJUnit4ClassRunner;
 import org.chromium.base.test.UiThreadTest;
 import org.chromium.chrome.browser.tab.MockTab;
@@ -34,12 +35,15 @@ public class PersistedTabDataTest {
     @UiThreadTest
     @Test
     public void testCacheCallbacks() throws InterruptedException {
-        Tab tab = new MockTab(1, false);
+        Tab tab = MockTab.createAndInitialize(1, false);
+        tab.setIsTabSaveEnabled(true);
         MockPersistedTabData mockPersistedTabData = new MockPersistedTabData(tab, INITIAL_VALUE);
+        registerObserverSupplier(mockPersistedTabData);
         mockPersistedTabData.save();
         // 1
         MockPersistedTabData.from(tab, (res) -> {
             Assert.assertEquals(INITIAL_VALUE, res.getField());
+            registerObserverSupplier(tab.getUserDataHost().getUserData(MockPersistedTabData.class));
             tab.getUserDataHost().getUserData(MockPersistedTabData.class).setField(CHANGED_VALUE);
             // Caching callbacks means 2) shouldn't overwrite CHANGED_VALUE
             // back to INITIAL_VALUE in the callback.
@@ -52,4 +56,10 @@ public class PersistedTabDataTest {
             mockPersistedTabData.delete();
         });
     }
+
+    private static void registerObserverSupplier(MockPersistedTabData mockPersistedTabData) {
+        ObservableSupplierImpl<Boolean> supplier = new ObservableSupplierImpl<>();
+        supplier.set(true);
+        mockPersistedTabData.registerIsTabSaveEnabledSupplier(supplier);
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/tab/state/ShoppingPersistedTabDataTest.java

@@ -24,6 +24,7 @@ import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
 
 import org.chromium.base.Callback;
+import org.chromium.base.supplier.ObservableSupplierImpl;
 import org.chromium.base.test.BaseJUnit4ClassRunner;
 import org.chromium.base.test.UiThreadTest;
 import org.chromium.base.test.util.JniMocker;
@@ -95,6 +96,9 @@ public class ShoppingPersistedTabDataTest {
     public void testShoppingProto() {
         Tab tab = new MockTab(TAB_ID, IS_INCOGNITO);
         ShoppingPersistedTabData shoppingPersistedTabData = new ShoppingPersistedTabData(tab);
+        ObservableSupplierImpl<Boolean> supplier = new ObservableSupplierImpl<>();
+        supplier.set(true);
+        shoppingPersistedTabData.registerIsTabSaveEnabledSupplier(supplier);
         shoppingPersistedTabData.setPriceMicros(PRICE_MICROS, null);
         byte[] serialized = shoppingPersistedTabData.serialize();
         ShoppingPersistedTabData deserialized = new ShoppingPersistedTabData(tab);

chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/Tab.java

@@ -285,4 +285,11 @@ public interface Tab extends TabLifecycle {
      */
     public void setShouldBlockNewNotificationRequests(boolean value);
     public boolean getShouldBlockNewNotificationRequests();
+
+    /**
+     * Set whether {@link Tab} metadata (specifically all {@link PersistedTabData})
+     * will be saved. Not all Tabs need to be persisted across restarts.
+     * The default value when a Tab is initialized is false.
+     */
+    void setIsTabSaveEnabled(boolean isSaveEnabled);
 }

chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/state/MockPersistedTabData.java

@@ -71,9 +71,6 @@ public class MockPersistedTabData extends PersistedTabData {
         return true;
     }
 
-    @Override
-    public void destroy() {}
-
     @Override
     public String getUmaTag() {
         return "MockCritical";

chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/state/PersistedTabData.java

@@ -15,6 +15,7 @@ import org.chromium.base.TraceEvent;
 import org.chromium.base.UserData;
 import org.chromium.base.UserDataHost;
 import org.chromium.base.metrics.RecordHistogram;
+import org.chromium.base.supplier.ObservableSupplierImpl;
 import org.chromium.base.supplier.Supplier;
 import org.chromium.base.task.PostTask;
 import org.chromium.chrome.browser.tab.Tab;
@@ -42,6 +43,9 @@ public abstract class PersistedTabData implements UserData {
     private final PersistedTabDataStorage mPersistedTabDataStorage;
     private final String mPersistedTabDataId;
     private long mLastUpdatedMs;
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    public ObservableSupplierImpl<Boolean> mIsTabSaveEnabledSupplier;
+    private Callback<Boolean> mTabSaveEnabledToggleCallback;
 
     /**
      * @param tab {@link Tab} {@link PersistedTabData} is being stored for
@@ -236,7 +240,9 @@ public abstract class PersistedTabData implements UserData {
      */
     @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED)
     protected void save() {
-        mPersistedTabDataStorage.save(mTab.getId(), mPersistedTabDataId, serializeAndLog());
+        if (mIsTabSaveEnabledSupplier != null && mIsTabSaveEnabledSupplier.get()) {
+            mPersistedTabDataStorage.save(mTab.getId(), mPersistedTabDataId, serializeAndLog());
+        }
     }
 
     /**
@@ -284,7 +290,12 @@ public abstract class PersistedTabData implements UserData {
      * in memory. It will not delete the stored data on a file or database.
      */
     @Override
-    public abstract void destroy();
+    public void destroy() {
+        if (mIsTabSaveEnabledSupplier != null) {
+            mIsTabSaveEnabledSupplier.removeObserver(mTabSaveEnabledToggleCallback);
+            mTabSaveEnabledToggleCallback = null;
+        }
+    }
 
     /**
      * @return unique tag for logging in Uma
@@ -315,4 +326,22 @@ public abstract class PersistedTabData implements UserData {
     protected long getLastUpdatedMs() {
         return mLastUpdatedMs;
     }
+
+    /**
+     * @param isTabSaveEnabledSupplier {@link ObservableSupplierImpl} which provides
+     * access to the flag indicating if the {@link Tab} metadata will be saved and
+     * forward changes to the flag's value.
+     */
+    public void registerIsTabSaveEnabledSupplier(
+            ObservableSupplierImpl<Boolean> isTabSaveEnabledSupplier) {
+        mIsTabSaveEnabledSupplier = isTabSaveEnabledSupplier;
+        mTabSaveEnabledToggleCallback = (isTabSaveEnabled) -> {
+            if (isTabSaveEnabled) {
+                save();
+            } else {
+                delete();
+            }
+        };
+        mIsTabSaveEnabledSupplier.addObserver(mTabSaveEnabledToggleCallback);
+    }
 }

chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/state/ShoppingPersistedTabData.java

@@ -289,9 +289,6 @@ public class ShoppingPersistedTabData extends PersistedTabData {
         return false;
     }
 
-    @Override
-    public void destroy() {}
-
     @Override
     public String getUmaTag() {
         return "SPTD";

chrome/test/android/javatests/src/org/chromium/chrome/browser/tab/MockTab.java

@@ -6,6 +6,7 @@ package org.chromium.chrome.browser.tab;
 
 import androidx.annotation.Nullable;
 
+import org.chromium.chrome.browser.tab.state.CriticalPersistedTabData;
 import org.chromium.content_public.browser.LoadUrlParams;
 import org.chromium.content_public.browser.WebContents;
 
@@ -22,6 +23,13 @@ public class MockTab extends TabImpl {
         return tab;
     }
 
+    public static TabImpl initializeWithCriticalPersistedTabData(
+            TabImpl tab, CriticalPersistedTabData criticalPersistedTabData) {
+        tab.getUserDataHost().setUserData(CriticalPersistedTabData.class, criticalPersistedTabData);
+        tab.initialize(null, null, null, null, null, false, null, null);
+        return tab;
+    }
+
     /**
      * Constructor for id and incognito attribute. Tests often need to initialize
      * these two fields only.