Last_n: do not save snapshot of custom tabs.

chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc

Index: chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc
diff --git a/chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc b/chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc
index 91f5a3a588165309596fe5a25dd281c6aa4e20e4..9da298ee5da742a3126a1816690e61a6cf6f522c 100644
--- a/chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc
+++ b/chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc
@@ -53,6 +53,9 @@ class TestDelegate: public RecentTabHelper::Delegate {
     // There is no expectations that tab_id is always present.
   bool GetTabId(content::WebContents* web_contents, int* tab_id) override;
   bool IsLowEndDevice() override { return is_low_end_device_; }
+  bool IsCustomTab(content::WebContents* web_contents) override {
+    return is_custom_tab_;
+  }
 
   void set_archive_result(
       offline_pages::OfflinePageArchiver::ArchiverResult result) {
@@ -63,6 +66,8 @@ class TestDelegate: public RecentTabHelper::Delegate {
 
   void SetAsLowEndDevice() { is_low_end_device_ = true; }
 
+  void set_is_custom_tab(bool is_custom_tab) { is_custom_tab_ = is_custom_tab; }
+
  private:
   OfflinePageTestArchiver::Observer* observer_;  // observer owns this.
   int tab_id_;
@@ -74,6 +79,7 @@ class TestDelegate: public RecentTabHelper::Delegate {
       offline_pages::OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED;
   int64_t archive_size_ = kArchiveSizeToReport;
   bool is_low_end_device_ = false;
+  bool is_custom_tab_ = false;
 };
 
 class RecentTabHelperTest
@@ -369,6 +375,41 @@ TEST_F(RecentTabHelperTest, LastNDisabledOnSvelte) {
   ASSERT_EQ(1U, GetAllPages().size());
 }
 
+// Checks that last_n will not save a snapshot if the tab while the tab is
+// presented as a custom tab. Download requests should be unaffected though.
+TEST_F(RecentTabHelperTest, LastNWontSaveCustomTab) {
+  // Simulates the tab running as a custom tab.
+  default_test_delegate()->set_is_custom_tab(true);
+
+  // Navigate and finish loading then hide the tab. Nothing should be saved.
+  NavigateAndCommit(kTestPageUrl);
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
+  FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
+  EXPECT_TRUE(model()->is_loaded());
+  EXPECT_EQ(0U, page_added_count());
+  ASSERT_EQ(0U, GetAllPages().size());
+
+  // But the following download request should work normally
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(NewDownloadClientId(),
+                                                     123L);
+  RunUntilIdle();
+  EXPECT_EQ(1U, page_added_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+
+  // Simulates the tab being transfered from the CustomTabActivity back to a
+  // ChromeActivity.
+  default_test_delegate()->set_is_custom_tab(false);
+
+  // Upon the next hide a last_n snapshot should be saved.
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
+  EXPECT_TRUE(model()->is_loaded());
+  EXPECT_EQ(2U, page_added_count());
+  ASSERT_EQ(2U, GetAllPages().size());
+}
+

[dewittj, 2017/03/27 20:37:45]

Is there a way to test with a real TabAndroid, and tell it to go from CustomTab
to NotCustomTab?  Could be an integration test.  This would be a nice safeguard
against unexpected WasHidden events.

[gone, 2017/03/27 22:22:10]

You could probably do this using a Java test... could be potentially really
flaky though because you'd need to start and track two different Android
Activities.

[Yusuf, 2017/03/27 22:28:27]

See CustomTabActivityTest#testTabReparentingBasic and the other ones there for
examples. Maybe you can call this API there and assert it gives the right value
before and after?

[carlosk, 2017/03/28 02:00:32]

I added asserts to CustomTabActivityTestBase.startCustomTabActivityWithIntent
and CustomTabActivityTest.reparentAndVerifyTab.

dewittj@: I think these changes should cover your concern. As long as that
method returns the correct state, WasHidden can be called at will. But while
updating these tests I found out that upon Activity reparenting there is a
specific assert to verify that WasHidden is *not* called [1]. So we should be
double-safe here.

yusufo@: should I also change CustomTabActivityTest.testOpenInBrowser to run
those checks? I experienced with it and it seemed it would make it very similar
to reparentAndVerifyTab. WDYT?


[1]
https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu...

[Yusuf, 2017/03/28 17:17:54]

testOpenInBrowser actually tests the default behavior only (where we dont have
any default browser set on the bot), so "Open in Browser" means no default is
set. "Open in Chrome" means the default is Chrome. So reparenting wont happen in
that test.

[carlosk, 2017/03/28 23:26:07]

I see. It's confusing though that in the comment of that test it is mentioned
"Open in Chrome".

As I couldn't find any test specifically named after Chrome I assumed you were
satisfied with the test changes as they are. Let me know if I missed something.

 // Triggers two last_n snapshot captures during a single page load. Should end
 // up with one snapshot, the 1st being replaced by the 2nd.
 TEST_F(RecentTabHelperTest, TwoCapturesSamePageLoad) {