Revert of Last_n: instrumentation tests to verify timely snapshot deletion....

Revert of Last_n: instrumentation tests to verify timely snapshot deletion. (patchset #7 id:120001 of https://codereview.chromium.org/2833653002/ )

Reason for revert:
Likely cause of https://crbug.com/746699.

testLastNPageIsDeletedUponNavigation is flaky.

Original issue's description:
> Last_n: instrumentation tests to verify timely snapshot deletion.
>
> Adds new tests to RecentTabsTest.java to verify that offline snapshots created
> by last_n are properly deleted when successfully navigating to another page and
> upon tab closure. These actions are important to comply with privacy
> requirements for this feature.
>
> This change re-adds instrumentation tests previously committed along with a
> related bugfix [1] that were reverted due to them being flaky. It seems that the
> underlying issue causing the flakyness has been fixed [2] [3] so these should
> now work as expected.
>
> [1] https://codereview.chromium.org/2824623002/
> [2] https://bugs.chromium.org/p/chromium/issues/detail?id=592961 (restricted)
> [3] https://bugs.chromium.org/p/chromium/issues/detail?id=738490
>
> BUG=711770
>
> Review-Url: https://codereview.chromium.org/2833653002
> Cr-Commit-Position: refs/heads/master@{#487727}
> Committed: https://chromium.googlesource.com/chromium/src/+/f6fd25d5826f1a62010b08037eb50c396b05d7a2

TBR=dewittj@chromium.org,carlosk@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=711770

Review-Url: https://codereview.chromium.org/2983953002
Cr-Commit-Position: refs/heads/master@{#488195}

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

@@ -85,14 +85,14 @@ public class OfflinePageBridge {
         /**
          * Called when the native side of offline pages is changed due to adding, removing or
          * update an offline page.
-         * @param addedPage The in-memory representation of the page added to the offline database.
          */
         public void offlinePageAdded(OfflinePageItem addedPage) {}
 
         /**
          * Called when an offline page is deleted. This can be called as a result of
          * #checkOfflinePageMetadata().
-         * @param deletedPage Information regarding the deleted offline page.
+         * @param offlineId The offline ID of the deleted offline page.
+         * @param clientId The client supplied ID of the deleted offline page.
          */
         public void offlinePageDeleted(DeletedPageInfo deletedPage) {}
     }

chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java

@@ -35,7 +35,6 @@ import java.util.List;
 import java.util.concurrent.Callable;
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicReference;
 
 /** Integration tests for the Last 1 feature of Offline Pages. */
 @RunWith(ChromeJUnit4ClassRunner.class)
@@ -46,13 +45,11 @@ public class RecentTabsTest {
     public ChromeTabbedActivityTestRule mActivityTestRule = new ChromeTabbedActivityTestRule();
 
     private static final String TEST_PAGE = "/chrome/test/data/android/about.html";
-    private static final String TEST_PAGE_2 = "/chrome/test/data/android/simple.html";
     private static final int TIMEOUT_MS = 5000;
 
     private OfflinePageBridge mOfflinePageBridge;
     private EmbeddedTestServer mTestServer;
     private String mTestPage;
-    private String mTestPage2;
 
     private void initializeBridgeForProfile(final boolean incognitoProfile)
             throws InterruptedException {
@@ -79,7 +76,7 @@ public class RecentTabsTest {
                 });
             }
         });
-        assertAcquire(semaphore);
+        Assert.assertTrue(semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS));
     }
 
     @Before
@@ -88,10 +85,11 @@ public class RecentTabsTest {
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
             @Override
             public void run() {
+                // Ensure we start in an offline state.
+                NetworkChangeNotifier.forceConnectivityState(false);
                 if (!NetworkChangeNotifier.isInitialized()) {
                     NetworkChangeNotifier.init();
                 }
-                NetworkChangeNotifier.forceConnectivityState(true);
             }
         });
 
@@ -100,7 +98,6 @@ public class RecentTabsTest {
         mTestServer = EmbeddedTestServer.createAndStartServer(
                 InstrumentationRegistry.getInstrumentation().getContext());
         mTestPage = mTestServer.getURL(TEST_PAGE);
-        mTestPage2 = mTestServer.getURL(TEST_PAGE_2);
     }
 
     @After
@@ -121,9 +118,9 @@ public class RecentTabsTest {
         // The tab should be foreground and so no snapshot should exist.
         Assert.assertNull(getPageByClientId(firstTabClientId));
 
-        // Note: switching to a new tab must occur after the SnapshotController believes the page
-        // quality is good enough.  With the debug flag, the delay after DomContentLoaded is 0 so we
-        // can definitely snapshot after onload (which is what |loadUrlInNewTab| waits for).
+        // Note, that switching to a new tab must occur after the SnapshotController believes the
+        // page quality is good enough.  With the debug flag, the delay after DomContentLoaded is 0
+        // so we can definitely snapshot after onload (which is what |loadUrlInNewTab| waits for).
 
         // Switch to a new tab to cause the WebContents hidden event.
         mActivityTestRule.loadUrlInNewTab("about:blank");
@@ -132,10 +129,10 @@ public class RecentTabsTest {
     }
 
     /**
-     * Note: this test relies on a sleeping period because some of the monitored actions are
-     * difficult to track deterministically. A sleep time of 100 ms was chosen based on local
-     * testing and is expected to be "safe". Nevertheless if flakiness is detected it might have to
-     * be further increased.
+     * Note: this test relies on a sleeping period because some of the taking actions are
+     * complicated to track otherwise, so there is the possibility of flakiness. I chose 100ms from
+     * local testing and I expect it to be "safe" but it flakiness is detected it might have to be
+     * further increased.
      */
     @Test
     @CommandLineFlags.Add("short-offline-page-snapshot-delay-for-test")
@@ -191,94 +188,10 @@ public class RecentTabsTest {
         waitForPageWithClientId(firstTabClientId);
     }
 
-    /**
-     * Verifies that a snapshot created by last_n is properly deleted when the tab is navigated to
-     * another page. The deletion of snapshots for pages that should not be available anymore is a
-     * privacy requirement for last_n.
-     */
-    @Test
-    @CommandLineFlags.Add("short-offline-page-snapshot-delay-for-test")
-    @MediumTest
-    public void testLastNPageIsDeletedUponNavigation() throws Exception {
-        // The tab of interest.
-        final Tab tab = mActivityTestRule.loadUrlInNewTab(mTestPage);
-        final TabModelSelector tabModelSelector = tab.getTabModelSelector();
-
-        final ClientId firstTabClientId =
-                new ClientId(OfflinePageBridge.LAST_N_NAMESPACE, Integer.toString(tab.getId()));
-
-        // Switch to a new tab and wait for the snapshot to be created.
-        mActivityTestRule.loadUrlInNewTab("about:blank");
-        Assert.assertFalse(tab.equals(tabModelSelector.getCurrentTab()));
-        OfflinePageItem offlinePage = waitForPageWithClientId(firstTabClientId);
-
-        // Switch back to the initial tab.
-        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
-            @Override
-            public void run() {
-                TabModel tabModel = tabModelSelector.getModelForTabId(tab.getId());
-                int tabIndex = TabModelUtils.getTabIndexById(tabModel, tab.getId());
-                TabModelUtils.setIndex(tabModel, tabIndex);
-            }
-        });
-        Assert.assertEquals(tabModelSelector.getCurrentTab(), tab);
-
-        // Navigate to a new page and confirm that the previously created snapshot has been deleted.
-        Semaphore deletionSemaphore = installPageDeletionSemaphore(offlinePage.getOfflineId());
-        mActivityTestRule.loadUrl(mTestPage2);
-        assertAcquire(deletionSemaphore);
-    }
-
-    /**
-     * Verifies that a snapshot created by last_n is properly deleted when the tab is closed. The
-     * deletion of snapshots for pages that should not be available anymore is a privacy requirement
-     * for last_n.
-     */
-    @Test
-    @CommandLineFlags.Add("short-offline-page-snapshot-delay-for-test")
-    @MediumTest
-    public void testLastNPageIsDeletedUponClosure() throws Exception {
-        // The tab of interest.
-        final Tab tab = mActivityTestRule.loadUrlInNewTab(mTestPage);
-        final TabModelSelector tabModelSelector = tab.getTabModelSelector();
-
-        final ClientId firstTabClientId =
-                new ClientId(OfflinePageBridge.LAST_N_NAMESPACE, Integer.toString(tab.getId()));
-
-        // Switch to a new tab and wait for the snapshot to be created.
-        mActivityTestRule.loadUrlInNewTab("about:blank");
-        OfflinePageItem offlinePage = waitForPageWithClientId(firstTabClientId);
-
-        // Requests closing the tab allowing for undo -- so to exercise the subscribed notification
-        // -- but immediately requests final closure.
-        final int tabId = tab.getId();
-        Semaphore deletionSemaphore = installPageDeletionSemaphore(offlinePage.getOfflineId());
-        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
-            @Override
-            public void run() {
-                TabModel tabModel = tabModelSelector.getModelForTabId(tabId);
-                tabModel.closeTab(tab, false, false, true);
-                tabModel.commitTabClosure(tabId);
-            }
-        });
-
-        // Checks that the tab is no more and that the snapshot was deleted.
-        Assert.assertNull(tabModelSelector.getTabById(tabId));
-        assertAcquire(deletionSemaphore);
-    }
-
-    private void assertAcquire(Semaphore semaphore) throws InterruptedException {
-        Assert.assertTrue(semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS));
-    }
-
-    private OfflinePageItem waitForPageWithClientId(final ClientId clientId)
-            throws InterruptedException {
-        OfflinePageItem item = getPageByClientId(clientId);
-        if (item != null) return item;
+    private void waitForPageWithClientId(final ClientId clientId) throws InterruptedException {
+        if (getPageByClientId(clientId) != null) return;
 
         final Semaphore semaphore = new Semaphore(0);
-        final AtomicReference<OfflinePageItem> itemReference =
-                new AtomicReference<OfflinePageItem>();
         ThreadUtils.runOnUiThread(new Runnable() {
             @Override
             public void run() {
@@ -286,7 +199,6 @@ public class RecentTabsTest {
                     @Override
                     public void offlinePageAdded(OfflinePageItem newPage) {
                         if (newPage.getClientId().equals(clientId)) {
-                            itemReference.set(newPage);
                             mOfflinePageBridge.removeObserver(this);
                             semaphore.release();
                         }
@@ -294,27 +206,7 @@ public class RecentTabsTest {
                 });
             }
         });
-        assertAcquire(semaphore);
-        return itemReference.get();
-    }
-
-    private Semaphore installPageDeletionSemaphore(final long offlineId) {
-        final Semaphore semaphore = new Semaphore(0);
-        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
-            @Override
-            public void run() {
-                mOfflinePageBridge.addObserver(new OfflinePageModelObserver() {
-                    @Override
-                    public void offlinePageDeleted(DeletedPageInfo deletedPage) {
-                        if (deletedPage.getOfflineId() == offlineId) {
-                            mOfflinePageBridge.removeObserver(this);
-                            semaphore.release();
-                        }
-                    }
-                });
-            }
-        });
-        return semaphore;
+        Assert.assertTrue(semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS));
     }
 
     private OfflinePageItem getPageByClientId(ClientId clientId) throws InterruptedException {
@@ -338,27 +230,7 @@ public class RecentTabsTest {
                         });
             }
         });
-        assertAcquire(semaphore);
-        return result[0];
-    }
-
-    private OfflinePageItem getPageByOfflineId(final long offlineId) throws InterruptedException {
-        final OfflinePageItem[] result = {null};
-        final Semaphore semaphore = new Semaphore(0);
-
-        ThreadUtils.runOnUiThread(new Runnable() {
-            @Override
-            public void run() {
-                mOfflinePageBridge.getPageByOfflineId(offlineId, new Callback<OfflinePageItem>() {
-                    @Override
-                    public void onResult(OfflinePageItem item) {
-                        result[0] = item;
-                        semaphore.release();
-                    }
-                });
-            }
-        });
-        assertAcquire(semaphore);
+        Assert.assertTrue(semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS));
         return result[0];
     }
 }